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

[Wu, Fei]

On 3/22/2023 8:37 PM, liweiwei wrote:
> 
> 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?
> 
Good point. Besides performance improvement, it sounds more reasonable
to return the effective privilege mode instead of the plain priv, why it
should use the M-mode tlb index when it behaves as S-mode.

Thanks,
Fei.

> 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 |
>