[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change
From: |
Wu, Fei |
Subject: |
Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change |
Date: |
Thu, 23 Mar 2023 08:38:47 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 |
On 3/22/2023 9:19 PM, Richard Henderson wrote:
> On 3/22/23 05: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
>
> This line no longer exists on master.
> The comment above should be updated, and perhaps moved.
>
>> #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)
>
> You can't do this, as you're now overlapping
>
As you mentioned below HYP_ACCESS_MASK is set directly by hyp
instruction translation, there is no overlapping if it's not part of
TB_FLAGS.
> FIELD(TB_FLAGS, LMUL, 3, 3)
>
> You'd need to shift all other fields up to do this.
> There is room, to be sure.
>
> Or you could reuse MMU mode number 2. For that you'd need to separate
> DisasContext.mem_idx from priv. Which should probably be done anyway,
> because tests such as
>
Yes, it looks good to reuse number 2. I tried this v3 patch again with a
different MMUIdx_S_SUM number, only 5 is okay below 8, for the other
number there is no kernel message from guest after opensbi output. I
need to find it out.
> insn_trans/trans_privileged.c.inc: if
> (semihosting_enabled(ctx->mem_idx < PRV_S) &&
>
> are already borderline wrong.
>Yes, it's better not to compare idx to priv.
> I suggest
>
> - #define TB_FLAGS_PRIV_MMU_MASK 3
> - #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2)
>
> HYP_ACCESS_MASK never needed to be part of TB_FLAGS; it is only set
> directly by the hyp access instruction translation. Drop the PRIV mask
> and represent that directly:
>
> - FIELD(TB_FLAGS, MEM_IDX, 0, 3)
> + FIELD(TB_FLAGS, PRIV, 0, 2)
> + FIELD(TB_FLAGS, SUM, 2, 1)
>
> Let SUM occupy the released bit.
>
> In internals.h,
>
> /*
> * The current MMU Modes are:
> * - U 0b000
> * - S 0b001
> * - S+SUM 0b010
> * - M 0b011
> * - HLV/HLVX/HSV adds 0b100
> */
> #define MMUIdx_U 0
> #define MMUIdx_S 1
> #define MMUIdx_S_SUM 2
> #define MMUIdx_M 3
> #define MMU_HYP_ACCESS_BIT (1 << 2)
>
>
> In riscv_tr_init_disas_context:
>
> ctx->priv = FIELD_EX32(tb_flags, TB_FLAGS, PRIV);
> ctx->mmu_idx = ctx->priv;
> if (ctx->mmu_idx == PRV_S && FIELD_EX32(tb_flags, TB_FLAGS, SUM)) {
> ctx->mmu_idx = MMUIdx_S_SUM;
> }
>
There is MSTATUS_MPRV and MSTATUS_MPP kind of thing, priv+sum is not
able to represent all of the status, probably we can just add an extra
'priv' at the back of TB_FLAGS?
Thanks,
Fei.
> and similarly in riscv_cpu_mmu_index.
>
> Fix all uses of ctx->mmu_idx that are not specifically for memory
> operations.
>
>
> r~