Re: [PATCH 3/6] target/riscv: Add support for Control Transfer Records extension CSRs.

[Rajnesh Kanwal <rkanwal@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 14237 bytes --]

Thanks Jason for your review.

On Tue, Jun 4, 2024 at 11:14 AM Jason Chien <jason.chien@sifive.com> wrote:
>
>
> Rajnesh Kanwal 於 2024/5/30 上午 12:09 寫道:
>
> This commit adds support for [m|s|vs]ctrcontrol, sctrstatus and
> sctrdepth CSRs handling.
>
> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
> ---
>  target/riscv/cpu.h     |   5 ++
>  target/riscv/cpu_cfg.h |   2 +
>  target/riscv/csr.c     | 159 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 166 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index a185e2d494..3d4d5172b8 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -263,6 +263,11 @@ struct CPUArchState {
>      target_ulong mcause;
>      target_ulong mtval;  /* since: priv-1.10.0 */
>
> +    uint64_t mctrctl;
> +    uint32_t sctrdepth;
> +    uint32_t sctrstatus;
> +    uint64_t vsctrctl;
> +
>      /* Machine and Supervisor interrupt priorities */
>      uint8_t miprio[64];
>      uint8_t siprio[64];
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index d9354dc80a..d329a65811 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -123,6 +123,8 @@ struct RISCVCPUConfig {
>      bool ext_zvfhmin;
>      bool ext_smaia;
>      bool ext_ssaia;
> +    bool ext_smctr;
> +    bool ext_ssctr;
>      bool ext_sscofpmf;
>      bool ext_smepmp;
>      bool rvv_ta_all_1s;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 2f92e4b717..888084d8e5 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -621,6 +621,61 @@ static RISCVException pointer_masking(CPURISCVState
*env, int csrno)
>      return RISCV_EXCP_ILLEGAL_INST;
>  }
>
> +/*
> + * M-mode:
> + * Without ext_smctr raise illegal inst excep.
> + * Otherwise everything is accessible to m-mode.
> + *
> + * S-mode:
> + * Without ext_ssctr or mstateen.ctr raise illegal inst excep.
> + * Otherwise everything other than mctrctl is accessible.
> + *
> + * VS-mode:
> + * Without ext_ssctr or mstateen.ctr raise illegal inst excep.
> + * Without hstateen.ctr raise virtual illegal inst excep.
> + * Otherwise allow vsctrctl, sctrstatus, 0x200-0x2ff entry range.
> + * Always raise illegal instruction exception for sctrdepth.
> + */
> +static RISCVException ctr_mmode(CPURISCVState *env, int csrno)
> +{
> +    /* Check if smctr-ext is present */
> +    if (riscv_cpu_cfg(env)->ext_smctr) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    return RISCV_EXCP_ILLEGAL_INST;
> +}
> +
> +static RISCVException ctr_smode(CPURISCVState *env, int csrno)
> +{
> +    if ((env->priv == PRV_M && riscv_cpu_cfg(env)->ext_smctr) ||
> +        (env->priv == PRV_S && !env->virt_enabled &&
> +         riscv_cpu_cfg(env)->ext_ssctr)) {
> +        return smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
> +    }
> +
> +    if (env->priv == PRV_S && env->virt_enabled &&
> +        riscv_cpu_cfg(env)->ext_ssctr) {
> +        if (csrno == CSR_SCTRSTATUS) {
>
> missing sctrctl?
>
> +            return smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
> +        }
> +
> +        return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +    }
> +
> +    return RISCV_EXCP_ILLEGAL_INST;
> +}
>
> I think there is no need to bind M-mode with ext_smctr, S-mode with
ext_ssctr and VS-mode with ext_ssctr, since this predicate function is for
S-mode CSRs, which are defined in both smctr and ssctr, we just need to
check at least one of ext_ssctr or ext_smctr is true.
>
> The spec states that:
> Attempts to access sctrdepth from VS-mode or VU-mode raise a
virtual-instruction exception, unless CTR state enable access restrictions
apply.
>
> In my understanding, we should check the presence of smstateen extension
first, and
>
> if smstateen is implemented:
>
> for sctrctl and sctrstatus, call smstateen_acc_ok()
> for sctrdepth, call smstateen_acc_ok(), and if there is any exception
returned, always report virtual-instruction exception.

For sctrdepth, we are supposed to always return a virt-inst exception in
case of
VS-VU mode unless CTR state enable access restrictions apply.

So for sctrdepth, call smstateen_acc_ok(), and if there is no exception
returned
(mstateen.CTR=1 and hstateen.CTR=1 for virt mode), check if we are in
virtual
mode and return virtual-instruction exception otherwise return
RISCV_EXCP_NONE.
Note that if hstateen.CTR=0, smstateen_acc_ok() will return
virtual-instruction
exception which means regardless of the hstateen.CTR state, we will always
return virtual-instruction exception for VS/VU mode access to sctrdepth.

Basically this covers following rules for sctrdepth:

if mstateen.ctr == 0
    return RISCV_EXCP_ILLEGAL_INST; // For all modes lower than M-mode.
else if in virt-mode // regardless of the state of hstateen.CTR
    return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
else
    return RISCV_EXCP_NONE

>
> If smstateen is not implemented:
>
> for sctrctl and sctrstatus, there is no check.
> for sctrdepth, I think the spec is ambiguous. What does "CTR state enable
access restrictions apply" mean when smstateen is not implemented?

As per my understanding, this means if mstateen.CTR=0 then we return an
illegal instruction exception regardless if it's virtual mode or not. This
is
the only effect of CTR state enable on sctrdepth CSR. If mstateen.CTR=1,
sctrdepth access from VS-mode results in virtual-instruction exception
regardless of hstateen.CTR.

Based on this, we have following model for predicate checks:

if smstateen is implemented:

for sctrctl and sctrstatus, call smstateen_acc_ok()
for sctrdepth, call smstateen_acc_ok(), and if there is no exception,
    check if we are in virtual mode and return virtual-instruction exception
    otherwise return RISCV_EXCP_NONE.

If smstateen is not implemented:

for sctrctl and sctrstatus, there is no check.
for sctrdepth, if in VS/VU mode return virtual-instruction exception
otherwise
    no check.

Here is the code to better understand this.

static RISCVException ctr_smode(CPURISCVState *env, int csrno)
{
    const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);

    if (!cfg->ext_ssctr && !cfg->ext_smctr) {
        return RISCV_EXCP_ILLEGAL_INST;
    }

    if (riscv_cpu_cfg(env)->ext_smstateen) {
        RISCVException ret = smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
        if (ret == RISCV_EXCP_NONE && csrno == CSR_SCTRDEPTH &&
env->virt_enabled) {
            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
        }
        return ret;
    } else {
        if (csrno == CSR_SCTRDEPTH && env->virt_enabled) {
            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
        }
    }

    return RISCV_EXCP_NONE;
}

Given smstateen_acc_ok() returns RISCV_EXCP_NONE in case if ext_smstateen
is not
implemented, this can be further simplified to:

static RISCVException ctr_smode(CPURISCVState *env, int csrno)
{
    const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);

    if (!cfg->ext_ssctr && !cfg->ext_smctr) {
        return RISCV_EXCP_ILLEGAL_INST;
    }

    RISCVException ret = smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
    if (ret == RISCV_EXCP_NONE && csrno == CSR_SCTRDEPTH &&
env->virt_enabled) {
        return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
    }
    return ret;
}

>
> Here is the code to better understand my description.
>
> static RISCVException ctr_smode(CPURISCVState *env, int csrno)
> {
>     const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>
>     if (!cfg->ext_ssctr && !cfg->ext_smctr) {
>         return RISCV_EXCP_ILLEGAL_INST;
>     }
>
>     if (riscv_cpu_cfg(env)->ext_smstateen) {
>         RISCVException ret = smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
>         if (ret != RISCV_EXCP_NONE) {
>             if (csrno == CSR_SCTRDEPTH && env->virt_enabled) {
>                 return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>             }
>
>             return ret;
>         }
>     } else {
>         /* The spec is ambiguous. */
>         if (csrno == CSR_SCTRDEPTH && env->virt_enabled) {
>             return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>         }
>     }
>
>     return RISCV_EXCP_NONE;
> }
>
> +
> +static RISCVException ctr_vsmode(CPURISCVState *env, int csrno)
> +{
> +    if (env->priv == PRV_S && env->virt_enabled &&
> +        riscv_cpu_cfg(env)->ext_ssctr) {
> +        return smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
>
> In riscv_csrrw_check(), an virtual-instruction exception is always
reported no matter what. Do we need this check?
>
> +    }
> +
> +    return ctr_smode(env, csrno);
> +}
> +
>  static RISCVException aia_hmode(CPURISCVState *env, int csrno)
>  {
>      int ret;
> @@ -3835,6 +3890,100 @@ static RISCVException write_satp(CPURISCVState
*env, int csrno,
>      return RISCV_EXCP_NONE;
>  }
>
> +static RISCVException rmw_sctrdepth(CPURISCVState *env, int csrno,
> +                                    target_ulong *ret_val,
> +                                    target_ulong new_val, target_ulong
wr_mask)
> +{
> +    uint64_t mask = wr_mask & SCTRDEPTH_MASK;
> +
> +    if (ret_val) {
> +        *ret_val = env->sctrdepth & SCTRDEPTH_MASK;
>
> We don't need to do bitwise and with SCTRDEPTH_MASK on read accesses when
we always do bitwise and with SCTRDEPTH_MASK on write accesses.
>
> +    }
> +
> +    env->sctrdepth = (env->sctrdepth & ~mask) | (new_val & mask);
> +
> +    /* Correct depth. */
> +    if (wr_mask & SCTRDEPTH_MASK) {
> +        uint64_t depth = get_field(env->sctrdepth, SCTRDEPTH_MASK);
> +
> +        if (depth > SCTRDEPTH_MAX) {
> +            env->sctrdepth =
> +                set_field(env->sctrdepth, SCTRDEPTH_MASK, SCTRDEPTH_MAX);
> +        }
> +
> +        /* Update sctrstatus.WRPTR with a legal value */
> +        depth = 16 << depth;
>
> The "depth" on the right side may exceed SCTRDEPTH_MAX.
>
> +        env->sctrstatus =
> +            env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth - 1));
> +    }
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException rmw_mctrctl(CPURISCVState *env, int csrno,
> +                                    target_ulong *ret_val,
> +                                    target_ulong new_val, target_ulong
wr_mask)
> +{
> +    uint64_t mask = wr_mask & MCTRCTL_MASK;
> +
> +    if (ret_val) {
> +        *ret_val = env->mctrctl & MCTRCTL_MASK;
>
> There is no need to do bitwise and with the mask on read accesses when we
always do bitwise and with the mask on write accesses.
>
> +    }
> +
> +    env->mctrctl = (env->mctrctl & ~mask) | (new_val & mask);
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException rmw_sctrctl(CPURISCVState *env, int csrno,
> +                                    target_ulong *ret_val,
> +                                    target_ulong new_val, target_ulong
wr_mask)
> +{
> +    uint64_t mask = wr_mask & SCTRCTL_MASK;
> +    RISCVException ret;
> +
> +    ret = rmw_mctrctl(env, csrno, ret_val, new_val, mask);
>
> When V=1, vsctrctl substitutes for sctrctl.
>
> +    if (ret_val) {
> +        *ret_val &= SCTRCTL_MASK;
> +    }
> +
> +    return ret;
> +}
> +
> +static RISCVException rmw_sctrstatus(CPURISCVState *env, int csrno,
> +                                     target_ulong *ret_val,
> +                                     target_ulong new_val, target_ulong
wr_mask)
> +{
> +    uint32_t depth = 16 << get_field(env->sctrdepth, SCTRDEPTH_MASK);
> +    uint32_t mask = wr_mask & SCTRSTATUS_MASK;
> +
> +    if (ret_val) {
> +        *ret_val = env->sctrstatus & SCTRSTATUS_MASK;
>
> There is no need to do bitwise and with the mask on read accesses when we
always do bitwise and with the mask on write accesses.
>
> +    }
> +
> +    env->sctrstatus = (env->sctrstatus & ~mask) | (new_val & mask);
> +
> +    /* Update sctrstatus.WRPTR with a legal value */
> +    env->sctrstatus = env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth
- 1));
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException rmw_vsctrctl(CPURISCVState *env, int csrno,
> +                                    target_ulong *ret_val,
> +                                    target_ulong new_val, target_ulong
wr_mask)
> +{
> +    uint64_t mask = wr_mask & VSCTRCTL_MASK;
> +
> +    if (ret_val) {
> +        *ret_val = env->vsctrctl & VSCTRCTL_MASK;
>
> There is no need to do bitwise and with the mask on read accesses when we
always do bitwise and with the mask on write accesses.
>
> +    }
> +
> +    env->vsctrctl = (env->vsctrctl & ~mask) | (new_val & mask);
> +
> +    return RISCV_EXCP_NONE;
> +}
>
> Is it possible to define rmw_xctrctl() instead of three individual rmw
functions and use a switch case to select the mask and the CSR for the
purpose of reducing code size?
>
> +
>  static RISCVException read_vstopi(CPURISCVState *env, int csrno,
>                                    target_ulong *val)
>  {
> @@ -5771,6 +5920,16 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>      [CSR_SPMBASE] =    { "spmbase", pointer_masking, read_spmbase,
>                           write_spmbase
   },
>
> +    [CSR_MCTRCTL]       = { "mctrctl",       ctr_mmode, NULL, NULL,
> +                                rmw_mctrctl },
>
> I think this can be one line.
>
> +    [CSR_SCTRCTL]       = { "sctrctl",       ctr_smode, NULL, NULL,
> +                                rmw_sctrctl },
>
> same here
>
> +    [CSR_SCTRDEPTH]       = { "sctrdepth",       ctr_smode, NULL, NULL,
> +                                rmw_sctrdepth },
>
> same here
>
> +    [CSR_SCTRSTATUS]       = { "sctrstatus",       ctr_smode, NULL, NULL,
> +                                rmw_sctrstatus },
>
> same here
>
> +    [CSR_VSCTRCTL]      = { "vsctrctl",      ctr_vsmode, NULL, NULL,
> +                                rmw_vsctrctl },
>
> same here
>
>      /* Performance Counters */
>      [CSR_HPMCOUNTER3]    = { "hpmcounter3",    ctr,    read_hpmcounter },
>      [CSR_HPMCOUNTER4]    = { "hpmcounter4",    ctr,    read_hpmcounter },

[-- Attachment #2: Type: text/html, Size: 17443 bytes --]