Re: [PATCH v2] xen/riscv: add H extenstion to -march

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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 be neededforhttps://lore.kernel.org/xen-devel/dae753618491b2a6e42f7ed3f24190d0dc13fe3f.1740754166.git.Slavisa.Petrovic@xxxxxxxxx/
> > > > 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