Re: [PATCH v3 6/6] RISC-V: selftests: Add CBO tests

[Andrew Jones <ajones@ventanamicro.com>]

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

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