On 25.03.2025 14:02, Oleksii Kurochko wrote: > > On 3/25/25 12:52 PM, Jan Beulich wrote: >> On 25.03.2025 12:48, Oleksii Kurochko wrote: >>> On 3/24/25 1:31 PM, Jan Beulich wrote: >>>> On 21.03.2025 17:17, Oleksii Kurochko wrote: >>>>> H provides additional instructions and CSRs that control the new stage of >>>>> address translation and support hosting a guest OS in virtual S-mode >>>>> (VS-mode). >>>>> >>>>> According to the Unprivileged Architecture (version 20240411) >>>>> specification: >>>>> ``` >>>>> Table 74 summarizes the standardized extension names. The table also >>>>> defines >>>>> the canonical order in which extension names must appear in the name >>>>> string, >>>>> with top-to-bottom in table indicating first-to-last in the name string, >>>>> e.g., >>>>> RV32IMACV is legal, whereas RV32IMAVC is not. >>>>> ``` >>>>> According to Table 74, the h extension is placed last in the one-letter >>>>> extensions name part of the ISA string. >>>>> >>>>> `h` is a standalone extension based on the patch [1] but it wasn't so >>>>> before. >>>>> As the minimal supported GCC version to build Xen for RISC-V is 12.2.0, >>>>> and for that version it will be needed to encode H extensions instructions >>>>> explicitly by checking if __risv_h is defined. >>>> Leaving aside the typo, what is this about? There's no use of __riscv_h in >>>> the patch here, and ... >>> It is going to be used in future >>> patches:https://gitlab.com/xen-project/people/olkur/xen/-/blob/riscv-next-upstreaming/xen/arch/riscv/p2m.c?ref_type=heads#L32 >> For this and ... >> >>>>> @@ -25,10 +24,13 @@ $(eval $(1) := \ >>>>> $(call as-insn,$(CC) $(riscv-generic-flags)_$(1),$(value >>>>> $(1)-insn),_$(1))) >>>>> endef >>>>> >>>>> +h-insn := "hfence.gvma" >>>>> +$(call check-extension,h) >>>> ... this, if it fails, will not have any effect on the build right now >>>> afaics. >>> No, it won't have any affect now as instruction from H extension isn't used >>> now. >>> But it will be >>> neededforhttps://lore.kernel.org/xen-devel/dae753618491b2a6e42f7ed3f24190d0dc13fe3f.1740754166.git.slavisa.petro...@rt-rk.com/ >>> and for p2m changes mentioned above. >> ... this both being future work, it might help if it could be made clear >> right here how things are going to work (with both gcc12 and up-to-date >> gcc). > > I can update the commit message with the following: > ``` > If 'H' extension is supported by compiler then __riscv_h will be defined by > compiler (for gcc version >= 13.1). > For gcc12 it will be needed to: > #ifdef __riscv_h > asm volatile ("h extension instruction"); > #else > asm volatile ("|.insn ..."); #endif ```
Okay, that's what I was concerned about. __riscv_h is a compiler indication. It means nothing about H extension insns being supported by the assembler (except perhaps for Clang's integrated one). The check-extension thing in the Makefile will actually check both in one go. Yet then the hfence.* insns have been in binutils since 2.38, i.e. pre-dating gcc12. > Or probably it will be easier not to ifdef-ing > everything with __riscv_h but just return back a workaround with the > following changes: ``` $ git diff diff --git a/xen/arch/riscv/arch.mk > b/xen/arch/riscv/arch.mk index f29ad332c1..3bd64e7e51 100644 --- > a/xen/arch/riscv/arch.mk +++ b/xen/arch/riscv/arch.mk @@ -24,13 +24,17 > @@ $(eval $(1) := \ $(call as-insn,$(CC) > $(riscv-generic-flags)_$(1),$(value $(1)-insn),_$(1))) endef -h-insn := > "hfence.gvma" -$(call check-extension,h) + > +h-extension-name-$(CONFIG_CC_IS_GCC) := $(call cc-ifversion,-lt,1301, > hh, h) +h-extension-name-$(CONFIG_CC_IS_CLANG) := h + > +$(h-extension-name-y)-insn := "hfence.gvma" +$(call > check-extension,$(h-extension-name-y)) zihintpause-insn := "pause" > $(call check-extension,zihintpause) -extensions := $(h) $(zihintpause) > _zicsr_zifencei_zbb +extensions := $($(h-extension-name-y)) > $(zihintpause) _zicsr_zifencei_zbb extensions := $(subst > $(space),,$(extensions)) ``` I prefer more a little bit the second > option with having the workaround for GCC version. ~ Oleksii | I fear this ended up unreadable. Jan
