[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 03/20] xen/riscv: introduce extenstion support check by compiler
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |