Re: [PATCH RFC 4/8] target/riscv: Support generic CSR indirect access

[Jason Chien]

The predicate functions should contain the access control by the 
state-enable CSRs, which is not presented in this patch. Do you mind 
that I take over the indirect CSR access control part? The Signed-off-by 
will be kept.

Atish Patra 於 2024/2/17 上午 08:01 寫道:
> 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      | 146 +++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 169 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 0ee91e502e8f..3a66f83009b5 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -176,6 +176,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
> @@ -225,6 +232,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
> @@ -291,6 +305,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
> @@ -847,10 +868,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
Could you rename ISELECT_MASK_SXCSRIND to ISELECT_MASK_CSRIND to keep 
the naming consistency with ISELECT_MASK_AIA?
>   
>   /* 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 89a1325a02a5..a1c10f1d010a 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -287,6 +287,17 @@ static int aia_any32(CPURISCVState *env, int csrno)
>       return any32(env, csrno);
>   }
>   
> +static RISCVException sxcsrind_any(CPURISCVState *env, int csrno)
Could you rename sxcsrind_any() to csrind_any() to keep naming 
consistency with aia_any()?
> +{
> +    RISCVCPU *cpu = env_archcpu(env);
> +
> +    if (!cpu->cfg.ext_smcsrind) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
>   static int sxcsrind_or_aia_any(CPURISCVState *env, int csrno)
>   {
>       if (!riscv_cpu_cfg(env)->ext_smaia && !riscv_cpu_cfg(env)->ext_smcsrind) {
> @@ -355,6 +366,17 @@ static int aia_smode32(CPURISCVState *env, int csrno)
>       return smode32(env, csrno);
>   }
>   
> +static RISCVException sxcsrind_smode(CPURISCVState *env, int csrno)
Could you rename sxcsrind_smode() to csrind_smode() to keep naming 
consistency with aia_smode()?
> +{
> +    RISCVCPU *cpu = env_archcpu(env);
> +
> +    if (!cpu->cfg.ext_sscsrind) {
S-mode CSRs are defined in Smcsrind as well. If both Smcsrind and 
Sscsrind are disabled, return RISCV_EXCP_ILLEGAL_INST.
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
A virtual instruction exception should be raised here for attempts from 
VU-mode to access siselect or sireg*.
> +    return smode(env, csrno);
> +}
> +
>   static int sxcsrind_or_aia_smode(CPURISCVState *env, int csrno)
>   {
>       if (!riscv_cpu_cfg(env)->ext_ssaia && !riscv_cpu_cfg(env)->ext_sscsrind) {
> @@ -383,6 +405,17 @@ static RISCVException hmode32(CPURISCVState *env, int 
> csrno)
>   
>   }
>   
> +static RISCVException sxcsrind_hmode(CPURISCVState *env, int csrno)
Could you rename sxcsrind_hmode() to csrind_hmode() to keep naming 
consistency with aia_hmode()?
> +{
> +    RISCVCPU *cpu = env_archcpu(env);
> +
> +    if (!cpu->cfg.ext_sscsrind) {
VS-mode CSRs are defined in Smcsrind as well. If both Smcsrind and 
Sscsrind are disabled, return RISCV_EXCP_ILLEGAL_INST.
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    return hmode(env, csrno);
> +}
> +
>   static int sxcsrind_or_aia_hmode(CPURISCVState *env, int csrno)
>   {
>       if (!riscv_cpu_cfg(env)->ext_ssaia && !riscv_cpu_cfg(env)->ext_sscsrind) {
> @@ -1926,7 +1959,12 @@ static int rmw_xiselect(CPURISCVState *env, int csrno, 
> target_ulong *val,
>           *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);
>       }
> @@ -2065,6 +2103,59 @@ done:
>       return RISCV_EXCP_NONE;
>   }
>   
> +/*
> + * rmw_xireg_sxcsrind: 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 sxcsrind should be implemented here.
> + */
> +static int rmw_xireg_sxcsrind(CPURISCVState *env, int csrno,
> +                              target_ulong isel, target_ulong *val,
> +                              target_ulong new_val, target_ulong wr_mask)
Could you rename rmw_xireg_sxcsrind() to rmw_xireg_csrind() to keep the 
naming consistency with rmw_xireg_aia()?
> +{
> +    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 = sxcsrind_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_sxcsrind(env, csrno, isel, val, new_val, wr_mask);
> +
> +done:
> +    if (ret) {
> +        return (env->virt_enabled && virt) ?
A virtual instruction exception is raised for attempts from VS-mode or 
VU-mode to directly access vsiselect or vsireg* in riscv_csrrw_check(), 
we don't need to check again here.
> +               RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
> +    }
> +    return RISCV_EXCP_NONE;
> +}
> +
>   static int rmw_xireg(CPURISCVState *env, int csrno, target_ulong *val,
>                        target_ulong new_val, target_ulong wr_mask)
>   {
> @@ -2096,8 +2187,21 @@ static int rmw_xireg(CPURISCVState *env, int csrno, 
> target_ulong *val,
>            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_sxcsrind(env, csrno, isel, val, new_val, wr_mask);
> +    } else {
> +        return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
>   done:
> @@ -2480,7 +2584,7 @@ static RISCVException write_mstateen0(CPURISCVState *env, 
> int csrno,
>        * TODO: Do we need to check ssaia as well ? Can we enable ssaia without
>        * smaia ?
>        */
> -    if (riscv_cpu_cfg(env)->ext_smaia) {
> +    if (riscv_cpu_cfg(env)->ext_smaia || riscv_cpu_cfg(env)->ext_smcsrind) {
>           wr_mask |= SMSTATEEN0_SVSLCT;
>       }
>   
> @@ -2569,7 +2673,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;
>       }
>   
> @@ -4866,6 +4970,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>       [CSR_MIREG]    = { "mireg",    sxcsrind_or_aia_any,   NULL, NULL,
>                          rmw_xireg                                       },
>   
> +    /* Machine Indirect Register Alias */
> +    [CSR_MIREG2]   = { "mireg2", sxcsrind_any, NULL, NULL, rmw_xiregi,
> +                       .min_priv_ver = PRIV_VERSION_1_12_0          },
> +    [CSR_MIREG3]   = { "mireg3", sxcsrind_any, NULL, NULL, rmw_xiregi,
> +                       .min_priv_ver = PRIV_VERSION_1_12_0          },
> +    [CSR_MIREG4]   = { "mireg4", sxcsrind_any, NULL, NULL, rmw_xiregi,
> +                       .min_priv_ver = PRIV_VERSION_1_12_0          },
> +    [CSR_MIREG5]   = { "mireg5", sxcsrind_any, NULL, NULL, rmw_xiregi,
> +                       .min_priv_ver = PRIV_VERSION_1_12_0          },
> +    [CSR_MIREG6]   = { "mireg6", sxcsrind_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 },
> @@ -4987,6 +5103,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>       [CSR_SIREG]      = { "sireg",      sxcsrind_or_aia_smode, NULL, NULL,
>                            rmw_xireg                                          },
>   
> +    /* Supervisor Indirect Register Alias */
> +    [CSR_SIREG2]      = { "sireg2", sxcsrind_smode, NULL, NULL, rmw_xiregi,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0                },
> +    [CSR_SIREG3]      = { "sireg3", sxcsrind_smode, NULL, NULL, rmw_xiregi,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0                },
> +    [CSR_SIREG4]      = { "sireg4", sxcsrind_smode, NULL, NULL, rmw_xiregi,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0                },
> +    [CSR_SIREG5]      = { "sireg5", sxcsrind_smode, NULL, NULL, rmw_xiregi,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0                },
> +    [CSR_SIREG6]      = { "sireg6", sxcsrind_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 },
> @@ -5069,6 +5197,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>       [CSR_VSIREG]      = { "vsireg",      sxcsrind_or_aia_hmode, NULL, NULL,
>                             rmw_xireg                                         },
>   
> +    /* Virtual Supervisor Indirect Alias */
> +    [CSR_VSIREG2]     = { "vsireg2", sxcsrind_hmode, NULL, NULL, rmw_xiregi,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0                },
> +    [CSR_VSIREG3]     = { "vsireg3", sxcsrind_hmode, NULL, NULL, rmw_xiregi,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0                },
> +    [CSR_VSIREG4]     = { "vsireg4", sxcsrind_hmode, NULL, NULL, rmw_xiregi,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0                },
> +    [CSR_VSIREG5]     = { "vsireg5", sxcsrind_hmode, NULL, NULL, rmw_xiregi,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0                },
> +    [CSR_VSIREG6]     = { "vsireg6", sxcsrind_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 },