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

[Alistair Francis]

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?

Otherwise

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

Alistair