[PATCH v2] riscv: sbi: Add SBI Debug Triggers Extension tests
Andrew Jones
andrew.jones at linux.dev
Wed Jun 4 08:40:21 PDT 2025
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 <jesse 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
> ---
> 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 <jesse at rivosinc.com>
> + */
> +
> +#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
More information about the kvm-riscv
mailing list