| Message ID | 4e1ee99a9ad71015b5e8860d20b63337b526d0e9.1708962629.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 26.02.2024 18:39, Oleksii Kurochko wrote: > --- /dev/null > +++ b/docs/misc/riscv/booting.txt > @@ -0,0 +1,8 @@ > +System requirements > +=================== > + > +The following extensions are expected to be supported by a system on which > +Xen is run: > +- Zihintpause: > + On a system that doesn't have this extension, cpu_relax() should be > + implemented properly. Otherwise, an illegal instruction exception will arise. This decision wants justifying in the (presently once again empty) description. Furthermore - will there really be an illegal instruction exception otherwise? Isn't it the nature of hints that they are NOPs if not serving their designated purpose? > --- a/xen/arch/riscv/arch.mk > +++ b/xen/arch/riscv/arch.mk > @@ -5,6 +5,12 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) > > CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64 > > +ifeq ($(CONFIG_RISCV_64),y) > +has_zihintpause = $(call as-insn,$(CC) -mabi=lp64 -march=rv64i_zihintpause, "pause",_zihintpause,) > +else > +has_zihintpause = $(call as-insn,$(CC) -mabi=ilp32 -march=rv32i_zihintpause, "pause",_zihintpause,) > +endif Considering that down the road likely more such tests will want adding, I think this wants further abstracting for the rv32/rv64 difference (ideally in a way that wouldn't make future RV128 wrongly and silently take the RV32 branch). This would include eliminating the -mabi=lp64 redundancy with what's visible in context, perhaps by way of introducing a separate helper macro, e.g. riscv-abi-$(CONFIG_RISCV_32) := -mabi=ilp32 riscv-abi-$(CONFIG_RISCV_64) := -mabi=lp64 I further see nothing wrong with also using $(riscv-march-y) here. I.e. overall _zihintpause := $(call as-insn,$(CC) $(riscv-abi-y) $(riscv-march-y)_zihintpause,"pause",_zihintpause) (still with potential of abstracting further through another macro such that not every such construct would need to spell out the ABI and arch compiler options). Plus a macro named has_* imo can be expected to expand to y or n. I would suggest to simply drop the "has", thus ... > @@ -12,7 +18,7 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c > # 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 += -march=$(riscv-march-y)$(has_zihintpause) -mstrict-align -mcmodel=medany ... also making the use site look > --- a/xen/arch/riscv/include/asm/processor.h > +++ b/xen/arch/riscv/include/asm/processor.h > @@ -12,6 +12,9 @@ > > #ifndef __ASSEMBLY__ > > +/* TODO: need to be implemeted */ > +#define smp_processor_id() 0 > + > /* On stack VCPU state */ > struct cpu_user_regs > { > @@ -53,6 +56,26 @@ struct cpu_user_regs > unsigned long pregs; > }; > > +/* TODO: need to implement */ > +#define cpu_to_core(cpu) (0) > +#define cpu_to_socket(cpu) (0) Nit: Like above in smp_processor_id() no need for parentheses here. > +static inline void cpu_relax(void) > +{ > +#ifdef __riscv_zihintpause > + /* > + * Reduce instruction retirement. > + * This assumes the PC changes. What is this 2nd sentence about? > + */ > + __asm__ __volatile__ ( "pause" ); > +#else > + /* Encoding of the pause instruction */ > + __asm__ __volatile__ ( ".insn 0x100000F" ); May I ask that you spell out the leading zero here, to make clear there aren't, by mistake, one to few zeroes in the middle? Jan
On Tue, 2024-03-05 at 09:05 +0100, Jan Beulich wrote: > On 26.02.2024 18:39, Oleksii Kurochko wrote: > > --- /dev/null > > +++ b/docs/misc/riscv/booting.txt > > @@ -0,0 +1,8 @@ > > +System requirements > > +=================== > > + > > +The following extensions are expected to be supported by a system > > on which > > +Xen is run: > > +- Zihintpause: > > + On a system that doesn't have this extension, cpu_relax() should > > be > > + implemented properly. Otherwise, an illegal instruction > > exception will arise. > > This decision wants justifying in the (presently once again empty) > description. > > Furthermore - will there really be an illegal instruction exception > otherwise? > Isn't it the nature of hints that they are NOPs if not serving their > designated > purpose? You are right, they are NOPs, so I will drop the part about an illegal instruction exception. > > --- a/xen/arch/riscv/arch.mk > > +++ b/xen/arch/riscv/arch.mk > > @@ -5,6 +5,12 @@ $(call cc-options- > > add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) > > > > CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64 > > > > +ifeq ($(CONFIG_RISCV_64),y) > > +has_zihintpause = $(call as-insn,$(CC) -mabi=lp64 - > > march=rv64i_zihintpause, "pause",_zihintpause,) > > +else > > +has_zihintpause = $(call as-insn,$(CC) -mabi=ilp32 - > > march=rv32i_zihintpause, "pause",_zihintpause,) > > +endif > > Considering that down the road likely more such tests will want > adding, I think > this wants further abstracting for the rv32/rv64 difference (ideally > in a way > that wouldn't make future RV128 wrongly and silently take the RV32 > branch). > This would include eliminating the -mabi=lp64 redundancy with what's > visible in > context, perhaps by way of introducing a separate helper macro, e.g. > > riscv-abi-$(CONFIG_RISCV_32) := -mabi=ilp32 > riscv-abi-$(CONFIG_RISCV_64) := -mabi=lp64 > > I further see nothing wrong with also using $(riscv-march-y) here. > I.e. > overall > > _zihintpause := $(call as-insn,$(CC) $(riscv-abi-y) $(riscv-march- > y)_zihintpause,"pause",_zihintpause) > > (still with potential of abstracting further through another macro > such > that not every such construct would need to spell out the ABI and > arch > compiler options). > > Plus a macro named has_* imo can be expected to expand to y or n. I > would > suggest to simply drop the "has", thus ... > > > @@ -12,7 +18,7 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := > > $(riscv-march-y)c > > # 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 += -march=$(riscv-march-y)$(has_zihintpause) -mstrict-align > > -mcmodel=medany > > ... also making the use site look > > > --- a/xen/arch/riscv/include/asm/processor.h > > +++ b/xen/arch/riscv/include/asm/processor.h > > @@ -12,6 +12,9 @@ > > > > #ifndef __ASSEMBLY__ > > > > +/* TODO: need to be implemeted */ > > +#define smp_processor_id() 0 > > + > > /* On stack VCPU state */ > > struct cpu_user_regs > > { > > @@ -53,6 +56,26 @@ struct cpu_user_regs > > unsigned long pregs; > > }; > > > > +/* TODO: need to implement */ > > +#define cpu_to_core(cpu) (0) > > +#define cpu_to_socket(cpu) (0) > > Nit: Like above in smp_processor_id() no need for parentheses here. > > > +static inline void cpu_relax(void) > > +{ > > +#ifdef __riscv_zihintpause > > + /* > > + * Reduce instruction retirement. > > + * This assumes the PC changes. > > What is this 2nd sentence about? cpu_relax() function was copied from Linux kernel and this comment exists there, but I couldn't find in zihintpause spec how it affects PC /IP, so it seems to me it can be dropped. My guess that the 2nd sentece was added because of the following words from the spec: The PAUSE instruction is a HINT that indicates the current hart’s rate of instruction retirement should be temporarily reduced or paused. The duration of its effect must be bounded and may be zero. So it says reduced or pause, but still doesn't make sense as no matter how long pause takes to complete, it will still advance PC. > > > + */ > > + __asm__ __volatile__ ( "pause" ); > > +#else > > + /* Encoding of the pause instruction */ > > + __asm__ __volatile__ ( ".insn 0x100000F" ); > > May I ask that you spell out the leading zero here, to make clear > there > aren't, by mistake, one to few zeroes in the middle? I will add a leading zero. The encoding is correct, I've verified with disassembler: c: 0100000f pause ~ Oleksii
diff --git a/docs/misc/riscv/booting.txt b/docs/misc/riscv/booting.txt new file mode 100644 index 0000000000..38fad74956 --- /dev/null +++ b/docs/misc/riscv/booting.txt @@ -0,0 +1,8 @@ +System requirements +=================== + +The following extensions are expected to be supported by a system on which +Xen is run: +- Zihintpause: + On a system that doesn't have this extension, cpu_relax() should be + implemented properly. Otherwise, an illegal instruction exception will arise. diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk index 8403f96b6f..fabe323ec5 100644 --- a/xen/arch/riscv/arch.mk +++ b/xen/arch/riscv/arch.mk @@ -5,6 +5,12 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64 +ifeq ($(CONFIG_RISCV_64),y) +has_zihintpause = $(call as-insn,$(CC) -mabi=lp64 -march=rv64i_zihintpause, "pause",_zihintpause,) +else +has_zihintpause = $(call as-insn,$(CC) -mabi=ilp32 -march=rv32i_zihintpause, "pause",_zihintpause,) +endif + riscv-march-$(CONFIG_RISCV_ISA_RV64G) := rv64g riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c @@ -12,7 +18,7 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c # 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 += -march=$(riscv-march-y)$(has_zihintpause) -mstrict-align -mcmodel=medany # TODO: Drop override when more of the build is working override ALL_OBJS-y = arch/$(SRCARCH)/built_in.o diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h index 6db681d805..b96af07660 100644 --- a/xen/arch/riscv/include/asm/processor.h +++ b/xen/arch/riscv/include/asm/processor.h @@ -12,6 +12,9 @@ #ifndef __ASSEMBLY__ +/* TODO: need to be implemeted */ +#define smp_processor_id() 0 + /* On stack VCPU state */ struct cpu_user_regs { @@ -53,6 +56,26 @@ struct cpu_user_regs unsigned long pregs; }; +/* TODO: need to implement */ +#define cpu_to_core(cpu) (0) +#define cpu_to_socket(cpu) (0) + +static inline void cpu_relax(void) +{ +#ifdef __riscv_zihintpause + /* + * Reduce instruction retirement. + * This assumes the PC changes. + */ + __asm__ __volatile__ ( "pause" ); +#else + /* Encoding of the pause instruction */ + __asm__ __volatile__ ( ".insn 0x100000F" ); +#endif + + barrier(); +} + static inline void wfi(void) { __asm__ __volatile__ ("wfi");
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V5: - Code style fixes. - drop introduced TOOLCHAIN_HAS_ZIHINTPAUSE and use as-insn instead and use as-insn istead. --- Changes in V4: - Change message -> subject in "Changes in V3" - Documentation about system requirement was added. In the future, it can be checked if the extension is supported by system __riscv_isa_extension_available() ( https://gitlab.com/xen-project/people/olkur/xen/-/commit/737998e89ed305eb92059300c374dfa53d2143fa ) - update cpu_relax() function to check if __riscv_zihintpause is supported by a toolchain - add conditional _zihintpause to -march if it is supported by a toolchain Changes in V3: - update the commit subject - rename get_processor_id to smp_processor_id - code style fixes - update the cpu_relax instruction: use pause instruction instead of div %0, %0, zero --- Changes in V2: - Nothing changed. Only rebase. --- docs/misc/riscv/booting.txt | 8 ++++++++ xen/arch/riscv/arch.mk | 8 +++++++- xen/arch/riscv/include/asm/processor.h | 23 +++++++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 docs/misc/riscv/booting.txt