Re: [PATCH v2 03/13] target/riscv: Enable S*stateen bits for AIA

[Atish Kumar Patra]

On Mon, Aug 5, 2024 at 5:12 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jul 24, 2024 at 9:31 AM Atish Patra <atishp@rivosinc.com> wrote:
> >
> > As per the ratified AIA spec v1.0, three stateen bits control AIA CSR
> > access.
> >
> > Bit 60 controls the indirect CSRs
> > Bit 59 controls the most AIA CSR state
> > Bit 58 controls the IMSIC state such as stopei and vstopei
> >
> > Enable the corresponding bits in [m|h]stateen and enable corresponding
> > checks in the CSR accessor functions.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  target/riscv/csr.c | 88 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 87 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 58be8bc3cc8c..18b9ae802b15 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -316,19 +316,42 @@ static RISCVException smode32(CPURISCVState *env, int 
> > csrno)
> >
> >  static RISCVException aia_smode(CPURISCVState *env, int csrno)
> >  {
> > +    int ret;
> > +
> >      if (!riscv_cpu_cfg(env)->ext_ssaia) {
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> >
> > +    if (csrno == CSR_STOPEI) {
> > +        ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
> > +    } else {
> > +        ret = smstateen_acc_ok(env, 0, SMSTATEEN0_AIA);
> > +    }
> > +
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      return smode(env, csrno);
> >  }
> >
> >  static RISCVException aia_smode32(CPURISCVState *env, int csrno)
> >  {
> > +    int ret;
> > +
> >      if (!riscv_cpu_cfg(env)->ext_ssaia) {
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> >
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_AIA);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      return smode32(env, csrno);
> >  }
> >
> > @@ -567,15 +590,38 @@ static RISCVException pointer_masking(CPURISCVState 
> > *env, int csrno)
> >
> >  static RISCVException aia_hmode(CPURISCVState *env, int csrno)
> >  {
> > +    int ret;
> > +
> >      if (!riscv_cpu_cfg(env)->ext_ssaia) {
> >          return RISCV_EXCP_ILLEGAL_INST;
> >       }
> >
> > -     return hmode(env, csrno);
> > +    if (csrno == CSR_VSTOPEI) {
> > +        ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
> > +    } else {
> > +        ret = smstateen_acc_ok(env, 0, SMSTATEEN0_AIA);
> > +    }
> > +
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> > +    return hmode(env, csrno);
> >  }
> >
> >  static RISCVException aia_hmode32(CPURISCVState *env, int csrno)
> >  {
> > +    int ret;
> > +
> > +    if (!riscv_cpu_cfg(env)->ext_ssaia) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +     }
> > +
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_AIA);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      if (!riscv_cpu_cfg(env)->ext_ssaia) {
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> > @@ -1992,6 +2038,12 @@ static RISCVException rmw_xiselect(CPURISCVState 
> > *env, int csrno,
> >                                     target_ulong wr_mask)
> >  {
> >      target_ulong *iselect;
> > +    int ret;
> > +
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_SVSLCT);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> >
> >      /* Translate CSR number for VS-mode */
> >      csrno = csrind_xlate_vs_csrno(env, csrno);
> > @@ -2162,6 +2214,11 @@ static RISCVException rmw_xireg(CPURISCVState *env, 
> > int csrno,
> >      int ret = -EINVAL;
> >      target_ulong isel;
> >
> > +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_SVSLCT);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      /* Translate CSR number for VS-mode */
> >      csrno = csrind_xlate_vs_csrno(env, csrno);
> >
> > @@ -2610,6 +2667,22 @@ static RISCVException write_mstateen0(CPURISCVState 
> > *env, int csrno,
> >      if (env->priv_ver >= PRIV_VERSION_1_13_0) {
> >          wr_mask |= SMSTATEEN0_P1P13;
> >      }
> > +    /*
> > +     * TODO: Do we need to check ssaia as well ? Can we enable ssaia 
> > without
> > +     * smaia ?
> > +     */
> > +    if (riscv_cpu_cfg(env)->ext_smaia) {
> > +        wr_mask |= SMSTATEEN0_SVSLCT;
> > +    }
>
> This looks right to me, do we need the TODO?
>

cool. I will remove the TODO.

> Otherwise
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair