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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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.

> 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. And still, it is an open question if in
reality someone will use binutils which doesn't support andn
instruction...

~ Oleksii