Hi Daniel,
> -----Original Message-----
> From: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
> Sent: Thursday, February 22, 2024 1:26 AM
> To: Alvin Che-Chia Chang(張哲嘉) <alvi...@andestech.com>;
> qemu-ri...@nongnu.org; qemu-devel@nongnu.org
> Cc: alistair.fran...@wdc.com; bin.m...@windriver.com;
> liwei1...@gmail.com; zhiwei_...@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 <alvi...@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:
