[v1] RISC-V: Modularize common match conditions for trigger

[PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger

Posted by Alvin Chang via 8 months, 4 weeks ago

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;
         }
         count = itrigger_get_count(env, i);
-- 
2.34.1

Re: [PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger

Posted by Daniel Henrique Barboza 8 months, 3 weeks ago


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);

RE: [PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger

Posted by Alvin Che-Chia Chang(張哲嘉) 8 months, 3 weeks ago

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);

[PATCH 1/4] target/riscv: Add functions for common matching conditions of trigger
[PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint
[PATCH 3/4] target/riscv: Apply modularized matching conditions for watchpoint
[PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger