Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change

[liweiwei <liweiwei@iscas.ac.cn>]

On 2023/3/22 20:12, Fei Wu wrote:
> Kernel needs to access user mode memory e.g. during syscalls, the window
> is usually opened up for a very limited time through MSTATUS.SUM, the
> overhead is too much if tlb_flush() gets called for every SUM change.
>
> This patch creates a separate MMU index for S+SUM, so that it's not
> necessary to flush tlb anymore when SUM changes. This is similar to how
> ARM handles Privileged Access Never (PAN).
>
> Result of 'pipe 10' from unixbench boosts from 223656 to 1705006. Many
> other syscalls benefit a lot from this too.
>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> ---
>   target/riscv/cpu-param.h  |  2 +-
>   target/riscv/cpu.h        |  2 +-
>   target/riscv/cpu_bits.h   |  1 +
>   target/riscv/cpu_helper.c | 11 +++++++++++
>   target/riscv/csr.c        |  2 +-
>   5 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
> index ebaf26d26d..9e21b943f9 100644
> --- a/target/riscv/cpu-param.h
> +++ b/target/riscv/cpu-param.h
> @@ -27,6 +27,6 @@
>    *  - S mode HLV/HLVX/HSV 0b101
>    *  - M mode HLV/HLVX/HSV 0b111
>    */
> -#define NB_MMU_MODES 8
> +#define NB_MMU_MODES 16
>   
>   #endif
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 638e47c75a..ac8bee11a7 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -624,7 +624,7 @@ target_ulong riscv_cpu_get_fflags(CPURISCVState *env);
>   void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong);
>   
>   #define TB_FLAGS_PRIV_MMU_MASK                3
> -#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
> +#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 3)
>   #define TB_FLAGS_MSTATUS_FS MSTATUS_FS
>   #define TB_FLAGS_MSTATUS_VS MSTATUS_VS
>   
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index fca7ef0cef..dd9e62b6e4 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -606,6 +606,7 @@ typedef enum {
>   #define PRV_S 1
>   #define PRV_H 2 /* Reserved */
>   #define PRV_M 3
> +#define MMUIdx_S_SUM 5
>   
>   /* Virtulisation Register Fields */
>   #define VIRT_ONOFF          1
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f88c503cf4..e52e9765d0 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -36,6 +36,17 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>   #ifdef CONFIG_USER_ONLY
>       return 0;
>   #else
> +    if (ifetch) {
> +        return env->priv;
> +    }
> +
> +    int mode = env->priv;
> +    if (mode == PRV_M && get_field(env->mstatus, MSTATUS_MPRV)) {
> +        mode = get_field(env->mstatus, MSTATUS_MPP);
> +    }
> +    if (mode == PRV_S && get_field(env->mstatus, MSTATUS_SUM)) {
> +        return MMUIdx_S_SUM;
> +    }
>       return env->priv;

If we return mode here, whether tlb needn't flush for changes to 
MSTATUS_MPRV and MSTATUS_MPP?

Regards,

Weiwei Li

>   #endif
>   }
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index ab566639e5..eacc40e912 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1246,7 +1246,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>   
>       /* flush tlb on mstatus fields that affect VM */
>       if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
> -            MSTATUS_MPRV | MSTATUS_SUM)) {
> +            MSTATUS_MPRV)) {
>           tlb_flush(env_cpu(env));
>       }
>       mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |