[kvm-unit-tests PATCH v6] riscv: sbi: Add SBI Debug Triggers Extension tests

Jesse Taube jesse at rivosinc.com
Fri Jun 27 12:06:02 PDT 2025


On Fri, Jun 27, 2025 at 6:02 AM Andrew Jones <andrew.jones at linux.dev> wrote:
>
> On Wed, Jun 18, 2025 at 08:44:52AM -0700, Jesse Taube wrote:
> > Add tests for the DBTR SBI extension.
> >
> > Signed-off-by: Jesse Taube <jesse at rivosinc.com>
> > Reviewed-by: Charlie Jenkins <charlie at rivosinc.com>
> > Tested-by: Charlie Jenkins <charlie at rivosinc.com>
> > ---
> > V1 -> V2:
> >  - Call report_prefix_pop before returning
> >  - Disable compressed instructions in exec_call, update related comment
> >  - Remove extra "| 1" in dbtr_test_load
> >  - Remove extra newlines
> >  - Remove extra tabs in check_exec
> >  - Remove typedefs from enums
> >  - Return when dbtr_install_trigger fails
> >  - s/avalible/available/g
> >  - s/unistall/uninstall/g
> > V2 -> V3:
> >  - Change SBI_DBTR_SHMEM_INVALID_ADDR to -1UL
> >  - Move all dbtr functions to sbi-dbtr.c
> >  - Move INSN_LEN to processor.h
> >  - Update include list
> >  - Use C-style comments
> > V3 -> V4:
> >  - Include libcflat.h
> >  - Remove #define SBI_DBTR_SHMEM_INVALID_ADDR
> > V4 -> V5:
> >  - Sort includes
> >  - Add kfail for update triggers
> > V5 -> V6:
> >  - Add assert in gen_tdata1
> >  - Add prefix to dbtr_test_type
> >  - Add TRIG_STATE_DMODE
> >  - Add TRIG_STATE_RESERVED
> >  - Align function paramaters with opening parenthesis
> >  - Change OpenSBI < v1.7 to < v1.5
> >  - Constantly use spaces in prefix rather than _
> >  - Export split_phys_addr
> >  - Fix MCONTROL_U and MCONTROL_M mix up
> >  - Fix swapped VU and VS
> >  - Move /* to own line
> >  - Print type in dbtr_test_type
> >  - Remove _BIT suffix from macros
> >  - Remove duplicate MODE_S
> >  - Remove spaces before include
> >  - Rename tdata1,2 to trigger and control in dbtr_install_trigger
> >  - Report skip in dbtr_test_multiple
> >  - Report variables in info not pass or fail
> >  - s/save/store/g
> >  - sbi_debug_set_shmem use split_phys_addr
> >  - Use if (!report(... in dbtr_test_disable_enable
> > ---
> >  lib/riscv/asm/sbi.h |   1 +
> >  riscv/Makefile      |   1 +
> >  riscv/sbi-dbtr.c    | 839 ++++++++++++++++++++++++++++++++++++++++++++
> >  riscv/sbi-tests.h   |   2 +
> >  riscv/sbi.c         |   3 +-
> >  5 files changed, 845 insertions(+), 1 deletion(-)
> >  create mode 100644 riscv/sbi-dbtr.c
> >
> > diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> > index a5738a5c..78fd6e2a 100644
> > --- a/lib/riscv/asm/sbi.h
> > +++ b/lib/riscv/asm/sbi.h
> > @@ -51,6 +51,7 @@ enum sbi_ext_id {
> >       SBI_EXT_SUSP = 0x53555350,
> >       SBI_EXT_FWFT = 0x46574654,
> >       SBI_EXT_SSE = 0x535345,
> > +     SBI_EXT_DBTR = 0x44425452,
> >  };
> >
> >  enum sbi_ext_base_fid {
> > diff --git a/riscv/Makefile b/riscv/Makefile
> > index 11e68eae..55c7ac93 100644
> > --- a/riscv/Makefile
> > +++ b/riscv/Makefile
> > @@ -20,6 +20,7 @@ all: $(tests)
> >  $(TEST_DIR)/sbi-deps += $(TEST_DIR)/sbi-asm.o
> >  $(TEST_DIR)/sbi-deps += $(TEST_DIR)/sbi-fwft.o
> >  $(TEST_DIR)/sbi-deps += $(TEST_DIR)/sbi-sse.o
> > +$(TEST_DIR)/sbi-deps += $(TEST_DIR)/sbi-dbtr.o
>
> My OCD says this should come after the sbi-asm to be alphabetical.
>
> >
> >  all_deps += $($(TEST_DIR)/sbi-deps)
> >
> > diff --git a/riscv/sbi-dbtr.c b/riscv/sbi-dbtr.c
> > new file mode 100644
> > index 00000000..7837553f
> > --- /dev/null
> > +++ b/riscv/sbi-dbtr.c
> > @@ -0,0 +1,839 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * SBI DBTR testsuite
> > + *
> > + * Copyright (C) 2025, Rivos Inc., Jesse Taube <jesse at rivosinc.com>
> > + */
> > +
> > +#include <libcflat.h>
> > +#include <bitops.h>
> > +
> > +#include <asm/io.h>
> > +#include <asm/processor.h>
> > +
> > +#include "sbi-tests.h"
> > +
> > +#define RV_MAX_TRIGGERS                      32
> > +
> > +#define SBI_DBTR_TRIG_STATE_MAPPED           BIT(0)
> > +#define SBI_DBTR_TRIG_STATE_U                        BIT(1)
> > +#define SBI_DBTR_TRIG_STATE_S                        BIT(2)
> > +#define SBI_DBTR_TRIG_STATE_VU                       BIT(3)
> > +#define SBI_DBTR_TRIG_STATE_VS                       BIT(4)
> > +#define SBI_DBTR_TRIG_STATE_HAVE_HW_TRIG     BIT(5)
> > +#define SBI_DBTR_TRIG_STATE_RESERVED         GENMASK(7, 6)
> > +
> > +#define SBI_DBTR_TRIG_STATE_HW_TRIG_IDX_SHIFT                8
> > +#define SBI_DBTR_TRIG_STATE_HW_TRIG_IDX(trig_state)  (trig_state >> SBI_DBTR_TRIG_STATE_HW_TRIG_IDX_SHIFT)
> > +
> > +#define SBI_DBTR_TDATA1_TYPE_SHIFT           (__riscv_xlen - 4)
> > +#define SBI_DBTR_TDATA1_DMODE                        BIT_UL(__riscv_xlen - 5)
> > +
> > +#define SBI_DBTR_TDATA1_MCONTROL6_LOAD               BIT(0)
> > +#define SBI_DBTR_TDATA1_MCONTROL6_STORE              BIT(1)
> > +#define SBI_DBTR_TDATA1_MCONTROL6_EXECUTE    BIT(2)
> > +#define SBI_DBTR_TDATA1_MCONTROL6_U          BIT(3)
> > +#define SBI_DBTR_TDATA1_MCONTROL6_S          BIT(4)
> > +#define SBI_DBTR_TDATA1_MCONTROL6_M          BIT(6)
> > +#define SBI_DBTR_TDATA1_MCONTROL6_SELECT     BIT(21)
> > +#define SBI_DBTR_TDATA1_MCONTROL6_VU         BIT(23)
> > +#define SBI_DBTR_TDATA1_MCONTROL6_VS         BIT(24)
> > +
> > +#define SBI_DBTR_TDATA1_MCONTROL_LOAD                BIT(0)
> > +#define SBI_DBTR_TDATA1_MCONTROL_STORE               BIT(1)
> > +#define SBI_DBTR_TDATA1_MCONTROL_EXECUTE     BIT(2)
> > +#define SBI_DBTR_TDATA1_MCONTROL_U           BIT(3)
> > +#define SBI_DBTR_TDATA1_MCONTROL_S           BIT(4)
> > +#define SBI_DBTR_TDATA1_MCONTROL_M           BIT(6)
> > +#define SBI_DBTR_TDATA1_MCONTROL_SELECT              BIT(19)
> > +
> > +enum McontrolType {
> > +     SBI_DBTR_TDATA1_TYPE_NONE =             (0UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_LEGACY =           (1UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_MCONTROL =         (2UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_ICOUNT =           (3UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_ITRIGGER =         (4UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_ETRIGGER =         (5UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_MCONTROL6 =        (6UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_TMEXTTRIGGER =     (7UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_RESERVED0 =        (8UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_RESERVED1 =        (9UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_RESERVED2 =        (10UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_RESERVED3 =        (11UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_CUSTOM0 =          (12UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_CUSTOM1 =          (13UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_CUSTOM2 =          (14UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +     SBI_DBTR_TDATA1_TYPE_DISABLED =         (15UL << SBI_DBTR_TDATA1_TYPE_SHIFT),
> > +};
> > +
> > +enum Tdata1Value {
> > +     VALUE_NONE =    0,
> > +     VALUE_LOAD =    BIT(0),
> > +     VALUE_STORE =   BIT(1),
> > +     VALUE_EXECUTE = BIT(2),
> > +};
> > +
> > +enum Tdata1Mode {
> > +     MODE_NONE =     0,
> > +     MODE_M =        BIT(0),
> > +     MODE_U =        BIT(1),
> > +     MODE_S =        BIT(2),
> > +     MODE_VU =       BIT(3),
> > +     MODE_VS =       BIT(4),
> > +};
> > +
> > +enum sbi_ext_dbtr_fid {
> > +     SBI_EXT_DBTR_NUM_TRIGGERS = 0,
> > +     SBI_EXT_DBTR_SETUP_SHMEM,
> > +     SBI_EXT_DBTR_TRIGGER_READ,
> > +     SBI_EXT_DBTR_TRIGGER_INSTALL,
> > +     SBI_EXT_DBTR_TRIGGER_UPDATE,
> > +     SBI_EXT_DBTR_TRIGGER_UNINSTALL,
> > +     SBI_EXT_DBTR_TRIGGER_ENABLE,
> > +     SBI_EXT_DBTR_TRIGGER_DISABLE,
> > +};
> > +
> > +struct sbi_dbtr_data_msg {
> > +     unsigned long tstate;
> > +     unsigned long tdata1;
> > +     unsigned long tdata2;
> > +     unsigned long tdata3;
> > +};
> > +
> > +struct sbi_dbtr_id_msg {
> > +     unsigned long idx;
> > +};
> > +
> > +/* SBI shared mem messages layout */
> > +struct sbi_dbtr_shmem_entry {
> > +     union {
> > +             struct sbi_dbtr_data_msg data;
> > +             struct sbi_dbtr_id_msg id;
> > +     };
> > +};
> > +
> > +static bool dbtr_handled;
> > +
> > +/* Expected to be leaf function as not to disrupt frame-pointer */
> > +static __attribute__((naked)) void exec_call(void)
> > +{
> > +     /* skip over nop when triggered instead of ret. */
> > +     asm volatile (".option push\n"
> > +                   ".option arch, -c\n"
> > +                   "nop\n"
> > +                   "ret\n"
> > +                   ".option pop\n");
> > +}
> > +
> > +static void dbtr_exception_handler(struct pt_regs *regs)
> > +{
> > +     dbtr_handled = true;
> > +
> > +     /* Reading *epc may cause a fault, skip over nop */
> > +     if ((void *)regs->epc == exec_call) {
> > +             regs->epc += 4;
> > +             return;
> > +     }
> > +
> > +     /* WARNING: Skips over the trapped intruction */
> > +     regs->epc += RV_INSN_LEN(readw((void *)regs->epc));
> > +}
> > +
> > +static bool do_store(void *tdata2)
> > +{
> > +     bool ret;
> > +
> > +     writel(0, tdata2);
> > +
> > +     ret = dbtr_handled;
> > +     dbtr_handled = false;
> > +
> > +     return ret;
> > +}
> > +
> > +static bool do_load(void *tdata2)
> > +{
> > +     bool ret;
> > +
> > +     readl(tdata2);
> > +
> > +     ret = dbtr_handled;
> > +     dbtr_handled = false;
> > +
> > +     return ret;
> > +}
> > +
> > +static bool do_exec(void)
> > +{
> > +     bool ret;
> > +
> > +     exec_call();
> > +
> > +     ret = dbtr_handled;
> > +     dbtr_handled = false;
> > +
> > +     return ret;
> > +}
> > +
> > +static unsigned long gen_tdata1_mcontrol(enum Tdata1Mode mode, enum Tdata1Value value)
> > +{
> > +     unsigned long tdata1 = SBI_DBTR_TDATA1_TYPE_MCONTROL;
> > +
> > +     if (value & VALUE_LOAD)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL_LOAD;
> > +
> > +     if (value & VALUE_STORE)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL_STORE;
> > +
> > +     if (value & VALUE_EXECUTE)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL_EXECUTE;
> > +
> > +     if (mode & MODE_M)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL_M;
> > +
> > +     if (mode & MODE_U)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL_U;
> > +
> > +     if (mode & MODE_S)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL_S;
> > +
> > +     return tdata1;
> > +}
> > +
> > +static unsigned long gen_tdata1_mcontrol6(enum Tdata1Mode mode, enum Tdata1Value value)
> > +{
> > +     unsigned long tdata1 = SBI_DBTR_TDATA1_TYPE_MCONTROL6;
> > +
> > +     if (value & VALUE_LOAD)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_LOAD;
> > +
> > +     if (value & VALUE_STORE)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_STORE;
> > +
> > +     if (value & VALUE_EXECUTE)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_EXECUTE;
> > +
> > +     if (mode & MODE_M)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_M;
> > +
> > +     if (mode & MODE_U)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_U;
> > +
> > +     if (mode & MODE_S)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_S;
> > +
> > +     if (mode & MODE_VU)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_VU;
> > +
> > +     if (mode & MODE_VS)
> > +             tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_VS;
> > +
> > +     return tdata1;
> > +}
> > +
> > +static unsigned long gen_tdata1(enum McontrolType type, enum Tdata1Value value, enum Tdata1Mode mode)
> > +{
> > +     switch (type) {
> > +     case SBI_DBTR_TDATA1_TYPE_MCONTROL:
> > +             return gen_tdata1_mcontrol(mode, value);
> > +     case SBI_DBTR_TDATA1_TYPE_MCONTROL6:
> > +             return gen_tdata1_mcontrol6(mode, value);
> > +     default:
> > +             assert_msg(false, "Invalid mcontrol type: %lu", type);
> > +             return 0;
>
> Can drop the 'return 0' line now that there's an unconditional assert.
>
> > +     }
> > +}
> > +
> > +static struct sbiret sbi_debug_num_triggers(unsigned long trig_tdata1)
> > +{
> > +     return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS, trig_tdata1, 0, 0, 0, 0, 0);
> > +}
> > +
> > +static struct sbiret sbi_debug_set_shmem_raw(unsigned long shmem_phys_lo,
> > +                                          unsigned long shmem_phys_hi,
> > +                                          unsigned long flags)
> > +{
> > +     return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_SETUP_SHMEM, shmem_phys_lo,
> > +                      shmem_phys_hi, flags, 0, 0, 0);
> > +}
> > +
> > +static struct sbiret sbi_debug_set_shmem(void *shmem)
> > +{
> > +     unsigned long base_addr_lo, base_addr_hi;
> > +
> > +     split_phys_addr(virt_to_phys(shmem), &base_addr_hi, &base_addr_lo);
> > +     return sbi_debug_set_shmem_raw(base_addr_lo, base_addr_hi, 0);
> > +}
> > +
> > +static struct sbiret sbi_debug_read_triggers(unsigned long trig_idx_base,
> > +                                          unsigned long trig_count)
> > +{
> > +     return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_READ, trig_idx_base,
> > +                      trig_count, 0, 0, 0, 0);
> > +}
> > +
> > +static struct sbiret sbi_debug_install_triggers(unsigned long trig_count)
> > +{
> > +     return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_INSTALL, trig_count, 0, 0, 0, 0, 0);
> > +}
> > +
> > +static struct sbiret sbi_debug_update_triggers(unsigned long trig_count)
> > +{
> > +     return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_UPDATE, trig_count, 0, 0, 0, 0, 0);
> > +}
> > +
> > +static struct sbiret sbi_debug_uninstall_triggers(unsigned long trig_idx_base,
> > +                                               unsigned long trig_idx_mask)
> > +{
> > +     return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_UNINSTALL, trig_idx_base,
> > +                      trig_idx_mask, 0, 0, 0, 0);
> > +}
> > +
> > +static struct sbiret sbi_debug_enable_triggers(unsigned long trig_idx_base,
> > +                                            unsigned long trig_idx_mask)
> > +{
> > +     return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_ENABLE, trig_idx_base,
> > +                      trig_idx_mask, 0, 0, 0, 0);
> > +}
> > +
> > +static struct sbiret sbi_debug_disable_triggers(unsigned long trig_idx_base,
> > +                                             unsigned long trig_idx_mask)
> > +{
> > +     return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_DISABLE, trig_idx_base,
> > +                      trig_idx_mask, 0, 0, 0, 0);
> > +}
> > +
> > +static bool dbtr_install_trigger(struct sbi_dbtr_shmem_entry *shmem, void *trigger,
> > +                              unsigned long control)
> > +{
> > +     struct sbiret sbi_ret;
> > +     bool ret;
> > +
> > +     shmem->data.tdata1 = control;
> > +     shmem->data.tdata2 = (unsigned long)trigger;
> > +
> > +     sbi_ret = sbi_debug_install_triggers(1);
> > +     ret = sbiret_report_error(&sbi_ret, SBI_SUCCESS, "sbi_debug_install_triggers");
> > +     if (ret)
> > +             install_exception_handler(EXC_BREAKPOINT, dbtr_exception_handler);
> > +
> > +     return ret;
> > +}
> > +
> > +static bool dbtr_uninstall_trigger(void)
> > +{
> > +     struct sbiret ret;
> > +
> > +     install_exception_handler(EXC_BREAKPOINT, NULL);
> > +
> > +     ret = sbi_debug_uninstall_triggers(0, 1);
> > +     return sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_uninstall_triggers");
> > +}
> > +
> > +static unsigned long dbtr_test_num_triggers(void)
> > +{
> > +     struct sbiret ret;
> > +     unsigned long tdata1 = 0;
> > +     /* sbi_debug_num_triggers will return trig_max in sbiret.value when trig_tdata1 == 0 */
> > +
> > +     /* should be at least one trigger. */
> > +     ret = sbi_debug_num_triggers(tdata1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_num_triggers");
> > +
> > +     if (ret.value == 0) {
> > +             report_fail("sbi_debug_num_triggers: Returned 0 triggers available");
> > +     } else {
> > +             report_pass("sbi_debug_num_triggers: Returned triggers available");
> > +             report_info("sbi_debug_num_triggers: Returned %lu triggers available", ret.value);
>
> Rather than put the sbi_debug_num_triggers prefix in each report call, we
> can just push it.
>
> > +     }
> > +
> > +     return ret.value;
> > +}
> > +
> > +static enum McontrolType dbtr_test_type(unsigned long *num_trig)
> > +{
> > +     struct sbiret ret;
> > +     unsigned long tdata1 = SBI_DBTR_TDATA1_TYPE_MCONTROL6;
> > +
> > +     report_prefix_push("test type");
> > +
> > +     ret = sbi_debug_num_triggers(tdata1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_num_triggers: mcontrol6");
> > +     *num_trig = ret.value;
> > +     if (ret.value > 0) {
> > +             report_pass("sbi_debug_num_triggers: Returned mcontrol6 triggers available");
> > +             report_info("sbi_debug_num_triggers: Returned %lu mcontrol6 triggers available",
>
> Same comment about pushing "sbi_debug_num_triggers".
>
> > +                         ret.value);
> > +             report_prefix_pop();
>
> If we push sbi_debug_num_triggers, then this can be a report_prefix_popn(2).
>
> > +             return tdata1;
> > +     }
> > +
> > +     tdata1 = SBI_DBTR_TDATA1_TYPE_MCONTROL;
> > +
> > +     ret = sbi_debug_num_triggers(tdata1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_num_triggers: mcontrol");
> > +     *num_trig = ret.value;
> > +     if (ret.value > 0) {
> > +             report_pass("sbi_debug_num_triggers: Returned mcontrol triggers available");
> > +             report_info("sbi_debug_num_triggers: Returned %lu mcontrol triggers available",
> > +                         ret.value);
> > +             report_prefix_pop();
> > +             return tdata1;
> > +     }
> > +
> > +     report_fail("sbi_debug_num_triggers: Returned 0 mcontrol(6) triggers available");
> > +     report_prefix_pop();
> > +
> > +     return SBI_DBTR_TDATA1_TYPE_NONE;
> > +}
> > +
> > +static struct sbiret dbtr_test_store_install_uninstall(struct sbi_dbtr_shmem_entry *shmem,
> > +                                                   enum McontrolType type)
> > +{
> > +     static unsigned long test;
> > +     struct sbiret ret;
> > +
> > +     report_prefix_push("store trigger");
> > +
> > +     shmem->data.tdata1 = gen_tdata1(type, VALUE_STORE, MODE_S);
> > +     shmem->data.tdata2 = (unsigned long)&test;
> > +
> > +     ret = sbi_debug_install_triggers(1);
> > +     if (!sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_install_triggers")) {
> > +             report_prefix_pop();
> > +             return ret;
> > +     }
> > +
> > +     install_exception_handler(EXC_BREAKPOINT, dbtr_exception_handler);
> > +
> > +     report(do_store(&test), "triggered");
> > +
> > +     if (do_load(&test))
> > +             report_fail("triggered by load");
> > +
> > +     ret = sbi_debug_uninstall_triggers(0, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_uninstall_triggers");
> > +
> > +     if (do_store(&test))
> > +             report_fail("triggered after uninstall");
> > +
> > +     install_exception_handler(EXC_BREAKPOINT, NULL);
> > +     report_prefix_pop();
> > +
> > +     return ret;
> > +}
> > +
> > +static void dbtr_test_update(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type)
> > +{
> > +     static unsigned long test;
> > +     struct sbiret ret;
> > +     bool kfail;
> > +
> > +     report_prefix_push("update trigger");
> > +
> > +     if (!dbtr_install_trigger(shmem, NULL, gen_tdata1(type, VALUE_NONE, MODE_NONE))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     shmem->id.idx = 0;
> > +     shmem->data.tdata1 = gen_tdata1(type, VALUE_STORE, MODE_S);
> > +     shmem->data.tdata2 = (unsigned long)&test;
> > +
> > +     ret = sbi_debug_update_triggers(1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_update_triggers");
> > +
> > +     /*
> > +      * Known broken update_triggers.
> > +      * https://lore.kernel.org/opensbi/aDdp1UeUh7GugeHp@ghost/T/#t
> > +      */
> > +     kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> > +             __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
> > +     report_kfail(kfail, do_store(&test), "triggered");
> > +
> > +     dbtr_uninstall_trigger();
> > +     report_prefix_pop();
> > +}
> > +
> > +static void dbtr_test_load(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type)
> > +{
> > +     static unsigned long test;
> > +
> > +     report_prefix_push("load trigger");
> > +     if (!dbtr_install_trigger(shmem, &test, gen_tdata1(type, VALUE_LOAD, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     report(do_load(&test), "triggered");
> > +
> > +     if (do_store(&test))
> > +             report_fail("triggered by store");
> > +
> > +     dbtr_uninstall_trigger();
> > +     report_prefix_pop();
> > +}
> > +
> > +static void dbtr_test_disable_enable(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type)
> > +{
> > +     static unsigned long test;
> > +     struct sbiret ret;
> > +
> > +     report_prefix_push("disable trigger");
> > +     if (!dbtr_install_trigger(shmem, &test, gen_tdata1(type, VALUE_STORE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     ret = sbi_debug_disable_triggers(0, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_disable_triggers");
> > +
> > +     if (!report(!do_store(&test), "should not trigger")) {
> > +             dbtr_uninstall_trigger();
> > +             report_prefix_pop();
> > +             report_skip("enable trigger: no disable");
> > +
> > +             return;
> > +     }
> > +
> > +     report_prefix_pop();
> > +     report_prefix_push("enable trigger");
> > +
> > +     ret = sbi_debug_enable_triggers(0, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_enable_triggers");
> > +
> > +     report(do_store(&test), "triggered");
> > +
> > +     dbtr_uninstall_trigger();
> > +     report_prefix_pop();
> > +}
> > +
> > +static void dbtr_test_exec(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type)
> > +{
> > +     static unsigned long test;
> > +
> > +     report_prefix_push("exec trigger");
> > +     /* check if loads and stores trigger exec */
> > +     if (!dbtr_install_trigger(shmem, &test, gen_tdata1(type, VALUE_EXECUTE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     if (do_load(&test))
> > +             report_fail("triggered by load");
> > +
> > +     if (do_store(&test))
> > +             report_fail("triggered by store");
> > +
> > +     dbtr_uninstall_trigger();
> > +
> > +     /* Check if exec works */
> > +     if (!dbtr_install_trigger(shmem, exec_call, gen_tdata1(type, VALUE_EXECUTE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +     report(do_exec(), "triggered");
> > +
> > +     dbtr_uninstall_trigger();
> > +     report_prefix_pop();
> > +}
> > +
> > +static void dbtr_test_read(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type)
> > +{
> > +     const unsigned long tstatus_expected = SBI_DBTR_TRIG_STATE_S | SBI_DBTR_TRIG_STATE_MAPPED;
> > +     const unsigned long tdata1 = gen_tdata1(type, VALUE_STORE, MODE_S);
> > +     static unsigned long test;
> > +     struct sbiret ret;
> > +
> > +     report_prefix_push("read trigger");
> > +     if (!dbtr_install_trigger(shmem, &test, tdata1)) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     ret = sbi_debug_read_triggers(0, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_read_triggers");
> > +
> > +     report(shmem->data.tdata1 == tdata1, "tdata1 expected: 0x%016lx", tdata1);
> > +     report_info("tdata1 found: 0x%016lx", shmem->data.tdata1);
> > +     report(shmem->data.tdata2 == ((unsigned long)&test), "tdata2 expected: 0x%016lx",
> > +            (unsigned long)&test);
> > +     report_info("tdata2 found: 0x%016lx", shmem->data.tdata2);
> > +     report(shmem->data.tstate == tstatus_expected, "tstate expected: 0x%016lx", tstatus_expected);
> > +     report_info("tstate found: 0x%016lx", shmem->data.tstate);
>
> These don't need to be split into report/report_info pairs because the
> report is checking for a specific value. We only split when report is
> checking for some nonzero value and we also want to output what that
> specific value was for informational purposes.

I'm a bit confused. Should it only report_info when the test fails?

>
> > +
> > +     dbtr_uninstall_trigger();
> > +     report_prefix_pop();
> > +}
> > +
> > +static void check_exec(unsigned long base)
> > +{
> > +     struct sbiret ret;
> > +
> > +     report(do_exec(), "exec triggered");
> > +
> > +     ret = sbi_debug_uninstall_triggers(base, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_uninstall_triggers");
> > +}
> > +
> > +static void dbtr_test_multiple(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type,
> > +                            unsigned long num_trigs)
> > +{
> > +     static unsigned long test[2];
> > +     struct sbiret ret;
> > +     bool have_three = num_trigs > 2;
> > +
> > +     if (num_trigs < 2) {
> > +             report_skip("test multiple");
> > +             return;
> > +     }
> > +
> > +     report_prefix_push("test multiple");
> > +
> > +     if (!dbtr_install_trigger(shmem, &test[0], gen_tdata1(type, VALUE_STORE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +     if (!dbtr_install_trigger(shmem, &test[1], gen_tdata1(type, VALUE_LOAD, MODE_S)))
> > +             goto error;
> > +     if (have_three &&
> > +         !dbtr_install_trigger(shmem, exec_call, gen_tdata1(type, VALUE_EXECUTE, MODE_S))) {
> > +             ret = sbi_debug_uninstall_triggers(1, 1);
> > +             sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_uninstall_triggers");
> > +             goto error;
> > +     }
> > +
> > +     report(do_store(&test[0]), "store triggered");
> > +
> > +     if (do_load(&test[0]))
> > +             report_fail("store triggered by load");
> > +
> > +     report(do_load(&test[1]), "load triggered");
> > +
> > +     if (do_store(&test[1]))
> > +             report_fail("load triggered by store");
> > +
> > +     if (have_three)
> > +             check_exec(2);
> > +
> > +     ret = sbi_debug_uninstall_triggers(1, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_uninstall_triggers");
> > +
> > +     if (do_load(&test[1]))
> > +             report_fail("load triggered after uninstall");
> > +
> > +     report(do_store(&test[0]), "store triggered");
> > +
> > +     if (!have_three &&
> > +         dbtr_install_trigger(shmem, exec_call, gen_tdata1(type, VALUE_EXECUTE, MODE_S)))
> > +             check_exec(1);
> > +
> > +error:
> > +     ret = sbi_debug_uninstall_triggers(0, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_uninstall_triggers");
> > +
> > +     install_exception_handler(EXC_BREAKPOINT, NULL);
> > +     report_prefix_pop();
> > +}
> > +
> > +static void dbtr_test_multiple_types(struct sbi_dbtr_shmem_entry *shmem, unsigned long type)
> > +{
> > +     static unsigned long test;
> > +
> > +     report_prefix_push("test multiple types");
> > +
> > +     /* check if loads and stores trigger exec */
> > +     if (!dbtr_install_trigger(shmem, &test,
> > +                          gen_tdata1(type, VALUE_EXECUTE | VALUE_LOAD | VALUE_STORE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     report(do_load(&test), "load triggered");
> > +
> > +     report(do_store(&test), "store triggered");
> > +
> > +     dbtr_uninstall_trigger();
> > +
> > +     /* Check if exec works */
> > +     if (!dbtr_install_trigger(shmem, exec_call,
> > +                          gen_tdata1(type, VALUE_EXECUTE | VALUE_LOAD | VALUE_STORE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     report(do_exec(), "exec triggered");
> > +
> > +     dbtr_uninstall_trigger();
> > +     report_prefix_pop();
> > +}
> > +
> > +static void dbtr_test_disable_uninstall(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type)
> > +{
> > +     static unsigned long test;
> > +     struct sbiret ret;
> > +
> > +     report_prefix_push("disable uninstall");
> > +     if (!dbtr_install_trigger(shmem, &test, gen_tdata1(type, VALUE_STORE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     ret = sbi_debug_disable_triggers(0, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_disable_triggers");
> > +
> > +     dbtr_uninstall_trigger();
> > +
> > +     if (!dbtr_install_trigger(shmem, &test, gen_tdata1(type, VALUE_STORE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     report(do_store(&test), "triggered");
> > +
> > +     dbtr_uninstall_trigger();
> > +     report_prefix_pop();
> > +}
> > +
> > +static void dbtr_test_uninstall_enable(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type)
> > +{
> > +     static unsigned long test;
> > +     struct sbiret ret;
> > +
> > +     report_prefix_push("uninstall enable");
> > +     if (!dbtr_install_trigger(shmem, &test, gen_tdata1(type, VALUE_STORE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +     dbtr_uninstall_trigger();
> > +
> > +     ret = sbi_debug_enable_triggers(0, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_enable_triggers");
> > +
> > +     install_exception_handler(EXC_BREAKPOINT, dbtr_exception_handler);
> > +
> > +     report(!do_store(&test), "should not trigger");
> > +
> > +     install_exception_handler(EXC_BREAKPOINT, NULL);
> > +     report_prefix_pop();
> > +}
> > +
> > +static void dbtr_test_uninstall_update(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type)
> > +{
> > +     static unsigned long test;
> > +     struct sbiret ret;
> > +     bool kfail;
> > +
> > +     report_prefix_push("uninstall update");
> > +     if (!dbtr_install_trigger(shmem, NULL, gen_tdata1(type, VALUE_NONE, MODE_NONE))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     dbtr_uninstall_trigger();
> > +
> > +     shmem->id.idx = 0;
> > +     shmem->data.tdata1 = gen_tdata1(type, VALUE_STORE, MODE_S);
> > +     shmem->data.tdata2 = (unsigned long)&test;
> > +
> > +     /*
> > +      * Known broken update_triggers.
> > +      * https://lore.kernel.org/opensbi/aDdp1UeUh7GugeHp@ghost/T/#t
> > +      */
> > +     kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> > +             __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
> > +     ret = sbi_debug_update_triggers(1);
> > +     sbiret_kfail_error(kfail, &ret, SBI_ERR_FAILURE, "sbi_debug_update_triggers");
> > +
> > +     install_exception_handler(EXC_BREAKPOINT, dbtr_exception_handler);
> > +
> > +     report(!do_store(&test), "should not trigger");
> > +
> > +     install_exception_handler(EXC_BREAKPOINT, NULL);
> > +     report_prefix_pop();
> > +}
> > +
> > +static void dbtr_test_disable_read(struct sbi_dbtr_shmem_entry *shmem, enum McontrolType type)
> > +{
> > +     const unsigned long tstatus_expected = SBI_DBTR_TRIG_STATE_S | SBI_DBTR_TRIG_STATE_MAPPED;
> > +     const unsigned long tdata1 = gen_tdata1(type, VALUE_STORE, MODE_NONE);
> > +     static unsigned long test;
> > +     struct sbiret ret;
> > +
> > +     report_prefix_push("disable read");
> > +     if (!dbtr_install_trigger(shmem, &test, gen_tdata1(type, VALUE_STORE, MODE_S))) {
> > +             report_prefix_pop();
> > +             return;
> > +     }
> > +
> > +     ret = sbi_debug_disable_triggers(0, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_disable_triggers");
> > +
> > +     ret = sbi_debug_read_triggers(0, 1);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_read_triggers");
> > +
> > +     report(shmem->data.tdata1 == tdata1, "tdata1 expected: 0x%016lx", tdata1);
> > +     report_info("tdata1 found: 0x%016lx", shmem->data.tdata1);
> > +     report(shmem->data.tdata2 == ((unsigned long)&test), "tdata2 expected: 0x%016lx",
> > +            (unsigned long)&test);
> > +     report_info("tdata2 found: 0x%016lx", shmem->data.tdata2);
> > +     report(shmem->data.tstate == tstatus_expected, "tstate expected: 0x%016lx", tstatus_expected);
> > +     report_info("tstate found: 0x%016lx", shmem->data.tstate);
>
> Same comment about not needing to split this.
>
> > +
> > +     dbtr_uninstall_trigger();
> > +     report_prefix_pop();
> > +}
> > +
> > +void check_dbtr(void)
> > +{
> > +     static struct sbi_dbtr_shmem_entry shmem[RV_MAX_TRIGGERS] = {};
> > +     unsigned long num_trigs;
> > +     enum McontrolType trig_type;
> > +     struct sbiret ret;
> > +
> > +     report_prefix_push("dbtr");
> > +
> > +     if (!sbi_probe(SBI_EXT_DBTR)) {
> > +             report_skip("extension not available");
> > +             report_prefix_pop();
> > +             return;
>
> Could rename the 'error' label to something else and goto it from here
> too.

Is `dtbr_exit_test` ok?

>
> > +     }
> > +
> > +     if (__sbi_get_imp_id() == SBI_IMPL_OPENSBI &&
> > +         __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 5)) {
> > +             report_skip("OpenSBI < v1.5 detected, skipping tests");
> > +             report_prefix_pop();
> > +             return;
>
> Why do we need to do this? Isn't the DBTR probe above enough?

I copied it from the sbi-sse.c file none of the other tests do this so
I'll remove it.

Thanks,
Jesse Taube

>
> > +     }
> > +
> > +     num_trigs = dbtr_test_num_triggers();
> > +     if (!num_trigs)
> > +             goto error;
> > +
> > +     trig_type = dbtr_test_type(&num_trigs);
> > +     if (trig_type == SBI_DBTR_TDATA1_TYPE_NONE)
> > +             goto error;
> > +
> > +     ret = sbi_debug_set_shmem(shmem);
> > +     sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_set_shmem");
> > +
> > +     ret = dbtr_test_store_install_uninstall(&shmem[0], trig_type);
> > +     /* install or uninstall failed */
> > +     if (ret.error != SBI_SUCCESS)
> > +             goto error;
> > +
> > +     dbtr_test_load(&shmem[0], trig_type);
> > +     dbtr_test_exec(&shmem[0], trig_type);
> > +     dbtr_test_read(&shmem[0], trig_type);
> > +     dbtr_test_disable_enable(&shmem[0], trig_type);
> > +     dbtr_test_update(&shmem[0], trig_type);
> > +     dbtr_test_multiple_types(&shmem[0], trig_type);
> > +     dbtr_test_multiple(shmem, trig_type, num_trigs);
> > +     dbtr_test_disable_uninstall(&shmem[0], trig_type);
> > +     dbtr_test_uninstall_enable(&shmem[0], trig_type);
> > +     dbtr_test_uninstall_update(&shmem[0], trig_type);
> > +     dbtr_test_disable_read(&shmem[0], trig_type);
> > +
> > +error:
> > +     report_prefix_pop();
> > +}
> > diff --git a/riscv/sbi-tests.h b/riscv/sbi-tests.h
> > index d5c4ae70..c1ebf016 100644
> > --- a/riscv/sbi-tests.h
> > +++ b/riscv/sbi-tests.h
> > @@ -97,8 +97,10 @@ static inline bool env_enabled(const char *env)
> >       return s && (*s == '1' || *s == 'y' || *s == 'Y');
> >  }
> >
> > +void split_phys_addr(phys_addr_t paddr, unsigned long *hi, unsigned long *lo);
> >  void sbi_bad_fid(int ext);
> >  void check_sse(void);
> > +void check_dbtr(void);
> >
> >  #endif /* __ASSEMBLER__ */
> >  #endif /* _RISCV_SBI_TESTS_H_ */
> > diff --git a/riscv/sbi.c b/riscv/sbi.c
> > index edb1a6be..3b8aadce 100644
> > --- a/riscv/sbi.c
> > +++ b/riscv/sbi.c
> > @@ -105,7 +105,7 @@ static int rand_online_cpu(prng_state *ps)
> >       return cpu;
> >  }
> >
> > -static void split_phys_addr(phys_addr_t paddr, unsigned long *hi, unsigned long *lo)
> > +void split_phys_addr(phys_addr_t paddr, unsigned long *hi, unsigned long *lo)
> >  {
> >       *lo = (unsigned long)paddr;
> >       *hi = 0;
> > @@ -1561,6 +1561,7 @@ int main(int argc, char **argv)
> >       check_susp();
> >       check_sse();
> >       check_fwft();
> > +     check_dbtr();
> >
> >       return report_summary();
> >  }
> > --
> > 2.43.0
> >
>
> Thanks,
> drew



More information about the kvm-riscv mailing list