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
