RISC-V does a conditional toolchain for the Zbb extension
(xen/arch/riscv/rules.mk), but unconditionally uses the
ANDN instruction in emulate_xchg_1_2().
Fixes: 51dabd6312c ("xen/riscv: introduce cmpxchg.h")
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V13:
- new patch
---
xen/arch/riscv/include/asm/cmpxchg.h | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/xen/arch/riscv/include/asm/cmpxchg.h b/xen/arch/riscv/include/asm/cmpxchg.h
index d5e678c036..12278be577 100644
--- 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
+ * 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"
+#endif
+
/*
* For LR and SC, the A extension requires that the address held in rs1 be
* naturally aligned to the size of the operand (i.e., eight-byte aligned
@@ -48,7 +62,7 @@
\
asm volatile ( \
"0: lr.w" lr_sfx " %[old], %[ptr_]\n" \
- " andn %[scratch], %[old], %[mask]\n" \
+ ANDN_INSN("%[scratch]", "%[old]", "%[mask]") \
" or %[scratch], %[scratch], %z[new_]\n" \
" sc.w" sc_sfx " %[scratch], %[scratch], %[ptr_]\n" \
" bnez %[scratch], 0b\n" \
--
2.45.2On 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? Plus, why would age of the tool chain matter? What you care about is whether you're permitted to use the extension at runtime. 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. Jan
On 25/06/2024 3:49 pm, 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? You deleted the commit message which explains why: > RISC-V does a conditional toolchain test for the Zbb extension > (xen/arch/riscv/rules.mk), but unconditionally uses the ANDN > instruction in emulate_xchg_1_2(). Either Zbb needs to be mandatory (both in the toolchain and the board running Xen), or emulate_xchg_1_2() needs to not use the ANDN instruction. I opted for the latter. ~Andrew
On 25.06.2024 23:10, Andrew Cooper wrote: > On 25/06/2024 3:49 pm, 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? > > You deleted the commit message which explains why: Not really. My whole point is that while the intention of the change looks okay, description and comment describe things insufficiently, to say the least. >> RISC-V does a conditional toolchain test for the Zbb extension >> (xen/arch/riscv/rules.mk), but unconditionally uses the ANDN >> instruction in emulate_xchg_1_2(). > > Either Zbb needs to be mandatory (both in the toolchain and the board > running Xen), or emulate_xchg_1_2() needs to not use the ANDN instruction. > > I opted for the latter. And I agree with that. Jan
On Wed, 2024-06-26 at 09:53 +0200, Jan Beulich wrote: > On 25.06.2024 23:10, Andrew Cooper wrote: > > On 25/06/2024 3:49 pm, 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? > > > > You deleted the commit message which explains why: > > Not really. My whole point is that while the intention of the change > looks > okay, description and comment describe things insufficiently, to say > the > least. > > > > RISC-V does a conditional toolchain test for the Zbb extension > > > (xen/arch/riscv/rules.mk), but unconditionally uses the ANDN > > > instruction in emulate_xchg_1_2(). > > > > Either Zbb needs to be mandatory (both in the toolchain and the > > board > > running Xen), or emulate_xchg_1_2() needs to not use the ANDN > > instruction. > > > > I opted for the latter. > > And I agree with that. Okay, then I'll update that in the next patch version. ~ Oleksii
On Tue, 2024-06-25 at 22:10 +0100, Andrew Cooper wrote: > On 25/06/2024 3:49 pm, 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? > > You deleted the commit message which explains why: > > > RISC-V does a conditional toolchain test for the Zbb extension > > (xen/arch/riscv/rules.mk), but unconditionally uses the ANDN > > instruction in emulate_xchg_1_2(). > > Either Zbb needs to be mandatory (both in the toolchain and the board > running Xen), or emulate_xchg_1_2() needs to not use the ANDN > instruction. But we can't go without Zbb there are some things in <xen/bitops.h> which now requires this extension. At the current state of development it is mandatory. ~ Oleksii > > I opted for the latter. > > ~Andrew
On 26.06.2024 09:13, Oleksii wrote: > On Tue, 2024-06-25 at 22:10 +0100, Andrew Cooper wrote: >> On 25/06/2024 3:49 pm, 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? >> >> You deleted the commit message which explains why: >> >>> RISC-V does a conditional toolchain test for the Zbb extension >>> (xen/arch/riscv/rules.mk), but unconditionally uses the ANDN >>> instruction in emulate_xchg_1_2(). >> >> Either Zbb needs to be mandatory (both in the toolchain and the board >> running Xen), or emulate_xchg_1_2() needs to not use the ANDN >> instruction. > But we can't go without Zbb there are some things in <xen/bitops.h> > which now requires this extension. At the current state of development > it is mandatory. Maybe that's the "another bug" that Andrew mentioned (without any details) on Matrix? Jan
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
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
© 2016 - 2024 Red Hat, Inc.