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...@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~