Message ID | 20230904170220.167816-14-ajones@ventanamicro.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | RISC-V: Enable cbo.zero in usermode | expand |
Hi, > -----Original Message----- > From: linux-riscv <linux-riscv-bounces@lists.infradead.org> On Behalf Of > Andrew Jones > Sent: Tuesday, September 5, 2023 1:02 AM > To: linux-riscv@lists.infradead.org > Cc: paul.walmsley@sifive.com; palmer@dabbelt.com; > aou@eecs.berkeley.edu; evan@rivosinc.com; conor.dooley@microchip.com; > apatel@ventanamicro.com > Subject: [PATCH v3 6/6] RISC-V: selftests: Add CBO tests > > Add hwprobe test for Zicboz and its block size. Also, when Zicboz is > present, test that cbo.zero may be issued and works. Additionally > test that the Zicbom instructions cause SIGILL and also that cbo.zero > causes SIGILL when Zicboz is not present. Pinning the test to a subset > of cpus with taskset will also restrict the hwprobe calls to that set. > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > .../testing/selftests/riscv/hwprobe/Makefile | 7 +- > tools/testing/selftests/riscv/hwprobe/cbo.c | 170 ++++++++++++++++++ > .../testing/selftests/riscv/hwprobe/hwprobe.c | 12 +- > .../testing/selftests/riscv/hwprobe/hwprobe.h | 15 ++ > 4 files changed, 192 insertions(+), 12 deletions(-) > create mode 100644 tools/testing/selftests/riscv/hwprobe/cbo.c > create mode 100644 tools/testing/selftests/riscv/hwprobe/hwprobe.h > > diff --git a/tools/testing/selftests/riscv/hwprobe/Makefile > b/tools/testing/selftests/riscv/hwprobe/Makefile > index 5f614c3ba598..f224b84591fb 100644 > --- a/tools/testing/selftests/riscv/hwprobe/Makefile > +++ b/tools/testing/selftests/riscv/hwprobe/Makefile > @@ -2,9 +2,14 @@ > # Copyright (C) 2021 ARM Limited > # Originally tools/testing/arm64/abi/Makefile > > -TEST_GEN_PROGS := hwprobe > +CFLAGS += -I$(top_srcdir)/tools/include > + > +TEST_GEN_PROGS := hwprobe cbo > > include ../../lib.mk > > $(OUTPUT)/hwprobe: hwprobe.c sys_hwprobe.S > $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ > + > +$(OUTPUT)/cbo: cbo.c sys_hwprobe.S > + $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ > diff --git a/tools/testing/selftests/riscv/hwprobe/cbo.c > b/tools/testing/selftests/riscv/hwprobe/cbo.c > new file mode 100644 > index 000000000000..50e85f31a2c7 > --- /dev/null > +++ b/tools/testing/selftests/riscv/hwprobe/cbo.c > @@ -0,0 +1,170 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2023 Ventana Micro Systems Inc. > + * > + * Run with 'taskset -c <cpu-list> cbo' to only execute hwprobe on a > + * subset of cpus, as well as only executing the tests on those cpus. > + */ Patch 3/6 exposes a common subset extensions of the cpu list to user space, so, if some of the cores support ZICBOZ while other don't, hwprobe() won't expose ZICBOZ ext, but in this case the cbo_zero insn might run w/o illegal insn exception when the task is scheduled to run on a core that support ZICBOZ. The test result may vary for each run for this case. Maybe we can add some comment for this special test case? > +#define _GNU_SOURCE > +#include <stdbool.h> > +#include <stdint.h> > +#include <sched.h> > +#include <signal.h> > +#include <assert.h> > +#include <linux/compiler.h> > +#include <asm/ucontext.h> > + > +#include "hwprobe.h" > +#include "../../kselftest.h" > + > +#define MK_CBO(fn) ((fn) << 20 | 10 << 15 | 2 << 12 | 0 << 7 | 15) > + > +static char mem[4096] __aligned(4096) = { [0 ... 4095] = 0xa5 }; > + > +static bool illegal_insn; > + > +static void sigill_handler(int sig, siginfo_t *info, void *context) > +{ > + unsigned long *regs = (unsigned long *)&((ucontext_t *)context)- > >uc_mcontext; > + uint32_t insn = *(uint32_t *)regs[0]; > + > + assert(insn == MK_CBO(regs[11])); The byte order of insn should always be little endian, while the CPU may be a big-endian one, then the check might fail. Maybe we can use __le32_to_cpu(insn) to convert it before the check. BRs, Xiao > + > + illegal_insn = true; > + regs[0] += 4; > +} > + > +static void cbo_insn(char *base, int fn) > +{ > + uint32_t insn = MK_CBO(fn); > + > + asm volatile( > + "mv a0, %0\n" > + "li a1, %1\n" > + ".4byte %2\n" > + : : "r" (base), "i" (fn), "i" (insn) : "a0", "a1", "memory"); > +} > + > +static void cbo_inval(char *base) { cbo_insn(base, 0); } > +static void cbo_clean(char *base) { cbo_insn(base, 1); } > +static void cbo_flush(char *base) { cbo_insn(base, 2); } > +static void cbo_zero(char *base) { cbo_insn(base, 4); } > + > +static void test_no_zicbom(void) > +{ > + illegal_insn = false; > + cbo_clean(&mem[0]); > + ksft_test_result(illegal_insn, "No cbo.clean\n"); > + > + illegal_insn = false; > + cbo_flush(&mem[0]); > + ksft_test_result(illegal_insn, "No cbo.flush\n"); > + > + illegal_insn = false; > + cbo_inval(&mem[0]); > + ksft_test_result(illegal_insn, "No cbo.inval\n"); > +} > + [...] > -- > 2.41.0 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Sep 05, 2023 at 02:36:44PM +0000, Wang, Xiao W wrote: > Hi, > > > -----Original Message----- > > From: linux-riscv <linux-riscv-bounces@lists.infradead.org> On Behalf Of > > Andrew Jones > > Sent: Tuesday, September 5, 2023 1:02 AM > > To: linux-riscv@lists.infradead.org > > Cc: paul.walmsley@sifive.com; palmer@dabbelt.com; > > aou@eecs.berkeley.edu; evan@rivosinc.com; conor.dooley@microchip.com; > > apatel@ventanamicro.com > > Subject: [PATCH v3 6/6] RISC-V: selftests: Add CBO tests > > > > Add hwprobe test for Zicboz and its block size. Also, when Zicboz is > > present, test that cbo.zero may be issued and works. Additionally > > test that the Zicbom instructions cause SIGILL and also that cbo.zero > > causes SIGILL when Zicboz is not present. Pinning the test to a subset > > of cpus with taskset will also restrict the hwprobe calls to that set. > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > .../testing/selftests/riscv/hwprobe/Makefile | 7 +- > > tools/testing/selftests/riscv/hwprobe/cbo.c | 170 ++++++++++++++++++ > > .../testing/selftests/riscv/hwprobe/hwprobe.c | 12 +- > > .../testing/selftests/riscv/hwprobe/hwprobe.h | 15 ++ > > 4 files changed, 192 insertions(+), 12 deletions(-) > > create mode 100644 tools/testing/selftests/riscv/hwprobe/cbo.c > > create mode 100644 tools/testing/selftests/riscv/hwprobe/hwprobe.h > > > > diff --git a/tools/testing/selftests/riscv/hwprobe/Makefile > > b/tools/testing/selftests/riscv/hwprobe/Makefile > > index 5f614c3ba598..f224b84591fb 100644 > > --- a/tools/testing/selftests/riscv/hwprobe/Makefile > > +++ b/tools/testing/selftests/riscv/hwprobe/Makefile > > @@ -2,9 +2,14 @@ > > # Copyright (C) 2021 ARM Limited > > # Originally tools/testing/arm64/abi/Makefile > > > > -TEST_GEN_PROGS := hwprobe > > +CFLAGS += -I$(top_srcdir)/tools/include > > + > > +TEST_GEN_PROGS := hwprobe cbo > > > > include ../../lib.mk > > > > $(OUTPUT)/hwprobe: hwprobe.c sys_hwprobe.S > > $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ > > + > > +$(OUTPUT)/cbo: cbo.c sys_hwprobe.S > > + $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ > > diff --git a/tools/testing/selftests/riscv/hwprobe/cbo.c > > b/tools/testing/selftests/riscv/hwprobe/cbo.c > > new file mode 100644 > > index 000000000000..50e85f31a2c7 > > --- /dev/null > > +++ b/tools/testing/selftests/riscv/hwprobe/cbo.c > > @@ -0,0 +1,170 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2023 Ventana Micro Systems Inc. > > + * > > + * Run with 'taskset -c <cpu-list> cbo' to only execute hwprobe on a > > + * subset of cpus, as well as only executing the tests on those cpus. > > + */ > > Patch 3/6 exposes a common subset extensions of the cpu list to user space, so, if some of the cores support ZICBOZ while other don't, hwprobe() won't expose ZICBOZ ext, but in this case the cbo_zero insn might run w/o illegal insn exception when the task is scheduled to run on a core that support ZICBOZ. The test result may vary for each run for this case. > Maybe we can add some comment for this special test case? Ah, you're right, and I'm afraid a comment isn't sufficient, since testers may not have the code handy nor think to check for comments before reporting issues. The test code needs to handle this case, at least by outputting helpful directions to the tester when the case is detected. It'd be nice to have a hwprobe variant which takes an extension as input and returns a cpu set describing each cpu which supports the extension. Instead, I'll create an inefficient function which does that, based on multiple hwprobe calls. Then, on a mixed set, the test will complain and skip, informing the tester to taskset a subset of cpus which are consistent wrt the extension. > > > +#define _GNU_SOURCE > > +#include <stdbool.h> > > +#include <stdint.h> > > +#include <sched.h> > > +#include <signal.h> > > +#include <assert.h> > > +#include <linux/compiler.h> > > +#include <asm/ucontext.h> > > + > > +#include "hwprobe.h" > > +#include "../../kselftest.h" > > + > > +#define MK_CBO(fn) ((fn) << 20 | 10 << 15 | 2 << 12 | 0 << 7 | 15) > > + > > +static char mem[4096] __aligned(4096) = { [0 ... 4095] = 0xa5 }; > > + > > +static bool illegal_insn; > > + > > +static void sigill_handler(int sig, siginfo_t *info, void *context) > > +{ > > + unsigned long *regs = (unsigned long *)&((ucontext_t *)context)- > > >uc_mcontext; > > + uint32_t insn = *(uint32_t *)regs[0]; > > + > > + assert(insn == MK_CBO(regs[11])); > > > The byte order of insn should always be little endian, while the CPU may be a big-endian one, then the check might fail. > Maybe we can use __le32_to_cpu(insn) to convert it before the check. Sounds good (minus the leading __, since we don't have __le32_to_cpu() in tools). Thanks, drew
On Tue, Sep 05, 2023 at 06:10:25PM +0200, Andrew Jones wrote: > On Tue, Sep 05, 2023 at 02:36:44PM +0000, Wang, Xiao W wrote: > > Hi, > > > > > -----Original Message----- > > > From: linux-riscv <linux-riscv-bounces@lists.infradead.org> On Behalf Of > > > Andrew Jones ... > > > +#define MK_CBO(fn) ((fn) << 20 | 10 << 15 | 2 << 12 | 0 << 7 | 15) > > > + > > > +static char mem[4096] __aligned(4096) = { [0 ... 4095] = 0xa5 }; > > > + > > > +static bool illegal_insn; > > > + > > > +static void sigill_handler(int sig, siginfo_t *info, void *context) > > > +{ > > > + unsigned long *regs = (unsigned long *)&((ucontext_t *)context)- > > > >uc_mcontext; > > > + uint32_t insn = *(uint32_t *)regs[0]; > > > + > > > + assert(insn == MK_CBO(regs[11])); > > > > > > The byte order of insn should always be little endian, while the CPU may be a big-endian one, then the check might fail. > > Maybe we can use __le32_to_cpu(insn) to convert it before the check. > > Sounds good (minus the leading __, since we don't have __le32_to_cpu() in > tools). > Actually, don't I also need to ensure byte order when creating the instruction in cbo_insn() with ".4byte %2" where %2 is insn which is MK_CBO(fn)? I presume so, which means the proper fix would be to add a cpu_to_le32() to MK_CBO(), i.e. #define MK_CBO(fn) cpu_to_le32((fn) << 20 | 10 << 15 | 2 << 12 | 0 << 7 | 15) Unless somebody tells me otherwise, then I'll do that for v4. Thanks, drew
On Mon, Sep 04, 2023 at 07:02:27PM +0200, Andrew Jones wrote: > Add hwprobe test for Zicboz and its block size. Also, when Zicboz is > present, test that cbo.zero may be issued and works. Additionally > test that the Zicbom instructions cause SIGILL and also that cbo.zero > causes SIGILL when Zicboz is not present. Pinning the test to a subset > of cpus with taskset will also restrict the hwprobe calls to that set. Palmer pointed out that there's no guarantee to get illegal-instruction exceptions when Zicbom/Zicboz are not present. Indeed, the unpriv spec states """ The behavior upon decoding a reserved instruction is UNSPECIFIED. Some platforms may require that opcodes reserved for standard use raise an illegal-instruction exception. Other platforms may permit reserved opcode space be used for non-conforming extensions. """ This implies that cannot do the illegal-instruction checks for when hwprobe reports the extension isn't present. That's a pity, but I guess I should rip it out. Or, maybe I'll keep it, but only run it under a command line switch. Thanks, drew
diff --git a/tools/testing/selftests/riscv/hwprobe/Makefile b/tools/testing/selftests/riscv/hwprobe/Makefile index 5f614c3ba598..f224b84591fb 100644 --- a/tools/testing/selftests/riscv/hwprobe/Makefile +++ b/tools/testing/selftests/riscv/hwprobe/Makefile @@ -2,9 +2,14 @@ # Copyright (C) 2021 ARM Limited # Originally tools/testing/arm64/abi/Makefile -TEST_GEN_PROGS := hwprobe +CFLAGS += -I$(top_srcdir)/tools/include + +TEST_GEN_PROGS := hwprobe cbo include ../../lib.mk $(OUTPUT)/hwprobe: hwprobe.c sys_hwprobe.S $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ + +$(OUTPUT)/cbo: cbo.c sys_hwprobe.S + $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ diff --git a/tools/testing/selftests/riscv/hwprobe/cbo.c b/tools/testing/selftests/riscv/hwprobe/cbo.c new file mode 100644 index 000000000000..50e85f31a2c7 --- /dev/null +++ b/tools/testing/selftests/riscv/hwprobe/cbo.c @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2023 Ventana Micro Systems Inc. + * + * Run with 'taskset -c <cpu-list> cbo' to only execute hwprobe on a + * subset of cpus, as well as only executing the tests on those cpus. + */ +#define _GNU_SOURCE +#include <stdbool.h> +#include <stdint.h> +#include <sched.h> +#include <signal.h> +#include <assert.h> +#include <linux/compiler.h> +#include <asm/ucontext.h> + +#include "hwprobe.h" +#include "../../kselftest.h" + +#define MK_CBO(fn) ((fn) << 20 | 10 << 15 | 2 << 12 | 0 << 7 | 15) + +static char mem[4096] __aligned(4096) = { [0 ... 4095] = 0xa5 }; + +static bool illegal_insn; + +static void sigill_handler(int sig, siginfo_t *info, void *context) +{ + unsigned long *regs = (unsigned long *)&((ucontext_t *)context)->uc_mcontext; + uint32_t insn = *(uint32_t *)regs[0]; + + assert(insn == MK_CBO(regs[11])); + + illegal_insn = true; + regs[0] += 4; +} + +static void cbo_insn(char *base, int fn) +{ + uint32_t insn = MK_CBO(fn); + + asm volatile( + "mv a0, %0\n" + "li a1, %1\n" + ".4byte %2\n" + : : "r" (base), "i" (fn), "i" (insn) : "a0", "a1", "memory"); +} + +static void cbo_inval(char *base) { cbo_insn(base, 0); } +static void cbo_clean(char *base) { cbo_insn(base, 1); } +static void cbo_flush(char *base) { cbo_insn(base, 2); } +static void cbo_zero(char *base) { cbo_insn(base, 4); } + +static void test_no_zicbom(void) +{ + illegal_insn = false; + cbo_clean(&mem[0]); + ksft_test_result(illegal_insn, "No cbo.clean\n"); + + illegal_insn = false; + cbo_flush(&mem[0]); + ksft_test_result(illegal_insn, "No cbo.flush\n"); + + illegal_insn = false; + cbo_inval(&mem[0]); + ksft_test_result(illegal_insn, "No cbo.inval\n"); +} + +static void test_no_zicboz(void) +{ + illegal_insn = false; + cbo_zero(&mem[0]); + ksft_test_result(illegal_insn, "No cbo.zero\n"); +} + +static bool is_power_of_2(__u64 n) +{ + return n != 0 && (n & (n - 1)) == 0; +} + +static void test_zicboz(__u64 block_size) +{ + int i, j; + + if (!is_power_of_2(block_size)) { + ksft_test_result_skip("cbo.zero check\n"); + return; + } + + assert(block_size <= 1024); + + illegal_insn = false; + cbo_zero(&mem[block_size]); + ksft_test_result(!illegal_insn, "cbo.zero\n"); + + if (illegal_insn) { + ksft_test_result_skip("cbo.zero check\n"); + return; + } + + for (i = 0; i < 4096 / block_size; ++i) { + if (i % 2) + cbo_zero(&mem[i * block_size]); + } + + for (i = 0; i < 4096 / block_size; ++i) { + char expected = i % 2 ? 0x0 : 0xa5; + + for (j = 0; j < block_size; ++j) { + if (mem[i * block_size + j] != expected) { + ksft_test_result_fail("cbo.zero check\n"); + ksft_print_msg("cbo.zero check: mem[%d] != 0x%x\n", + i * block_size + j, expected); + return; + } + } + } + + ksft_test_result_pass("cbo.zero check\n"); +} + +int main(int argc, char **argv) +{ + struct sigaction act = { + .sa_sigaction = &sigill_handler, + .sa_flags = SA_SIGINFO, + }; + bool has_zicboz = false; + struct riscv_hwprobe pair; + cpu_set_t cpus; + size_t nr_cpus; + long rc; + + rc = sigaction(SIGILL, &act, NULL); + assert(rc == 0); + + rc = sched_getaffinity(0, sizeof(cpu_set_t), &cpus); + assert(rc == 0); + nr_cpus = CPU_COUNT(&cpus); + + ksft_print_header(); + + pair.key = RISCV_HWPROBE_KEY_IMA_EXT_0; + rc = riscv_hwprobe(&pair, 1, nr_cpus, (unsigned long *)&cpus, 0); + if (rc < 0) + ksft_exit_fail_msg("hwprobe() failed with %d\n", rc); + + if (pair.key != -1 && (pair.value & RISCV_HWPROBE_EXT_ZICBOZ)) { + has_zicboz = true; + ksft_set_plan(6); + } else { + ksft_print_msg("No Zicboz, testing cbo.zero remains privileged\n"); + ksft_set_plan(4); + } + + /* Ensure Zicbom instructions remain privileged */ + test_no_zicbom(); + + if (has_zicboz) { + pair.key = RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE; + rc = riscv_hwprobe(&pair, 1, nr_cpus, (unsigned long *)&cpus, 0); + ksft_test_result(rc == 0 && pair.key == RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE && + is_power_of_2(pair.value), "Zicboz block size\n"); + ksft_print_msg("Zicboz block size: %ld\n", pair.value); + test_zicboz(pair.value); + } else { + test_no_zicboz(); + } + + ksft_finished(); +} diff --git a/tools/testing/selftests/riscv/hwprobe/hwprobe.c b/tools/testing/selftests/riscv/hwprobe/hwprobe.c index 4f15f1f3b4c3..c474891df307 100644 --- a/tools/testing/selftests/riscv/hwprobe/hwprobe.c +++ b/tools/testing/selftests/riscv/hwprobe/hwprobe.c @@ -1,17 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-only -#include <stddef.h> -#include <asm/hwprobe.h> - +#include "hwprobe.h" #include "../../kselftest.h" -/* - * Rather than relying on having a new enough libc to define this, just do it - * ourselves. This way we don't need to be coupled to a new-enough libc to - * contain the call. - */ -long riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count, - size_t cpu_count, unsigned long *cpus, unsigned int flags); - int main(int argc, char **argv) { struct riscv_hwprobe pairs[8]; diff --git a/tools/testing/selftests/riscv/hwprobe/hwprobe.h b/tools/testing/selftests/riscv/hwprobe/hwprobe.h new file mode 100644 index 000000000000..721b0ce73a56 --- /dev/null +++ b/tools/testing/selftests/riscv/hwprobe/hwprobe.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef SELFTEST_RISCV_HWPROBE_H +#define SELFTEST_RISCV_HWPROBE_H +#include <stddef.h> +#include <asm/hwprobe.h> + +/* + * Rather than relying on having a new enough libc to define this, just do it + * ourselves. This way we don't need to be coupled to a new-enough libc to + * contain the call. + */ +long riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count, + size_t cpu_count, unsigned long *cpus, unsigned int flags); + +#endif
Add hwprobe test for Zicboz and its block size. Also, when Zicboz is present, test that cbo.zero may be issued and works. Additionally test that the Zicbom instructions cause SIGILL and also that cbo.zero causes SIGILL when Zicboz is not present. Pinning the test to a subset of cpus with taskset will also restrict the hwprobe calls to that set. Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- .../testing/selftests/riscv/hwprobe/Makefile | 7 +- tools/testing/selftests/riscv/hwprobe/cbo.c | 170 ++++++++++++++++++ .../testing/selftests/riscv/hwprobe/hwprobe.c | 12 +- .../testing/selftests/riscv/hwprobe/hwprobe.h | 15 ++ 4 files changed, 192 insertions(+), 12 deletions(-) create mode 100644 tools/testing/selftests/riscv/hwprobe/cbo.c create mode 100644 tools/testing/selftests/riscv/hwprobe/hwprobe.h