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

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

Posted by Daniel Henrique Barboza 1 year ago

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

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

Posted by Philippe Mathieu-Daudé 1 year ago

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

Posted by Daniel Henrique Barboza 1 year ago


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

Posted by Philippe Mathieu-Daudé 1 year ago

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