| Message ID | 20240219032559.79665-3-alvinga@andestech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: Modularize common match conditions for trigger | expand |
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 > 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. 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:
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:
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:
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; + } + 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:
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 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(-)