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...@intel.com wrote:
>>>>>> From: Fei Wu <fei2...@intel.com>
>>>>>>
>>>>>> 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 <fei2...@intel.com>
>>>>>> Reviewed-by: LIU Zhiwei <zhiwei_...@linux.alibaba.com>
>>>>>> ---
>>>>>> 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) &&
>>>>>> + 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.
> 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 |
>
