| Message ID | 20240219032559.79665-5-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. We > can invoke trigger_common_match() to check the privilege levels of the > type 3 triggers. > > Signed-off-by: Alvin Chang <alvinga@andestech.com> > --- > target/riscv/debug.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/riscv/debug.c b/target/riscv/debug.c > index 67ba19c966..de996a393c 100644 > --- a/target/riscv/debug.c > +++ b/target/riscv/debug.c > @@ -624,7 +624,7 @@ void helper_itrigger_match(CPURISCVState *env) > if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) { > continue; > } > - if (check_itrigger_priv(env, i)) { > + if (!trigger_common_match(env, TRIGGER_TYPE_INST_CNT, i)) { > continue; > } Looks good. Shouldn't we also change riscv_itrigger_enabled() to also use trigger_common_match()? riscv_itrigger_enabled() is remarkably similar to helper_itrigger_match() so I believe we can also use the new function there. Thanks, Daniel > count = itrigger_get_count(env, i);
Hi Daniel, > -----Original Message----- > From: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > Sent: Thursday, February 22, 2024 2:06 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 4/4] target/riscv: Apply modularized matching conditions > for icount trigger > > > > 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. We can invoke trigger_common_match() to check the privilege > > levels of the type 3 triggers. > > > > Signed-off-by: Alvin Chang <alvinga@andestech.com> > > --- > > target/riscv/debug.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/target/riscv/debug.c b/target/riscv/debug.c index > > 67ba19c966..de996a393c 100644 > > --- a/target/riscv/debug.c > > +++ b/target/riscv/debug.c > > @@ -624,7 +624,7 @@ void helper_itrigger_match(CPURISCVState *env) > > if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) { > > continue; > > } > > - if (check_itrigger_priv(env, i)) { > > + if (!trigger_common_match(env, TRIGGER_TYPE_INST_CNT, i)) { > > continue; > > } > > > Looks good. Shouldn't we also change riscv_itrigger_enabled() to also use > trigger_common_match()? riscv_itrigger_enabled() is remarkably similar to > helper_itrigger_match() so I believe we can also use the new function there. I think we might not want to apply trigger_common_match() into riscv_itrigger_enabled(). The trigger_common_match() is used to check if the trigger can be matched in current privilege level. It will check many conditions: trigger privilege levels, textra, tcontrol, etc. The riscv_itrigger_enabled() is used to check if any icount trigger is enabled by checking vs/vu/count/s/u fields of tdata1 only. In fact, we found the comparisons between tdata1 bit-fields and env->priv in check_itrigger_priv() are bugs. And we have a patch to fix that: bool riscv_itrigger_enabled(CPURISCVState *env) { int count; for (int i = 0; i < RV_MAX_TRIGGERS; i++) { if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) { continue; } - if (check_itrigger_priv(env, i)) { + if ((env->tdata1[i] & ITRIGGER_VS) == 0 && + (env->tdata1[i] & ITRIGGER_VU) == 0 && + (env->tdata1[i] & ITRIGGER_U) == 0 && + (env->tdata1[i] & ITRIGGER_S) == 0 && + (env->tdata1[i] & ITRIGGER_M) == 0 ) { continue; } count = itrigger_get_count(env, i); if (!count) { continue; } return true; } return false; } Sincerely, Alvin Chang > > > Thanks, > > Daniel > > > > > > > count = itrigger_get_count(env, i);
diff --git a/target/riscv/debug.c b/target/riscv/debug.c index 67ba19c966..de996a393c 100644 --- a/target/riscv/debug.c +++ b/target/riscv/debug.c @@ -624,7 +624,7 @@ void helper_itrigger_match(CPURISCVState *env) if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) { continue; } - if (check_itrigger_priv(env, i)) { + if (!trigger_common_match(env, TRIGGER_TYPE_INST_CNT, i)) { continue; } count = itrigger_get_count(env, i);
We have implemented trigger_common_match(), which checks if the enabled privilege levels of the trigger match CPU's current privilege level. We can invoke trigger_common_match() to check the privilege levels of the type 3 triggers. Signed-off-by: Alvin Chang <alvinga@andestech.com> --- target/riscv/debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)