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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.04.2024 10:26, Oleksii wrote:
> 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.

Well, as per this v3 went in already. Hence ...

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

... if you now want to further update the logic, it'll be a new patch
anyway. The adjustments below look okay to me, but I'm not going to
insist at this point that this be further fiddled with.

Jan

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