On 3/23/2023 8:38 AM, Wu, Fei wrote: > 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. > In get_physical_address(): int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK;
We do need separate priv from idx. Thanks, Fei. >> 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~ >