RE: [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint

[張哲嘉]

Hi Daniel,

> -----Original Message-----
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Sent: Thursday, February 22, 2024 1:26 AM
> To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>;
> qemu-riscv@nongnu.org; qemu-devel@nongnu.org
> Cc: alistair.francis@wdc.com; bin.meng@windriver.com;
> liwei1518@gmail.com; zhiwei_liu@linux.alibaba.com
> Subject: Re: [PATCH 2/4] target/riscv: Apply modularized matching conditions
> for breakpoint
> 
> 
> 
> On 2/19/24 00:25, Alvin Chang wrote:
> > We have implemented trigger_common_match(), which checks if the
> > enabled privilege levels of the trigger match CPU's current privilege level.
> > Remove the related code in riscv_cpu_debug_check_breakpoint() and
> > invoke
> > trigger_common_match() to check the privilege levels of the type 2 and
> > type 6 triggers for the breakpoints.
> >
> > Only the execution bit and the executed PC should be futher checked in
> 
> typo: further

Thanks! Will fix it.

> 
> > riscv_cpu_debug_check_breakpoint().
> >
> > Signed-off-by: Alvin Chang <alvinga@andestech.com>
> > ---
> >   target/riscv/debug.c | 26 ++++++--------------------
> >   1 file changed, 6 insertions(+), 20 deletions(-)
> >
> > diff --git a/target/riscv/debug.c b/target/riscv/debug.c index
> > 7932233073..b971ed5d7a 100644
> > --- a/target/riscv/debug.c
> > +++ b/target/riscv/debug.c
> > @@ -855,21 +855,17 @@ bool riscv_cpu_debug_check_breakpoint(CPUState
> *cs)
> >           for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> >               trigger_type = get_trigger_type(env, i);
> >
> > +            if (!trigger_common_match(env, trigger_type, i)) {
> > +                continue;
> > +            }
> > +
> 
> I believe this will change how the function behaves. Before this patch, we
> would 'return false' if we have a TRIGGER_TYPE_AD_MATCH and
> env->virt_enabled is true.
> 
> Now, for the same case, we'll keep looping until we found a match, or return
> 'false'
> if we run out of triggers.

Oh! I didn't notice that the behavior is changed.. thank you.

Image that CPU supports both type 2 and type 6 triggers, and the debugger sets 
two identical breakpoints.(this should be a mistake, but we should not restrict 
the debugger)
One of them is type 2 breakpoint at trigger index 0, and the other is type 6 
breakpoint at trigger index 1.
Now if the system runs in VS/VU modes, it could match type 6 breakpoint, so the 
looping is necessary.
If the system runs in M/HS/U modes, it could match type 2 breakpoint.
Is my understanding correct?


Sincerely,
Alvin

> 
> This seems ok to do, but we should state in the commit msg that we're
> intentionally change how the function works. If that's not the idea and we 
> want
> to preserve the existing behavior, we would need to do:
> 
> >
> > +            if (!trigger_common_match(env, trigger_type, i)) {
> > +                return false;
> > +            }
> 
> Instead of just continue looping. Thanks,
> 
> 
> Daniel
> 
> >               switch (trigger_type) {
> >               case TRIGGER_TYPE_AD_MATCH:
> > -                /* type 2 trigger cannot be fired in VU/VS mode */
> > -                if (env->virt_enabled) {
> > -                    return false;
> > -                }
> > -
> >                   ctrl = env->tdata1[i];
> >                   pc = env->tdata2[i];
> >
> >                   if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
> > -                    /* check U/S/M bit against current privilege level
> */
> > -                    if ((ctrl >> 3) & BIT(env->priv)) {
> > -                        return true;
> > -                    }
> > +                    return true;
> >                   }
> >                   break;
> >               case TRIGGER_TYPE_AD_MATCH6:
> > @@ -877,17 +873,7 @@ bool riscv_cpu_debug_check_breakpoint(CPUState
> *cs)
> >                   pc = env->tdata2[i];
> >
> >                   if ((ctrl & TYPE6_EXEC) && (bp->pc == pc)) {
> > -                    if (env->virt_enabled) {
> > -                        /* check VU/VS bit against current privilege
> level */
> > -                        if ((ctrl >> 23) & BIT(env->priv)) {
> > -                            return true;
> > -                        }
> > -                    } else {
> > -                        /* check U/S/M bit against current privilege
> level */
> > -                        if ((ctrl >> 3) & BIT(env->priv)) {
> > -                            return true;
> > -                        }
> > -                    }
> > +                    return true;
> >                   }
> >                   break;
> >               default: