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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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 ):
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