| Message ID | 20250120204910.1317013-2-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: throw debug exception before page fault | expand |
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]); > } > }
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]); >> } >> } >
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
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]); } }
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(-)