| Message ID | d4df95eb7a30df3f882b67f200964232fee9d6c1.1710517542.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 15.03.2024 19:05, Oleksii Kurochko wrote: > Currently, RISC-V requires two extensions: _zbb and _zihintpause. Do we really require Zbb already? > 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? > --- a/xen/arch/riscv/arch.mk > +++ b/xen/arch/riscv/arch.mk > @@ -3,16 +3,22 @@ > > $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) > > -CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64 > +riscv-abi-$(CONFIG_RISCV_32) := -mabi=ilp32 > +riscv-abi-$(CONFIG_RISCV_64) := -mabi=lp64 > > riscv-march-$(CONFIG_RISCV_ISA_RV64G) := rv64g > riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c > > +extensions := $(call as-insn,$(CC) $(riscv-abi-y) -march=$(riscv-march-y)_zbb,"",_zbb) \ > + $(call as-insn,$(CC) $(riscv-abi-y) -march=$(riscv-march-y)_zihintpause,"pause",_zihintpause) Imo you want another helper macro here, where all one needs to pass in is the extension name (i.e. zbb and zihintpause as per above). That'll also help with line length, I hope. Jan
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@citrix.com/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. 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. The most closest thing I see how to check that without dts is how it is done in OpenSBI: #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); } ~ Oleksii > > > --- a/xen/arch/riscv/arch.mk > > +++ b/xen/arch/riscv/arch.mk > > @@ -3,16 +3,22 @@ > > > > $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) > > > > -CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64 > > +riscv-abi-$(CONFIG_RISCV_32) := -mabi=ilp32 > > +riscv-abi-$(CONFIG_RISCV_64) := -mabi=lp64 > > > > riscv-march-$(CONFIG_RISCV_ISA_RV64G) := rv64g > > riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c > > > > +extensions := $(call as-insn,$(CC) $(riscv-abi-y) -march=$(riscv- > > march-y)_zbb,"",_zbb) \ > > + $(call as-insn,$(CC) $(riscv-abi-y) -march=$(riscv- > > march-y)_zihintpause,"pause",_zihintpause) > > Imo you want another helper macro here, where all one needs to pass > in is > the extension name (i.e. zbb and zihintpause as per above). That'll > also > help with line length, I hope. > > Jan
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@citrix.com/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@jrtc27.com/ 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); > }
On 20.03.2024 20:44, Conor Dooley wrote: > 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@citrix.com/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. And there was never a consideration to add, at the architecture level, some straightforward way for all layers of software to be able to easily detect availability of extensions? I find the lack thereof pretty surprising ... Jan
On Wed, 2024-03-20 at 19:44 +0000, Conor Dooley wrote: > 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 i understand you correctly, then it is done in this way now. Only build time check is done, but it would be nice also have some runtime check, and for now, for runtime the "check" is only exists in booting.txt where it mentioned that Xen expects from CPU to support some extenstions; otherwise "please implement the following functions". Anyway, at the moment, the best one runtime check which we can provide is detect availability of extensions from DT and what I mentioned in commit message. ~ Oleksii
On Thu, Mar 21, 2024 at 09:31:59AM +0100, Jan Beulich wrote: > On 20.03.2024 20:44, Conor Dooley wrote: > > 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@citrix.com/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. > > And there was never a consideration to add, at the architecture level, > some straightforward way for all layers of software to be able to > easily detect availability of extensions? I find the lack thereof > pretty surprising ... No, there sorta is. There's a "configuration structure" spec in the works, but the (public?) documentation for it is very nascent: https://github.com/riscv/configuration-structure/ It uses (what I seem to recall is an m-mode) CSR to give the address of an ASN.1 structure in memory which is intended to encode a whole raft of information. The CSR itself might be M-Mode, but that doesn't prevent accessing the data itself at any layer in the stack. The last time I read it I got the impression it was supposed to be usable for describing an entire system (including things like i2c or spi controllers). I don't think it ever explicitly says that in the spec, but there's a note (IIRC) that mentions being unsuitable for describing devices on dynamic buses like PCI, so that kinda implies anything else is fair game. Hope that helps?
diff --git a/docs/misc/riscv/booting.txt b/docs/misc/riscv/booting.txt new file mode 100644 index 0000000000..cb4d79f12c --- /dev/null +++ b/docs/misc/riscv/booting.txt @@ -0,0 +1,16 @@ +System requirements +=================== + +The following extensions are expected to be supported by a system on which +Xen is run: +- Zbb: + RISC-V doesn't have a CLZ instruction in the base ISA. + As a consequence, __builtin_ffs() emits a library call to ffs() on GCC, + or a de Bruijn sequence on Clang. + Zbb extension adds a CLZ instruction, after which __builtin_ffs() emits + a very simple sequence. + The similar issue occurs with other __builtin_<bitop>, so it is needed to + provide a generic version of bitops in RISC-V bitops.h +- Zihintpause: + On a system that doesn't have this extension, cpu_relax() should be + implemented properly. diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk index 8403f96b6f..da6f8c82eb 100644 --- a/xen/arch/riscv/arch.mk +++ b/xen/arch/riscv/arch.mk @@ -3,16 +3,22 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) -CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64 +riscv-abi-$(CONFIG_RISCV_32) := -mabi=ilp32 +riscv-abi-$(CONFIG_RISCV_64) := -mabi=lp64 riscv-march-$(CONFIG_RISCV_ISA_RV64G) := rv64g riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c +extensions := $(call as-insn,$(CC) $(riscv-abi-y) -march=$(riscv-march-y)_zbb,"",_zbb) \ + $(call as-insn,$(CC) $(riscv-abi-y) -march=$(riscv-march-y)_zihintpause,"pause",_zihintpause) + +extensions := $(subst $(space),,$(extensions)) + # Note that -mcmodel=medany is used so that Xen can be mapped # into the upper half _or_ the lower half of the address space. # -mcmodel=medlow would force Xen into the lower half. -CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany +CFLAGS += $(riscv-abi-y) -march=$(riscv-march-y)$(extensions) -mstrict-align -mcmodel=medany # TODO: Drop override when more of the build is working override ALL_OBJS-y = arch/$(SRCARCH)/built_in.o
Currently, RISC-V requires two extensions: _zbb and _zihintpause. 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. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V6: - new patch for this patch series --- docs/misc/riscv/booting.txt | 16 ++++++++++++++++ xen/arch/riscv/arch.mk | 10 ++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 docs/misc/riscv/booting.txt