Re: [RFC PATCH v2 4/4] target/riscv: smstateen check for AIA/IMSIC

[Weiwei Li]

在 2022/3/23 下午7:13, Mayuresh Chitale 写道:
> If smstateen is implemented then accesses to AIA
> registers CSRS, IMSIC CSRs and other IMSIC registers
> is controlled by setting of corresponding bits in
> mstateen/hstateen registers. Otherwise an illegal
> instruction trap or virtual instruction trap is
> generated.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>   target/riscv/csr.c | 248 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 246 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 5959adc9b3..cfdda8dc2b 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -68,6 +68,53 @@ static RISCVException smstateen_acc_ok(CPURISCVState *env, 
> int mode, int bit)
>       return RISCV_EXCP_NONE;
>   }
>   
> +static RISCVException smstateen_aia_acc_ok(CPURISCVState *env, int csrno)
> +{
> +    int bit, mode;
> +
> +    switch (csrno) {
> +    case CSR_SSETEIPNUM:
> +    case CSR_SCLREIPNUM:
> +    case CSR_SSETEIENUM:
> +    case CSR_SCLREIENUM:
> +    case CSR_STOPEI:
> +    case CSR_VSSETEIPNUM:
> +    case CSR_VSCLREIPNUM:
> +    case CSR_VSSETEIENUM:
> +    case CSR_VSCLREIENUM:
> +    case CSR_VSTOPEI:
> +    case CSR_HSTATUS:
> +        mode = PRV_S;
> +        bit = SMSTATEEN0_IMSIC;
> +        break;
> +
> +    case CSR_SIEH:
> +    case CSR_SIPH:
> +    case CSR_HVIPH:
> +    case CSR_HVICTL:
> +    case CSR_HVIPRIO1:
> +    case CSR_HVIPRIO2:
> +    case CSR_HVIPRIO1H:
> +    case CSR_HVIPRIO2H:
> +    case CSR_VSIEH:
> +    case CSR_VSIPH:
> +        mode = PRV_S;
> +        bit = SMSTATEEN0_AIA;
> +        break;
> +
> +    case CSR_SISELECT:
> +    case CSR_VSISELECT:
> +        mode = PRV_S;
> +        bit = SMSTATEEN0_SVSLCT;
> +        break;
> +
> +    default:
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    return smstateen_acc_ok(env, mode, bit);
> +}
> +
>   static RISCVException fs(CPURISCVState *env, int csrno)
>   {
>   #if !defined(CONFIG_USER_ONLY)
> @@ -1402,6 +1449,13 @@ static int rmw_xiselect(CPURISCVState *env, int csrno, 
> target_ulong *val,
>                           target_ulong new_val, target_ulong wr_mask)
>   {
>       target_ulong *iselect;
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
>   
>       /* Translate CSR number for VS-mode */
>       csrno = aia_xlate_vs_csrno(env, csrno);
> @@ -1484,7 +1538,9 @@ static int rmw_xireg(CPURISCVState *env, int csrno, 
> target_ulong *val,
>       bool virt;
>       uint8_t *iprio;
>       int ret = -EINVAL;
> -    target_ulong priv, isel, vgein;
> +    target_ulong priv, isel, vgein = 0;
> +    CPUState *cs = env_cpu(env);
> +    RISCVCPU *cpu = RISCV_CPU(cs);
>   
>       /* Translate CSR number for VS-mode */
>       csrno = aia_xlate_vs_csrno(env, csrno);
> @@ -1513,11 +1569,20 @@ static int rmw_xireg(CPURISCVState *env, int csrno, 
> target_ulong *val,
>       };
>   
>       /* Find the selected guest interrupt file */
> -    vgein = (virt) ? get_field(env->hstatus, HSTATUS_VGEIN) : 0;
> +    if (virt) {
> +        if (!cpu->cfg.ext_smstateen ||
> +                (env->hstateen[0] & 1UL << SMSTATEEN0_IMSIC)) {
It seems better that '(' aligned with '!'.
> +            vgein = get_field(env->hstatus, HSTATUS_VGEIN);
> +        }
> +    }
>   
>       if (ISELECT_IPRIO0 <= isel && isel <= ISELECT_IPRIO15) {
>           /* Local interrupt priority registers not available for VS-mode */
>           if (!virt) {
> +            if (priv == PRV_S && cpu->cfg.ext_smstateen &&
> +                !(env->hstateen[0] & 1UL << SMSTATEEN0_AIA)) {
> +                goto done;
> +            }
>               ret = rmw_iprio(riscv_cpu_mxl_bits(env),
>                               isel, iprio, val, new_val, wr_mask,
>                               (priv == PRV_M) ? IRQ_M_EXT : IRQ_S_EXT);
> @@ -1551,6 +1616,13 @@ static int rmw_xsetclreinum(CPURISCVState *env, int 
> csrno, target_ulong *val,
>       int ret = -EINVAL;
>       bool set, pend, virt;
>       target_ulong priv, isel, vgein, xlen, nval, wmask;
> +    RISCVException excp;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    excp = smstateen_aia_acc_ok(env, csrno);
> +    if (excp != RISCV_EXCP_NONE) {
> +        return excp;
> +    }
>   
>       /* Translate CSR number for VS-mode */
>       csrno = aia_xlate_vs_csrno(env, csrno);
> @@ -1669,6 +1741,13 @@ static int rmw_xtopei(CPURISCVState *env, int csrno, 
> target_ulong *val,
>       bool virt;
>       int ret = -EINVAL;
>       target_ulong priv, vgein;
> +    RISCVException excp;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    excp = smstateen_aia_acc_ok(env, csrno);
> +    if (excp != RISCV_EXCP_NONE) {
> +        return excp;
> +    }
>   
>       /* Translate CSR number for VS-mode */
>       csrno = aia_xlate_vs_csrno(env, csrno);
> @@ -2014,6 +2093,12 @@ static RISCVException write_mstateen(CPURISCVState *env, 
> int csrno,
>           wr_mask |= 1UL << SMSTATEEN0_FCSR;
>       }
>   
> +    if (riscv_feature(env, RISCV_FEATURE_AIA)) {
> +        wr_mask |= (1UL << SMSTATEEN0_IMSIC)
> +                | (1UL << SMSTATEEN0_AIA)
> +                | (1UL << SMSTATEEN0_SVSLCT);

I think it's better as follows:

+        wr_mask |= (1UL << SMSTATEEN0_IMSIC) |
+                   (1UL << SMSTATEEN0_AIA) |
+                   (1UL << SMSTATEEN0_SVSLCT);

Regards,

Weiwei Li