[PATCH v2 0/2] target/riscv: throw debug exception before page fault

[PATCH v2 0/2] target/riscv: throw debug exception before page fault

[Daniel Henrique Barboza]

Hi,

This second version implements the change Richard suggested in v1, i.e.
do not search for wps twice.

To do that we need to make an assumption that a watchpoint for a 64 bit
address will have size 8, in particular when the user does not set a
watchpoint size. To be consistent we also need to consider that 32 bit
CPUs would use watchpoints of size 4, which is something we're not
considering.  Patch 1 was added to make things more consistent in this
regard.

Patches based on master.

Changes from v1:
- patch 1 (new):
  - add watchpoints of size 4 when dealing with 32 bit addresses
- patch 2:
  - instead of looking for watchpoints twice, call
    cpu_check_watchpoint() and rely on fall-through in case no
    watchpoints are found
- v1 link: https://mail.gnu.org/archive/html/qemu-devel/2025-01/msg03575.html

Daniel Henrique Barboza (2):
  target/riscv/debug.c: use wp size = 4 for 32-bit CPUs
  target/riscv: throw debug exception before page fault

 target/riscv/cpu_helper.c | 19 +++++++++++++++++++
 target/riscv/debug.c      |  6 ++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

-- 
2.47.1

[PATCH v2 1/2] target/riscv/debug.c: use wp size = 4 for 32-bit CPUs

[Daniel Henrique Barboza]

The mcontrol select bit (19) is always zero, meaning our triggers will
always match virtual addresses. In this condition, if the user does not
specify a size for the trigger, the access size defaults to XLEN.

At this moment we're using def_size = 8 regardless of CPU XLEN. Use
def_size = 4 in case we're running 32 bits.

Fixes: 95799e36c1 ("target/riscv: Add initial support for the Sdtrig extension")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/debug.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index f6241a80be..9db4048523 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -478,7 +478,7 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
     bool enabled = type2_breakpoint_enabled(ctrl);
     CPUState *cs = env_cpu(env);
     int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
-    uint32_t size;
+    uint32_t size, def_size;
 
     if (!enabled) {
         return;
@@ -501,7 +501,9 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
             cpu_watchpoint_insert(cs, addr, size, flags,
                                   &env->cpu_watchpoint[index]);
         } else {
-            cpu_watchpoint_insert(cs, addr, 8, flags,
+            def_size = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;
+
+            cpu_watchpoint_insert(cs, addr, def_size, flags,
                                   &env->cpu_watchpoint[index]);
         }
     }
-- 
2.47.1

[PATCH v2 2/2] target/riscv: throw debug exception before page fault

[Daniel Henrique Barboza]

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.1

Re: [PATCH v2 2/2] target/riscv: throw debug exception before page fault

[Richard Henderson]

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?


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);

Re: [PATCH v2 2/2] target/riscv: throw debug exception before page fault

[Daniel Henrique Barboza]

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);
> 

Re: [PATCH v2 1/2] target/riscv/debug.c: use wp size = 4 for 32-bit CPUs

[Philippe Mathieu-Daudé]

On 20/1/25 21:49, Daniel Henrique Barboza wrote:
> The mcontrol select bit (19) is always zero, meaning our triggers will
> always match virtual addresses. In this condition, if the user does not
> specify a size for the trigger, the access size defaults to XLEN.
> 
> At this moment we're using def_size = 8 regardless of CPU XLEN. Use
> def_size = 4 in case we're running 32 bits.
> 
> Fixes: 95799e36c1 ("target/riscv: Add initial support for the Sdtrig extension")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/debug.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index f6241a80be..9db4048523 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -478,7 +478,7 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>       bool enabled = type2_breakpoint_enabled(ctrl);
>       CPUState *cs = env_cpu(env);
>       int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
> -    uint32_t size;
> +    uint32_t size, def_size;
>   
>       if (!enabled) {
>           return;
> @@ -501,7 +501,9 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>               cpu_watchpoint_insert(cs, addr, size, flags,
>                                     &env->cpu_watchpoint[index]);
>           } else {
> -            cpu_watchpoint_insert(cs, addr, 8, flags,
> +            def_size = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;

riscv_cpu_mxl() seems bugprone w.r.t. MXL_RV128, better could be
some riscv_cpu_mxl_wordsize() helper like riscv_cpu_mxl_bits()
(or better named).

Anyway this pattern is already all over, so meanwhile:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +
> +            cpu_watchpoint_insert(cs, addr, def_size, flags,
>                                     &env->cpu_watchpoint[index]);
>           }
>       }

Re: [PATCH v2 1/2] target/riscv/debug.c: use wp size = 4 for 32-bit CPUs

[Daniel Henrique Barboza]

On 1/21/25 2:40 PM, Philippe Mathieu-Daudé wrote:
> On 20/1/25 21:49, Daniel Henrique Barboza wrote:
>> The mcontrol select bit (19) is always zero, meaning our triggers will
>> always match virtual addresses. In this condition, if the user does not
>> specify a size for the trigger, the access size defaults to XLEN.
>>
>> At this moment we're using def_size = 8 regardless of CPU XLEN. Use
>> def_size = 4 in case we're running 32 bits.
>>
>> Fixes: 95799e36c1 ("target/riscv: Add initial support for the Sdtrig extension")
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/debug.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
>> index f6241a80be..9db4048523 100644
>> --- a/target/riscv/debug.c
>> +++ b/target/riscv/debug.c
>> @@ -478,7 +478,7 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>>       bool enabled = type2_breakpoint_enabled(ctrl);
>>       CPUState *cs = env_cpu(env);
>>       int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
>> -    uint32_t size;
>> +    uint32_t size, def_size;
>>       if (!enabled) {
>>           return;
>> @@ -501,7 +501,9 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>>               cpu_watchpoint_insert(cs, addr, size, flags,
>>                                     &env->cpu_watchpoint[index]);
>>           } else {
>> -            cpu_watchpoint_insert(cs, addr, 8, flags,
>> +            def_size = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;
> 
> riscv_cpu_mxl() seems bugprone w.r.t. MXL_RV128, better could be
> some riscv_cpu_mxl_wordsize() helper like riscv_cpu_mxl_bits()
> (or better named).

This existing pattern is benign since we don't have a functional RV128 and
is safe seems to interpret RV64 == RV128.

However, if/when RV128 becomes a thing, we'll spare a moderate amount of agony
if we choose to have a little suffering right now. I'll take a note about it
and perhaps a refactor might be in order.


Thanks,

Daniel

> 
> Anyway this pattern is already all over, so meanwhile:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> +
>> +            cpu_watchpoint_insert(cs, addr, def_size, flags,
>>                                     &env->cpu_watchpoint[index]);
>>           }
>>       }
> 

Re: [PATCH v2 1/2] target/riscv/debug.c: use wp size = 4 for 32-bit CPUs

[Philippe Mathieu-Daudé]

On 21/1/25 19:47, Daniel Henrique Barboza wrote:
> 
> 
> On 1/21/25 2:40 PM, Philippe Mathieu-Daudé wrote:
>> On 20/1/25 21:49, Daniel Henrique Barboza wrote:
>>> The mcontrol select bit (19) is always zero, meaning our triggers will
>>> always match virtual addresses. In this condition, if the user does not
>>> specify a size for the trigger, the access size defaults to XLEN.
>>>
>>> At this moment we're using def_size = 8 regardless of CPU XLEN. Use
>>> def_size = 4 in case we're running 32 bits.
>>>
>>> Fixes: 95799e36c1 ("target/riscv: Add initial support for the Sdtrig 
>>> extension")
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   target/riscv/debug.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)


>>> @@ -501,7 +501,9 @@ static void type2_breakpoint_insert(CPURISCVState 
>>> *env, target_ulong index)
>>>               cpu_watchpoint_insert(cs, addr, size, flags,
>>>                                     &env->cpu_watchpoint[index]);
>>>           } else {
>>> -            cpu_watchpoint_insert(cs, addr, 8, flags,
>>> +            def_size = riscv_cpu_mxl(env) == MXL_RV64 ? 8 : 4;
>>
>> riscv_cpu_mxl() seems bugprone w.r.t. MXL_RV128, better could be
>> some riscv_cpu_mxl_wordsize() helper like riscv_cpu_mxl_bits()
>> (or better named).
> 
> This existing pattern is benign since we don't have a functional RV128 and
> is safe seems to interpret RV64 == RV128.
> 
> However, if/when RV128 becomes a thing, we'll spare a moderate amount of 
> agony

😱

> if we choose to have a little suffering right now. I'll take a note 
> about it
> and perhaps a refactor might be in order.
> 
> 
> Thanks,
> 
> Daniel