| Message ID | 9efbc232f64b6192cf83f865b8987846fe082720.1707146506.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 05.02.2024 16:32, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/Kconfig > +++ b/xen/arch/riscv/Kconfig > @@ -45,6 +45,13 @@ config RISCV_ISA_C > > If unsure, say Y. > > +config TOOLCHAIN_HAS_ZIHINTPAUSE > + bool > + default y Shorter as "def_bool y". > + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zihintpause) > + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zihintpause) So for a reason I cannot really see -mabi= is indeed required here, or else the compiler sees an issue with the D extension. But enabling both M and A shouldn't really be needed in this check, as being unrelated? > + depends on LLD_VERSION >= 150000 || LD_VERSION >= 23600 What's the linker dependency here? Depending on the answer I might further ask why "TOOLCHAIN" when elsewhere we use CC_HAS_ or HAS_CC_ or HAS_AS_. That said, you may or may not be aware that personally I'm against encoding such in Kconfig, and my repeated attempts to get the respective discussion unstuck have not led anywhere. Therefore if you keep this, I'll be in trouble whether to actually ack the change as a whole. > --- 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 Like elsewhere, nit: Missing blanks immediately inside the parentheses. > + barrier(); It's probably okay to be separate, but I'd suggest folding this right into the asm()-s. Jan
On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote: > On 05.02.2024 16:32, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/Kconfig > > +++ b/xen/arch/riscv/Kconfig > > @@ -45,6 +45,13 @@ config RISCV_ISA_C > > > > If unsure, say Y. > > > > +config TOOLCHAIN_HAS_ZIHINTPAUSE > > + bool > > + default y > > Shorter as "def_bool y". > > > + depends on !64BIT || $(cc-option,-mabi=lp64 - > > march=rv64ima_zihintpause) > > + depends on !32BIT || $(cc-option,-mabi=ilp32 - > > march=rv32ima_zihintpause) > > So for a reason I cannot really see -mabi= is indeed required here, > or else the compiler sees an issue with the D extension. But enabling > both M and A shouldn't really be needed in this check, as being > unrelated? Agree, that M and A could be dropped. Regarding -mabi my guess is because D extension can be emulated by compiler, doesn't matter if D is set in -march. If it is set then hardware instruction will be used, otherwise emulated instruction will be used. And if D extenstion is always present it is need to know which ABI should be used. If D extenstion has h/w support then -mabi should be also update to lp64d instead of lp64. > > > + depends on LLD_VERSION >= 150000 || LD_VERSION >= 23600 > > What's the linker dependency here? Depending on the answer I might > further > ask why "TOOLCHAIN" when elsewhere we use CC_HAS_ or HAS_CC_ or > HAS_AS_. I missed to introduce {L}LLD_VERSION config. It should output from the command: riscv64-linux-gnu-ld --version > > That said, you may or may not be aware that personally I'm against > encoding such in Kconfig, and my repeated attempts to get the > respective > discussion unstuck have not led anywhere. Therefore if you keep this, > I'll > be in trouble whether to actually ack the change as a whole. Could I ask what is wrong with introduction of such things on KConfig? Would it be better to put everything in riscv/arch.mk? ~ Oleksii
On 15.02.2024 17:38, Oleksii wrote: > On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote: >> On 05.02.2024 16:32, Oleksii Kurochko wrote: >>> + depends on LLD_VERSION >= 150000 || LD_VERSION >= 23600 >> >> What's the linker dependency here? Depending on the answer I might >> further >> ask why "TOOLCHAIN" when elsewhere we use CC_HAS_ or HAS_CC_ or >> HAS_AS_. > I missed to introduce {L}LLD_VERSION config. It should output from the > command: > riscv64-linux-gnu-ld --version Doesn't answer my question though where the linker version matters here. >> That said, you may or may not be aware that personally I'm against >> encoding such in Kconfig, and my repeated attempts to get the >> respective >> discussion unstuck have not led anywhere. Therefore if you keep this, >> I'll >> be in trouble whether to actually ack the change as a whole. > Could I ask what is wrong with introduction of such things on KConfig? Just one of several possible pointers: https://lists.xen.org/archives/html/xen-devel/2022-09/msg01793.html > Would it be better to put everything in riscv/arch.mk? Or a mix of both, as per the proposal. Just to be clear, if I say "yes" to your question, someone else may come along and tell you to turn around again. Jan
On Thu, 2024-02-15 at 17:43 +0100, Jan Beulich wrote: > On 15.02.2024 17:38, Oleksii wrote: > > On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote: > > > On 05.02.2024 16:32, Oleksii Kurochko wrote: > > > > + depends on LLD_VERSION >= 150000 || LD_VERSION >= > > > > 23600 > > > > > > What's the linker dependency here? Depending on the answer I > > > might > > > further > > > ask why "TOOLCHAIN" when elsewhere we use CC_HAS_ or HAS_CC_ or > > > HAS_AS_. > > I missed to introduce {L}LLD_VERSION config. It should output from > > the > > command: > > riscv64-linux-gnu-ld --version > > Doesn't answer my question though where the linker version matters > here. Then I misinterpreted your initial question. Could you please provide further clarification or rephrase it for better understanding? Probably, your question was about why linker dependency is needed here, then it is not sufficient to check if a toolchain supports a particular extension without checking if the linker supports that extension too. For example, Clang 15 supports Zihintpause but GNU bintutils 2.35.2 does not, leading build errors like so: riscv64-linux-gnu-ld: -march=rv64i_zihintpause2p0: Invalid or unknown z ISA extension: 'zihintpause' ~ Oleksii
On 16.02.2024 12:16, Oleksii wrote: > On Thu, 2024-02-15 at 17:43 +0100, Jan Beulich wrote: >> On 15.02.2024 17:38, Oleksii wrote: >>> On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote: >>>> On 05.02.2024 16:32, Oleksii Kurochko wrote: >>>>> + depends on LLD_VERSION >= 150000 || LD_VERSION >= >>>>> 23600 >>>> >>>> What's the linker dependency here? Depending on the answer I >>>> might >>>> further >>>> ask why "TOOLCHAIN" when elsewhere we use CC_HAS_ or HAS_CC_ or >>>> HAS_AS_. >>> I missed to introduce {L}LLD_VERSION config. It should output from >>> the >>> command: >>> riscv64-linux-gnu-ld --version >> >> Doesn't answer my question though where the linker version matters >> here. > Then I misinterpreted your initial question. > Could you please provide further clarification or rephrase it for > better understanding? > > Probably, your question was about why linker dependency is needed here, > then > it is not sufficient to check if a toolchain supports a particular > extension without checking if the linker supports that extension > too. > For example, Clang 15 supports Zihintpause but GNU bintutils > 2.35.2 does not, leading build errors like so: > > riscv64-linux-gnu-ld: -march=rv64i_zihintpause2p0: Invalid or > unknown z ISA extension: 'zihintpause' Hmm, that's certainly "interesting" behavior of the RISC-V linker. Yet isn't the linker capability expected to be tied to that of gas? I would find it far more natural if a gas dependency existed here. If such a connection cannot be taken for granted, I'm pretty sure you'd need to probe both then anyway. Jan
On Mon, 2024-02-19 at 09:06 +0100, Jan Beulich wrote: > On 16.02.2024 12:16, Oleksii wrote: > > On Thu, 2024-02-15 at 17:43 +0100, Jan Beulich wrote: > > > On 15.02.2024 17:38, Oleksii wrote: > > > > On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote: > > > > > On 05.02.2024 16:32, Oleksii Kurochko wrote: > > > > > > + depends on LLD_VERSION >= 150000 || LD_VERSION >= > > > > > > 23600 > > > > > > > > > > What's the linker dependency here? Depending on the answer I > > > > > might > > > > > further > > > > > ask why "TOOLCHAIN" when elsewhere we use CC_HAS_ or HAS_CC_ > > > > > or > > > > > HAS_AS_. > > > > I missed to introduce {L}LLD_VERSION config. It should output > > > > from > > > > the > > > > command: > > > > riscv64-linux-gnu-ld --version > > > > > > Doesn't answer my question though where the linker version > > > matters > > > here. > > Then I misinterpreted your initial question. > > Could you please provide further clarification or rephrase it for > > better understanding? > > > > Probably, your question was about why linker dependency is needed > > here, > > then > > it is not sufficient to check if a toolchain supports a particular > > extension without checking if the linker supports that extension > > too. > > For example, Clang 15 supports Zihintpause but GNU bintutils > > 2.35.2 does not, leading build errors like so: > > > > riscv64-linux-gnu-ld: -march=rv64i_zihintpause2p0: Invalid or > > unknown z ISA extension: 'zihintpause' > > Hmm, that's certainly "interesting" behavior of the RISC-V linker. > Yet > isn't the linker capability expected to be tied to that of gas? I > would > find it far more natural if a gas dependency existed here. If such a > connection cannot be taken for granted, I'm pretty sure you'd need to > probe both then anyway. Wouldn't it be enough in this case instead of introducing of new configs and etc, just to do the following: +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 -riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march- y)_zihintpause # 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 += -march=$(riscv-march-y)$(has_zihintpause) -mstrict-align - mcmodel=medany Probably, it would be better: ... +CFLAGS += -march=$(riscv-march-y)$(call or,$(has_zihintpause)) - mstrict-align - mcmodel=medany ~ Oleksii
On 23.02.2024 18:00, Oleksii wrote: > On Mon, 2024-02-19 at 09:06 +0100, Jan Beulich wrote: >> On 16.02.2024 12:16, Oleksii wrote: >>> On Thu, 2024-02-15 at 17:43 +0100, Jan Beulich wrote: >>>> On 15.02.2024 17:38, Oleksii wrote: >>>>> On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote: >>>>>> On 05.02.2024 16:32, Oleksii Kurochko wrote: >>>>>>> + depends on LLD_VERSION >= 150000 || LD_VERSION >= >>>>>>> 23600 >>>>>> >>>>>> What's the linker dependency here? Depending on the answer I >>>>>> might >>>>>> further >>>>>> ask why "TOOLCHAIN" when elsewhere we use CC_HAS_ or HAS_CC_ >>>>>> or >>>>>> HAS_AS_. >>>>> I missed to introduce {L}LLD_VERSION config. It should output >>>>> from >>>>> the >>>>> command: >>>>> riscv64-linux-gnu-ld --version >>>> >>>> Doesn't answer my question though where the linker version >>>> matters >>>> here. >>> Then I misinterpreted your initial question. >>> Could you please provide further clarification or rephrase it for >>> better understanding? >>> >>> Probably, your question was about why linker dependency is needed >>> here, >>> then >>> it is not sufficient to check if a toolchain supports a particular >>> extension without checking if the linker supports that extension >>> too. >>> For example, Clang 15 supports Zihintpause but GNU bintutils >>> 2.35.2 does not, leading build errors like so: >>> >>> riscv64-linux-gnu-ld: -march=rv64i_zihintpause2p0: Invalid or >>> unknown z ISA extension: 'zihintpause' >> >> Hmm, that's certainly "interesting" behavior of the RISC-V linker. >> Yet >> isn't the linker capability expected to be tied to that of gas? I >> would >> find it far more natural if a gas dependency existed here. If such a >> connection cannot be taken for granted, I'm pretty sure you'd need to >> probe both then anyway. > > Wouldn't it be enough in this case instead of introducing of new > configs and etc, just to do the following: > +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 > -riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march- > y)_zihintpause > > # 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 += -march=$(riscv-march-y)$(has_zihintpause) -mstrict-align > - > mcmodel=medany Yes, this is kind of what I'd expect. > Probably, it would be better: > ... > +CFLAGS += -march=$(riscv-march-y)$(call or,$(has_zihintpause)) - > mstrict-align - > mcmodel=medany Why the use of "or"? IOW right now I don't see what's "better" here. Jan
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/Kconfig b/xen/arch/riscv/Kconfig index f382b36f6c..383ce06771 100644 --- a/xen/arch/riscv/Kconfig +++ b/xen/arch/riscv/Kconfig @@ -45,6 +45,13 @@ config RISCV_ISA_C If unsure, say Y. +config TOOLCHAIN_HAS_ZIHINTPAUSE + bool + default y + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zihintpause) + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zihintpause) + depends on LLD_VERSION >= 150000 || LD_VERSION >= 23600 + endmenu source "common/Kconfig" diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk index 8403f96b6f..a4b53adaf7 100644 --- a/xen/arch/riscv/arch.mk +++ b/xen/arch/riscv/arch.mk @@ -7,6 +7,7 @@ CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64 riscv-march-$(CONFIG_RISCV_ISA_RV64G) := rv64g riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause # Note that -mcmodel=medany is used so that Xen can be mapped # into the upper half _or_ the lower half of the address space. diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h index 6db681d805..289dc35ea0 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 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/Kconfig | 7 +++++++ xen/arch/riscv/arch.mk | 1 + xen/arch/riscv/include/asm/processor.h | 23 +++++++++++++++++++++++ 4 files changed, 39 insertions(+) create mode 100644 docs/misc/riscv/booting.txt