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

From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: Vincent Chen <vincent.chen@sifive.com>,
	 Peter Zijlstra <peterz@infradead.org>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	 linux-riscv <linux-riscv@lists.infradead.org>,
	 Paul Walmsley <paul.walmsley@sifive.com>
Subject: Re: [PATCH v3 2/2] rseq/selftests: Add support for RISC-V
Date: Wed, 2 Mar 2022 11:38:26 -0500 (EST)	[thread overview]
Message-ID: <529535828.117301.1646239106382.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <20220302023048.6140-3-vincent.chen@sifive.com>

----- On Mar 1, 2022, at 9:30 PM, Vincent Chen vincent.chen@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@sifive.com>

If you also ran those tests on riscv, can you state so with 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

> + * 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".

> +#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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv