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

[張哲嘉]

Hi Daniel,

> -----Original Message-----
> From: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
> Sent: Thursday, February 22, 2024 2:06 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 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 <alvi...@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);