Re: [PATCH 18/18] target/riscv: Move configuration check to envcfg CSRs predicate()

[weiwei]

On 2023/2/15 10:22, Bin Meng wrote:
> On Tue, Feb 14, 2023 at 10:59 PM weiwei <liweiwei@iscas.ac.cn> wrote:
> > On 2023/2/14 22:27, Bin Meng wrote:
> > > At present the envcfg CSRs predicate() routines are generic one like
> > > smode(), hmode. The configuration check is done in the read / write
> > > routine. Create a new predicate routine to cover such check, so that
> > > gdbstub can correctly report its existence.
> > >
> > > Signed-off-by: Bin Meng <bmeng@tinylab.org>
> > >
> > > ---
> > >
> > >    target/riscv/csr.c | 98 +++++++++++++++++++++++++++++-----------------
> > >    1 file changed, 61 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > index 37350b8a6d..284ccc09dd 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> > > @@ -41,40 +41,6 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
> > >    }
> > >
> > >    /* Predicates */
> > > -#if !defined(CONFIG_USER_ONLY)
> > > -static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
> > > -                                       uint64_t bit)
> > > -{
> > > -    bool virt = riscv_cpu_virt_enabled(env);
> > > -    RISCVCPU *cpu = env_archcpu(env);
> > > -
> > > -    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
> > > -        return RISCV_EXCP_NONE;
> > > -    }
> > > -
> > > -    if (!(env->mstateen[index] & bit)) {
> > > -        return RISCV_EXCP_ILLEGAL_INST;
> > > -    }
> > > -
> > > -    if (virt) {
> > > -        if (!(env->hstateen[index] & bit)) {
> > > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > -        }
> > > -
> > > -        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
> > > -            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > -        }
> > > -    }
> > > -
> > > -    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
> > > -        if (!(env->sstateen[index] & bit)) {
> > > -            return RISCV_EXCP_ILLEGAL_INST;
> > > -        }
> > > -    }
> > > -
> > > -    return RISCV_EXCP_NONE;
> > > -}
> > > -#endif
> > >
> > >    static RISCVException fs(CPURISCVState *env, int csrno)
> > >    {
> > > @@ -318,6 +284,32 @@ static RISCVException umode32(CPURISCVState *env, int 
> > > csrno)
> > >        return umode(env, csrno);
> > >    }
> > >
> > > +static RISCVException envcfg(CPURISCVState *env, int csrno)
> > > +{
> > > +    RISCVCPU *cpu = env_archcpu(env);
> > > +    riscv_csr_predicate_fn predicate;
> > > +
> > > +    if (cpu->cfg.ext_smstateen) {
> > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > +    }
> > This check seems not right here.  Why  ILLEGAL_INST is directly
> > triggered if smstateen is enabled?
> This logic was there in the original codes. I was confused when I
> looked at this as well.

Sorry, I didn't find the original codes. Do you mean this:

    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
        return RISCV_EXCP_NONE;
    }

If so, I think the check here is to make the following *stateen related
check ignored when smstateen extension is disabled.

Regards,

Weiwei Li

> Anyway, if it is an issue, it should be a separate patch.
>
> > It seems that smstateen related check will be done  for
> > senvcfg/henvcfg{h} when smstateen is enabled.
> Regards,
> Bin