On Wed, Jun 5, 2024 at 4:49 AM Jason Chien <jason.ch...@sifive.com> wrote: > > 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 <kaiw...@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 <ati...@rivosinc.com> > > Signed-off-by: Atish Patra <ati...@rivosinc.com> > > Signed-off-by: Kaiwen Xue <kaiw...@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*.
That should be covered in riscv_csrrw_check. > > + 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. The check is for invalid value while accessing siregi variants. Thanks for the review. Addressed all other suggestions in v2. > > + 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 },