[PATCH v3 2/2] rseq/selftests: Add support for RISC-V

[Vincent Chen]

On Thu, Mar 3, 2022 at 12:38 AM Mathieu Desnoyers
<mathieu.desnoyers at efficios.com> wrote:
>
> ----- On Mar 1, 2022, at 9:30 PM, Vincent Chen vincent.chen at sifive.com wrote:
>
> > Add support for RISC-V in the rseq selftests, which covers both
> > 64-bit and 32-bit ISA with little endian mode.
> >
> > Signed-off-by: Vincent Chen <vincent.chen at sifive.com>
>
> If you also ran those tests on riscv, can you state so with a "Tested-by" ?
>
Yes, I ran all the tests under the selftests/rseq folder. Maybe I can
ask my colleagues to give it a test as well and give me a "Tested-by".

> Small nits below,
>
> > ---
> > tools/testing/selftests/rseq/param_test.c |  23 +
> > tools/testing/selftests/rseq/rseq-riscv.h | 676 ++++++++++++++++++++++
> > tools/testing/selftests/rseq/rseq.h       |   2 +
> > 3 files changed, 701 insertions(+)
> > create mode 100644 tools/testing/selftests/rseq/rseq-riscv.h
> >
> > diff --git a/tools/testing/selftests/rseq/param_test.c
> > b/tools/testing/selftests/rseq/param_test.c
> > index 699ad5f93c34..0a6b8eafd444 100644
> > --- a/tools/testing/selftests/rseq/param_test.c
> > +++ b/tools/testing/selftests/rseq/param_test.c
> > @@ -207,6 +207,29 @@ unsigned int yield_mod_cnt, nr_abort;
> >       "addiu " INJECT_ASM_REG ", -1\n\t" \
> >       "bnez " INJECT_ASM_REG ", 222b\n\t" \
> >       "333:\n\t"
> > +#elif defined(__riscv)
> > +
> > +#define RSEQ_INJECT_INPUT \
> > +     , [loop_cnt_1]"m"(loop_cnt[1]) \
> > +     , [loop_cnt_2]"m"(loop_cnt[2]) \
> > +     , [loop_cnt_3]"m"(loop_cnt[3]) \
> > +     , [loop_cnt_4]"m"(loop_cnt[4]) \
> > +     , [loop_cnt_5]"m"(loop_cnt[5]) \
> > +     , [loop_cnt_6]"m"(loop_cnt[6])
> > +
> > +#define INJECT_ASM_REG       "t1"
> > +
> > +#define RSEQ_INJECT_CLOBBER \
> > +     , INJECT_ASM_REG
> > +
> > +#define RSEQ_INJECT_ASM(n)                                   \
> > +     "lw " INJECT_ASM_REG ", %[loop_cnt_" #n "]\n\t"         \
> > +     "beqz " INJECT_ASM_REG ", 333f\n\t"                     \
> > +     "222:\n\t"                                              \
> > +     "addi  " INJECT_ASM_REG "," INJECT_ASM_REG ", -1\n\t"   \
> > +     "bnez " INJECT_ASM_REG ", 222b\n\t"                     \
> > +     "333:\n\t"
> > +
> >
> > #else
> > #error unsupported target
> > diff --git a/tools/testing/selftests/rseq/rseq-riscv.h
> > b/tools/testing/selftests/rseq/rseq-riscv.h
> > new file mode 100644
> > index 000000000000..845ec7d0f2ed
> > --- /dev/null
> > +++ b/tools/testing/selftests/rseq/rseq-riscv.h
> > @@ -0,0 +1,676 @@
> > +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
> > +/*
> > + * Select the instruction "csrw mhartid, x0" as the RSEQ_SIG. Unlike
> > + * other architecture, the ebreak instruction has no immediate field for
>
> architecture -> architectures

Thanks. I will correct it in my next version patch.
>
> > + * distinguishing purposes. Hence, ebreak is not suitable as RSEQ_SIG.
> > + * "csrw mhartid, x0" can also satisfy the RSEQ requirement because it
> > + * is an uncommon instruction and will raise an illegal instruction
> > + * exception when executed in all modes.
> > + */
> > +
> > +#if __ORDER_LITTLE_ENDIAN__ == 1234
>
> I think we'll want to standardize on this for endianness checking (same as
> the updated uapi rseq.h):
>
> #if defined(__BYTE_ORDER) ? (__BYTE_ORDER == __LITTLE_ENDIAN) : defined(__LITTLE_ENDIAN)
>
> We may have to change rseq-mips.h in the rseq selftests to do the same as well rather than
> using "# ifdef __BIG_ENDIAN".
>

OK, I can follow it. However, I found the endianness checking in
include/uapi/linux/rseq.h is
#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) ||
defined(__BIG_ENDIAN)

It is a little different than what you mentioned early. Should I
follow the format in include/uapi/linux/rseq.h? or both formats are
OK?

Thanks,
Vincent

> > +#define RSEQ_SIG   0xf1401073  /* csrr mhartid, x0 */
> > +#else
> > +#error "Currently, RSEQ only supports Little-Endian version"
> > +#endif
> > +
>
> [...]
>
> Thanks,
>
> Mathieu
>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com