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

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

Posted by Oleksii Kurochko 3 weeks, 2 days ago

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@gmail.com>
---
 xen/arch/riscv/arch.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk
index 53f3575e7d..6c53953acb 100644
--- 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"
+zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,${zbb_insn},_zbb)
 zihintpause := $(call as-insn, \
                       $(CC) $(riscv-generic-flags)_zihintpause,"pause",_zihintpause)
 
-- 
2.44.0

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

Posted by Jan Beulich 1 week, 6 days ago

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@gmail.com>

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

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.

Jan

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

Posted by Oleksii 1 week, 6 days ago

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@gmail.com>
> 
> 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

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

Posted by Jan Beulich 1 week, 6 days ago

On 18.04.2024 15:09, Oleksii wrote:
> 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@gmail.com>
>>
>> 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.

Well, as said - this is perhaps largely personal taste. Yet technically
it isn't needed, assuming you're aware of the "comma" and "space" macros
that we have at the top of ./Config.mk. You'll find $(comma) used for
similar purposes in x86.

Jan