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

On Thu, Jun 12, 2025 at 11:17:23AM -0700, Jesse Taube wrote:

Add tests for the DBTR SBI extension.

Signed-off-by: Jesse Taube jesse@rivosinc.com Reviewed-by: Charlie Jenkins charlie@rivosinc.com Tested-by: Charlie Jenkins charlie@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

---

lib/riscv/asm/sbi.h | 1 + riscv/Makefile | 1 + riscv/sbi-dbtr.c | 824 ++++++++++++++++++++++++++++++++++++++++++++ riscv/sbi-tests.h | 1 + riscv/sbi.c | 1 + 5 files changed, 828 insertions(+) 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 all_deps += $($(TEST_DIR)/sbi-deps) diff --git a/riscv/sbi-dbtr.c b/riscv/sbi-dbtr.c new file mode 100644 index 00000000..ebaa6f59 --- /dev/null +++ b/riscv/sbi-dbtr.c @@ -0,0 +1,824 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • SBI DBTR testsuite
• • Copyright (C) 2025, Rivos Inc., Jesse Taube jesse@rivosinc.com
• */
• #include <libcflat.h>
• #include <bitops.h>
• #include <asm/io.h>
• #include <asm/processor.h>
• #include "sbi-tests.h"

There shouldn't be spaces in front of the # of the #include lines.

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

We should probably also provide a shift and mask for the reserved bits in order to enable a test to easily set them to something other than zero in order to test the SBI implementation returns SBI_ERR_INVALID_PARAM when it is not zero.

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

Need a DMODE shift for tests to try setting it to something other than zero to check for SBI_ERR_INVALID_PARAM.

+#define SBI_DBTR_TDATA1_MCONTROL6_LOAD_BIT BIT(0) +#define SBI_DBTR_TDATA1_MCONTROL6_STORE_BIT BIT(1) +#define SBI_DBTR_TDATA1_MCONTROL6_EXECUTE_BIT BIT(2) +#define SBI_DBTR_TDATA1_MCONTROL6_U_BIT BIT(3) +#define SBI_DBTR_TDATA1_MCONTROL6_S_BIT BIT(4) +#define SBI_DBTR_TDATA1_MCONTROL6_SELECT_BIT BIT(21) +#define SBI_DBTR_TDATA1_MCONTROL6_VS_BIT BIT(23) +#define SBI_DBTR_TDATA1_MCONTROL6_VU_BIT BIT(24)

These are backwards, VS is 24, VU is 23.

+#define SBI_DBTR_TDATA1_MCONTROL_LOAD_BIT BIT(0) +#define SBI_DBTR_TDATA1_MCONTROL_STORE_BIT BIT(1) +#define SBI_DBTR_TDATA1_MCONTROL_EXECUTE_BIT BIT(2) +#define SBI_DBTR_TDATA1_MCONTROL_U_BIT BIT(3) +#define SBI_DBTR_TDATA1_MCONTROL_S_BIT BIT(4)

We should provide an "_M" definition for a test case to set it to check for SBI_ERR_INVALID_PARAM.

+#define SBI_DBTR_TDATA1_MCONTROL_SELECT_BIT BIT(19)

Remove "_BIT" from all the defines above since that's not in the specified field names.

+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_save(void *tdata2)

Is 'save' the right word here? Or should it be 'store'?

+{

• 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_BIT;
  

• if (value & VALUE_STORE)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL_STORE_BIT;
  

• if (value & VALUE_EXECUTE)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL_EXECUTE_BIT;
  

• if (mode & MODE_M)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL_U_BIT;
  

Setting U when mode has M set?

• if (mode & MODE_U)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL_U_BIT;
  

• if (mode & MODE_S)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL_S_BIT;
  

• 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_BIT;
  

• if (value & VALUE_STORE)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_STORE_BIT;
  

• if (value & VALUE_EXECUTE)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_EXECUTE_BIT;
  

• if (mode & MODE_M)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_U_BIT;
  

Setting U when mode has M set?

• if (mode & MODE_U)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_U_BIT;
  

• if (mode & MODE_S)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_S_BIT;
  

• if (mode & MODE_VU)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_VU_BIT;
  

• if (mode & MODE_VS)

• tdata1 |= SBI_DBTR_TDATA1_MCONTROL6_VS_BIT;
  

• 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:

• return 0;
  

This should be an 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)
  

The parameters on separate lines should line up under the first one, i.e.

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) +{

• phys_addr_t p = virt_to_phys(shmem);
• return sbi_debug_set_shmem_raw(lower_32_bits(p), upper_32_bits(p), 0);

This is a bug on rv64 and we have the same bug in SSE tests and SBI console puts() too. On rv64 we should have hi = 0 and lo = p. See split_phys_addr() in riscv/sbi.c where we got it right for the DBCN tests. We could export split_phys_addr() with riscv/sbi-tests.h and use it here too, but puts() will need its own thing.

+}

+static struct sbiret sbi_debug_read_triggers(unsigned long trig_idx_base,

• 		      unsigned long trig_count)
  

Same parameters formatting comment as above.

+{

• 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)
  

Same parameters formatting comment as above.

+{

• 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)
  

Same parameters formatting comment as above.

+{

• 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)
  

Same parameters formatting comment as above.

+{

• 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 *tdata2,

• 		 unsigned long tdata1)
  

It's a bit odd to call tdata2 a pointer and to order it before tdata1 in the param list.

+{

• struct sbiret sbi_ret;
• bool ret;
• shmem->data.tdata1 = tdata1;
• shmem->data.tdata2 = (unsigned long)tdata2;
• 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 %lu triggers available", ret.value);
  

Need to break this into a report_pass() which doesn't include the number of triggers in the output and a report_info() which outputs the number of triggers. That's because test runners that parse the output should be able to work on multiple platforms which have different trigger numbers. Test runners should ignore INFO output when comparing with expected outputs.

• return ret.value;

+}

+static enum McontrolType dbtr_test_type(unsigned long *num_trig) +{

• struct sbiret ret;
• unsigned long tdata1 = SBI_DBTR_TDATA1_TYPE_MCONTROL6;
• ret = sbi_debug_num_triggers(tdata1);
• sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_num_triggers");

We need a report prefix to distinguish this "sbi_debug_num_triggers" from the other ones.

• if (ret.value > 0) {

• report_pass("sbi_debug_num_triggers: Returned %lu mcontrol6 triggers available",
  

• 	    ret.value);
  

• *num_trig = ret.value;
  

• return tdata1;
  

• }
• tdata1 = SBI_DBTR_TDATA1_TYPE_MCONTROL;
• ret = sbi_debug_num_triggers(tdata1);
• sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_num_triggers");

Same comment, prefix above would be mcontrol6 and this one mcontrol.

• *num_trig = ret.value;
• if (ret.value > 0) {

• report_pass("sbi_debug_num_triggers: Returned %lu mcontrol triggers available",
  

• 	    ret.value);
  

• return tdata1;
  

• }
• report_fail("sbi_debug_num_triggers: Returned 0 mcontrol(6) triggers available");
• return SBI_DBTR_TDATA1_TYPE_NONE;

+}

+static struct sbiret dbtr_test_save_install_uninstall(struct sbi_dbtr_shmem_entry *shmem,

• 				      enum McontrolType type)
  

+{

• static unsigned long test;
• struct sbiret ret;
• report_prefix_push("save_trigger");

Same question about the word 'save' vs. 'store' as above.

• shmem->data.tdata1 = gen_tdata1(type, VALUE_STORE, MODE_S | MODE_S);

^ same mode twice

• 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_save(&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_save(&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
  

• */
  

nit: use opening comment wing (/*) on its own line.

• kfail = __sbi_get_imp_id() == SBI_IMPL_OPENSBI &&

• __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 7);
  

• report_kfail(kfail, do_save(&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_save(&test))

• report_fail("triggered by save");
  

• 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("sbi_debug_disable_triggers");
• 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 (do_save(&test)) {

• report_fail("should not trigger");
  

• dbtr_uninstall_trigger();
  

• report_prefix_pop();
  

• report_skip("sbi_debug_enable_triggers: no disable");
  

• return;
  

• }
• report_pass("should not trigger");

Since we want to report "should not trigger" for both fail and success then we can write this as

if (!report(!do_save(&test), "should not trigger")) { dbtr_uninstall_trigger(); report_prefix_pop(); report_skip("sbi_debug_enable_triggers: no disable"); return; }

• report_prefix_pop();
• report_prefix_push("sbi_debug_enable_triggers");
• ret = sbi_debug_enable_triggers(0, 1);
• sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_enable_triggers");

^ this is already the prefix, so 'enable' is enough.

• report(do_save(&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 saves 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_save(&test))

• report_fail("triggered by save");
  

• 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(), "exec trigger");
• 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("sbi_debug_read_triggers");
• 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");

Same comment about having the same string as the prefix. 'read' is enough.

• report(shmem->data.tdata1 == tdata1, "tdata1 expected: 0x%016lx, found: 0x%016lx",

•       tdata1, shmem->data.tdata1);
  

• report(shmem->data.tdata2 == ((unsigned long)&test),

•       "tdata2 expected: 0x%016lx, found: 0x%016lx", ((unsigned long)&test),
  

•       shmem->data.tdata2);
  

• report(shmem->data.tstate == tstatus_expected, "tstate expected: 0x%016lx, found: 0x%016lx",

•       tstatus_expected, shmem->data.tstate);
  

• 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)

• return;
  

report_skip?

• 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_save(&test[0]), "save triggered");
• if (do_load(&test[0]))

• report_fail("save triggered by load");
  

• report(do_load(&test[1]), "load triggered");
• if (do_save(&test[1]))

• report_fail("load triggered by save");
  

• 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_save(&test[0]), "save 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("dbtr_test_multiple_types");
• /* check if loads and saves 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 trigger");
• report(do_save(&test), "save trigger");
• 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 trigger");
• 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_save(&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_save(&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
  

• */
  

Opening /*

• 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_save(&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, found: 0x%016lx",

•       tdata1, shmem->data.tdata1);
  

• report(shmem->data.tdata2 == ((unsigned long)&test),

•       "tdata2 expected: 0x%016lx, found: 0x%016lx",
  

•       ((unsigned long)&test), shmem->data.tdata2);
  

• report(shmem->data.tstate == tstatus_expected, "tstate expected: 0x%016lx, found: 0x%016lx",

•       tstatus_expected, shmem->data.tstate);
  

• 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;
  

• }
• if (__sbi_get_imp_id() == SBI_IMPL_OPENSBI &&

•    __sbi_get_imp_version() < sbi_impl_opensbi_mk_version(1, 6)) {
  

This is checking less than 1.6, but...

• report_skip("OpenSBI < v1.7 detected, skipping tests");
  

...this is complaining about 1.7

• report_prefix_pop();
  

• return;
  

• }
• 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_save_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..6a227745 100644 --- a/riscv/sbi-tests.h +++ b/riscv/sbi-tests.h @@ -99,6 +99,7 @@ static inline bool env_enabled(const char *env) 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..5bd496d0 100644 --- a/riscv/sbi.c +++ b/riscv/sbi.c @@ -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 for this great start to a very complicated SBI extension. It'll need lots more tests, but we can add them over time.

drew