Re: [PATCH v13 09/10] xen/riscv: introduce ANDN_INSN

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.06.2024 20:26, Oleksii wrote:
> On Tue, 2024-06-25 at 16:49 +0200, Jan Beulich wrote:
>> On 25.06.2024 15:51, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/cmpxchg.h
>>> +++ b/xen/arch/riscv/include/asm/cmpxchg.h
>>> @@ -18,6 +18,20 @@
>>>          : "r" (new) \
>>>          : "memory" );
>>>  
>>> +/*
>>> + * Binutils < 2.37 doesn't understand ANDN.  If the toolchain is
>>> too
>>> +ld, form
>>
>> Same question: Why's 2.37 suddenly of interest?
> Andrew has (or had) an older version of binutils and started to face
> the issues mentioned in this patch. As a result, some
> changes were suggested.
> 
>> Plus, why would age of the
>> tool chain matter?
> As it is mentioned in the comment binutils < 2.37 doesn't understand
> andn instruction.

But that's not the point. If the tool chain is too old, our logic to
detect that should arrange for __riscv_zbb to not be set. That's all
that needed to cover gas not understanding the insn. The rest here
isn't about the capabilities of the tool chain: Either we make Zbb a
requirement (at which point .insn can be used to encode ANDN), or we
don't (at which point the replacement code you have comes into play).

>> What you care about is whether you're permitted to use
>> the extension at runtime. 
> At the moment we can't check that at runtime, w/o having an exception,
> ( there is some option to check when dts parsing will be available in
> Xen ). I will add the check when dts parsing functionality will be
> available. Right now the best what we can do it is just mentioned that
> as requirement in docs.
> 
>> Otherwise you could again ...
>>
>> Also something went wrong with line wrapping here.
>>
>>> + * it of a NOT+AND pair
>>> + */
>>> +#ifdef __riscv_zbb
>>> +#define ANDN_INSN(rd, rs1, rs2)                 \
>>> +    "andn " rd ", " rs1 ", " rs2 "\n"
>>> +#else
>>> +#define ANDN_INSN(rd, rs1, rs2)                 \
>>> +    "not " rd ", " rs2 "\n"                     \
>>> +    "and " rd ", " rs1 ", " rd "\n"
>>
>> ... resort to .insn.
> Hmm, good point, it could be an issue.
> 
> 
> If this patch is still needed (Andrew, have you updated your
> toolchain?), then it should use .insn instead of andn. However, using
> .insn requires encoding rd, rs1, and rs2 through asm ("some_reg") (?),
> which seems overly complicated.

Why? You don't want to use the raw form of .insn (which, as per the
other sub-thread on this series, is available from gas 2.38 only anyway),
but the one permitting operands to be spelled out (.insn r ...), along
the lines of what I suggested for "pause".

Jan