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 <cle...@rivosinc.com>
>>> Reviewed-by: Alistair Francis <alistair.fran...@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 },
>
