On 2023/3/21 20:00, Wu, Fei wrote: > On 3/21/2023 5:47 PM, liweiwei wrote: >> On 2023/3/21 17:14, Wu, Fei wrote: >>> On 3/21/2023 4:50 PM, liweiwei wrote: >>>> On 2023/3/21 16:40, Wu, Fei wrote: >>>>> On 3/21/2023 4:28 PM, liweiwei wrote: >>>>>> On 2023/3/21 14:37,fei2.wu@intel.com wrote: >>>>>>> From: Fei Wu >>>>>>> >>>>>>> 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 saves addresses accessed when SUM=1, and flushs only these >>>>>>> pages when SUM changes to 0. If the buffer is not large enough to >>>>>>> save >>>>>>> all the pages during SUM=1, it will fall back to tlb_flush when >>>>>>> necessary. >>>>>>> >>>>>>> The buffer size is set to 4 since in this MSTATUS.SUM open-up window, >>>>>>> most of the time kernel accesses 1 or 2 pages, it's very rare to see >>>>>>> more than 4 pages accessed. >>>>>>> >>>>>>> It's not necessary to save/restore these new added status, as >>>>>>> tlb_flush() is always called after restore. >>>>>>> >>>>>>> Result of 'pipe 10' from unixbench boosts from 223656 to 1327407. >>>>>>> Many >>>>>>> other syscalls benefit a lot from this one too. >>>>>>> >>>>>>> Signed-off-by: Fei Wu >>>>>>> Reviewed-by: LIU Zhiwei >>>>>>> --- >>>>>>>     target/riscv/cpu.h        |  4 ++++ >>>>>>>     target/riscv/cpu_helper.c |  7 +++++++ >>>>>>>     target/riscv/csr.c        | 14 +++++++++++++- >>>>>>>     3 files changed, 24 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>>>>>> index 638e47c75a..926dbce59f 100644 >>>>>>> --- a/target/riscv/cpu.h >>>>>>> +++ b/target/riscv/cpu.h >>>>>>> @@ -383,6 +383,10 @@ struct CPUArchState { >>>>>>>         uint64_t kvm_timer_compare; >>>>>>>         uint64_t kvm_timer_state; >>>>>>>         uint64_t kvm_timer_frequency; >>>>>>> + >>>>>>> +#define MAX_CACHED_SUM_U_ADDR_NUM 4 >>>>>>> +    uint64_t sum_u_count; >>>>>>> +    uint64_t sum_u_addr[MAX_CACHED_SUM_U_ADDR_NUM]; >>>>>>>     }; >>>>>>>       OBJECT_DECLARE_CPU_TYPE(RISCVCPU, RISCVCPUClass, RISCV_CPU) >>>>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>>>>>> index f88c503cf4..5ad0418eb6 100644 >>>>>>> --- a/target/riscv/cpu_helper.c >>>>>>> +++ b/target/riscv/cpu_helper.c >>>>>>> @@ -1068,6 +1068,13 @@ restart: >>>>>>>                         (access_type == MMU_DATA_STORE || (pte & >>>>>>> PTE_D))) { >>>>>>>                     *prot |= PAGE_WRITE; >>>>>>>                 } >>>>>>> +            if ((pte & PTE_U) && (mode & PRV_S) && It's more readable to use "mode == PRV_S" instead of  "mode & PRV_S" here. >>>>>>> +                    get_field(env->mstatus, MSTATUS_SUM)) { >>>>>>> +                if (env->sum_u_count < MAX_CACHED_SUM_U_ADDR_NUM) { >>>>>>> +                    env->sum_u_addr[env->sum_u_count] = addr; >>>>>>> +                } >>>>>>> +                ++env->sum_u_count; >>>>>>> +            } >>>>>>>                 return TRANSLATE_SUCCESS; >>>>>>>             } >>>>>>>         } >>>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >>>>>>> index ab566639e5..74b7638c8a 100644 >>>>>>> --- a/target/riscv/csr.c >>>>>>> +++ b/target/riscv/csr.c >>>>>>> @@ -1246,9 +1246,21 @@ 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)); >>>>>>> +        env->sum_u_count = 0; >>>>>>> +    } else if ((mstatus & MSTATUS_SUM) && !(val & MSTATUS_SUM)) { >>>>>>> +        if (env->sum_u_count > MAX_CACHED_SUM_U_ADDR_NUM) { >>>>>>> +            tlb_flush(env_cpu(env)); >>>>>>> +        } else { >>>>>>> +            for (int i = 0; i < env->sum_u_count; ++i) { >>>>>>> +                tlb_flush_page_by_mmuidx(env_cpu(env), >>>>>>> env->sum_u_addr[i], >>>>>>> +                                         1 << PRV_S | 1 << PRV_M); >>>>>>> +            } >>>>>>> +        } >>>>>>> +        env->sum_u_count = 0; >>>>>>>         } >>>>>> Whether tlb should  be flushed when SUM is changed from 0 to 1? >>>>>> >>>>> When SUM is changed from 0 to 1, all the existing tlb entries remain >>>>> valid as the permission is elevated instead of reduced, so I don't >>>>> think >>>>> it's necessary to flush tlb. >>>> If elevated not unchanged, I think the tlb also needs update, since new >>>> permitted access rights may be added to the tlb. >>>> >>> Assume the following flow, if the new rights have been added to tlb >>> during SUM=0, they're visible and still valid after setting SUM=1 again. >>> Could you please add a specific counter example in this flow? >>> >> Assuming addr0 cannot be access from S mode when SUM = 0, but can be >> accessed from S mode if SUM=1, >> >> and there is a tlb entry for it when SUM = 0 >> >>> enable uaccess (set SUM = 1) >> if we don't flush it when we change SUM to 1 in this step >>> ... (access user mem from S mode) >> when we access addr0 here, tlb will be hit( not updated) and the access >> will trigger fault instead of allowing the access >>> disable uaccess (set SUM = 0) >>> >>> ... (update TLB_SUM_0) >>> >>>      <-- flush tlb or not right before enabling uaccess? >>> enable uaccess (set SUM = 1) >>>      <-- okay to access TLB_SUM_0? >>> disable uaccess (set SUM = 0) >> So, I think the question is whether the rights in TLB entry can be >> elevated. Or whether there is legal tlb entry for this addr0 when SUM = 0? >> > I think there is no such tlb entry: > * If it's loaded into tlb when SUM = 0. This is impossible as it's not > accessible when SUM = 0, it's a fault instead of being loaded into tlb. > * If it's loaded into tlb when SUM = 1. It will be flushed when SUM sets > to 0. > > Thanks, > Fei. > OK. It's acceptable to me. Reviewed-by: Weiwei Li Weiwei Li >> If not,  all my above assumption will not be right. >> >> Regards, >> >> Weiwei Li >> >>> Thanks, >>> Fei. >>> >> >> >>>> Regards, >>>> >>>> Weiwei Li >>>> >>>>> Thanks, >>>>> Fei. >>>>> >>>>>> Regards, >>>>>> >>>>>> Weiwei Li >>>>>> >>>>>>> + >>>>>>>         mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | >>>>>>> MSTATUS_MPIE | >>>>>>>             MSTATUS_SPP | MSTATUS_MPRV | MSTATUS_SUM | >>>>>>>             MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR |