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

[Wu, Fei]

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~