diff mbox series

[v3,1/8] target/riscv: Add Ssdbltrp CSRs handling

Message ID 20241014112225.90297-2-cleger@rivosinc.com (mailing list archive)
State New
Headers show
Series target/riscv: Add support for Smdbltrp and Ssdbltrp extensions | expand

Commit Message

Clément Léger Oct. 14, 2024, 11:22 a.m. UTC
Add ext_ssdbltrp in RISCVCPUConfig and implement MSTATUS.SDT,
{H|M}ENVCFG.DTE and modify the availability of MTVAL2 based on the
presence of the Ssdbltrp ISA extension.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.h        |  1 +
 target/riscv/cpu_bits.h   |  6 ++++++
 target/riscv/cpu_cfg.h    |  1 +
 target/riscv/cpu_helper.c | 20 +++++++++++++++++
 target/riscv/csr.c        | 45 ++++++++++++++++++++++++++++++---------
 5 files changed, 63 insertions(+), 10 deletions(-)

Comments

LIU Zhiwei Oct. 16, 2024, 9:40 a.m. UTC | #1
On 2024/10/14 19:22, Clément Léger wrote:
> Add ext_ssdbltrp in RISCVCPUConfig and implement MSTATUS.SDT,
> {H|M}ENVCFG.DTE and modify the availability of MTVAL2 based on the
> presence of the Ssdbltrp ISA extension.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/cpu.h        |  1 +
>   target/riscv/cpu_bits.h   |  6 ++++++
>   target/riscv/cpu_cfg.h    |  1 +
>   target/riscv/cpu_helper.c | 20 +++++++++++++++++
>   target/riscv/csr.c        | 45 ++++++++++++++++++++++++++++++---------
>   5 files changed, 63 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index a84e719d3f..ee984bf270 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -553,6 +553,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen);
>   bool riscv_cpu_vector_enabled(CPURISCVState *env);
>   void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
>   int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
> +bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt);
>   G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>                                                  MMUAccessType access_type,
>                                                  int mmu_idx, uintptr_t retaddr);
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index da1723496c..3a5588d4df 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -558,6 +558,7 @@
>   #define MSTATUS_TVM         0x00100000 /* since: priv-1.10 */
>   #define MSTATUS_TW          0x00200000 /* since: priv-1.10 */
>   #define MSTATUS_TSR         0x00400000 /* since: priv-1.10 */
> +#define MSTATUS_SDT         0x01000000
>   #define MSTATUS_GVA         0x4000000000ULL
>   #define MSTATUS_MPV         0x8000000000ULL
>   
> @@ -588,6 +589,7 @@ typedef enum {
>   #define SSTATUS_XS          0x00018000
>   #define SSTATUS_SUM         0x00040000 /* since: priv-1.10 */
>   #define SSTATUS_MXR         0x00080000
> +#define SSTATUS_SDT         0x01000000
>   
>   #define SSTATUS64_UXL       0x0000000300000000ULL
>   
> @@ -777,11 +779,13 @@ typedef enum RISCVException {
>   #define MENVCFG_CBIE                       (3UL << 4)
>   #define MENVCFG_CBCFE                      BIT(6)
>   #define MENVCFG_CBZE                       BIT(7)
> +#define MENVCFG_DTE                        (1ULL << 59)
>   #define MENVCFG_ADUE                       (1ULL << 61)
>   #define MENVCFG_PBMTE                      (1ULL << 62)
>   #define MENVCFG_STCE                       (1ULL << 63)
>   
>   /* For RV32 */
> +#define MENVCFGH_DTE                       BIT(27)
>   #define MENVCFGH_ADUE                      BIT(29)
>   #define MENVCFGH_PBMTE                     BIT(30)
>   #define MENVCFGH_STCE                      BIT(31)
> @@ -795,11 +799,13 @@ typedef enum RISCVException {
>   #define HENVCFG_CBIE                       MENVCFG_CBIE
>   #define HENVCFG_CBCFE                      MENVCFG_CBCFE
>   #define HENVCFG_CBZE                       MENVCFG_CBZE
> +#define HENVCFG_DTE                        MENVCFG_DTE
>   #define HENVCFG_ADUE                       MENVCFG_ADUE
>   #define HENVCFG_PBMTE                      MENVCFG_PBMTE
>   #define HENVCFG_STCE                       MENVCFG_STCE
>   
>   /* For RV32 */
> +#define HENVCFGH_DTE                        MENVCFGH_DTE
>   #define HENVCFGH_ADUE                       MENVCFGH_ADUE
>   #define HENVCFGH_PBMTE                      MENVCFGH_PBMTE
>   #define HENVCFGH_STCE                       MENVCFGH_STCE
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index ae2a945b5f..dd804f95d4 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -77,6 +77,7 @@ struct RISCVCPUConfig {
>       bool ext_smstateen;
>       bool ext_sstc;
>       bool ext_smcntrpmf;
> +    bool ext_ssdbltrp;
>       bool ext_svadu;
>       bool ext_svinval;
>       bool ext_svnapot;
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 9d0400035f..77e7736d8a 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -63,6 +63,22 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
>   #endif
>   }
>   
> +bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    return false;
> +#else
> +    if (!riscv_cpu_cfg(env)->ext_ssdbltrp) {
> +        return false;
> +    }

As we have guard the write to henvcfg and menvcfg by ext_ssdbltrp, I 
think it is enough only check henvcfg or menvcfg.

The only miss is we don't guard the writhe to henvcfgh. I think we can 
add the guard there.

> +    if (virt) {
> +        return (env->henvcfg & HENVCFG_DTE) != 0;
> +    } else {
> +        return (env->menvcfg & MENVCFG_DTE) != 0;
> +    }
> +#endif
> +}
> +
>   void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>                             uint64_t *cs_base, uint32_t *pflags)
>   {
> @@ -562,6 +578,10 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env)
>   
>       g_assert(riscv_has_ext(env, RVH));
>   
> +    if (riscv_env_smode_dbltrp_enabled(env, current_virt)) {
> +        mstatus_mask |= MSTATUS_SDT;
> +    }
> +
>       if (current_virt) {
>           /* Current V=1 and we are about to change to V=0 */
>           env->vsstatus = env->mstatus & mstatus_mask;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e5de72453c..d8280ec956 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -540,6 +540,15 @@ static RISCVException aia_hmode32(CPURISCVState *env, int csrno)
>       return hmode32(env, csrno);
>   }
>   
> +static RISCVException dbltrp_hmode(CPURISCVState *env, int csrno)
> +{
> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    return hmode(env, csrno);
> +}
> +
>   static RISCVException pmp(CPURISCVState *env, int csrno)
>   {
>       if (riscv_cpu_cfg(env)->pmp) {
> @@ -1402,7 +1411,7 @@ static const target_ulong vs_delegable_excps = DELEGABLE_EXCPS &
>         (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
>   static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
>       SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
> -    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS;
> +    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS | SSTATUS_SDT;
This breaks  the v_1_10 constraint, as it is not part of 1.10 specification.
>   
>   /*
>    * Spec allows for bits 13:63 to be either read-only or writable.
> @@ -1600,6 +1609,14 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>           mask |= MSTATUS_VS;
>       }
>   
> +    if (riscv_env_smode_dbltrp_enabled(env, env->virt_enabled)) {
> +        mask |= MSTATUS_SDT;
> +        if ((val & MSTATUS_SDT) != 0) {
> +            mstatus &= ~MSTATUS_SIE;
No need to clean it, if MSTATUS_SIE will be cleaned in val.
> +            val &= ~MSTATUS_SIE;
> +        }
> +    }
> +
I think we should also consider vsstatus for SIE field, as 
write_vsstatus doesn't fall through to write_mstatus.
>       if (xl != MXL_RV32 || env->debugger) {
>           if (riscv_has_ext(env, RVH)) {
>               mask |= MSTATUS_MPV | MSTATUS_GVA;
> @@ -2354,7 +2371,8 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>       if (riscv_cpu_mxl(env) == MXL_RV64) {
>           mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>                   (cfg->ext_sstc ? MENVCFG_STCE : 0) |
> -                (cfg->ext_svadu ? MENVCFG_ADUE : 0);
> +                (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
> +                (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
>       }
>       env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
>   
> @@ -2374,7 +2392,8 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>       const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>       uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>                       (cfg->ext_sstc ? MENVCFG_STCE : 0) |
> -                    (cfg->ext_svadu ? MENVCFG_ADUE : 0);
> +                    (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
> +                    (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
>       uint64_t valh = (uint64_t)val << 32;
>   
>       env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> @@ -2425,9 +2444,10 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>        * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
>        * henvcfg.stce is read_only 0 when menvcfg.stce = 0
>        * henvcfg.adue is read_only 0 when menvcfg.adue = 0
> +     * henvcfg.dte is read_only 0 when menvcfg.dte = 0
>        */
> -    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) |
> -                           env->menvcfg);
> +    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE |
> +                             HENVCFG_DTE) | env->menvcfg);
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -2435,6 +2455,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>                                       target_ulong val)
>   {
>       uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
> +    uint64_t menvcfg_mask;
>       RISCVException ret;
>   
>       ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> @@ -2443,7 +2464,11 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>       }
>   
>       if (riscv_cpu_mxl(env) == MXL_RV64) {
> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
> +        if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
> +            menvcfg_mask |= HENVCFG_DTE;
> +        }
> +        mask |= env->menvcfg & menvcfg_mask;
>       }
>   
>       env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> @@ -2461,8 +2486,8 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>           return ret;
>       }
>   
> -    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) |
> -                            env->menvcfg)) >> 32;
> +    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE |
> +                              HENVCFG_DTE) | env->menvcfg)) >> 32;
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -2470,7 +2495,7 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>                                        target_ulong val)
>   {
>       uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
> -                                    HENVCFG_ADUE);
> +                                    HENVCFG_ADUE | HENVCFG_DTE);

Add the ssdbltrp guard here.

Thanks,
Zhiwei

>       uint64_t valh = (uint64_t)val << 32;
>       RISCVException ret;
>   
> @@ -5246,7 +5271,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>       [CSR_VSATP]       = { "vsatp",       hmode,   read_vsatp,    write_vsatp,
>                             .min_priv_ver = PRIV_VERSION_1_12_0                },
>   
> -    [CSR_MTVAL2]      = { "mtval2",      hmode,   read_mtval2,   write_mtval2,
> +    [CSR_MTVAL2]      = { "mtval2", dbltrp_hmode, read_mtval2, write_mtval2,
>                             .min_priv_ver = PRIV_VERSION_1_12_0                },
>       [CSR_MTINST]      = { "mtinst",      hmode,   read_mtinst,   write_mtinst,
>                             .min_priv_ver = PRIV_VERSION_1_12_0                },
Clément Léger Oct. 17, 2024, 8:35 a.m. UTC | #2
On 16/10/2024 11:40, LIU Zhiwei wrote:
> 
> On 2024/10/14 19:22, Clément Léger wrote:
>> Add ext_ssdbltrp in RISCVCPUConfig and implement MSTATUS.SDT,
>> {H|M}ENVCFG.DTE and modify the availability of MTVAL2 based on the
>> presence of the Ssdbltrp ISA extension.
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>   target/riscv/cpu.h        |  1 +
>>   target/riscv/cpu_bits.h   |  6 ++++++
>>   target/riscv/cpu_cfg.h    |  1 +
>>   target/riscv/cpu_helper.c | 20 +++++++++++++++++
>>   target/riscv/csr.c        | 45 ++++++++++++++++++++++++++++++---------
>>   5 files changed, 63 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index a84e719d3f..ee984bf270 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -553,6 +553,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env,
>> target_ulong geilen);
>>   bool riscv_cpu_vector_enabled(CPURISCVState *env);
>>   void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
>>   int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
>> +bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt);
>>   G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr
>> addr,
>>                                                  MMUAccessType
>> access_type,
>>                                                  int mmu_idx,
>> uintptr_t retaddr);
>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> index da1723496c..3a5588d4df 100644
>> --- a/target/riscv/cpu_bits.h
>> +++ b/target/riscv/cpu_bits.h
>> @@ -558,6 +558,7 @@
>>   #define MSTATUS_TVM         0x00100000 /* since: priv-1.10 */
>>   #define MSTATUS_TW          0x00200000 /* since: priv-1.10 */
>>   #define MSTATUS_TSR         0x00400000 /* since: priv-1.10 */
>> +#define MSTATUS_SDT         0x01000000
>>   #define MSTATUS_GVA         0x4000000000ULL
>>   #define MSTATUS_MPV         0x8000000000ULL
>>   @@ -588,6 +589,7 @@ typedef enum {
>>   #define SSTATUS_XS          0x00018000
>>   #define SSTATUS_SUM         0x00040000 /* since: priv-1.10 */
>>   #define SSTATUS_MXR         0x00080000
>> +#define SSTATUS_SDT         0x01000000
>>     #define SSTATUS64_UXL       0x0000000300000000ULL
>>   @@ -777,11 +779,13 @@ typedef enum RISCVException {
>>   #define MENVCFG_CBIE                       (3UL << 4)
>>   #define MENVCFG_CBCFE                      BIT(6)
>>   #define MENVCFG_CBZE                       BIT(7)
>> +#define MENVCFG_DTE                        (1ULL << 59)
>>   #define MENVCFG_ADUE                       (1ULL << 61)
>>   #define MENVCFG_PBMTE                      (1ULL << 62)
>>   #define MENVCFG_STCE                       (1ULL << 63)
>>     /* For RV32 */
>> +#define MENVCFGH_DTE                       BIT(27)
>>   #define MENVCFGH_ADUE                      BIT(29)
>>   #define MENVCFGH_PBMTE                     BIT(30)
>>   #define MENVCFGH_STCE                      BIT(31)
>> @@ -795,11 +799,13 @@ typedef enum RISCVException {
>>   #define HENVCFG_CBIE                       MENVCFG_CBIE
>>   #define HENVCFG_CBCFE                      MENVCFG_CBCFE
>>   #define HENVCFG_CBZE                       MENVCFG_CBZE
>> +#define HENVCFG_DTE                        MENVCFG_DTE
>>   #define HENVCFG_ADUE                       MENVCFG_ADUE
>>   #define HENVCFG_PBMTE                      MENVCFG_PBMTE
>>   #define HENVCFG_STCE                       MENVCFG_STCE
>>     /* For RV32 */
>> +#define HENVCFGH_DTE                        MENVCFGH_DTE
>>   #define HENVCFGH_ADUE                       MENVCFGH_ADUE
>>   #define HENVCFGH_PBMTE                      MENVCFGH_PBMTE
>>   #define HENVCFGH_STCE                       MENVCFGH_STCE
>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
>> index ae2a945b5f..dd804f95d4 100644
>> --- a/target/riscv/cpu_cfg.h
>> +++ b/target/riscv/cpu_cfg.h
>> @@ -77,6 +77,7 @@ struct RISCVCPUConfig {
>>       bool ext_smstateen;
>>       bool ext_sstc;
>>       bool ext_smcntrpmf;
>> +    bool ext_ssdbltrp;
>>       bool ext_svadu;
>>       bool ext_svinval;
>>       bool ext_svnapot;
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 9d0400035f..77e7736d8a 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -63,6 +63,22 @@ int riscv_env_mmu_index(CPURISCVState *env, bool
>> ifetch)
>>   #endif
>>   }
>>   +bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt)
>> +{
>> +#ifdef CONFIG_USER_ONLY
>> +    return false;
>> +#else
>> +    if (!riscv_cpu_cfg(env)->ext_ssdbltrp) {
>> +        return false;
>> +    }
> 
> As we have guard the write to henvcfg and menvcfg by ext_ssdbltrp, I
> think it is enough only check henvcfg or menvcfg.
> 
> The only miss is we don't guard the writhe to henvcfgh. I think we can
> add the guard there.

Hi Liu,

Actually, since write_henvcfgh() uses mencvfg & (... | HENVCFG_DTE) as a
mask to write the value, if the MENVCFG_DTE bit is not set in menvcfg,
then it won't be set in henvcfgh as well. So I guess you are right about
removing the ext_ssdbltrp check.

> 
>> +    if (virt) {
>> +        return (env->henvcfg & HENVCFG_DTE) != 0;
>> +    } else {
>> +        return (env->menvcfg & MENVCFG_DTE) != 0;
>> +    }
>> +#endif
>> +}
>> +
>>   void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>>                             uint64_t *cs_base, uint32_t *pflags)
>>   {
>> @@ -562,6 +578,10 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState
>> *env)
>>         g_assert(riscv_has_ext(env, RVH));
>>   +    if (riscv_env_smode_dbltrp_enabled(env, current_virt)) {
>> +        mstatus_mask |= MSTATUS_SDT;
>> +    }
>> +
>>       if (current_virt) {
>>           /* Current V=1 and we are about to change to V=0 */
>>           env->vsstatus = env->mstatus & mstatus_mask;
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index e5de72453c..d8280ec956 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -540,6 +540,15 @@ static RISCVException aia_hmode32(CPURISCVState
>> *env, int csrno)
>>       return hmode32(env, csrno);
>>   }
>>   +static RISCVException dbltrp_hmode(CPURISCVState *env, int csrno)
>> +{
>> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
>> +        return RISCV_EXCP_NONE;
>> +    }
>> +
>> +    return hmode(env, csrno);
>> +}
>> +
>>   static RISCVException pmp(CPURISCVState *env, int csrno)
>>   {
>>       if (riscv_cpu_cfg(env)->pmp) {
>> @@ -1402,7 +1411,7 @@ static const target_ulong vs_delegable_excps =
>> DELEGABLE_EXCPS &
>>         (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
>>   static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE |
>> SSTATUS_SPIE |
>>       SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS |
>> SSTATUS_XS |
>> -    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS;
>> +    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS | SSTATUS_SDT;
> This breaks  the v_1_10 constraint, as it is not part of 1.10
> specification.

Yes, you are right, I'll add individual checks for this bit.

>>     /*
>>    * Spec allows for bits 13:63 to be either read-only or writable.
>> @@ -1600,6 +1609,14 @@ static RISCVException
>> write_mstatus(CPURISCVState *env, int csrno,
>>           mask |= MSTATUS_VS;
>>       }
>>   +    if (riscv_env_smode_dbltrp_enabled(env, env->virt_enabled)) {
>> +        mask |= MSTATUS_SDT;
>> +        if ((val & MSTATUS_SDT) != 0) {
>> +            mstatus &= ~MSTATUS_SIE;
> No need to clean it, if MSTATUS_SIE will be cleaned in val.

Indeed.

>> +            val &= ~MSTATUS_SIE;
>> +        }
>> +    }
>> +
> I think we should also consider vsstatus for SIE field, as
> write_vsstatus doesn't fall through to write_mstatus.

It seems like write_vsstatus() allows any value to be written when
accessed directly. But it is masked correctly when swapping for hs to
virt in riscv_cpu_swap_hypervisor_regs(). So I guess the check for SIE
should actually be done in riscv_cpu_swap_hypervisor_regs() itself
before setting mstatus from vsstatus ?


>>       if (xl != MXL_RV32 || env->debugger) {
>>           if (riscv_has_ext(env, RVH)) {
>>               mask |= MSTATUS_MPV | MSTATUS_GVA;
>> @@ -2354,7 +2371,8 @@ static RISCVException
>> write_menvcfg(CPURISCVState *env, int csrno,
>>       if (riscv_cpu_mxl(env) == MXL_RV64) {
>>           mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>>                   (cfg->ext_sstc ? MENVCFG_STCE : 0) |
>> -                (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>> +                (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
>> +                (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
>>       }
>>       env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
>>   @@ -2374,7 +2392,8 @@ static RISCVException
>> write_menvcfgh(CPURISCVState *env, int csrno,
>>       const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>>       uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>>                       (cfg->ext_sstc ? MENVCFG_STCE : 0) |
>> -                    (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>> +                    (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
>> +                    (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
>>       uint64_t valh = (uint64_t)val << 32;
>>         env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
>> @@ -2425,9 +2444,10 @@ static RISCVException
>> read_henvcfg(CPURISCVState *env, int csrno,
>>        * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
>>        * henvcfg.stce is read_only 0 when menvcfg.stce = 0
>>        * henvcfg.adue is read_only 0 when menvcfg.adue = 0
>> +     * henvcfg.dte is read_only 0 when menvcfg.dte = 0
>>        */
>> -    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>> HENVCFG_ADUE) |
>> -                           env->menvcfg);
>> +    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>> HENVCFG_ADUE |
>> +                             HENVCFG_DTE) | env->menvcfg);
>>       return RISCV_EXCP_NONE;
>>   }
>>   @@ -2435,6 +2455,7 @@ static RISCVException
>> write_henvcfg(CPURISCVState *env, int csrno,
>>                                       target_ulong val)
>>   {
>>       uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE |
>> HENVCFG_CBZE;
>> +    uint64_t menvcfg_mask;
>>       RISCVException ret;
>>         ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
>> @@ -2443,7 +2464,11 @@ static RISCVException
>> write_henvcfg(CPURISCVState *env, int csrno,
>>       }
>>         if (riscv_cpu_mxl(env) == MXL_RV64) {
>> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
>> HENVCFG_ADUE);
>> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
>> +        if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
>> +            menvcfg_mask |= HENVCFG_DTE;
>> +        }
>> +        mask |= env->menvcfg & menvcfg_mask;
>>       }
>>         env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
>> @@ -2461,8 +2486,8 @@ static RISCVException
>> read_henvcfgh(CPURISCVState *env, int csrno,
>>           return ret;
>>       }
>>   -    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>> HENVCFG_ADUE) |
>> -                            env->menvcfg)) >> 32;
>> +    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>> HENVCFG_ADUE |
>> +                              HENVCFG_DTE) | env->menvcfg)) >> 32;
>>       return RISCV_EXCP_NONE;
>>   }
>>   @@ -2470,7 +2495,7 @@ static RISCVException
>> write_henvcfgh(CPURISCVState *env, int csrno,
>>                                        target_ulong val)
>>   {
>>       uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
>> -                                    HENVCFG_ADUE);
>> +                                    HENVCFG_ADUE | HENVCFG_DTE);
> 
> Add the ssdbltrp guard here.

As said at the beginning, since henvcfgh uses the menvcfg value as the
mask, if DTE isn't enabled in menvcfg then henvcfg will not be written
as well (and the DTE write in menvcfg is guarded by ext_ssdbltrp).

Thanks for the review,

Clément

> 
> Thanks,
> Zhiwei
> 
>>       uint64_t valh = (uint64_t)val << 32;
>>       RISCVException ret;
>>   @@ -5246,7 +5271,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>       [CSR_VSATP]       = { "vsatp",       hmode,   read_vsatp,   
>> write_vsatp,
>>                             .min_priv_ver =
>> PRIV_VERSION_1_12_0                },
>>   -    [CSR_MTVAL2]      = { "mtval2",      hmode,   read_mtval2,  
>> write_mtval2,
>> +    [CSR_MTVAL2]      = { "mtval2", dbltrp_hmode, read_mtval2,
>> write_mtval2,
>>                             .min_priv_ver =
>> PRIV_VERSION_1_12_0                },
>>       [CSR_MTINST]      = { "mtinst",      hmode,   read_mtinst,  
>> write_mtinst,
>>                             .min_priv_ver =
>> PRIV_VERSION_1_12_0                },
Clément Léger Oct. 17, 2024, 9:12 a.m. UTC | #3
On 17/10/2024 10:35, Clément Léger wrote:
> 
> 
> On 16/10/2024 11:40, LIU Zhiwei wrote:
>>
>> On 2024/10/14 19:22, Clément Léger wrote:
>>> Add ext_ssdbltrp in RISCVCPUConfig and implement MSTATUS.SDT,
>>> {H|M}ENVCFG.DTE and modify the availability of MTVAL2 based on the
>>> presence of the Ssdbltrp ISA extension.
>>>
>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>>   target/riscv/cpu.h        |  1 +
>>>   target/riscv/cpu_bits.h   |  6 ++++++
>>>   target/riscv/cpu_cfg.h    |  1 +
>>>   target/riscv/cpu_helper.c | 20 +++++++++++++++++
>>>   target/riscv/csr.c        | 45 ++++++++++++++++++++++++++++++---------
>>>   5 files changed, 63 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>> index a84e719d3f..ee984bf270 100644
>>> --- a/target/riscv/cpu.h
>>> +++ b/target/riscv/cpu.h
>>> @@ -553,6 +553,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env,
>>> target_ulong geilen);
>>>   bool riscv_cpu_vector_enabled(CPURISCVState *env);
>>>   void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
>>>   int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
>>> +bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt);
>>>   G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr
>>> addr,
>>>                                                  MMUAccessType
>>> access_type,
>>>                                                  int mmu_idx,
>>> uintptr_t retaddr);
>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>> index da1723496c..3a5588d4df 100644
>>> --- a/target/riscv/cpu_bits.h
>>> +++ b/target/riscv/cpu_bits.h
>>> @@ -558,6 +558,7 @@
>>>   #define MSTATUS_TVM         0x00100000 /* since: priv-1.10 */
>>>   #define MSTATUS_TW          0x00200000 /* since: priv-1.10 */
>>>   #define MSTATUS_TSR         0x00400000 /* since: priv-1.10 */
>>> +#define MSTATUS_SDT         0x01000000
>>>   #define MSTATUS_GVA         0x4000000000ULL
>>>   #define MSTATUS_MPV         0x8000000000ULL
>>>   @@ -588,6 +589,7 @@ typedef enum {
>>>   #define SSTATUS_XS          0x00018000
>>>   #define SSTATUS_SUM         0x00040000 /* since: priv-1.10 */
>>>   #define SSTATUS_MXR         0x00080000
>>> +#define SSTATUS_SDT         0x01000000
>>>     #define SSTATUS64_UXL       0x0000000300000000ULL
>>>   @@ -777,11 +779,13 @@ typedef enum RISCVException {
>>>   #define MENVCFG_CBIE                       (3UL << 4)
>>>   #define MENVCFG_CBCFE                      BIT(6)
>>>   #define MENVCFG_CBZE                       BIT(7)
>>> +#define MENVCFG_DTE                        (1ULL << 59)
>>>   #define MENVCFG_ADUE                       (1ULL << 61)
>>>   #define MENVCFG_PBMTE                      (1ULL << 62)
>>>   #define MENVCFG_STCE                       (1ULL << 63)
>>>     /* For RV32 */
>>> +#define MENVCFGH_DTE                       BIT(27)
>>>   #define MENVCFGH_ADUE                      BIT(29)
>>>   #define MENVCFGH_PBMTE                     BIT(30)
>>>   #define MENVCFGH_STCE                      BIT(31)
>>> @@ -795,11 +799,13 @@ typedef enum RISCVException {
>>>   #define HENVCFG_CBIE                       MENVCFG_CBIE
>>>   #define HENVCFG_CBCFE                      MENVCFG_CBCFE
>>>   #define HENVCFG_CBZE                       MENVCFG_CBZE
>>> +#define HENVCFG_DTE                        MENVCFG_DTE
>>>   #define HENVCFG_ADUE                       MENVCFG_ADUE
>>>   #define HENVCFG_PBMTE                      MENVCFG_PBMTE
>>>   #define HENVCFG_STCE                       MENVCFG_STCE
>>>     /* For RV32 */
>>> +#define HENVCFGH_DTE                        MENVCFGH_DTE
>>>   #define HENVCFGH_ADUE                       MENVCFGH_ADUE
>>>   #define HENVCFGH_PBMTE                      MENVCFGH_PBMTE
>>>   #define HENVCFGH_STCE                       MENVCFGH_STCE
>>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
>>> index ae2a945b5f..dd804f95d4 100644
>>> --- a/target/riscv/cpu_cfg.h
>>> +++ b/target/riscv/cpu_cfg.h
>>> @@ -77,6 +77,7 @@ struct RISCVCPUConfig {
>>>       bool ext_smstateen;
>>>       bool ext_sstc;
>>>       bool ext_smcntrpmf;
>>> +    bool ext_ssdbltrp;
>>>       bool ext_svadu;
>>>       bool ext_svinval;
>>>       bool ext_svnapot;
>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>> index 9d0400035f..77e7736d8a 100644
>>> --- a/target/riscv/cpu_helper.c
>>> +++ b/target/riscv/cpu_helper.c
>>> @@ -63,6 +63,22 @@ int riscv_env_mmu_index(CPURISCVState *env, bool
>>> ifetch)
>>>   #endif
>>>   }
>>>   +bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt)
>>> +{
>>> +#ifdef CONFIG_USER_ONLY
>>> +    return false;
>>> +#else
>>> +    if (!riscv_cpu_cfg(env)->ext_ssdbltrp) {
>>> +        return false;
>>> +    }
>>
>> As we have guard the write to henvcfg and menvcfg by ext_ssdbltrp, I
>> think it is enough only check henvcfg or menvcfg.
>>
>> The only miss is we don't guard the writhe to henvcfgh. I think we can
>> add the guard there.
> 
> Hi Liu,
> 
> Actually, since write_henvcfgh() uses mencvfg & (... | HENVCFG_DTE) as a
> mask to write the value, if the MENVCFG_DTE bit is not set in menvcfg,
> then it won't be set in henvcfgh as well. So I guess you are right about
> removing the ext_ssdbltrp check.
> 
>>
>>> +    if (virt) {
>>> +        return (env->henvcfg & HENVCFG_DTE) != 0;
>>> +    } else {
>>> +        return (env->menvcfg & MENVCFG_DTE) != 0;
>>> +    }
>>> +#endif
>>> +}
>>> +
>>>   void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>>>                             uint64_t *cs_base, uint32_t *pflags)
>>>   {
>>> @@ -562,6 +578,10 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState
>>> *env)
>>>         g_assert(riscv_has_ext(env, RVH));
>>>   +    if (riscv_env_smode_dbltrp_enabled(env, current_virt)) {
>>> +        mstatus_mask |= MSTATUS_SDT;
>>> +    }
>>> +
>>>       if (current_virt) {
>>>           /* Current V=1 and we are about to change to V=0 */
>>>           env->vsstatus = env->mstatus & mstatus_mask;
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index e5de72453c..d8280ec956 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -540,6 +540,15 @@ static RISCVException aia_hmode32(CPURISCVState
>>> *env, int csrno)
>>>       return hmode32(env, csrno);
>>>   }
>>>   +static RISCVException dbltrp_hmode(CPURISCVState *env, int csrno)
>>> +{
>>> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
>>> +        return RISCV_EXCP_NONE;
>>> +    }
>>> +
>>> +    return hmode(env, csrno);
>>> +}
>>> +
>>>   static RISCVException pmp(CPURISCVState *env, int csrno)
>>>   {
>>>       if (riscv_cpu_cfg(env)->pmp) {
>>> @@ -1402,7 +1411,7 @@ static const target_ulong vs_delegable_excps =
>>> DELEGABLE_EXCPS &
>>>         (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
>>>   static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE |
>>> SSTATUS_SPIE |
>>>       SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS |
>>> SSTATUS_XS |
>>> -    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS;
>>> +    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS | SSTATUS_SDT;
>> This breaks  the v_1_10 constraint, as it is not part of 1.10
>> specification.
> 
> Yes, you are right, I'll add individual checks for this bit.
> 
>>>     /*
>>>    * Spec allows for bits 13:63 to be either read-only or writable.
>>> @@ -1600,6 +1609,14 @@ static RISCVException
>>> write_mstatus(CPURISCVState *env, int csrno,
>>>           mask |= MSTATUS_VS;
>>>       }
>>>   +    if (riscv_env_smode_dbltrp_enabled(env, env->virt_enabled)) {
>>> +        mask |= MSTATUS_SDT;
>>> +        if ((val & MSTATUS_SDT) != 0) {
>>> +            mstatus &= ~MSTATUS_SIE;
>> No need to clean it, if MSTATUS_SIE will be cleaned in val.
> 
> Indeed.
> 
>>> +            val &= ~MSTATUS_SIE;
>>> +        }
>>> +    }
>>> +
>> I think we should also consider vsstatus for SIE field, as
>> write_vsstatus doesn't fall through to write_mstatus.
> 
> It seems like write_vsstatus() allows any value to be written when
> accessed directly. But it is masked correctly when swapping for hs to
> virt in riscv_cpu_swap_hypervisor_regs(). So I guess the check for SIE
> should actually be done in riscv_cpu_swap_hypervisor_regs() itself
> before setting mstatus from vsstatus ?

Hum actually, clearing SIE when writing vsstatus with SDT seems easier
to handler but the specification does not seems to imply anything about
legal vsstatus values when written  with V=0. And currently, the qemu
implementation allow any value to be written so that seems that clearing
vsstatus when using it to enter V=1 is better. But if any expert could
step in there, that would be helpful :)

Thanks,

Clément

> 
> 
>>>       if (xl != MXL_RV32 || env->debugger) {
>>>           if (riscv_has_ext(env, RVH)) {
>>>               mask |= MSTATUS_MPV | MSTATUS_GVA;
>>> @@ -2354,7 +2371,8 @@ static RISCVException
>>> write_menvcfg(CPURISCVState *env, int csrno,
>>>       if (riscv_cpu_mxl(env) == MXL_RV64) {
>>>           mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>>>                   (cfg->ext_sstc ? MENVCFG_STCE : 0) |
>>> -                (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>>> +                (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
>>> +                (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
>>>       }
>>>       env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
>>>   @@ -2374,7 +2392,8 @@ static RISCVException
>>> write_menvcfgh(CPURISCVState *env, int csrno,
>>>       const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>>>       uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>>>                       (cfg->ext_sstc ? MENVCFG_STCE : 0) |
>>> -                    (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>>> +                    (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
>>> +                    (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
>>>       uint64_t valh = (uint64_t)val << 32;
>>>         env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
>>> @@ -2425,9 +2444,10 @@ static RISCVException
>>> read_henvcfg(CPURISCVState *env, int csrno,
>>>        * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
>>>        * henvcfg.stce is read_only 0 when menvcfg.stce = 0
>>>        * henvcfg.adue is read_only 0 when menvcfg.adue = 0
>>> +     * henvcfg.dte is read_only 0 when menvcfg.dte = 0
>>>        */
>>> -    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>>> HENVCFG_ADUE) |
>>> -                           env->menvcfg);
>>> +    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>>> HENVCFG_ADUE |
>>> +                             HENVCFG_DTE) | env->menvcfg);
>>>       return RISCV_EXCP_NONE;
>>>   }
>>>   @@ -2435,6 +2455,7 @@ static RISCVException
>>> write_henvcfg(CPURISCVState *env, int csrno,
>>>                                       target_ulong val)
>>>   {
>>>       uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE |
>>> HENVCFG_CBZE;
>>> +    uint64_t menvcfg_mask;
>>>       RISCVException ret;
>>>         ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
>>> @@ -2443,7 +2464,11 @@ static RISCVException
>>> write_henvcfg(CPURISCVState *env, int csrno,
>>>       }
>>>         if (riscv_cpu_mxl(env) == MXL_RV64) {
>>> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
>>> HENVCFG_ADUE);
>>> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
>>> +        if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
>>> +            menvcfg_mask |= HENVCFG_DTE;
>>> +        }
>>> +        mask |= env->menvcfg & menvcfg_mask;
>>>       }
>>>         env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
>>> @@ -2461,8 +2486,8 @@ static RISCVException
>>> read_henvcfgh(CPURISCVState *env, int csrno,
>>>           return ret;
>>>       }
>>>   -    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>>> HENVCFG_ADUE) |
>>> -                            env->menvcfg)) >> 32;
>>> +    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>>> HENVCFG_ADUE |
>>> +                              HENVCFG_DTE) | env->menvcfg)) >> 32;
>>>       return RISCV_EXCP_NONE;
>>>   }
>>>   @@ -2470,7 +2495,7 @@ static RISCVException
>>> write_henvcfgh(CPURISCVState *env, int csrno,
>>>                                        target_ulong val)
>>>   {
>>>       uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
>>> -                                    HENVCFG_ADUE);
>>> +                                    HENVCFG_ADUE | HENVCFG_DTE);
>>
>> Add the ssdbltrp guard here.
> 
> As said at the beginning, since henvcfgh uses the menvcfg value as the
> mask, if DTE isn't enabled in menvcfg then henvcfg will not be written
> as well (and the DTE write in menvcfg is guarded by ext_ssdbltrp).
> 
> Thanks for the review,
> 
> Clément
> 
>>
>> Thanks,
>> Zhiwei
>>
>>>       uint64_t valh = (uint64_t)val << 32;
>>>       RISCVException ret;
>>>   @@ -5246,7 +5271,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>>       [CSR_VSATP]       = { "vsatp",       hmode,   read_vsatp,   
>>> write_vsatp,
>>>                             .min_priv_ver =
>>> PRIV_VERSION_1_12_0                },
>>>   -    [CSR_MTVAL2]      = { "mtval2",      hmode,   read_mtval2,  
>>> write_mtval2,
>>> +    [CSR_MTVAL2]      = { "mtval2", dbltrp_hmode, read_mtval2,
>>> write_mtval2,
>>>                             .min_priv_ver =
>>> PRIV_VERSION_1_12_0                },
>>>       [CSR_MTINST]      = { "mtinst",      hmode,   read_mtinst,  
>>> write_mtinst,
>>>                             .min_priv_ver =
>>> PRIV_VERSION_1_12_0                },
>
LIU Zhiwei Oct. 18, 2024, 5:42 a.m. UTC | #4
On 2024/10/17 17:12, Clément Léger wrote:
>
> On 17/10/2024 10:35, Clément Léger wrote:
>>
>> On 16/10/2024 11:40, LIU Zhiwei wrote:
>>> On 2024/10/14 19:22, Clément Léger wrote:
>>>> Add ext_ssdbltrp in RISCVCPUConfig and implement MSTATUS.SDT,
>>>> {H|M}ENVCFG.DTE and modify the availability of MTVAL2 based on the
>>>> presence of the Ssdbltrp ISA extension.
>>>>
>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>>> ---
>>>>    target/riscv/cpu.h        |  1 +
>>>>    target/riscv/cpu_bits.h   |  6 ++++++
>>>>    target/riscv/cpu_cfg.h    |  1 +
>>>>    target/riscv/cpu_helper.c | 20 +++++++++++++++++
>>>>    target/riscv/csr.c        | 45 ++++++++++++++++++++++++++++++---------
>>>>    5 files changed, 63 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>> index a84e719d3f..ee984bf270 100644
>>>> --- a/target/riscv/cpu.h
>>>> +++ b/target/riscv/cpu.h
>>>> @@ -553,6 +553,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env,
>>>> target_ulong geilen);
>>>>    bool riscv_cpu_vector_enabled(CPURISCVState *env);
>>>>    void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
>>>>    int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
>>>> +bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt);
>>>>    G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr
>>>> addr,
>>>>                                                   MMUAccessType
>>>> access_type,
>>>>                                                   int mmu_idx,
>>>> uintptr_t retaddr);
>>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>>> index da1723496c..3a5588d4df 100644
>>>> --- a/target/riscv/cpu_bits.h
>>>> +++ b/target/riscv/cpu_bits.h
>>>> @@ -558,6 +558,7 @@
>>>>    #define MSTATUS_TVM         0x00100000 /* since: priv-1.10 */
>>>>    #define MSTATUS_TW          0x00200000 /* since: priv-1.10 */
>>>>    #define MSTATUS_TSR         0x00400000 /* since: priv-1.10 */
>>>> +#define MSTATUS_SDT         0x01000000
>>>>    #define MSTATUS_GVA         0x4000000000ULL
>>>>    #define MSTATUS_MPV         0x8000000000ULL
>>>>    @@ -588,6 +589,7 @@ typedef enum {
>>>>    #define SSTATUS_XS          0x00018000
>>>>    #define SSTATUS_SUM         0x00040000 /* since: priv-1.10 */
>>>>    #define SSTATUS_MXR         0x00080000
>>>> +#define SSTATUS_SDT         0x01000000
>>>>      #define SSTATUS64_UXL       0x0000000300000000ULL
>>>>    @@ -777,11 +779,13 @@ typedef enum RISCVException {
>>>>    #define MENVCFG_CBIE                       (3UL << 4)
>>>>    #define MENVCFG_CBCFE                      BIT(6)
>>>>    #define MENVCFG_CBZE                       BIT(7)
>>>> +#define MENVCFG_DTE                        (1ULL << 59)
>>>>    #define MENVCFG_ADUE                       (1ULL << 61)
>>>>    #define MENVCFG_PBMTE                      (1ULL << 62)
>>>>    #define MENVCFG_STCE                       (1ULL << 63)
>>>>      /* For RV32 */
>>>> +#define MENVCFGH_DTE                       BIT(27)
>>>>    #define MENVCFGH_ADUE                      BIT(29)
>>>>    #define MENVCFGH_PBMTE                     BIT(30)
>>>>    #define MENVCFGH_STCE                      BIT(31)
>>>> @@ -795,11 +799,13 @@ typedef enum RISCVException {
>>>>    #define HENVCFG_CBIE                       MENVCFG_CBIE
>>>>    #define HENVCFG_CBCFE                      MENVCFG_CBCFE
>>>>    #define HENVCFG_CBZE                       MENVCFG_CBZE
>>>> +#define HENVCFG_DTE                        MENVCFG_DTE
>>>>    #define HENVCFG_ADUE                       MENVCFG_ADUE
>>>>    #define HENVCFG_PBMTE                      MENVCFG_PBMTE
>>>>    #define HENVCFG_STCE                       MENVCFG_STCE
>>>>      /* For RV32 */
>>>> +#define HENVCFGH_DTE                        MENVCFGH_DTE
>>>>    #define HENVCFGH_ADUE                       MENVCFGH_ADUE
>>>>    #define HENVCFGH_PBMTE                      MENVCFGH_PBMTE
>>>>    #define HENVCFGH_STCE                       MENVCFGH_STCE
>>>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
>>>> index ae2a945b5f..dd804f95d4 100644
>>>> --- a/target/riscv/cpu_cfg.h
>>>> +++ b/target/riscv/cpu_cfg.h
>>>> @@ -77,6 +77,7 @@ struct RISCVCPUConfig {
>>>>        bool ext_smstateen;
>>>>        bool ext_sstc;
>>>>        bool ext_smcntrpmf;
>>>> +    bool ext_ssdbltrp;
>>>>        bool ext_svadu;
>>>>        bool ext_svinval;
>>>>        bool ext_svnapot;
>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>> index 9d0400035f..77e7736d8a 100644
>>>> --- a/target/riscv/cpu_helper.c
>>>> +++ b/target/riscv/cpu_helper.c
>>>> @@ -63,6 +63,22 @@ int riscv_env_mmu_index(CPURISCVState *env, bool
>>>> ifetch)
>>>>    #endif
>>>>    }
>>>>    +bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt)
>>>> +{
>>>> +#ifdef CONFIG_USER_ONLY
>>>> +    return false;
>>>> +#else
>>>> +    if (!riscv_cpu_cfg(env)->ext_ssdbltrp) {
>>>> +        return false;
>>>> +    }
>>> As we have guard the write to henvcfg and menvcfg by ext_ssdbltrp, I
>>> think it is enough only check henvcfg or menvcfg.
>>>
>>> The only miss is we don't guard the writhe to henvcfgh. I think we can
>>> add the guard there.
>> Hi Liu,
>>
>> Actually, since write_henvcfgh() uses mencvfg & (... | HENVCFG_DTE) as a
>> mask to write the value, if the MENVCFG_DTE bit is not set in menvcfg,
>> then it won't be set in henvcfgh as well. So I guess you are right about
>> removing the ext_ssdbltrp check.
>>
>>>> +    if (virt) {
>>>> +        return (env->henvcfg & HENVCFG_DTE) != 0;
>>>> +    } else {
>>>> +        return (env->menvcfg & MENVCFG_DTE) != 0;
>>>> +    }
>>>> +#endif
>>>> +}
>>>> +
>>>>    void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>>>>                              uint64_t *cs_base, uint32_t *pflags)
>>>>    {
>>>> @@ -562,6 +578,10 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState
>>>> *env)
>>>>          g_assert(riscv_has_ext(env, RVH));
>>>>    +    if (riscv_env_smode_dbltrp_enabled(env, current_virt)) {
>>>> +        mstatus_mask |= MSTATUS_SDT;
>>>> +    }
>>>> +
>>>>        if (current_virt) {
>>>>            /* Current V=1 and we are about to change to V=0 */
>>>>            env->vsstatus = env->mstatus & mstatus_mask;
>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>> index e5de72453c..d8280ec956 100644
>>>> --- a/target/riscv/csr.c
>>>> +++ b/target/riscv/csr.c
>>>> @@ -540,6 +540,15 @@ static RISCVException aia_hmode32(CPURISCVState
>>>> *env, int csrno)
>>>>        return hmode32(env, csrno);
>>>>    }
>>>>    +static RISCVException dbltrp_hmode(CPURISCVState *env, int csrno)
>>>> +{
>>>> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
>>>> +        return RISCV_EXCP_NONE;
>>>> +    }
>>>> +
>>>> +    return hmode(env, csrno);
>>>> +}
>>>> +
>>>>    static RISCVException pmp(CPURISCVState *env, int csrno)
>>>>    {
>>>>        if (riscv_cpu_cfg(env)->pmp) {
>>>> @@ -1402,7 +1411,7 @@ static const target_ulong vs_delegable_excps =
>>>> DELEGABLE_EXCPS &
>>>>          (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
>>>>    static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE |
>>>> SSTATUS_SPIE |
>>>>        SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS |
>>>> SSTATUS_XS |
>>>> -    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS;
>>>> +    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS | SSTATUS_SDT;
>>> This breaks  the v_1_10 constraint, as it is not part of 1.10
>>> specification.
>> Yes, you are right, I'll add individual checks for this bit.
>>
>>>>      /*
>>>>     * Spec allows for bits 13:63 to be either read-only or writable.
>>>> @@ -1600,6 +1609,14 @@ static RISCVException
>>>> write_mstatus(CPURISCVState *env, int csrno,
>>>>            mask |= MSTATUS_VS;
>>>>        }
>>>>    +    if (riscv_env_smode_dbltrp_enabled(env, env->virt_enabled)) {
>>>> +        mask |= MSTATUS_SDT;
>>>> +        if ((val & MSTATUS_SDT) != 0) {
>>>> +            mstatus &= ~MSTATUS_SIE;
>>> No need to clean it, if MSTATUS_SIE will be cleaned in val.
>> Indeed.
>>
>>>> +            val &= ~MSTATUS_SIE;
>>>> +        }
>>>> +    }
>>>> +
>>> I think we should also consider vsstatus for SIE field, as
>>> write_vsstatus doesn't fall through to write_mstatus.
>> It seems like write_vsstatus() allows any value to be written when
>> accessed directly. But it is masked correctly when swapping for hs to
>> virt in riscv_cpu_swap_hypervisor_regs(). So I guess the check for SIE
>> should actually be done in riscv_cpu_swap_hypervisor_regs() itself
>> before setting mstatus from vsstatus ?
> Hum actually, clearing SIE when writing vsstatus with SDT seems easier
> to handler but the specification does not seems to imply anything about
> legal vsstatus values when written  with V=0. And currently, the qemu
> implementation allow any value to be written so that seems that clearing
> vsstatus when using it to enter V=1 is better. But if any expert could
> step in there, that would be helpful :)

I think it is better to protect the write of SIE and SDT in V=0. This 
behavior will be observed by gdb. Another possible
situation(although not a common case) is that M mode software may want 
to set SDT in vsstatus, then any interrupt in vsmode will be redirect 
into M mode.

Thanks,
Zhiwei

>
> Thanks,
>
> Clément
>
>>
>>>>        if (xl != MXL_RV32 || env->debugger) {
>>>>            if (riscv_has_ext(env, RVH)) {
>>>>                mask |= MSTATUS_MPV | MSTATUS_GVA;
>>>> @@ -2354,7 +2371,8 @@ static RISCVException
>>>> write_menvcfg(CPURISCVState *env, int csrno,
>>>>        if (riscv_cpu_mxl(env) == MXL_RV64) {
>>>>            mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>>>>                    (cfg->ext_sstc ? MENVCFG_STCE : 0) |
>>>> -                (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>>>> +                (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
>>>> +                (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
>>>>        }
>>>>        env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
>>>>    @@ -2374,7 +2392,8 @@ static RISCVException
>>>> write_menvcfgh(CPURISCVState *env, int csrno,
>>>>        const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>>>>        uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>>>>                        (cfg->ext_sstc ? MENVCFG_STCE : 0) |
>>>> -                    (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>>>> +                    (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
>>>> +                    (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
>>>>        uint64_t valh = (uint64_t)val << 32;
>>>>          env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
>>>> @@ -2425,9 +2444,10 @@ static RISCVException
>>>> read_henvcfg(CPURISCVState *env, int csrno,
>>>>         * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
>>>>         * henvcfg.stce is read_only 0 when menvcfg.stce = 0
>>>>         * henvcfg.adue is read_only 0 when menvcfg.adue = 0
>>>> +     * henvcfg.dte is read_only 0 when menvcfg.dte = 0
>>>>         */
>>>> -    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>>>> HENVCFG_ADUE) |
>>>> -                           env->menvcfg);
>>>> +    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>>>> HENVCFG_ADUE |
>>>> +                             HENVCFG_DTE) | env->menvcfg);
>>>>        return RISCV_EXCP_NONE;
>>>>    }
>>>>    @@ -2435,6 +2455,7 @@ static RISCVException
>>>> write_henvcfg(CPURISCVState *env, int csrno,
>>>>                                        target_ulong val)
>>>>    {
>>>>        uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE |
>>>> HENVCFG_CBZE;
>>>> +    uint64_t menvcfg_mask;
>>>>        RISCVException ret;
>>>>          ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
>>>> @@ -2443,7 +2464,11 @@ static RISCVException
>>>> write_henvcfg(CPURISCVState *env, int csrno,
>>>>        }
>>>>          if (riscv_cpu_mxl(env) == MXL_RV64) {
>>>> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
>>>> HENVCFG_ADUE);
>>>> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
>>>> +        if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
>>>> +            menvcfg_mask |= HENVCFG_DTE;
>>>> +        }
>>>> +        mask |= env->menvcfg & menvcfg_mask;
>>>>        }
>>>>          env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
>>>> @@ -2461,8 +2486,8 @@ static RISCVException
>>>> read_henvcfgh(CPURISCVState *env, int csrno,
>>>>            return ret;
>>>>        }
>>>>    -    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>>>> HENVCFG_ADUE) |
>>>> -                            env->menvcfg)) >> 32;
>>>> +    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE |
>>>> HENVCFG_ADUE |
>>>> +                              HENVCFG_DTE) | env->menvcfg)) >> 32;
>>>>        return RISCV_EXCP_NONE;
>>>>    }
>>>>    @@ -2470,7 +2495,7 @@ static RISCVException
>>>> write_henvcfgh(CPURISCVState *env, int csrno,
>>>>                                         target_ulong val)
>>>>    {
>>>>        uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
>>>> -                                    HENVCFG_ADUE);
>>>> +                                    HENVCFG_ADUE | HENVCFG_DTE);
>>> Add the ssdbltrp guard here.
>> As said at the beginning, since henvcfgh uses the menvcfg value as the
>> mask, if DTE isn't enabled in menvcfg then henvcfg will not be written
>> as well (and the DTE write in menvcfg is guarded by ext_ssdbltrp).
>>
>> Thanks for the review,
>>
>> Clément
>>
>>> Thanks,
>>> Zhiwei
>>>
>>>>        uint64_t valh = (uint64_t)val << 32;
>>>>        RISCVException ret;
>>>>    @@ -5246,7 +5271,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>>>        [CSR_VSATP]       = { "vsatp",       hmode,   read_vsatp,
>>>> write_vsatp,
>>>>                              .min_priv_ver =
>>>> PRIV_VERSION_1_12_0                },
>>>>    -    [CSR_MTVAL2]      = { "mtval2",      hmode,   read_mtval2,
>>>> write_mtval2,
>>>> +    [CSR_MTVAL2]      = { "mtval2", dbltrp_hmode, read_mtval2,
>>>> write_mtval2,
>>>>                              .min_priv_ver =
>>>> PRIV_VERSION_1_12_0                },
>>>>        [CSR_MTINST]      = { "mtinst",      hmode,   read_mtinst,
>>>> write_mtinst,
>>>>                              .min_priv_ver =
>>>> PRIV_VERSION_1_12_0                },
diff mbox series

Patch

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index a84e719d3f..ee984bf270 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -553,6 +553,7 @@  void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen);
 bool riscv_cpu_vector_enabled(CPURISCVState *env);
 void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
 int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
+bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt);
 G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
                                                MMUAccessType access_type,
                                                int mmu_idx, uintptr_t retaddr);
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index da1723496c..3a5588d4df 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -558,6 +558,7 @@ 
 #define MSTATUS_TVM         0x00100000 /* since: priv-1.10 */
 #define MSTATUS_TW          0x00200000 /* since: priv-1.10 */
 #define MSTATUS_TSR         0x00400000 /* since: priv-1.10 */
+#define MSTATUS_SDT         0x01000000
 #define MSTATUS_GVA         0x4000000000ULL
 #define MSTATUS_MPV         0x8000000000ULL
 
@@ -588,6 +589,7 @@  typedef enum {
 #define SSTATUS_XS          0x00018000
 #define SSTATUS_SUM         0x00040000 /* since: priv-1.10 */
 #define SSTATUS_MXR         0x00080000
+#define SSTATUS_SDT         0x01000000
 
 #define SSTATUS64_UXL       0x0000000300000000ULL
 
@@ -777,11 +779,13 @@  typedef enum RISCVException {
 #define MENVCFG_CBIE                       (3UL << 4)
 #define MENVCFG_CBCFE                      BIT(6)
 #define MENVCFG_CBZE                       BIT(7)
+#define MENVCFG_DTE                        (1ULL << 59)
 #define MENVCFG_ADUE                       (1ULL << 61)
 #define MENVCFG_PBMTE                      (1ULL << 62)
 #define MENVCFG_STCE                       (1ULL << 63)
 
 /* For RV32 */
+#define MENVCFGH_DTE                       BIT(27)
 #define MENVCFGH_ADUE                      BIT(29)
 #define MENVCFGH_PBMTE                     BIT(30)
 #define MENVCFGH_STCE                      BIT(31)
@@ -795,11 +799,13 @@  typedef enum RISCVException {
 #define HENVCFG_CBIE                       MENVCFG_CBIE
 #define HENVCFG_CBCFE                      MENVCFG_CBCFE
 #define HENVCFG_CBZE                       MENVCFG_CBZE
+#define HENVCFG_DTE                        MENVCFG_DTE
 #define HENVCFG_ADUE                       MENVCFG_ADUE
 #define HENVCFG_PBMTE                      MENVCFG_PBMTE
 #define HENVCFG_STCE                       MENVCFG_STCE
 
 /* For RV32 */
+#define HENVCFGH_DTE                        MENVCFGH_DTE
 #define HENVCFGH_ADUE                       MENVCFGH_ADUE
 #define HENVCFGH_PBMTE                      MENVCFGH_PBMTE
 #define HENVCFGH_STCE                       MENVCFGH_STCE
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index ae2a945b5f..dd804f95d4 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -77,6 +77,7 @@  struct RISCVCPUConfig {
     bool ext_smstateen;
     bool ext_sstc;
     bool ext_smcntrpmf;
+    bool ext_ssdbltrp;
     bool ext_svadu;
     bool ext_svinval;
     bool ext_svnapot;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 9d0400035f..77e7736d8a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -63,6 +63,22 @@  int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
 #endif
 }
 
+bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt)
+{
+#ifdef CONFIG_USER_ONLY
+    return false;
+#else
+    if (!riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        return false;
+    }
+    if (virt) {
+        return (env->henvcfg & HENVCFG_DTE) != 0;
+    } else {
+        return (env->menvcfg & MENVCFG_DTE) != 0;
+    }
+#endif
+}
+
 void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
                           uint64_t *cs_base, uint32_t *pflags)
 {
@@ -562,6 +578,10 @@  void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env)
 
     g_assert(riscv_has_ext(env, RVH));
 
+    if (riscv_env_smode_dbltrp_enabled(env, current_virt)) {
+        mstatus_mask |= MSTATUS_SDT;
+    }
+
     if (current_virt) {
         /* Current V=1 and we are about to change to V=0 */
         env->vsstatus = env->mstatus & mstatus_mask;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index e5de72453c..d8280ec956 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -540,6 +540,15 @@  static RISCVException aia_hmode32(CPURISCVState *env, int csrno)
     return hmode32(env, csrno);
 }
 
+static RISCVException dbltrp_hmode(CPURISCVState *env, int csrno)
+{
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        return RISCV_EXCP_NONE;
+    }
+
+    return hmode(env, csrno);
+}
+
 static RISCVException pmp(CPURISCVState *env, int csrno)
 {
     if (riscv_cpu_cfg(env)->pmp) {
@@ -1402,7 +1411,7 @@  static const target_ulong vs_delegable_excps = DELEGABLE_EXCPS &
       (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
 static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
     SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
-    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS;
+    SSTATUS_SUM | SSTATUS_MXR | SSTATUS_VS | SSTATUS_SDT;
 
 /*
  * Spec allows for bits 13:63 to be either read-only or writable.
@@ -1600,6 +1609,14 @@  static RISCVException write_mstatus(CPURISCVState *env, int csrno,
         mask |= MSTATUS_VS;
     }
 
+    if (riscv_env_smode_dbltrp_enabled(env, env->virt_enabled)) {
+        mask |= MSTATUS_SDT;
+        if ((val & MSTATUS_SDT) != 0) {
+            mstatus &= ~MSTATUS_SIE;
+            val &= ~MSTATUS_SIE;
+        }
+    }
+
     if (xl != MXL_RV32 || env->debugger) {
         if (riscv_has_ext(env, RVH)) {
             mask |= MSTATUS_MPV | MSTATUS_GVA;
@@ -2354,7 +2371,8 @@  static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
     if (riscv_cpu_mxl(env) == MXL_RV64) {
         mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
                 (cfg->ext_sstc ? MENVCFG_STCE : 0) |
-                (cfg->ext_svadu ? MENVCFG_ADUE : 0);
+                (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
+                (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
     }
     env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
 
@@ -2374,7 +2392,8 @@  static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
     const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
     uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
                     (cfg->ext_sstc ? MENVCFG_STCE : 0) |
-                    (cfg->ext_svadu ? MENVCFG_ADUE : 0);
+                    (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
+                    (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
     uint64_t valh = (uint64_t)val << 32;
 
     env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
@@ -2425,9 +2444,10 @@  static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
      * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
      * henvcfg.stce is read_only 0 when menvcfg.stce = 0
      * henvcfg.adue is read_only 0 when menvcfg.adue = 0
+     * henvcfg.dte is read_only 0 when menvcfg.dte = 0
      */
-    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) |
-                           env->menvcfg);
+    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE |
+                             HENVCFG_DTE) | env->menvcfg);
     return RISCV_EXCP_NONE;
 }
 
@@ -2435,6 +2455,7 @@  static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
     uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
+    uint64_t menvcfg_mask;
     RISCVException ret;
 
     ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
@@ -2443,7 +2464,11 @@  static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
     }
 
     if (riscv_cpu_mxl(env) == MXL_RV64) {
-        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
+        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
+        if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+            menvcfg_mask |= HENVCFG_DTE;
+        }
+        mask |= env->menvcfg & menvcfg_mask;
     }
 
     env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
@@ -2461,8 +2486,8 @@  static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
         return ret;
     }
 
-    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) |
-                            env->menvcfg)) >> 32;
+    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE |
+                              HENVCFG_DTE) | env->menvcfg)) >> 32;
     return RISCV_EXCP_NONE;
 }
 
@@ -2470,7 +2495,7 @@  static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
                                      target_ulong val)
 {
     uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
-                                    HENVCFG_ADUE);
+                                    HENVCFG_ADUE | HENVCFG_DTE);
     uint64_t valh = (uint64_t)val << 32;
     RISCVException ret;
 
@@ -5246,7 +5271,7 @@  riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_VSATP]       = { "vsatp",       hmode,   read_vsatp,    write_vsatp,
                           .min_priv_ver = PRIV_VERSION_1_12_0                },
 
-    [CSR_MTVAL2]      = { "mtval2",      hmode,   read_mtval2,   write_mtval2,
+    [CSR_MTVAL2]      = { "mtval2", dbltrp_hmode, read_mtval2, write_mtval2,
                           .min_priv_ver = PRIV_VERSION_1_12_0                },
     [CSR_MTINST]      = { "mtinst",      hmode,   read_mtinst,   write_mtinst,
                           .min_priv_ver = PRIV_VERSION_1_12_0                },