Re: [PATCH v3 04/11] target/riscv: Support generic CSR indirect access

[Atish Kumar Patra]

On Thu, Nov 28, 2024 at 3:52 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 11/17/24 10:15 PM, Atish Patra wrote:
> > From: Kaiwen Xue <kaiwenx@rivosinc.com>
> >
> > This adds the indirect access registers required by sscsrind/smcsrind
> > and the operations on them. Note that xiselect and xireg are used for
> > both AIA and sxcsrind, and the behavior of accessing them depends on
> > whether each extension is enabled and the value stored in xiselect.
> >
> > Co-developed-by: Atish Patra <atishp@rivosinc.com>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
> > ---
> >   target/riscv/cpu_bits.h |  28 ++++++++-
> >   target/riscv/csr.c      | 149 
> > ++++++++++++++++++++++++++++++++++++++++++++++--
> >   2 files changed, 171 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 385a2c67c24b..e13c5420a251 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -173,6 +173,13 @@
> >   #define CSR_MISELECT        0x350
> >   #define CSR_MIREG           0x351
> >
> > +/* Machine Indirect Register Alias */
> > +#define CSR_MIREG2          0x352
> > +#define CSR_MIREG3          0x353
> > +#define CSR_MIREG4          0x355
> > +#define CSR_MIREG5          0x356
> > +#define CSR_MIREG6          0x357
> > +
> >   /* Machine-Level Interrupts (AIA) */
> >   #define CSR_MTOPEI          0x35c
> >   #define CSR_MTOPI           0xfb0
> > @@ -222,6 +229,13 @@
> >   #define CSR_SISELECT        0x150
> >   #define CSR_SIREG           0x151
> >
> > +/* Supervisor Indirect Register Alias */
> > +#define CSR_SIREG2          0x152
> > +#define CSR_SIREG3          0x153
> > +#define CSR_SIREG4          0x155
> > +#define CSR_SIREG5          0x156
> > +#define CSR_SIREG6          0x157
> > +
> >   /* Supervisor-Level Interrupts (AIA) */
> >   #define CSR_STOPEI          0x15c
> >   #define CSR_STOPI           0xdb0
> > @@ -288,6 +302,13 @@
> >   #define CSR_VSISELECT       0x250
> >   #define CSR_VSIREG          0x251
> >
> > +/* Virtual Supervisor Indirect Alias */
> > +#define CSR_VSIREG2         0x252
> > +#define CSR_VSIREG3         0x253
> > +#define CSR_VSIREG4         0x255
> > +#define CSR_VSIREG5         0x256
> > +#define CSR_VSIREG6         0x257
> > +
> >   /* VS-Level Interrupts (H-extension with AIA) */
> >   #define CSR_VSTOPEI         0x25c
> >   #define CSR_VSTOPI          0xeb0
> > @@ -863,10 +884,13 @@ typedef enum RISCVException {
> >   #define ISELECT_IMSIC_EIE63                0xff
> >   #define ISELECT_IMSIC_FIRST                ISELECT_IMSIC_EIDELIVERY
> >   #define ISELECT_IMSIC_LAST                 ISELECT_IMSIC_EIE63
> > -#define ISELECT_MASK                       0x1ff
> > +#define ISELECT_MASK_AIA                   0x1ff
> > +
> > +/* MISELECT, SISELECT, and VSISELECT bits (AIA) */
> > +#define ISELECT_MASK_SXCSRIND              0xfff
> >
> >   /* Dummy [M|S|VS]ISELECT value for emulating [M|S|VS]TOPEI CSRs */
> > -#define ISELECT_IMSIC_TOPEI                (ISELECT_MASK + 1)
> > +#define ISELECT_IMSIC_TOPEI                (ISELECT_MASK_AIA + 1)
> >
> >   /* IMSIC bits (AIA) */
> >   #define IMSIC_TOPEI_IID_SHIFT              16
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index c91a26a52ef6..57e9c9e25f02 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -306,6 +306,17 @@ static RISCVException aia_any32(CPURISCVState *env, 
> > int csrno)
> >       return any32(env, csrno);
> >   }
> >
> > +static RISCVException csrind_any(CPURISCVState *env, int csrno)
> > +{
> > +    RISCVCPU *cpu = env_archcpu(env);
>
> It is desirable to avoid env_archcpu() calls because it's a bit costly. If 
> you're
> doing something else with the RISCVCPU pointer then it is what it is, but if 
> you
> want it just to access cpu_cfg you can use riscv_cpu_cfg(env). This is being 
> done
> in patch 2:
>

Yeah. This one slipped from the original patch. Thanks for noticing it. Fixed.

>
> +    if (!riscv_cpu_cfg(env)->ext_smaia && !riscv_cpu_cfg(env)->ext_smcsrind) 
> {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
>
>
> In fact this is also being done in rmw_xiselect down below.
>
> > +
> > +    if (!cpu->cfg.ext_smcsrind) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >   static RISCVException csrind_or_aia_any(CPURISCVState *env, int csrno)
> >   {
> >       if (!riscv_cpu_cfg(env)->ext_smaia && 
> > !riscv_cpu_cfg(env)->ext_smcsrind) {
> > @@ -389,6 +400,15 @@ static bool 
> > csrind_or_aia_extensions_present(CPURISCVState *env)
> >       return csrind_extensions_present(env) || aia_extensions_present(env);
> >   }
> >
> > +static RISCVException csrind_smode(CPURISCVState *env, int csrno)
> > +{
> > +    if (!csrind_extensions_present(env)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return smode(env, csrno);
> > +}
> > +
> >   static RISCVException csrind_or_aia_smode(CPURISCVState *env, int csrno)
> >   {
> >       if (!csrind_or_aia_extensions_present(env)) {
> > @@ -417,6 +437,15 @@ static RISCVException hmode32(CPURISCVState *env, int 
> > csrno)
> >
> >   }
> >
> > +static RISCVException csrind_hmode(CPURISCVState *env, int csrno)
> > +{
> > +    if (!csrind_extensions_present(env)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return hmode(env, csrno);
> > +}
> > +
> >   static RISCVException csrind_or_aia_hmode(CPURISCVState *env, int csrno)
> >   {
> >       if (!csrind_or_aia_extensions_present(env)) {
> > @@ -2065,7 +2094,12 @@ static int csrind_xlate_vs_csrno(CPURISCVState *env, 
> > int csrno)
> >       case CSR_SISELECT:
> >           return CSR_VSISELECT;
> >       case CSR_SIREG:
> > -        return CSR_VSIREG;
> > +    case CSR_SIREG2:
> > +    case CSR_SIREG3:
> > +    case CSR_SIREG4:
> > +    case CSR_SIREG5:
> > +    case CSR_SIREG6:
> > +        return CSR_VSIREG + (csrno - CSR_SIREG);
> >       default:
> >           return csrno;
> >       };
> > @@ -2105,7 +2139,12 @@ static RISCVException rmw_xiselect(CPURISCVState 
> > *env, int csrno,
> >           *val = *iselect;
> >       }
> >
> > -    wr_mask &= ISELECT_MASK;
> > +    if (riscv_cpu_cfg(env)->ext_smcsrind || 
> > riscv_cpu_cfg(env)->ext_sscsrind) {
> > +        wr_mask &= ISELECT_MASK_SXCSRIND;
> > +    } else {
> > +        wr_mask &= ISELECT_MASK_AIA;
> > +    }
> > +
> >       if (wr_mask) {
> >           *iselect = (*iselect & ~wr_mask) | (new_val & wr_mask);
> >       }
> > @@ -2244,6 +2283,59 @@ done:
> >       return RISCV_EXCP_NONE;
> >   }
> >
> > +/*
> > + * rmw_xireg_csrind: Perform indirect access to xireg and xireg2-xireg6
> > + *
> > + * Perform indirect access to xireg and xireg2-xireg6.
> > + * This is a generic interface for all xireg CSRs. Apart from AIA, all 
> > other
> > + * extension using csrind should be implemented here.
> > + */
> > +static int rmw_xireg_csrind(CPURISCVState *env, int csrno,
> > +                              target_ulong isel, target_ulong *val,
> > +                              target_ulong new_val, target_ulong wr_mask)
> > +{
> > +    return -EINVAL;
> > +}
> > +
> > +static int rmw_xiregi(CPURISCVState *env, int csrno, target_ulong *val,
> > +                      target_ulong new_val, target_ulong wr_mask)
> > +{
> > +    bool virt = false;
> > +    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);
> > +
> > +    if (CSR_MIREG <= csrno && csrno <= CSR_MIREG6 &&
> > +        csrno != CSR_MIREG4 - 1) {
> > +        isel = env->miselect;
> > +    } else if (CSR_SIREG <= csrno && csrno <= CSR_SIREG6 &&
> > +               csrno != CSR_SIREG4 - 1) {
> > +        isel = env->siselect;
> > +    } else if (CSR_VSIREG <= csrno && csrno <= CSR_VSIREG6 &&
> > +               csrno != CSR_VSIREG4 - 1) {
> > +        isel = env->vsiselect;
> > +        virt = true;
> > +    } else {
> > +        goto done;
> > +    }
> > +
> > +    return rmw_xireg_csrind(env, csrno, isel, val, new_val, wr_mask);
> > +
> > +done:
> > +    if (ret) {
>
> Do you need this check? Up there you're doing:
>
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
>
>
> And 'ret' is not changed afterwards, meaning it's guaranteed to be 
> RISCV_EXCP_NONE and
> "if (ret)" will always be true.
>
>

Yeah. I have removed it. Thanks!

> Thanks,
>
> Daniel
>
>
> > +        return (env->virt_enabled && virt) ?
> > +               RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >   static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
> >                                   target_ulong *val, target_ulong new_val,
> >                                   target_ulong wr_mask)
> > @@ -2276,8 +2368,21 @@ static RISCVException rmw_xireg(CPURISCVState *env, 
> > int csrno,
> >            goto done;
> >       };
> >
> > +    /*
> > +     * Use the xiselect range to determine actual op on xireg.
> > +     *
> > +     * Since we only checked the existence of AIA or Indirect Access in the
> > +     * predicate, we should check the existence of the exact extension when
> > +     * we get to a specific range and return illegal instruction exception 
> > even
> > +     * in VS-mode.
> > +     */
> >       if (xiselect_aia_range(isel)) {
> >           return rmw_xireg_aia(env, csrno, isel, val, new_val, wr_mask);
> > +    } else if (riscv_cpu_cfg(env)->ext_smcsrind ||
> > +               riscv_cpu_cfg(env)->ext_sscsrind) {
> > +        return rmw_xireg_csrind(env, csrno, isel, val, new_val, wr_mask);
> > +    } else {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> >       }
> >
> >   done:
> > @@ -2735,7 +2840,7 @@ static RISCVException write_mstateen0(CPURISCVState 
> > *env, int csrno,
> >           wr_mask |= SMSTATEEN0_P1P13;
> >       }
> >
> > -    if (riscv_cpu_cfg(env)->ext_smaia) {
> > +    if (riscv_cpu_cfg(env)->ext_smaia || riscv_cpu_cfg(env)->ext_smcsrind) 
> > {
> >           wr_mask |= SMSTATEEN0_SVSLCT;
> >       }
> >
> > @@ -2828,7 +2933,7 @@ static RISCVException write_hstateen0(CPURISCVState 
> > *env, int csrno,
> >           wr_mask |= SMSTATEEN0_FCSR;
> >       }
> >
> > -    if (riscv_cpu_cfg(env)->ext_ssaia) {
> > +    if (riscv_cpu_cfg(env)->ext_ssaia || riscv_cpu_cfg(env)->ext_sscsrind) 
> > {
> >           wr_mask |= SMSTATEEN0_SVSLCT;
> >       }
> >
> > @@ -5261,6 +5366,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >       [CSR_MIREG]    = { "mireg",    csrind_or_aia_any,   NULL, NULL,
> >                          rmw_xireg                                       },
> >
> > +    /* Machine Indirect Register Alias */
> > +    [CSR_MIREG2]   = { "mireg2", csrind_any, NULL, NULL, rmw_xiregi,
> > +                       .min_priv_ver = PRIV_VERSION_1_12_0          },
> > +    [CSR_MIREG3]   = { "mireg3", csrind_any, NULL, NULL, rmw_xiregi,
> > +                       .min_priv_ver = PRIV_VERSION_1_12_0          },
> > +    [CSR_MIREG4]   = { "mireg4", csrind_any, NULL, NULL, rmw_xiregi,
> > +                       .min_priv_ver = PRIV_VERSION_1_12_0          },
> > +    [CSR_MIREG5]   = { "mireg5", csrind_any, NULL, NULL, rmw_xiregi,
> > +                       .min_priv_ver = PRIV_VERSION_1_12_0          },
> > +    [CSR_MIREG6]   = { "mireg6", csrind_any, NULL, NULL, rmw_xiregi,
> > +                       .min_priv_ver = PRIV_VERSION_1_12_0          },
> > +
> >       /* Machine-Level Interrupts (AIA) */
> >       [CSR_MTOPEI]   = { "mtopei",   aia_any, NULL, NULL, rmw_xtopei },
> >       [CSR_MTOPI]    = { "mtopi",    aia_any, read_mtopi },
> > @@ -5382,6 +5499,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >       [CSR_SIREG]      = { "sireg",      csrind_or_aia_smode, NULL, NULL,
> >                            rmw_xireg                                        
> >   },
> >
> > +    /* Supervisor Indirect Register Alias */
> > +    [CSR_SIREG2]      = { "sireg2", csrind_smode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +    [CSR_SIREG3]      = { "sireg3", csrind_smode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +    [CSR_SIREG4]      = { "sireg4", csrind_smode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +    [CSR_SIREG5]      = { "sireg5", csrind_smode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +    [CSR_SIREG6]      = { "sireg6", csrind_smode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +
> >       /* Supervisor-Level Interrupts (AIA) */
> >       [CSR_STOPEI]     = { "stopei",     aia_smode, NULL, NULL, rmw_xtopei 
> > },
> >       [CSR_STOPI]      = { "stopi",      aia_smode, read_stopi },
> > @@ -5464,6 +5593,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >       [CSR_VSIREG]      = { "vsireg",      csrind_or_aia_hmode, NULL, NULL,
> >                             rmw_xireg                                       
> >   },
> >
> > +    /* Virtual Supervisor Indirect Alias */
> > +    [CSR_VSIREG2]     = { "vsireg2", csrind_hmode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +    [CSR_VSIREG3]     = { "vsireg3", csrind_hmode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +    [CSR_VSIREG4]     = { "vsireg4", csrind_hmode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +    [CSR_VSIREG5]     = { "vsireg5", csrind_hmode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +    [CSR_VSIREG6]     = { "vsireg6", csrind_hmode, NULL, NULL, rmw_xiregi,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > +
> >       /* VS-Level Interrupts (H-extension with AIA) */
> >       [CSR_VSTOPEI]     = { "vstopei",     aia_hmode, NULL, NULL, 
> > rmw_xtopei },
> >       [CSR_VSTOPI]      = { "vstopi",      aia_hmode, read_vstopi },
> >
>