Hi Jesse, Thanks for this patch, but please see the README of the repository, section "Patches", where we ask that a 'kvm-unit-tests' patch prefix is used. Also please see the RISCV section of the MAINTAINERS file for where to send the patches. You got the kvm-riscv list, but I'd appreciate a CC to my linux.dev account provided there as well.
I'm just taking a quick review pass to look for superficial stuff, since I think others such as Himanshu will look at it from a "does it cover the spec" perspective. On Tue, Jun 03, 2025 at 12:59:31PM -0700, Jesse Taube wrote: > Add tests for the DBTR SBI extension. > > Signed-off-by: Jesse Taube <[email protected]> > --- > 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 > --- > lib/riscv/asm/sbi.h | 28 ++ > lib/riscv/sbi.c | 58 ++++ > riscv/Makefile | 1 + > riscv/sbi-dbtr.c | 751 ++++++++++++++++++++++++++++++++++++++++++++ > riscv/sbi-tests.h | 1 + > riscv/sbi.c | 1 + > 6 files changed, 840 insertions(+) > create mode 100644 riscv/sbi-dbtr.c > > diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h > index a5738a5c..ce19ab89 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 { > @@ -125,6 +126,17 @@ enum sbi_ext_fwft_fid { > > #define SBI_FWFT_SET_FLAG_LOCK BIT(0) > > +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, > +}; I think we can put these and all the dbtr function wrappers below in riscv/sbi-dbtr.c, as I doubt we'll use them outside their test cases. > + > enum sbi_ext_sse_fid { > SBI_EXT_SSE_READ_ATTRS = 0, > SBI_EXT_SSE_WRITE_ATTRS, > @@ -282,6 +294,22 @@ static inline bool sbi_sse_event_is_global(uint32_t > event_id) > return !!(event_id & SBI_SSE_EVENT_GLOBAL_BIT); > } > > +struct sbiret sbi_debug_num_triggers(unsigned long trig_tdata1); > +struct sbiret sbi_debug_set_shmem(void *shmem); > +struct sbiret sbi_debug_set_shmem_raw(unsigned long shmem_phys_lo, > + unsigned long shmem_phys_hi, > + unsigned long flags); > +struct sbiret sbi_debug_read_triggers(unsigned long trig_idx_base, > + unsigned long trig_count); > +struct sbiret sbi_debug_install_triggers(unsigned long trig_count); > +struct sbiret sbi_debug_update_triggers(unsigned long trig_count); > +struct sbiret sbi_debug_uninstall_triggers(unsigned long trig_idx_base, > + unsigned long trig_idx_mask); > +struct sbiret sbi_debug_enable_triggers(unsigned long trig_idx_base, > + unsigned long trig_idx_mask); > +struct sbiret sbi_debug_disable_triggers(unsigned long trig_idx_base, > + unsigned long trig_idx_mask); We won't need these once we've moved the function wrappers. > + > struct sbiret sbi_sse_read_attrs_raw(unsigned long event_id, unsigned long > base_attr_id, > unsigned long attr_count, unsigned long > phys_lo, > unsigned long phys_hi); > diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c > index 2959378f..39c0d3bd 100644 > --- a/lib/riscv/sbi.c > +++ b/lib/riscv/sbi.c > @@ -32,6 +32,64 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long > arg0, > return ret; > } > > +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); > +} > + > +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); > +} > + > +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); > +} > + > +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); > +} > + > +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); > +} > + > +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); > +} > + > +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); > +} > + > +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); > +} > + > +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); > +} When moving these, they'll of course become static, but I don't mind keeping the sbi_ prefix since that's consistent with the other wrappers and the names in the spec. > + > struct sbiret sbi_sse_read_attrs_raw(unsigned long event_id, unsigned long > base_attr_id, > unsigned long attr_count, unsigned long > phys_lo, > unsigned long phys_hi) > 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..fe323f0f > --- /dev/null > +++ b/riscv/sbi-dbtr.c > @@ -0,0 +1,751 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * SBI DBTR testsuite > + * > + * Copyright (C) 2025, Rivos Inc., Jesse Taube <[email protected]> > + */ > + > +#include <asm/io.h> Looks pretty light for the #include list... I prefer we try to add the appropriate includes for everything we use. We don't have to be perfect, but I think a best effort is better than the minimalist approach. > + > +#include "sbi-tests.h" > + > +#define INSN_LEN(insn) ((((insn) & 0x3) < 0x3) ? 2 : 4) Clement just introduced this in the dbltrp test too. Let's add it to lib/riscv/asm/processor.h instead as RV_INSN_LEN() > + > +#if __riscv_xlen == 64 > +#define SBI_DBTR_SHMEM_INVALID_ADDR 0xFFFFFFFFFFFFFFFFUL > +#elif __riscv_xlen == 32 > +#define SBI_DBTR_SHMEM_INVALID_ADDR 0xFFFFFFFFUL > +#else > +#error "Unexpected __riscv_xlen" > +#endif No need for the #if, just use #define SBI_DBTR_SHMEM_INVALID_ADDR -1UL but, we've been giving the users the ability to define invalid addresses themselves (see get_invalid_addr() in riscv/sbi.c). We can export that function (along with get_highest_addr()) and then put their prototypes in riscv/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_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_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) > + > +#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) > +#define SBI_DBTR_TDATA1_MCONTROL_SELECT_BIT 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), > +}; > + > +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 nit: Use /* */ > +static __attribute__((naked)) void exec_call(void) > +{ > + // skip over nop when triggered instead of ret. same nit > + 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 += INSN_LEN(readw((void *)regs->epc)); > +} > + > +static bool do_save(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_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; > + > + 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; > + > + 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; > + } > +} > + > +static bool dbtr_install_trigger(struct sbi_dbtr_shmem_entry *shmem, void > *tdata2, > + unsigned long tdata1) > +{ > + 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; > + //sbi_debug_num_triggers will return trig_max in sbiret.value when > trig_tdata1 == 0 /**/ and maybe don't break the variable declaration list. > + unsigned long tdata1 = 0; > + > + // should be atleast 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); > + > + return ret.value; > +} > + > +static enum McontrolType dbtr_test_type(unsigned long *num_trig) > +{ > + struct sbiret ret; > + > + //sbi_debug_num_triggers will return trig_max in sbiret.value when > trig_tdata1 == 0 /**/ and maybe don't break the variable declaration list. > + unsigned long tdata1 = SBI_DBTR_TDATA1_TYPE_MCONTROL6; > + > + ret = sbi_debug_num_triggers(tdata1); > + sbiret_report_error(&ret, SBI_SUCCESS, "sbi_debug_num_triggers"); > + 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"); > + *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; > +} > + The rest looked good (from a superficial perspective). Nice! Thanks, drew
