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

[Daniel Henrique Barboza]

On 2/21/24 22:46, Alvin Che-Chia Chang(張哲嘉) wrote:
> 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?

It looks correct to me. We just need to mention in the commit msg that the 
behavior
change is intentional, not an accident.


Thanks,

Daniel


> 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: