* [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change @ 2023-03-22 12:12 Fei Wu 2023-03-22 12:37 ` liweiwei 2023-03-22 13:19 ` Richard Henderson 0 siblings, 2 replies; 9+ messages in thread From: Fei Wu @ 2023-03-22 12:12 UTC (permalink / raw) Cc: Fei Wu, Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs, open list:All patches CC here 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 #endif diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 638e47c75a..ac8bee11a7 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -624,7 +624,7 @@ target_ulong riscv_cpu_get_fflags(CPURISCVState *env); void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); #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) #define TB_FLAGS_MSTATUS_FS MSTATUS_FS #define TB_FLAGS_MSTATUS_VS MSTATUS_VS diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index fca7ef0cef..dd9e62b6e4 100644 --- a/target/riscv/cpu_bits.h +++ b/target/riscv/cpu_bits.h @@ -606,6 +606,7 @@ typedef enum { #define PRV_S 1 #define PRV_H 2 /* Reserved */ #define PRV_M 3 +#define MMUIdx_S_SUM 5 /* Virtulisation Register Fields */ #define VIRT_ONOFF 1 diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index f88c503cf4..e52e9765d0 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -36,6 +36,17 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) #ifdef CONFIG_USER_ONLY return 0; #else + if (ifetch) { + return env->priv; + } + + int mode = env->priv; + if (mode == PRV_M && get_field(env->mstatus, MSTATUS_MPRV)) { + mode = get_field(env->mstatus, MSTATUS_MPP); + } + if (mode == PRV_S && get_field(env->mstatus, MSTATUS_SUM)) { + return MMUIdx_S_SUM; + } return env->priv; #endif } diff --git a/target/riscv/csr.c b/target/riscv/csr.c index ab566639e5..eacc40e912 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -1246,7 +1246,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, /* flush tlb on mstatus fields that affect VM */ if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | - MSTATUS_MPRV | MSTATUS_SUM)) { + MSTATUS_MPRV)) { tlb_flush(env_cpu(env)); } mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE | -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change 2023-03-22 12:12 [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change Fei Wu @ 2023-03-22 12:37 ` liweiwei 2023-03-22 13:12 ` Wu, Fei 2023-03-22 13:19 ` Richard Henderson 1 sibling, 1 reply; 9+ messages in thread From: liweiwei @ 2023-03-22 12:37 UTC (permalink / raw) To: Fei Wu Cc: liweiwei, Palmer Dabbelt, Alistair Francis, Bin Meng, Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs, open list:All patches CC here On 2023/3/22 20: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 > > #endif > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 638e47c75a..ac8bee11a7 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -624,7 +624,7 @@ target_ulong riscv_cpu_get_fflags(CPURISCVState *env); > void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); > > #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) > #define TB_FLAGS_MSTATUS_FS MSTATUS_FS > #define TB_FLAGS_MSTATUS_VS MSTATUS_VS > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > index fca7ef0cef..dd9e62b6e4 100644 > --- a/target/riscv/cpu_bits.h > +++ b/target/riscv/cpu_bits.h > @@ -606,6 +606,7 @@ typedef enum { > #define PRV_S 1 > #define PRV_H 2 /* Reserved */ > #define PRV_M 3 > +#define MMUIdx_S_SUM 5 > > /* Virtulisation Register Fields */ > #define VIRT_ONOFF 1 > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index f88c503cf4..e52e9765d0 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -36,6 +36,17 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > #ifdef CONFIG_USER_ONLY > return 0; > #else > + if (ifetch) { > + return env->priv; > + } > + > + int mode = env->priv; > + if (mode == PRV_M && get_field(env->mstatus, MSTATUS_MPRV)) { > + mode = get_field(env->mstatus, MSTATUS_MPP); > + } > + if (mode == PRV_S && get_field(env->mstatus, MSTATUS_SUM)) { > + return MMUIdx_S_SUM; > + } > return env->priv; If we return mode here, whether tlb needn't flush for changes to MSTATUS_MPRV and MSTATUS_MPP? Regards, Weiwei Li > #endif > } > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index ab566639e5..eacc40e912 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -1246,7 +1246,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno, > > /* flush tlb on mstatus fields that affect VM */ > if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | > - MSTATUS_MPRV | MSTATUS_SUM)) { > + MSTATUS_MPRV)) { > tlb_flush(env_cpu(env)); > } > mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE | ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change 2023-03-22 12:37 ` liweiwei @ 2023-03-22 13:12 ` Wu, Fei 0 siblings, 0 replies; 9+ messages in thread From: Wu, Fei @ 2023-03-22 13:12 UTC (permalink / raw) To: liweiwei Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs, open list:All patches CC here On 3/22/2023 8:37 PM, liweiwei wrote: > > On 2023/3/22 20: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 >> #endif >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 638e47c75a..ac8bee11a7 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -624,7 +624,7 @@ target_ulong riscv_cpu_get_fflags(CPURISCVState >> *env); >> void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); >> #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) >> #define TB_FLAGS_MSTATUS_FS MSTATUS_FS >> #define TB_FLAGS_MSTATUS_VS MSTATUS_VS >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h >> index fca7ef0cef..dd9e62b6e4 100644 >> --- a/target/riscv/cpu_bits.h >> +++ b/target/riscv/cpu_bits.h >> @@ -606,6 +606,7 @@ typedef enum { >> #define PRV_S 1 >> #define PRV_H 2 /* Reserved */ >> #define PRV_M 3 >> +#define MMUIdx_S_SUM 5 >> /* Virtulisation Register Fields */ >> #define VIRT_ONOFF 1 >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> index f88c503cf4..e52e9765d0 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -36,6 +36,17 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool >> ifetch) >> #ifdef CONFIG_USER_ONLY >> return 0; >> #else >> + if (ifetch) { >> + return env->priv; >> + } >> + >> + int mode = env->priv; >> + if (mode == PRV_M && get_field(env->mstatus, MSTATUS_MPRV)) { >> + mode = get_field(env->mstatus, MSTATUS_MPP); >> + } >> + if (mode == PRV_S && get_field(env->mstatus, MSTATUS_SUM)) { >> + return MMUIdx_S_SUM; >> + } >> return env->priv; > > If we return mode here, whether tlb needn't flush for changes to > MSTATUS_MPRV and MSTATUS_MPP? > Good point. Besides performance improvement, it sounds more reasonable to return the effective privilege mode instead of the plain priv, why it should use the M-mode tlb index when it behaves as S-mode. Thanks, Fei. > Regards, > > Weiwei Li > >> #endif >> } >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index ab566639e5..eacc40e912 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -1246,7 +1246,7 @@ static RISCVException >> write_mstatus(CPURISCVState *env, int csrno, >> /* flush tlb on mstatus fields that affect VM */ >> if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV | >> - MSTATUS_MPRV | MSTATUS_SUM)) { >> + MSTATUS_MPRV)) { >> tlb_flush(env_cpu(env)); >> } >> mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE | > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change 2023-03-22 12:12 [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change Fei Wu 2023-03-22 12:37 ` liweiwei @ 2023-03-22 13:19 ` Richard Henderson 2023-03-23 0:38 ` Wu, Fei 1 sibling, 1 reply; 9+ messages in thread From: Richard Henderson @ 2023-03-22 13:19 UTC (permalink / raw) To: Fei Wu Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs, open list:All patches CC here 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~ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change 2023-03-22 13:19 ` Richard Henderson @ 2023-03-23 0:38 ` Wu, Fei 2023-03-23 1:26 ` Wu, Fei ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Wu, Fei @ 2023-03-23 0:38 UTC (permalink / raw) To: Richard Henderson Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs, open list:All patches CC here 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~ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change 2023-03-23 0:38 ` Wu, Fei @ 2023-03-23 1:26 ` Wu, Fei 2023-03-23 13:24 ` Wu, Fei 2023-03-23 16:11 ` Richard Henderson 2 siblings, 0 replies; 9+ messages in thread From: Wu, Fei @ 2023-03-23 1:26 UTC (permalink / raw) To: Richard Henderson Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs, open list:All patches CC here 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.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. > 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~ > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change 2023-03-23 0:38 ` Wu, Fei 2023-03-23 1:26 ` Wu, Fei @ 2023-03-23 13:24 ` Wu, Fei 2023-03-23 16:41 ` Richard Henderson 2023-03-23 16:11 ` Richard Henderson 2 siblings, 1 reply; 9+ messages in thread From: Wu, Fei @ 2023-03-23 13:24 UTC (permalink / raw) To: Richard Henderson Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs, open list:All patches CC here 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.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? > MPRV+MPP looks not necessary be in TB_FLAGS, it's just used to calculate the mmu_idx. Is it necessary to encode SUM into TB_FLAGS? Thanks, Fei. > Thanks, > Fei. > >> and similarly in riscv_cpu_mmu_index. >> >> Fix all uses of ctx->mmu_idx that are not specifically for memory >> operations. >> >> >> r~ > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change 2023-03-23 13:24 ` Wu, Fei @ 2023-03-23 16:41 ` Richard Henderson 0 siblings, 0 replies; 9+ messages in thread From: Richard Henderson @ 2023-03-23 16:41 UTC (permalink / raw) To: Wu, Fei Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs, open list:All patches CC here On 3/23/23 06:24, Wu, Fei wrote: >> 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? >> > MPRV+MPP looks not necessary be in TB_FLAGS, it's just used to calculate > the mmu_idx. Is it necessary to encode SUM into TB_FLAGS? We need priv separate from midx for priv_level() and similar uses. We do not want MPRV=1, MPP=U to prevent M-mode from executing M instructions. With PRIV and MIDX in TB_FLAGS, we do not need to separately encode MPRV+MPP+SUM, because all of those are composited into MIDX. r~ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change 2023-03-23 0:38 ` Wu, Fei 2023-03-23 1:26 ` Wu, Fei 2023-03-23 13:24 ` Wu, Fei @ 2023-03-23 16:11 ` Richard Henderson 2 siblings, 0 replies; 9+ messages in thread From: Richard Henderson @ 2023-03-23 16:11 UTC (permalink / raw) To: Wu, Fei Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs, open list:All patches CC here On 3/22/23 17:38, Wu, Fei wrote: > 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? Yes, I think that's required. r~ ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-03-23 16:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-22 12:12 [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change Fei Wu 2023-03-22 12:37 ` liweiwei 2023-03-22 13:12 ` Wu, Fei 2023-03-22 13:19 ` Richard Henderson 2023-03-23 0:38 ` Wu, Fei 2023-03-23 1:26 ` Wu, Fei 2023-03-23 13:24 ` Wu, Fei 2023-03-23 16:41 ` Richard Henderson 2023-03-23 16:11 ` Richard Henderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).