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

[Richard Henderson <richard.henderson@linaro.org>]

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

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

insn_trans/trans_privileged.c.inc:    if (semihosting_enabled(ctx->mem_idx < PRV_S) &&

are already borderline wrong.

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;
     }

and similarly in riscv_cpu_mmu_index.

Fix all uses of ctx->mmu_idx that are not specifically for memory operations.


r~