| Message ID | 0a072ab36b54ea7c4f9a6f94465fb7b79f9f49b2.1742573085.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xen/riscv: add H extenstion to -march | expand |
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 ... > @@ -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. Jan
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 >> @@ -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 needed forhttps://lore.kernel.org/xen-devel/dae753618491b2a6e42f7ed3f24190d0dc13fe3f.1740754166.git.Slavisa.Petrovic@rt-rk.com/ and for p2m changes mentioned above. ~ Oleksii
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 needed forhttps://lore.kernel.org/xen-devel/dae753618491b2a6e42f7ed3f24190d0dc13fe3f.1740754166.git.Slavisa.Petrovic@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). Jan
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.Petrovic@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 ``` 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 |
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.Petrovic@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
On 3/25/25 2:47 PM, Jan Beulich wrote: > 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 beneededforhttps://lore.kernel.org/xen-devel/dae753618491b2a6e42f7ed3f24190d0dc13fe3f.1740754166.git.Slavisa.Petrovic@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. It is still needed to have or #ifdef-ing or workaround mentioned below ... > >> 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. ... something happen with formatting: 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)) ~ Oleksii
On 25.03.2025 15:46, Oleksii Kurochko wrote: > > On 3/25/25 2:47 PM, Jan Beulich wrote: >> 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 beneededforhttps://lore.kernel.org/xen-devel/dae753618491b2a6e42f7ed3f24190d0dc13fe3f.1740754166.git.Slavisa.Petrovic@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. > > It is still needed to have or #ifdef-ing or workaround mentioned below ... > >> >>> 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. > > ... something happen with formatting: > > 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) It all else fails, that's an option. The downside is that such version checks break if someone backports the respective change to an older version. Probing support for the actual command line option would be better. Why would $(call check-extension,hh) $(call check-extension,h) not work, btw? (Of course, if the above was an option, something slightly smarter may still want using, so that e.g. we avoid probing both in the more common case [going forward] that "h" is what is supported.) Jan > +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)) > > ~ Oleksii >
On 3/25/25 5:26 PM, Jan Beulich wrote: > On 25.03.2025 15:46, Oleksii Kurochko wrote: >> On 3/25/25 2:47 PM, Jan Beulich wrote: >>> 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 willbeneededforhttps://lore.kernel.org/xen-devel/dae753618491b2a6e42f7ed3f24190d0dc13fe3f.1740754166.git.Slavisa.Petrovic@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. >> It is still needed to have or #ifdef-ing or workaround mentioned below ... >> >>>> 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. >> ... something happen with formatting: >> >> 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) > It all else fails, that's an option. The downside is that such version checks > break if someone backports the respective change to an older version. Probing > support for the actual command line option would be better. Why would > > $(call check-extension,hh) > $(call check-extension,h) > > not work, btw? (Of course, if the above was an option, something slightly > smarter may still want using, so that e.g. we avoid probing both in the more > common case [going forward] that "h" is what is supported.) It will work, I just decided to do in the way mentioned above because clang doesn't have this issue with having single 'h' at all so for clang it isn't needed to have $(call check-extension, hh) at all. I have similar patch somewhere. I will re-apply it. Thanks. ~ Oleksii >> +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)) >> >> ~ Oleksii >>
diff --git a/docs/misc/riscv/booting.txt b/docs/misc/riscv/booting.txt index cb4d79f12c..3a8474a27d 100644 --- a/docs/misc/riscv/booting.txt +++ b/docs/misc/riscv/booting.txt @@ -3,6 +3,10 @@ System requirements The following extensions are expected to be supported by a system on which Xen is run: +- 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). - 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, diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk index 236ea7c8a6..f29ad332c1 100644 --- a/xen/arch/riscv/arch.mk +++ b/xen/arch/riscv/arch.mk @@ -9,7 +9,6 @@ riscv-abi-$(CONFIG_RISCV_64) := -mabi=lp64 riscv-march-$(CONFIG_RISCV_64) := rv64 riscv-march-y += ima riscv-march-$(CONFIG_RISCV_ISA_C) += c -riscv-march-y += _zicsr_zifencei_zbb riscv-generic-flags := $(riscv-abi-y) -march=$(subst $(space),,$(riscv-march-y)) @@ -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) + zihintpause-insn := "pause" $(call check-extension,zihintpause) -extensions := $(zihintpause) +extensions := $(h) $(zihintpause) _zicsr_zifencei_zbb extensions := $(subst $(space),,$(extensions)) diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c index bf09aa1170..5aafab0f49 100644 --- a/xen/arch/riscv/cpufeature.c +++ b/xen/arch/riscv/cpufeature.c @@ -146,6 +146,7 @@ static const struct riscv_isa_ext_data __initconst required_extensions[] = { #ifdef CONFIG_RISCV_ISA_C RISCV_ISA_EXT_DATA(c), #endif + RISCV_ISA_EXT_DATA(h), RISCV_ISA_EXT_DATA(zicsr), RISCV_ISA_EXT_DATA(zifencei), RISCV_ISA_EXT_DATA(zihintpause),
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. [1] https://github.com/gcc-mirror/gcc/commit/0cd11d301013af50a3fae0694c909952e94e20d5#diff-d6f7db0db31bfb339b01bec450f1b905381eb4730cc5ab2b2794971e34647d64R148 Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in v2: - Update the commit message. - Use check-extension macros to verify that 'H' extension is available. Also it works for clang. I verified this by compiling Xen with Clang-17 (the minimal necessary version for Xen, as RISC-V64 support for Clang starts from 17.0.0: f873029386("[BOLT] Add minimal RISC-V 64-bit support")). The changes can be found here: https://paste.vates.tech/?015af79b1e7413e6#3fsRb4QbjYDPseK7FU8wbaCWbsuu8xhANUmuChCfDoFD --- docs/misc/riscv/booting.txt | 4 ++++ xen/arch/riscv/arch.mk | 6 ++++-- xen/arch/riscv/cpufeature.c | 1 + 3 files changed, 9 insertions(+), 2 deletions(-)