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