| Message ID | 0c9b0317d0fc4f93bf5cc0893d480853110b8287.1712137031.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 03.04.2024 12:19, Oleksii Kurochko wrote: > 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> Acked-by: Jan Beulich <jbeulich@suse.com> However, ... > --- a/xen/arch/riscv/arch.mk > +++ b/xen/arch/riscv/arch.mk > @@ -3,16 +3,27 @@ > > $(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 > > +riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) > + > +zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) > +zihintpause := $(call as-insn,\ > + $(CC) $(riscv-generic-flags)_zihintpause,"pause",_zihintpause) ... this would better be indented thus zihintpause := $(call as-insn, \ $(CC) $(riscv-generic-flags)_zihintpause,"pause",_zihintpause) to make nesting entirely obvious. I guess I'll table the liberty to adjust while committing. I'd also like to note that this is specifically not what I had suggested. But it at least improves readability. Jan
On 03.04.2024 12:19, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/arch.mk > +++ b/xen/arch/riscv/arch.mk > @@ -3,16 +3,27 @@ > > $(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 > > +riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) > + > +zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) While committing another question popped up: Why "" (i.e. no insn) here, ... > +zihintpause := $(call as-insn,\ > + $(CC) $(riscv-generic-flags)_zihintpause,"pause",_zihintpause) ... but "pause" here? Jan
On Thu, 2024-04-04 at 12:07 +0200, Jan Beulich wrote: > On 03.04.2024 12:19, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/arch.mk > > +++ b/xen/arch/riscv/arch.mk > > @@ -3,16 +3,27 @@ > > > > $(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 > > > > +riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) > > + > > +zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) > > While committing another question popped up: Why "" (i.e. no insn) > here, ... > > > +zihintpause := $(call as-insn,\ > > + $(CC) $(riscv-generic- > > flags)_zihintpause,"pause",_zihintpause) > > ... but "pause" here? In the case of the Zbb extension, we don't check for a specific instruction, but with the Zihintpause, the idea was to verify if the pause instruction is supported or not. However, in both checks, there might be no instruction as an argument of as-insn. ~ Oleksii
On 04.04.2024 17:18, Oleksii wrote: > On Thu, 2024-04-04 at 12:07 +0200, Jan Beulich wrote: >> On 03.04.2024 12:19, Oleksii Kurochko wrote: >>> --- a/xen/arch/riscv/arch.mk >>> +++ b/xen/arch/riscv/arch.mk >>> @@ -3,16 +3,27 @@ >>> >>> $(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 >>> >>> +riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) >>> + >>> +zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) >> >> While committing another question popped up: Why "" (i.e. no insn) >> here, ... >> >>> +zihintpause := $(call as-insn,\ >>> + $(CC) $(riscv-generic- >>> flags)_zihintpause,"pause",_zihintpause) >> >> ... but "pause" here? > > In the case of the Zbb extension, we don't check for a specific > instruction, but with the Zihintpause, the idea was to verify if the > pause instruction is supported or not. And why's this verification relevant here, but not for Zbb? Jan > However, in both checks, there > might be no instruction as an argument of as-insn. > > ~ Oleksii
On Thu, 2024-04-04 at 17:43 +0200, Jan Beulich wrote: > On 04.04.2024 17:18, Oleksii wrote: > > On Thu, 2024-04-04 at 12:07 +0200, Jan Beulich wrote: > > > On 03.04.2024 12:19, Oleksii Kurochko wrote: > > > > --- a/xen/arch/riscv/arch.mk > > > > +++ b/xen/arch/riscv/arch.mk > > > > @@ -3,16 +3,27 @@ > > > > > > > > $(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 > > > > > > > > +riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) > > > > + > > > > +zbb := $(call as-insn,$(CC) $(riscv-generic- > > > > flags)_zbb,"",_zbb) > > > > > > While committing another question popped up: Why "" (i.e. no > > > insn) > > > here, ... > > > > > > > +zihintpause := $(call as-insn,\ > > > > + $(CC) $(riscv-generic- > > > > flags)_zihintpause,"pause",_zihintpause) > > > > > > ... but "pause" here? > > > > In the case of the Zbb extension, we don't check for a specific > > instruction, but with the Zihintpause, the idea was to verify if > > the > > pause instruction is supported or not. > > And why's this verification relevant here, but not for Zbb? It is not relevant and can be dropped. I'll do a follow-up patch. ~ Oleksii > > Jan > > > However, in both checks, there > > might be no instruction as an argument of as-insn. > > > > ~ Oleksii >
On 04.04.2024 18:17, Oleksii wrote: > On Thu, 2024-04-04 at 17:43 +0200, Jan Beulich wrote: >> On 04.04.2024 17:18, Oleksii wrote: >>> On Thu, 2024-04-04 at 12:07 +0200, Jan Beulich wrote: >>>> On 03.04.2024 12:19, Oleksii Kurochko wrote: >>>>> --- a/xen/arch/riscv/arch.mk >>>>> +++ b/xen/arch/riscv/arch.mk >>>>> @@ -3,16 +3,27 @@ >>>>> >>>>> $(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 >>>>> >>>>> +riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) >>>>> + >>>>> +zbb := $(call as-insn,$(CC) $(riscv-generic- >>>>> flags)_zbb,"",_zbb) >>>> >>>> While committing another question popped up: Why "" (i.e. no >>>> insn) >>>> here, ... >>>> >>>>> +zihintpause := $(call as-insn,\ >>>>> + $(CC) $(riscv-generic- >>>>> flags)_zihintpause,"pause",_zihintpause) >>>> >>>> ... but "pause" here? >>> >>> In the case of the Zbb extension, we don't check for a specific >>> instruction, but with the Zihintpause, the idea was to verify if >>> the >>> pause instruction is supported or not. >> >> And why's this verification relevant here, but not for Zbb? > It is not relevant and can be dropped. Is it really not? Aren't you checking two things for Zihintpause (compiler and assembler support), while checking only one (compiler) for Zbb? Jan
On Fri, 2024-04-05 at 08:08 +0200, Jan Beulich wrote: > On 04.04.2024 18:17, Oleksii wrote: > > On Thu, 2024-04-04 at 17:43 +0200, Jan Beulich wrote: > > > On 04.04.2024 17:18, Oleksii wrote: > > > > On Thu, 2024-04-04 at 12:07 +0200, Jan Beulich wrote: > > > > > On 03.04.2024 12:19, Oleksii Kurochko wrote: > > > > > > --- a/xen/arch/riscv/arch.mk > > > > > > +++ b/xen/arch/riscv/arch.mk > > > > > > @@ -3,16 +3,27 @@ > > > > > > > > > > > > $(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 > > > > > > > > > > > > +riscv-generic-flags := $(riscv-abi-y) -march=$(riscv- > > > > > > march-y) > > > > > > + > > > > > > +zbb := $(call as-insn,$(CC) $(riscv-generic- > > > > > > flags)_zbb,"",_zbb) > > > > > > > > > > While committing another question popped up: Why "" (i.e. no > > > > > insn) > > > > > here, ... > > > > > > > > > > > +zihintpause := $(call as-insn,\ > > > > > > + $(CC) $(riscv-generic- > > > > > > flags)_zihintpause,"pause",_zihintpause) > > > > > > > > > > ... but "pause" here? > > > > > > > > In the case of the Zbb extension, we don't check for a specific > > > > instruction, but with the Zihintpause, the idea was to verify > > > > if > > > > the > > > > pause instruction is supported or not. > > > > > > And why's this verification relevant here, but not for Zbb? > > It is not relevant and can be dropped. > > Is it really not? Aren't you checking two things for Zihintpause > (compiler > and assembler support), while checking only one (compiler) for Zbb? You are right. I made again an assumption that binutils are as new as a compiler. Then it makes sense to replace an argument if as-insn "" with andn t0, t0, t0 for Zbb: -zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) +zbb_insn := "andn t0, t0, t0" +zbb := $(call as-insn,$(CC) $(riscv-generic- flags)_zbb,${zbb_insn},_zbb) ~ Oleksii > > Jan
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..24a7461bcc 100644 --- a/xen/arch/riscv/arch.mk +++ b/xen/arch/riscv/arch.mk @@ -3,16 +3,27 @@ $(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 +riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) + +zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) +zihintpause := $(call as-insn,\ + $(CC) $(riscv-generic-flags)_zihintpause,"pause",_zihintpause) + +extensions := $(zbb) $(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-generic-flags)$(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 V7: - add variables for each extension separately. - create variable for abi and compilation flags to not repeat the same in several places. - update architectures to use generic implementations --- Changes in V6: - new patch for this patch series --- docs/misc/riscv/booting.txt | 16 ++++++++++++++++ xen/arch/riscv/arch.mk | 15 +++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 docs/misc/riscv/booting.txt