The current SBI DBTR extension implementation accesses tdata2 and tdata3
without first checking whether either register is implemented on the
underlying hart. This produces an illegal instruction exception on
otherwise spec-compliant cores that legitimately omit one or both
registers.
Per the RISC-V Debug Specification, Chapter 5 (Sdtrig ISA Extension)
and Section 5.7 (Trigger Module Registers):
Section 5 (Sdtrig introduction):
"If Sdtrig is implemented, the Trigger Module must support at least
one trigger. Accessing trigger CSRs that are not used by any of the
implemented triggers must result in an illegal instruction
exception. M-Mode and Debug Mode accesses to trigger CSRs that are
used by any of the implemented triggers must succeed, regardless of
the current type of the currently selected trigger."
Section 5.7 (Trigger Module Registers):
"Attempts to access an unimplemented Trigger Module Register raise
an illegal instruction exception."
Per-register optionality is also explicit:
Section 5.7.3 (Trigger Data 2, at 0x7a2):
"Trigger-specific data. It is optional if no implemented triggers
use it."
Section 5.7.4 (Trigger Data 3, at 0x7a3):
"Trigger-specific data. It is optional if no implemented triggers
use it."
Section 5.7.17 (Trigger Extra (RV32), at 0x7a3), which also applies
via textra64 on RV64:
"All functionality in this register is optional. Any number of
upper bits of mhvalue and svalue may be tied to 0. mhselect and
sselect may only support 0 (ignore)."
Unconditionally accessing tdata2/tdata3 in the install/update/read/
uninstall paths causes SBI calls to fail with an illegal instruction
exception on hardware that does not implement one or both CSRs, even
if the supervisor-supplied trigger configuration does not require the
missing CSR(s).
This patch:
1. Introduces tdata_read_safe() / tdata_write_safe() helpers that
wrap csr_read_allowed / csr_write_allowed so that an illegal-
instruction trap raised by an unimplemented CSR is caught locally
rather than propagated. On the read path, a trapped read yields
zero; on the write path, the trap is silently absorbed (writes to
an unimplemented CSR are no-ops by definition). Every tdata2/tdata3
read and write in the install/update/read/uninstall paths is
converted to these helpers.
2. On the install and update paths, rejects requests that program
a non-zero trig_tdata2 or trig_tdata3 into an unimplemented CSR
with SBI_ERR_NOT_SUPPORTED, matching the SBI spec
wording in sections 19.4 / 19.5:
"One of the trigger configuration can't be programmed due to
unimplemented optional bits in tdata1, tdata2, or tdata3
CSRs."
Implementation status is probed once per call via the
tdata_implemented() helper. This only catches the "whole CSR
unimplemented" case; tied-off WARL bits inside an otherwise-
implemented CSR are not caught here and would require programming
the trigger and reading the value back for comparison, which can
be addressed separately.
3. Enable tdata3 configuration in the debug trigger install path.
References:
- RISC-V Debug Specification, Chapter 5 (Sdtrig), sections 5, 5.7,
5.7.3, 5.7.4, 5.7.17.
- RISC-V SBI Specification v3.0, Chapter 19 (Debug Triggers
Extension), sections 19.4, 19.5.
Fixes: 97f234f15c ("lib: sbi: Introduce the SBI debug triggers extension support")
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Suggested-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
Signed-off-by: David E. Garcia Porras <david.garcia@aheadcomputing.com>
Reviewed-By: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
Link: https://lore.kernel.org/r/20260616170118.3515676-1-david.garcia@aheadcomputing.com
Signed-off-by: Anup Patel <anup@brainfault.org>
This removes redundant hartid to hartindex conversions from four call
sites and provides a net reduction in code size.
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
In `csr_read_allowed` and `csr_write_allowed` macros, has already
converted second param to `ulong`. So delete redundant `ulong`
where uses csr_read/write_allowed macros.
Signed-off-by: Zhang RunMin <runmin.zhang@ingenic.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
In several dbtr functions, we first check that the dbtr trigger is not
NULL and that its state is what we expect. However, it only makes
sense to perform the second check if the dbtr trigger is not NULL.
Othwerwise we will dereference a NULL pointer. Thus, change the
condition so that it shortcuts to the first check if necessary.
Signed-off-by: Carlos López <carlos.lopezr4096@gmail.com>
Reviewed-By: Anup Patel <anup@brainfault.org>
Current Debug Trigger SBI extension proposal suggests to activate
shmem area and obtain its physical address from S-mode software
in the following way:
: If both `shmem_phys_lo` and `shmem_phys_hi` parameters are not
: all-ones bitwise then `shmem_phys_lo` specifies the lower XLEN
: bits and `shmem_phys_hi` specifies the upper XLEN bits of the
: shared memory physical base address. The `shmem_phys_lo` MUST
: be `(XLEN / 8)` byte aligned and the size of shared memory is
: assumed to be `trig_max * (XLEN / 2)` bytes.
For more details see the current version of the proposal:
- https://lists.riscv.org/g/tech-debug/message/1302
On the other hand, on RV32, the M-mode can only access the first 4GB of
the physical address space because M-mode does not have MMU to access
full 34-bit physical address space. Similarly, on RV64, the M-mode can
only access memory addressed by 64 bits.
This commit checks shmem address in function sbi_dbtr_setup_shmem
to make sure that shmem_phys_hi part of the valid address is zero.
Besides, the macro DBTR_SHMEM_MAKE_PHYS is updated to take into
account only low XLEN part.
Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
Reviewed-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
The inline function can simplify the code by setting some call
restrictions. This ensures logical smoothness
Signed-off-by: Xiang W <wxjstz@126.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
Modify the return value of the sbi_dbtr_shmem_disabled function to
bool to make the semantics clearer.
Signed-off-by: Xiang W <wxjstz@126.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
The previous code detected shmem_phys_hi and shmem_phys_lo as two
addresses. fix this bug
Signed-off-by: Xiang W <wxjstz@126.com>
Reviewed-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
Obtaining a 64-bit address under rv64 does not require combining
32-bit registers
Signed-off-by: Xiang W <wxjstz@126.com>
Reviewed-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
After getting hart_shmem_base, dbtr_thishart_state_ptr cannot be
empty. So remove check code.
Signed-off-by: Xiang W <wxjstz@126.com>
Reviewed-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
When only phys_hi is equal to SBI_DBTR_SHMEM_INVALID_ADDR, it may be
a legal address. The old code would modify the legal address.
Signed-off-by: Xiang W <wxjstz@126.com>
Reviewed-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
The trigger allocation function uses bit shift instead of mask to check the
mapped status of the triggers. This causes index 0 to be return always. As a
result, the older triggers are overwritten.
Use the mask for MAPPED field in state word to check if the trigger is mapped.
Fixes: 97f234f15 ("lib: sbi: Introduce the SBI debug triggers extension support")
Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
RISC-V Debug specification includes Sdtrig ISA extension
which describes Trigger Module. Triggers can cause
a breakpoint exception or trace action without execution
of a special instruction. They can be used to implement
hardware breakpoints and watchpoints for native debugging.
The SBI Debut Trigger extension (Draft v6) can be found at:
https://lists.riscv.org/g/tech-debug/topic/99825362#1302
This patch is an initial implementation of SBI Debug
Trigger Extension (Draft v6) in OpenSBI.
The following features are supported:
* mcontrol, mcontrol6 triggers
* Breakpoint and trace actions
NOTE: Chained triggers are not supported
Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
David E. Garcia Porras