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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-04-18 at 12:00 +0200, Jan Beulich wrote:
> On 09.04.2024 10:00, 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.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> While technically all if fine here, I'm afraid I have a couple of
> nits:
> 
> > --- a/xen/arch/riscv/arch.mk
> > +++ b/xen/arch/riscv/arch.mk
> > @@ -11,7 +11,8 @@ 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)
> > +zbb_insn := "andn t0, t0, t0"
> 
> As can be seen on the following line (as-insn, riscv-generic-flags)
> we
> prefer dashes over underscores in new variables' names. (Another
> question is
> whether the variable is needed in the first place, but that's pretty
> surely
> personal taste territory.)

It seems to me that we need it; otherwise, if we don't use the variable
and put everything directly:
  zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"andn t0, t0,
t0",_zbb)
Then as-insn will receive incorrect arguments because of the ',' used
in the instruction. It will parse it as 3 separete arguments ("and, t0
and t0"), which will lead to a compilation error:
   /bin/sh: -c: line 1: unexpected EOF while looking for matching `''
   /bin/sh: -c: line 2: syntax error: unexpected end of file

Probably I am missing something and it can be done in a different way.

> 
> Furthermore this extra variable suggests there's yet more room for
> abstraction (as already suggested before).
> 
> > +zbb := $(call as-insn,$(CC) $(riscv-generic-
> > flags)_zbb,${zbb_insn},_zbb)
> 
> Why figure braces in one case when everywhere else we use parentheses
> for
> variable references? There's no functional difference sure, but
> inconsistent
> use specifically may raise the question for some future reader
> whether there
> actually is one.
I see the usage of {} somewhere else in code base, so automatically use
them here too. Sure, it should be consistent. Thanks.

~ Oleksii