Re: [PATCH v2] target/riscv: fix H extension TVM trap

[liweiwei]

On 2023/3/10 22:33, chenyi2000@zju.edu.cn wrote:
> From: Yi Chen <chenyi2000@zju.edu.cn>
>
> - Trap satp/hgatp accesses from HS-mode when MSTATUS.TVM is enabled.
> - Trap satp accesses from VS-mode when HSTATUS.VTVM is enabled.
> - Raise RISCV_EXCP_ILLEGAL_INST when U-mode executes SFENCE.VMA/SINVAL.VMA.
> - Raise RISCV_EXCP_VIRT_INSTRUCTION_FAULT when VU-mode executes
>    SFENCE.VMA/SINVAL.VMA or VS-mode executes SFENCE.VMA/SINVAL.VMA with
>    HSTATUS.VTVM enabled.
> - Raise RISCV_EXCP_VIRT_INSTRUCTION_FAULT when VU-mode executes
>    HFENCE.GVMA/HFENCE.VVMA/HINVAL.GVMA/HINVAL.VVMA.
>
> Signed-off-by: Yi Chen <chenyi2000@zju.edu.cn>
> ---
>   target/riscv/csr.c       | 56 +++++++++++++++++++++++++---------------
>   target/riscv/op_helper.c |  9 ++++---
>   2 files changed, 40 insertions(+), 25 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d522efc0b6..26a02e57bd 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -443,6 +443,30 @@ static RISCVException sstc_32(CPURISCVState *env, int 
> csrno)
>       return sstc(env, csrno);
>   }
>   
> +static RISCVException satp(CPURISCVState *env, int csrno)
> +{
> +    if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) &&
> +        get_field(env->mstatus, MSTATUS_TVM)) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +    if (env->priv == PRV_S && riscv_cpu_virt_enabled(env) &&
> +        get_field(env->hstatus, HSTATUS_VTVM)) {
> +        return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +    }
> +
> +    return smode(env, csrno);
> +}
> +
> +static RISCVException hgatp(CPURISCVState *env, int csrno)
> +{
> +    if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) &&
> +        get_field(env->mstatus, MSTATUS_TVM)) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    return hmode(env, csrno);
> +}
> +
>   /* Checks if PointerMasking registers could be accessed */
>   static RISCVException pointer_masking(CPURISCVState *env, int csrno)
>   {
> @@ -2655,13 +2679,7 @@ static RISCVException read_satp(CPURISCVState *env, int 
> csrno,
>           *val = 0;
>           return RISCV_EXCP_NONE;
>       }
> -
> -    if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> -        return RISCV_EXCP_ILLEGAL_INST;
> -    } else {
> -        *val = env->satp;
> -    }
> -
> +    *val = env->satp;
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -2684,18 +2702,14 @@ static RISCVException write_satp(CPURISCVState *env, int csrno,
>       }
>   
>       if (vm && mask) {
> -        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> -            return RISCV_EXCP_ILLEGAL_INST;
> -        } else {
> -            /*
> -             * The ISA defines SATP.MODE=Bare as "no translation", but we still
> -             * pass these through QEMU's TLB emulation as it improves
> -             * performance.  Flushing the TLB on SATP writes with paging
> -             * enabled avoids leaking those invalid cached mappings.
> -             */
> -            tlb_flush(env_cpu(env));
> -            env->satp = val;
> -        }
> +        /*
> +         * The ISA defines SATP.MODE=Bare as "no translation", but we still
> +         * pass these through QEMU's TLB emulation as it improves
> +         * performance.  Flushing the TLB on SATP writes with paging
> +         * enabled avoids leaking those invalid cached mappings.
> +         */
> +        tlb_flush(env_cpu(env));
> +        env->satp = val;
>       }
>       return RISCV_EXCP_NONE;
>   }
> @@ -4180,7 +4194,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>                            .min_priv_ver = PRIV_VERSION_1_12_0 },
>   
>       /* Supervisor Protection and Translation */
> -    [CSR_SATP]     = { "satp",     smode, read_satp,     write_satp     },
> +    [CSR_SATP]     = { "satp",     satp, read_satp,     write_satp     },
>   
>       /* Supervisor-Level Window to Indirectly Accessed Registers (AIA) */
>       [CSR_SISELECT]   = { "siselect",   aia_smode, NULL, NULL, rmw_xiselect },
> @@ -4217,7 +4231,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>                             .min_priv_ver = PRIV_VERSION_1_12_0                
> },
>       [CSR_HGEIP]       = { "hgeip",       hmode,   read_hgeip,
>                             .min_priv_ver = PRIV_VERSION_1_12_0                
> },
> -    [CSR_HGATP]       = { "hgatp",       hmode,   read_hgatp,   write_hgatp,
> +    [CSR_HGATP]       = { "hgatp",       hgatp,   read_hgatp,   write_hgatp,
>                             .min_priv_ver = PRIV_VERSION_1_12_0                
> },
>       [CSR_HTIMEDELTA]  = { "htimedelta",  hmode,   read_htimedelta,
>                             write_htimedelta,
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 84ee018f7d..fbccca9e0b 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -381,12 +381,13 @@ void helper_wfi(CPURISCVState *env)
>   void helper_tlb_flush(CPURISCVState *env)
>   {
>       CPUState *cs = env_cpu(env);
> -    if (!(env->priv >= PRV_S) ||
> -        (env->priv == PRV_S &&
> +    if ((!(env->priv >= PRV_S) && !riscv_cpu_virt_enabled(env)) ||
> +        (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) &&

"!riscv_cpu_virt_enabled(env)" can be extracted out to be shared by the 
two condition.

And it may be more clear if "!(env->priv >= PRV_S)" is replaced by 
"env->priv == PRV_U"

Otherwise, this patch is LGTM.

Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

>            get_field(env->mstatus, MSTATUS_TVM))) {
>           riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>       } else if (riscv_has_ext(env, RVH) && riscv_cpu_virt_enabled(env) &&

By the way, "riscv_has_ext(env, RVH) && riscv_cpu_virt_enabled(env) " 
can be simplified to use

"riscv_cpu_virt_enabled(env)" only here.

Regards,

Weiwei Li
> -               get_field(env->hstatus, HSTATUS_VTVM)) {
> +               (!(env->priv >= PRV_S) ||
> +                get_field(env->hstatus, HSTATUS_VTVM))) {
>           riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, 
> GETPC());
>       } else {
>           tlb_flush(cs);
> @@ -403,7 +404,7 @@ void helper_hyp_tlb_flush(CPURISCVState *env)
>   {
>       CPUState *cs = env_cpu(env);
>   
> -    if (env->priv == PRV_S && riscv_cpu_virt_enabled(env)) {
> +    if (env->priv <= PRV_S && riscv_cpu_virt_enabled(env)) {
>           riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, 
> GETPC());
>       }
>