Re: [PATCH v6 03/20] xen/riscv: introduce extenstion support check by compiler

[Conor Dooley <conor@xxxxxxxxxx>]

On Wed, Mar 20, 2024 at 07:58:05PM +0100, Oleksii wrote:
> On Mon, 2024-03-18 at 17:58 +0100, Jan Beulich wrote:
> > On 15.03.2024 19:05, Oleksii Kurochko wrote:
> > > Currently, RISC-V requires two extensions: _zbb and _zihintpause.
> > 
> > Do we really require Zbb already?
> After an introduction of Andrew C. patches [1] it is requited for
> __builtin_ffs{l}
> 
> [1]
> https://lore.kernel.org/xen-devel/20240313172716.2325427-1-andrew.cooper3@xxxxxxxxxx/T/#t
> > 
> > > This patch introduces a compiler check to check if these extensions
> > > are supported.
> > > Additionally, it introduces the riscv/booting.txt file, which
> > > contains
> > > information about the extensions that should be supported by the
> > > platform.
> > > 
> > > In the future, a feature will be introduced to check whether an
> > > extension
> > > is supported at runtime.
> > > However, this feature requires functionality for parsing device
> > > tree
> > > source (DTS), which is not yet available.
> > 
> > Can't you query the CPU for its features?
> I couldn't find such reg ( or SBI call ) in the spec.

There isn't.

> SBI call sbi_probe_extension() exists, but it doesn't check for every
> possible extension. As far as I understand it checks only for that one
> which are present in SBI spec.

Yeah, it only checks for SBI extensions, not ISA extensions.

> The most closest thing I see how to check that without dts is how it is
> done in OpenSBI:

IIRC this only "works" because the OpenSBI devs assume that there are no
non-normative behaviours and all CSRs have their ~God~ RVI defined
meanings. Reading a CSR to see if it traps is not behaviour you can really
rely on unless the platform claims to support Sstrict - but Sstrict you'd
have to detect from the DT so chicken and egg for you! It's one of these
new "extensions" from the profiles spec, so it doesn't even have support
in Linux's dt-bindings yet. Up to Xen devs if you guys want to make the
same assumptions as OpenSBI. Linux doesn't and when we discussed this
not too long ago on the U-Boot ML in the context of the rng CSR it was
also decided not to do make the assumption there either.

Personally I wonder if you can just apply the same policy here as you
did with Zbb in the other thread and assume that something with H will
have Zihintpause and leave implementing a "fake" pause as an exercise
for someone that introduces such a system?
If Jess is correct, and I do remember testing this, it's actually
"always" safe to call the pause instruction on CPUs where the extension
is not supported as it uses an encoding of fence that effectively makes
it a into a nop:
https://lore.kernel.org/linux-riscv/2E96A836-764D-4D07-AB79-3861B9CC2B1F@xxxxxxxxxx/
At worst, that'd make cpu_relax() a nop if someone didn't meet that
requirement.

FWIW, Linux's cpu_relax() on RISC-V looks like:
        static inline void cpu_relax(void)
        {
        #ifdef __riscv_muldiv
                int dummy;
                /* In lieu of a halt instruction, induce a long-latency stall. 
*/
                __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
        #endif
        
        #ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
                /*
                 * Reduce instruction retirement.
                 * This assumes the PC changes.
                 */
                __asm__ __volatile__ ("pause");
        #else
                /* Encoding of the pause instruction */
                __asm__ __volatile__ (".4byte 0x100000F");
        #endif
                barrier();
        }

I figure having div is part of your base requirements, so maybe you can
just do something similar in the !zihintpause case if making that
extension a requirement is problematic? 
Doing that invalid div used to be conditional, but cpu_relax() is in the
vdso so the static branch it used to be gated with got removed and its
now unconditional. Probably that's not a constraint on Xen's cpu_relax()?

Oh ye, and we do the .4byte crap so that toolchain support wasn't needed
for Zihintpause given we are using it in exactly one place.

Cheers,
Conor.

> #define csr_read_allowed(csr_num, trap)                               \
>       ({                                                      \
>       register ulong tinfo asm("a3") = (ulong)trap;           \
>       register ulong ttmp asm("a4");                          \
>       register ulong mtvec = sbi_hart_expected_trap_addr();   \
>       register ulong ret =
> 0;                                    \
>       ((struct sbi_trap_info *)(trap))->cause = 0;            \
>       asm volatile(                                           \
>               "add %[ttmp], %[tinfo],
> zero\n"                       \
>               "csrrw %[mtvec], " STR(CSR_MTVEC) ", %[mtvec]\n"\
>               "csrr %[ret],
> %[csr]\n"                             \
>               "csrw " STR(CSR_MTVEC) ", %[mtvec]"             \
>           : [mtvec] "+&r"(mtvec), [tinfo] "+&r"(tinfo),       \
>             [ttmp] "+&r"(ttmp), [ret] "=&r" (ret)             \
>           : [csr] "i" (csr_num)                               \
>           : "memory");                                        \
>       ret;                                                    \
>       })                                                      \
> ...
>       /* Detect if hart supports stimecmp CSR(Sstc extension) */
>       if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) {
>               csr_read_allowed(CSR_STIMECMP, (unsigned long)&trap);
>               if (!trap.cause)
>                       __sbi_hart_update_extension(hfeatures,
>                                       SBI_HART_EXT_SSTC, true);
>       }

Attachment: signature.asc
Description: PGP signature