| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPd4Wexw_DtndDqRZzXK0PEOAkb4yWB4x8rf5eRdJOLMS-+8SQ@mail.gmail.com>
Date: Tue, 26 Aug 2025 10:08:04 +0530
From: Himanshu Chauhan <hchauhan@...tanamicro.com>
To: Jesse Taube <jesse@...osinc.com>
Cc: linux-riscv@...ts.infradead.org, Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Alexandre Ghiti <alex@...ti.fr>, Oleg Nesterov <oleg@...hat.com>, Kees Cook <kees@...nel.org>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>,
Liang Kan <kan.liang@...ux.intel.com>, Shuah Khan <shuah@...nel.org>,
Charlie Jenkins <charlie@...osinc.com>, Samuel Holland <samuel.holland@...ive.com>,
Conor Dooley <conor.dooley@...rochip.com>, Deepak Gupta <debug@...osinc.com>,
Andrew Jones <ajones@...tanamicro.com>, Atish Patra <atishp@...osinc.com>,
Anup Patel <apatel@...tanamicro.com>, Mayuresh Chitale <mchitale@...tanamicro.com>,
Evan Green <evan@...osinc.com>, WangYuli <wangyuli@...ontech.com>,
Huacai Chen <chenhuacai@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Andrew Morton <akpm@...ux-foundation.org>, Luis Chamberlain <mcgrof@...nel.org>,
"Mike Rapoport (Microsoft)" <rppt@...nel.org>, Nam Cao <namcao@...utronix.de>,
Yunhui Cui <cuiyunhui@...edance.com>, Joel Granados <joel.granados@...nel.org>,
Clément Léger <cleger@...osinc.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>, Celeste Liu <coelacanthushex@...il.com>,
Chunyan Zhang <zhangchunyan@...as.ac.cn>, Nylon Chen <nylon.chen@...ive.com>,
Thomas Gleixner <tglx@...utronix.de>, Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
Vincenzo Frascino <vincenzo.frascino@....com>, Joey Gouly <joey.gouly@....com>,
Ravi Bangoria <ravi.bangoria@....com>, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
linux-perf-users@...r.kernel.org, linux-kselftest@...r.kernel.org,
Joel Stanley <joel@....id.au>
Subject: Re: [PATCH 5/8] riscv: hw_breakpoint: Use icount for single stepping
On Fri, Aug 22, 2025 at 11:17 PM Jesse Taube <jesse@...osinc.com> wrote:
>
> The Sdtrig RISC-V ISA extension does not have a resume flag for
> returning to and executing the instruction at the breakpoint.
> To avoid skipping the instruction or looping, it is necessary to remove
> the hardware breakpoint and single step. Use the icount feature of
> Sdtrig to accomplish this. Use icount as default with an option to allow
> software-based single stepping when hardware or SBI does not have
> icount functionality, as it may cause unwanted side effects when reading
> the instruction from memory.
Can you please elaborate on this? I remember noticing the absence of
the resume flag which was causing loops.
>
> Signed-off-by: Jesse Taube <jesse@...osinc.com>
> ---
> OpenSBI implementation of sbi_debug_read_triggers does not return the
> updated CSR values. There needs to be a check for working
> sbi_debug_read_triggers before this works.
>
> https://lists.riscv.org/g/tech-prs/message/1476
>
> RFC -> V1:
> - Add dbtr_mode to rv_init_icount_trigger
> - Add icount_triggered to check which breakpoint was triggered
> - Fix typo: s/affects/effects
> - Move HW_BREAKPOINT_COMPUTE_STEP to Platform type
> V1 -> V2:
> - Remove HW_BREAKPOINT_COMPUTE_STEP kconfig option
> ---
> arch/riscv/kernel/hw_breakpoint.c | 173 ++++++++++++++++++++++++++----
> 1 file changed, 155 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> index 3f96e744a711..f12306247436 100644
> --- a/arch/riscv/kernel/hw_breakpoint.c
> +++ b/arch/riscv/kernel/hw_breakpoint.c
> @@ -20,6 +20,7 @@
> #define DBTR_TDATA1_DMODE BIT_UL(__riscv_xlen - 5)
>
> #define DBTR_TDATA1_TYPE_MCONTROL (2UL << DBTR_TDATA1_TYPE_SHIFT)
> +#define DBTR_TDATA1_TYPE_ICOUNT (3UL << DBTR_TDATA1_TYPE_SHIFT)
> #define DBTR_TDATA1_TYPE_MCONTROL6 (6UL << DBTR_TDATA1_TYPE_SHIFT)
>
> #define DBTR_TDATA1_MCONTROL6_LOAD BIT(0)
> @@ -62,6 +63,14 @@
> (FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZELO_FIELD, lo) | \
> FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZEHI_FIELD, hi))
>
> +#define DBTR_TDATA1_ICOUNT_U BIT(6)
> +#define DBTR_TDATA1_ICOUNT_S BIT(7)
> +#define DBTR_TDATA1_ICOUNT_PENDING BIT(8)
> +#define DBTR_TDATA1_ICOUNT_M BIT(9)
> +#define DBTR_TDATA1_ICOUNT_COUNT_FIELD GENMASK(23, 10)
> +#define DBTR_TDATA1_ICOUNT_VU BIT(25)
> +#define DBTR_TDATA1_ICOUNT_VS BIT(26)
> +
> enum dbtr_mode {
> DBTR_MODE_U = 0,
> DBTR_MODE_S,
> @@ -79,6 +88,7 @@ static DEFINE_PER_CPU(union sbi_dbtr_shmem_entry, sbi_dbtr_shmem);
>
> /* number of debug triggers on this cpu . */
> static int dbtr_total_num __ro_after_init;
> +static bool have_icount __ro_after_init;
> static unsigned long dbtr_type __ro_after_init;
> static unsigned long dbtr_init __ro_after_init;
>
> @@ -129,6 +139,7 @@ static int arch_smp_teardown_sbi_shmem(unsigned int cpu)
> static void init_sbi_dbtr(void)
> {
> struct sbiret ret;
> + unsigned long dbtr_count = 0;
>
> /*
> * Called by hw_breakpoint_slots and arch_hw_breakpoint_init.
> @@ -143,6 +154,19 @@ static void init_sbi_dbtr(void)
> return;
> }
>
> + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> + DBTR_TDATA1_TYPE_ICOUNT, 0, 0, 0, 0, 0);
> + if (ret.error) {
> + pr_warn("%s: failed to detect icount triggers. error: %ld.\n",
> + __func__, ret.error);
> + } else if (!ret.value) {
> + pr_warn("%s: No icount triggers available. "
> + "Falling-back to computing single step address.\n", __func__);
> + } else {
> + dbtr_count = ret.value;
> + have_icount = true;
> + }
> +
> ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> DBTR_TDATA1_TYPE_MCONTROL6, 0, 0, 0, 0, 0);
> if (ret.error) {
> @@ -151,7 +175,7 @@ static void init_sbi_dbtr(void)
> } else if (!ret.value) {
> pr_warn("%s: No mcontrol6 triggers available.\n", __func__);
> } else {
> - dbtr_total_num = ret.value;
> + dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count);
> dbtr_type = DBTR_TDATA1_TYPE_MCONTROL6;
> return;
> }
> @@ -166,7 +190,7 @@ static void init_sbi_dbtr(void)
> pr_err("%s: No mcontrol triggers available.\n", __func__);
> dbtr_total_num = 0;
> } else {
> - dbtr_total_num = ret.value;
> + dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count);
> dbtr_type = DBTR_TDATA1_TYPE_MCONTROL;
> }
> }
> @@ -320,6 +344,36 @@ static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr,
> return 0;
> }
>
> +static int rv_init_icount_trigger(struct arch_hw_breakpoint *hw, enum dbtr_mode mode)
> +{
> + unsigned long tdata1 = DBTR_TDATA1_TYPE_ICOUNT;
> +
> + /* Step one instruction */
> + tdata1 |= FIELD_PREP(DBTR_TDATA1_ICOUNT_COUNT_FIELD, 1);
> +
> + switch (mode) {
> + case DBTR_MODE_U:
> + tdata1 |= DBTR_TDATA1_ICOUNT_U;
> + break;
> + case DBTR_MODE_S:
> + tdata1 |= DBTR_TDATA1_ICOUNT_S;
> + break;
> + case DBTR_MODE_VS:
> + tdata1 |= DBTR_TDATA1_ICOUNT_VS;
> + break;
> + case DBTR_MODE_VU:
> + tdata1 |= DBTR_TDATA1_ICOUNT_VU;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + hw->tdata1 = tdata1;
> + hw->tdata2 = 0;
> +
> + return 0;
> +}
> +
> int hw_breakpoint_arch_parse(struct perf_event *bp,
> const struct perf_event_attr *attr,
> struct arch_hw_breakpoint *hw)
> @@ -372,24 +426,28 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs)
> /* Remove breakpoint even if return error as not to loop */
> arch_uninstall_hw_breakpoint(event);
>
> - ret = get_insn_nofault(regs, regs->epc, &insn);
> - if (ret < 0)
> - return ret;
> + if (have_icount) {
> + rv_init_icount_trigger(bp, DBTR_MODE_U);
> + } else {
> + ret = get_insn_nofault(regs, regs->epc, &insn);
> + if (ret < 0)
> + return ret;
>
> - next_addr = get_step_address(regs, insn);
> + next_addr = get_step_address(regs, insn);
>
> - ret = get_insn_nofault(regs, next_addr, &insn);
> - if (ret < 0)
> - return ret;
> + ret = get_insn_nofault(regs, next_addr, &insn);
> + if (ret < 0)
> + return ret;
>
> - bp_insn.bp_type = HW_BREAKPOINT_X;
> - bp_insn.bp_addr = next_addr;
> - /* Get the size of the intruction */
> - bp_insn.bp_len = GET_INSN_LENGTH(insn);
> + bp_insn.bp_type = HW_BREAKPOINT_X;
> + bp_insn.bp_addr = next_addr;
> + /* Get the size of the intruction */
> + bp_insn.bp_len = GET_INSN_LENGTH(insn);
>
> - ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);
> - if (ret)
> - return ret;
> + ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);
> + if (ret)
> + return ret;
> + }
>
> ret = arch_install_hw_breakpoint(event);
> if (ret)
> @@ -400,6 +458,79 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs)
> return 0;
> }
>
> +/**
> + * icount_triggered - Check if event's icount was triggered.
> + * @event: Perf event to check
> + *
> + * Check the given perf event's icount breakpoint was triggered.
> + *
> + * Returns: 1 if icount was triggered.
> + * 0 if icount was not triggered.
> + * negative on failure.
> + */
> +static int icount_triggered(struct perf_event *event)
> +{
> + union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(&sbi_dbtr_shmem);
> + struct sbiret ret;
> + struct perf_event **slot;
> + unsigned long tdata1;
> + int i;
> +
> + for (i = 0; i < dbtr_total_num; i++) {
> + slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);
> +
> + if (*slot == event)
> + break;
> + }
> +
> + if (i == dbtr_total_num) {
> + pr_warn("%s: Breakpoint not installed.\n", __func__);
> + return -ENOENT;
> + }
> +
> + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> + *this_cpu_ptr(&ecall_lock_flags));
> +
> + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ,
> + i, 1, 0, 0, 0, 0);
> + tdata1 = shmem->data.tdata1;
> +
> + raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock),
> + *this_cpu_ptr(&ecall_lock_flags));
> + if (ret.error) {
> + pr_warn("%s: failed to read trigger. error: %ld\n", __func__, ret.error);
> + return sbi_err_map_linux_errno(ret.error);
To avoid a flurry of events or messages, it would probably be good to
disable the trigger.
> + }
> +
> + /*
> + * The RISC-V Debug Specification
> + * Tim Newsome, Paul Donahue (Ventana Micro Systems)
> + * Version 1.0, Revised 2025-02-21: Ratified
I think mentioning the version number and section would be enough.
> + * 5.7.13. Instruction Count (icount, at 0x7a1)
> + * When count is 1 and the trigger matches, then pending becomes set.
> + * In addition count will become 0 unless it is hard-wired to 1.
> + * When pending is set, the trigger fires just before any further
> + * instructions are executed in a mode where the trigger is enabled.
> + * As the trigger fires, pending is cleared. In addition, if count is
> + * hard-wired to 1 then m, s, u, vs, and vu are all cleared.
> + */
> + if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) == 0)
> + return 1;
> +
> + if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) != 1)
> + return 0;
> +
> + if (tdata1 & DBTR_TDATA1_ICOUNT_U)
> + return 0;
> + if (tdata1 & DBTR_TDATA1_ICOUNT_S)
> + return 0;
> + if (tdata1 & DBTR_TDATA1_ICOUNT_VU)
> + return 0;
> + if (tdata1 & DBTR_TDATA1_ICOUNT_VU)
> + return 0;
> + return 1;
> +}
> +
> /*
> * HW Breakpoint/watchpoint handler
> */
> @@ -460,7 +591,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
>
> if (bp->in_callback) {
> expecting_callback = true;
> - if (regs->epc != bp->next_addr) {
> + if (have_icount) {
> + if (icount_triggered(event) != 1)
> + continue;
> + } else if (regs->epc != bp->next_addr) {
> continue;
> }
>
> @@ -477,7 +611,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
>
> }
>
> - if (expecting_callback) {
> + if (expecting_callback && have_icount) {
> + pr_err("%s: in_callback was set, but icount was not triggered, epc (%lx).\n",
> + __func__, regs->epc);
> + } else if (expecting_callback) {
> pr_err("%s: in_callback was set, but epc (%lx) was not at next address(%lx).\n",
> __func__, regs->epc, bp->next_addr);
> }
Is this just for debugging or do you want to commit it?
Regards
Himanshu
> --
> 2.43.0
>
Powered by blists - more mailing lists