diff mbox series

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

Message ID 20240529160950.132754-4-rkanwal@rivosinc.com (mailing list archive)
State New, archived
Headers show
Series target/riscv: Add support for Control Transfer Records Ext. | expand

Commit Message

Rajnesh Kanwal May 29, 2024, 4:09 p.m. UTC 
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(+)

Comments

Jason Chien June 4, 2024, 10:14 a.m. UTC | #1 
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.

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?

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 },
Rajnesh Kanwal June 10, 2024, 2:11 p.m. UTC | #2 
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 },
Jason Chien June 12, 2024, 3:13 a.m. UTC | #3 
It makes sense. Thank you for the explanation.

Rajnesh Kanwal <rkanwal@rivosinc.com> 於 2024年6月10日 週一 下午10:12寫道：

>
> 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
> },
>
diff mbox series

Patch

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) {
+            return smstateen_acc_ok(env, 0, SMSTATEEN0_CTR);
+        }
+
+        return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+    }
+
+    return RISCV_EXCP_ILLEGAL_INST;
+}
+
+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);
+    }
+
+    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;
+    }
+
+    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;
+        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;
+    }
+
+    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);
+    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;
+    }
+
+    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;
+    }
+
+    env->vsctrctl = (env->vsctrctl & ~mask) | (new_val & mask);
+
+    return RISCV_EXCP_NONE;
+}
+
 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 },
+    [CSR_SCTRCTL]       = { "sctrctl",       ctr_smode, NULL, NULL,
+                                rmw_sctrctl },
+    [CSR_SCTRDEPTH]       = { "sctrdepth",       ctr_smode, NULL, NULL,
+                                rmw_sctrdepth },
+    [CSR_SCTRSTATUS]       = { "sctrstatus",       ctr_smode, NULL, NULL,
+                                rmw_sctrstatus },
+    [CSR_VSCTRCTL]      = { "vsctrctl",      ctr_vsmode, NULL, NULL,
+                                rmw_vsctrctl },
     /* Performance Counters */
     [CSR_HPMCOUNTER3]    = { "hpmcounter3",    ctr,    read_hpmcounter },
     [CSR_HPMCOUNTER4]    = { "hpmcounter4",    ctr,    read_hpmcounter },