Re: [PATCH v3] xen/riscv: check whether the assembler has Zbb extension support

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2024-04-22 at 17:41 +0200, Oleksii wrote:
> On Mon, 2024-04-22 at 11:43 +0200, Jan Beulich wrote:
> > On 19.04.2024 16:23, Oleksii Kurochko wrote:
> > > Update the argument of the as-insn for the Zbb case to verify
> > > that
> > > Zbb is supported not only by a compiler, but also by an
> > > assembler.
> > > 
> > > Also, check-extenstion(ext_name, "insn") helper macro is
> > > introduced
> > > to check whether extension is supported by a compiler and an
> > > assembler.
> > > 
> > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > 
> > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > despite ...
> > 
> > > --- a/xen/arch/riscv/arch.mk
> > > +++ b/xen/arch/riscv/arch.mk
> > > @@ -11,9 +11,14 @@ riscv-march-$(CONFIG_RISCV_ISA_C)       :=
> > > $(riscv-march-y)c
> > >  
> > >  riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y)
> > >  
> > > -zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb)
> > > -zihintpause := $(call as-insn, \
> > > -                      $(CC) $(riscv-generic-
> > > flags)_zihintpause,"pause",_zihintpause)
> > > +# check-extension: Check whether extenstion is supported by a
> > > compiler and
> > > +#                  an assembler.
> > > +# Usage: $(call check-extension,extension_name,"instr")
> > > +check-extension = $(call as-insn,$(CC) $(riscv-generic-
> > > flags)_$(1),$(2),_$(1))
> > > +
> > > +zbb-insn := "andn t0, t0, t0"
> > > +zbb := $(call check-extension,zbb,$(zbb-insn))
> > > +zihintpause := $(call check-extension,zihintpause,"pause")
> > 
> > ... still not really being happy with this: Either, as said before,
> > zbb-insn
> > would better be avoided (by using $(comma) as necessary), or you
> > should have
> > gone yet a step further to fully address my "redundancy" concern.
> > Note how
> > the two ultimate lines still have 3 (zbb) and 2 (zihintpause)
> > references
> > respectively, when the goal ought to be to have exactly one. E.g.
> > along the
> > lines of
> > 
> > $(call check-extension,zbb)
> > $(call check-extension,zihintpause)
> > 
> > suitably using $(eval ...) (to effect the variable definitions) and
> > defining
> > both zbb-insn and zihintpause-insn.
> > 
> > But I'll nevertheless put this in as is, unless you tell me you're
> > up
> > to
> > going the extra step.
> I am okay with all your suggestions. So the final version will look
> like ( if I understood you correctly ):
Jan,

Could you please review the changes below? I just want to ensure that
you are okay with them. If you are, I'll add your Acked-by that you
gave to this patch in previous answers.

Thanks in advance.

~ Oleksii
> diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk
> index dd242c91d1..f172187144 100644
> --- a/xen/arch/riscv/arch.mk
> +++ b/xen/arch/riscv/arch.mk
> @@ -13,12 +13,19 @@ riscv-generic-flags := $(riscv-abi-y) -
> march=$(riscv-march-y)
>  
>  # check-extension: Check whether extenstion is supported by a
> compiler
> and
>  #                  an assembler.
> -# Usage: $(call check-extension,extension_name,"instr")
> -check-extension = $(call as-insn,$(CC) $(riscv-generic-
> flags)_$(1),$(2),_$(1))
> +# Usage: $(call check-extension,extension_name).
> +#        it should be defined variable with name: extension-name :=
> "insn"
>  
> -zbb-insn := "andn t0, t0, t0"
> -zbb := $(call check-extension,zbb,$(zbb-insn))
> -zihintpause := $(call check-extension,zihintpause,"pause")
> +define check-extension =
> +$(eval $(1) := \
> +       $(call as-insn,$(CC) $(riscv-generic-flags)_$(1),$(value
> $(1)-
> insn),_$(1)))
> +endef
> +
> +zbb-insn := "andn t0$(comma)t0$(comma)t0"
> +$(call check-extension,zbb)
> +
> +zihintpause-insn := "pause"
> +$(call check-extension,zihintpause)
>  
>  extensions := $(zbb) $(zihintpause)
> 
> If the diff above looks good, I'll sent a new patch version.
> 
> Thanks.
> 
> ~ Oleksii