| Message ID | 07c628e8552f6e31a07e4261b273553cb4a3669b.1719917348.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 02.07.2024 13:01, Oleksii Kurochko wrote: > 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> > Suggested-By: Jan Beulich <jbeulich@suse.com> Hmm, meant to say that for the earlier patch already: A Suggested-by: after an S-o-b is somewhat odd. You can't have signed off a change before it was suggested, as you can't possibly have written it then. Otoh I'm not sure tagging me is appropriate here anyway. > --- a/xen/arch/riscv/include/asm/cmpxchg.h > +++ b/xen/arch/riscv/include/asm/cmpxchg.h > @@ -18,6 +18,19 @@ > : "r" (new) \ > : "memory" ); > > +/* > + * To not face an issue that gas doesn't understand ANDN instruction > + * it is encoded using .insn directive. > + */ > +#ifdef __riscv_zbb > +#define ANDN_INSN(rd, rs1, rs2) \ > + ".insn r 0x33, 0x7, 0x20, " rd ", " rs1 ", " rs2 "\n" Why 0x33? Just like in the other patch you used MISC_MEM (as suggested, for being more descriptive), you want to use OP here. Then Reviewed-by: Jan Beulich <jbeulich@suse.com> (and I'd certainly be okay making the change while committing) Jan
On Tue, 2024-07-02 at 14:08 +0200, Jan Beulich wrote: > > --- a/xen/arch/riscv/include/asm/cmpxchg.h > > +++ b/xen/arch/riscv/include/asm/cmpxchg.h > > @@ -18,6 +18,19 @@ > > : "r" (new) \ > > : "memory" ); > > > > +/* > > + * To not face an issue that gas doesn't understand ANDN > > instruction > > + * it is encoded using .insn directive. > > + */ > > +#ifdef __riscv_zbb > > +#define ANDN_INSN(rd, rs1, rs2) \ > > + ".insn r 0x33, 0x7, 0x20, " rd ", " rs1 ", " rs2 "\n" > > Why 0x33? Just like in the other patch you used MISC_MEM (as > suggested, > for being more descriptive), you want to use OP here. Then > Reviewed-by: Jan Beulich <jbeulich@suse.com> > (and I'd certainly be okay making the change while committing) According to Volume 1, Unprivileged Specification version 20240411 the andn instruction looks like: 31 25 24 20 19 15 14 12 11 7 6 0 0 1 0 0 0 0 0 rs2 rs1 1 1 1 rd 0 1 1 0 0 1 1 OP This instruction is R-type. According to the gcc manual ( https://sourceware.org/binutils/docs/as/RISC_002dV_002dFormats.html ): R type: .insn r opcode6, func3, func7, rd, rs1, rs2 +-------+-----+-----+-------+----+---------+ | func7 | rs2 | rs1 | func3 | rd | opcode6 | +-------+-----+-----+-------+----+---------+ 31 25 20 15 12 7 0 so opcode6 in case of andn is 0x33 (0110011) ~ Oleksii
On 02.07.2024 17:51, Oleksii wrote: > On Tue, 2024-07-02 at 14:08 +0200, Jan Beulich wrote: >>> --- a/xen/arch/riscv/include/asm/cmpxchg.h >>> +++ b/xen/arch/riscv/include/asm/cmpxchg.h >>> @@ -18,6 +18,19 @@ >>> : "r" (new) \ >>> : "memory" ); >>> >>> +/* >>> + * To not face an issue that gas doesn't understand ANDN >>> instruction >>> + * it is encoded using .insn directive. >>> + */ >>> +#ifdef __riscv_zbb >>> +#define ANDN_INSN(rd, rs1, rs2) \ >>> + ".insn r 0x33, 0x7, 0x20, " rd ", " rs1 ", " rs2 "\n" >> >> Why 0x33? Just like in the other patch you used MISC_MEM (as >> suggested, >> for being more descriptive), you want to use OP here. Then >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> (and I'd certainly be okay making the change while committing) > > According to Volume 1, Unprivileged Specification version 20240411 the > andn instruction looks > like: > 31 25 24 20 19 15 14 12 11 7 6 0 > 0 1 0 0 0 0 0 rs2 rs1 1 1 1 rd 0 1 1 0 0 1 1 > OP > > This instruction is R-type. According to the gcc manual > ( https://sourceware.org/binutils/docs/as/RISC_002dV_002dFormats.html > ): > R type: .insn r opcode6, func3, func7, rd, rs1, rs2 > +-------+-----+-----+-------+----+---------+ > | func7 | rs2 | rs1 | func3 | rd | opcode6 | > +-------+-----+-----+-------+----+---------+ > 31 25 20 15 12 7 0 > > so opcode6 in case of andn is 0x33 (0110011) Right. And "OP" is the identifier you can use with .insn to get 0x33 without needing to write 0x33. As said - the requested replacement is solely to help readability a little. Jan
diff --git a/xen/arch/riscv/include/asm/cmpxchg.h b/xen/arch/riscv/include/asm/cmpxchg.h index d5e678c036..34d5bd48b3 100644 --- a/xen/arch/riscv/include/asm/cmpxchg.h +++ b/xen/arch/riscv/include/asm/cmpxchg.h @@ -18,6 +18,19 @@ : "r" (new) \ : "memory" ); +/* + * To not face an issue that gas doesn't understand ANDN instruction + * it is encoded using .insn directive. + */ +#ifdef __riscv_zbb +#define ANDN_INSN(rd, rs1, rs2) \ + ".insn r 0x33, 0x7, 0x20, " 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 +61,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" \
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> Suggested-By: Jan Beulich <jbeulich@suse.com> --- Andrew, Could you please take a look if you are okay with suggested changes. Thanks in advance. --- Changes in V14: - update the commit on top of ANDN definition. - use .insn instead of andn to support gas which doesn't understand andn instruction. - add Suggested-By: Jan Beulich <jbeulich@suse.com> --- Changes in V13: - new patch --- xen/arch/riscv/include/asm/cmpxchg.h | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)