In the RISC-V privileged ISA section 3.1.15 table 15, it is determined
that a debug exception that is triggered from a load/store has a higher
priority than a possible fault that this access might trigger.
This is not the case ATM as shown in [1]. Adding a breakpoint in an
address that deliberately will fault is causing a load page fault
instead of a debug exception. The reason is that we're throwing in the
page fault as soon as the fault occurs (end of riscv_cpu_tlb_fill(),
raise_mmu_exception()), not allowing the installed watchpoints to
trigger.
Call cpu_check_watchpoint() in the page fault path to search and execute
any watchpoints that might exist for the address, never returning back
to the fault path. If no watchpoints are found cpu_check_watchpoint()
will return and we'll fall-through the regular path to
raise_mmu_exception().
[1] https://gitlab.com/qemu-project/qemu/-/issues/2627
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2627
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
target/riscv/cpu_helper.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e1dfc4ecbf..ae0a659ec7 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -27,6 +27,7 @@
#include "exec/page-protection.h"
#include "instmap.h"
#include "tcg/tcg-op.h"
+#include "hw/core/tcg-cpu-ops.h"
#include "trace.h"
#include "semihosting/common-semi.h"
#include "system/cpu-timers.h"
@@ -1708,6 +1709,24 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
} else if (probe) {
return false;
} else {
+ int wp_len = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;
+ int wp_access = 0;
+
+ if (access_type == MMU_DATA_LOAD) {
+ wp_access |= BP_MEM_READ;
+ } else if (access_type == MMU_DATA_STORE) {
+ wp_access |= BP_MEM_WRITE;
+ }
+
+ /*
+ * If a watchpoint isn't found for 'addr' this will
+ * be a no-op and we'll resume the mmu_exception path.
+ * Otherwise we'll throw a debug exception and execution
+ * will continue elsewhere.
+ */
+ cpu_check_watchpoint(cs, address, wp_len, MEMTXATTRS_UNSPECIFIED,
+ wp_access, retaddr);
+
raise_mmu_exception(env, address, access_type, pmp_violation,
first_stage_error, two_stage_lookup,
two_stage_indirect_error);
--
2.47.1On 1/20/25 12:49, Daniel Henrique Barboza wrote:
> In the RISC-V privileged ISA section 3.1.15 table 15, it is determined
> that a debug exception that is triggered from a load/store has a higher
> priority than a possible fault that this access might trigger.
>
> This is not the case ATM as shown in [1]. Adding a breakpoint in an
> address that deliberately will fault is causing a load page fault
> instead of a debug exception. The reason is that we're throwing in the
> page fault as soon as the fault occurs (end of riscv_cpu_tlb_fill(),
> raise_mmu_exception()), not allowing the installed watchpoints to
> trigger.
>
> Call cpu_check_watchpoint() in the page fault path to search and execute
> any watchpoints that might exist for the address, never returning back
> to the fault path. If no watchpoints are found cpu_check_watchpoint()
> will return and we'll fall-through the regular path to
> raise_mmu_exception().
>
> [1] https://gitlab.com/qemu-project/qemu/-/issues/2627
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2627
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
> target/riscv/cpu_helper.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e1dfc4ecbf..ae0a659ec7 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -27,6 +27,7 @@
> #include "exec/page-protection.h"
> #include "instmap.h"
> #include "tcg/tcg-op.h"
> +#include "hw/core/tcg-cpu-ops.h"
> #include "trace.h"
> #include "semihosting/common-semi.h"
> #include "system/cpu-timers.h"
> @@ -1708,6 +1709,24 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> } else if (probe) {
> return false;
> } else {
> + int wp_len = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;
Surely 'size' may be relevant?
r~
> + int wp_access = 0;
> +
> + if (access_type == MMU_DATA_LOAD) {
> + wp_access |= BP_MEM_READ;
> + } else if (access_type == MMU_DATA_STORE) {
> + wp_access |= BP_MEM_WRITE;
> + }
> +
> + /*
> + * If a watchpoint isn't found for 'addr' this will
> + * be a no-op and we'll resume the mmu_exception path.
> + * Otherwise we'll throw a debug exception and execution
> + * will continue elsewhere.
> + */
> + cpu_check_watchpoint(cs, address, wp_len, MEMTXATTRS_UNSPECIFIED,
> + wp_access, retaddr);
> +
> raise_mmu_exception(env, address, access_type, pmp_violation,
> first_stage_error, two_stage_lookup,
> two_stage_indirect_error);
On 1/21/25 12:47 PM, Richard Henderson wrote:
> On 1/20/25 12:49, Daniel Henrique Barboza wrote:
>> In the RISC-V privileged ISA section 3.1.15 table 15, it is determined
>> that a debug exception that is triggered from a load/store has a higher
>> priority than a possible fault that this access might trigger.
>>
>> This is not the case ATM as shown in [1]. Adding a breakpoint in an
>> address that deliberately will fault is causing a load page fault
>> instead of a debug exception. The reason is that we're throwing in the
>> page fault as soon as the fault occurs (end of riscv_cpu_tlb_fill(),
>> raise_mmu_exception()), not allowing the installed watchpoints to
>> trigger.
>>
>> Call cpu_check_watchpoint() in the page fault path to search and execute
>> any watchpoints that might exist for the address, never returning back
>> to the fault path. If no watchpoints are found cpu_check_watchpoint()
>> will return and we'll fall-through the regular path to
>> raise_mmu_exception().
>>
>> [1] https://gitlab.com/qemu-project/qemu/-/issues/2627
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2627
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>> target/riscv/cpu_helper.c | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index e1dfc4ecbf..ae0a659ec7 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -27,6 +27,7 @@
>> #include "exec/page-protection.h"
>> #include "instmap.h"
>> #include "tcg/tcg-op.h"
>> +#include "hw/core/tcg-cpu-ops.h"
>> #include "trace.h"
>> #include "semihosting/common-semi.h"
>> #include "system/cpu-timers.h"
>> @@ -1708,6 +1709,24 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>> } else if (probe) {
>> return false;
>> } else {
>> + int wp_len = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;
>
> Surely 'size' may be relevant?
Ooops. Yeah, no need to infer wp_len if we can use the access size. I'll do a v3.
Thanks,
Daniel
>
>
> r~
>
>> + int wp_access = 0;
>> +
>> + if (access_type == MMU_DATA_LOAD) {
>> + wp_access |= BP_MEM_READ;
>> + } else if (access_type == MMU_DATA_STORE) {
>> + wp_access |= BP_MEM_WRITE;
>> + }
>> +
>> + /*
>> + * If a watchpoint isn't found for 'addr' this will
>> + * be a no-op and we'll resume the mmu_exception path.
>> + * Otherwise we'll throw a debug exception and execution
>> + * will continue elsewhere.
>> + */
>> + cpu_check_watchpoint(cs, address, wp_len, MEMTXATTRS_UNSPECIFIED,
>> + wp_access, retaddr);
>> +
>> raise_mmu_exception(env, address, access_type, pmp_violation,
>> first_stage_error, two_stage_lookup,
>> two_stage_indirect_error);
>
© 2016 - 2025 Red Hat, Inc.