qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: liweiwei
Subject: Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change
Date: Tue, 21 Mar 2023 17:47:54 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0


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 <fei2.wu@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.wu@intel.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@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?

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 |




reply via email to

[Prev in Thread] Current Thread [Next in Thread]