| Message ID | 20210315184615.1985590-14-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/ppc: Fix truncation of env->hflags | expand |
On Mon, Mar 15, 2021 at 12:46:11PM -0600, Richard Henderson wrote: > We weren't recording MSR_GS in hflags, which means that BookE > memory accesses were essentially random vs Guest State. > > Instead of adding this bit directly, record the completed mmu > indexes instead. This makes it obvious that we are recording > exactly the information that we need. > > This also means that we can stop directly recording MSR_IR. What still uses MSR_DR, that you can't also drop it? > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > target/ppc/cpu.h | 12 ++++-- > target/ppc/helper_regs.h | 1 - > target/ppc/helper_regs.c | 88 ++++++++++++++++++++-------------------- > target/ppc/mem_helper.c | 2 +- > target/ppc/translate.c | 6 +-- > 5 files changed, 55 insertions(+), 54 deletions(-) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 23ff16c154..2f8d7fa13c 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -597,7 +597,6 @@ enum { > HFLAGS_64 = 2, /* computed from MSR_CE and MSR_SF */ > HFLAGS_PR = 3, /* MSR_PR */ > HFLAGS_DR = 4, /* MSR_DR */ > - HFLAGS_IR = 5, /* MSR_IR */ > HFLAGS_SPE = 6, /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */ > HFLAGS_TM = 8, /* computed from MSR_TM */ > HFLAGS_BE = 9, /* MSR_BE -- from elsewhere on embedded ppc */ > @@ -606,6 +605,9 @@ enum { > HFLAGS_FP = 13, /* MSR_FP */ > HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */ > HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */ > + > + HFLAGS_IMMU_IDX = 26, /* 26..28 -- the composite immu_idx */ > + HFLAGS_DMMU_IDX = 29, /* 29..31 -- the composite dmmu_idx */ > }; > > /*****************************************************************************/ > @@ -1130,8 +1132,6 @@ struct CPUPPCState { > /* These resources are used only in TCG */ > uint32_t hflags; > target_ulong hflags_compat_nmsr; /* for migration compatibility */ > - int immu_idx; /* precomputed MMU index to speed up insn accesses */ > - int dmmu_idx; /* precomputed MMU index to speed up data accesses */ > > /* Power management */ > int (*check_pow)(CPUPPCState *env); > @@ -1367,7 +1367,11 @@ int ppc_dcr_write(ppc_dcr_t *dcr_env, int dcrn, uint32_t val); > #define MMU_USER_IDX 0 > static inline int cpu_mmu_index(CPUPPCState *env, bool ifetch) > { > - return ifetch ? env->immu_idx : env->dmmu_idx; > +#ifdef CONFIG_USER_ONLY > + return MMU_USER_IDX; > +#else > + return (env->hflags >> (ifetch ? HFLAGS_IMMU_IDX : HFLAGS_DMMU_IDX)) & 7; > +#endif > } > > /* Compatibility modes */ > diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h > index 4148a442b3..42f26870b9 100644 > --- a/target/ppc/helper_regs.h > +++ b/target/ppc/helper_regs.h > @@ -21,7 +21,6 @@ > #define HELPER_REGS_H > > void hreg_swap_gpr_tgpr(CPUPPCState *env); > -void hreg_compute_mem_idx(CPUPPCState *env); > void hreg_compute_hflags(CPUPPCState *env); > void cpu_interrupt_exittb(CPUState *cs); > int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv); > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c > index d62921c322..b28037ca24 100644 > --- a/target/ppc/helper_regs.c > +++ b/target/ppc/helper_regs.c > @@ -43,49 +43,6 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env) > env->tgpr[3] = tmp; > } > > -void hreg_compute_mem_idx(CPUPPCState *env) > -{ > - /* > - * This is our encoding for server processors. The architecture > - * specifies that there is no such thing as userspace with > - * translation off, however it appears that MacOS does it and some > - * 32-bit CPUs support it. Weird... > - * > - * 0 = Guest User space virtual mode > - * 1 = Guest Kernel space virtual mode > - * 2 = Guest User space real mode > - * 3 = Guest Kernel space real mode > - * 4 = HV User space virtual mode > - * 5 = HV Kernel space virtual mode > - * 6 = HV User space real mode > - * 7 = HV Kernel space real mode > - * > - * For BookE, we need 8 MMU modes as follow: > - * > - * 0 = AS 0 HV User space > - * 1 = AS 0 HV Kernel space > - * 2 = AS 1 HV User space > - * 3 = AS 1 HV Kernel space > - * 4 = AS 0 Guest User space > - * 5 = AS 0 Guest Kernel space > - * 6 = AS 1 Guest User space > - * 7 = AS 1 Guest Kernel space > - */ > - if (env->mmu_model & POWERPC_MMU_BOOKE) { > - env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1; > - env->immu_idx += msr_is ? 2 : 0; > - env->dmmu_idx += msr_ds ? 2 : 0; > - env->immu_idx += msr_gs ? 4 : 0; > - env->dmmu_idx += msr_gs ? 4 : 0; > - } else { > - env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1; > - env->immu_idx += msr_ir ? 0 : 2; > - env->dmmu_idx += msr_dr ? 0 : 2; > - env->immu_idx += msr_hv ? 4 : 0; > - env->dmmu_idx += msr_hv ? 4 : 0; > - } > -} > - > void hreg_compute_hflags(CPUPPCState *env) > { > target_ulong msr = env->msr; > @@ -95,7 +52,7 @@ void hreg_compute_hflags(CPUPPCState *env) > > /* Some bits come straight across from MSR. */ > msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) | > - (1 << MSR_DR) | (1 << MSR_IR) | (1 << MSR_FP)); > + (1 << MSR_DR) | (1 << MSR_FP)); > > if (ppc_flags & POWERPC_FLAG_HID0_LE) { > /* > @@ -146,10 +103,51 @@ void hreg_compute_hflags(CPUPPCState *env) > if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) { > hflags |= 1 << HFLAGS_HV; > } > + > + /* > + * This is our encoding for server processors. The architecture > + * specifies that there is no such thing as userspace with > + * translation off, however it appears that MacOS does it and some > + * 32-bit CPUs support it. Weird... > + * > + * 0 = Guest User space virtual mode > + * 1 = Guest Kernel space virtual mode > + * 2 = Guest User space real mode > + * 3 = Guest Kernel space real mode > + * 4 = HV User space virtual mode > + * 5 = HV Kernel space virtual mode > + * 6 = HV User space real mode > + * 7 = HV Kernel space real mode > + * > + * For BookE, we need 8 MMU modes as follow: > + * > + * 0 = AS 0 HV User space > + * 1 = AS 0 HV Kernel space > + * 2 = AS 1 HV User space > + * 3 = AS 1 HV Kernel space > + * 4 = AS 0 Guest User space > + * 5 = AS 0 Guest Kernel space > + * 6 = AS 1 Guest User space > + * 7 = AS 1 Guest Kernel space > + */ > + unsigned immu_idx, dmmu_idx; > + dmmu_idx = msr & (1 << MSR_PR) ? 0 : 1; > + if (env->mmu_model & POWERPC_MMU_BOOKE) { > + dmmu_idx |= msr & (1 << MSR_GS) ? 4 : 0; > + immu_idx = dmmu_idx; > + immu_idx |= msr & (1 << MSR_IS) ? 2 : 0; > + dmmu_idx |= msr & (1 << MSR_DS) ? 2 : 0; > + } else { > + dmmu_idx |= msr & (1ull << MSR_HV) ? 4 : 0; > + immu_idx = dmmu_idx; > + immu_idx |= msr & (1 << MSR_IR) ? 0 : 2; > + dmmu_idx |= msr & (1 << MSR_DR) ? 0 : 2; > + } > + hflags |= immu_idx << HFLAGS_IMMU_IDX; > + hflags |= dmmu_idx << HFLAGS_DMMU_IDX; > #endif > > env->hflags = hflags | (msr & msr_mask); > - hreg_compute_mem_idx(env); > } > > void cpu_interrupt_exittb(CPUState *cs) > diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c > index f4f7e730de..444b2a30ef 100644 > --- a/target/ppc/mem_helper.c > +++ b/target/ppc/mem_helper.c > @@ -278,7 +278,7 @@ static void dcbz_common(CPUPPCState *env, target_ulong addr, > target_ulong mask, dcbz_size = env->dcache_line_size; > uint32_t i; > void *haddr; > - int mmu_idx = epid ? PPC_TLB_EPID_STORE : env->dmmu_idx; > + int mmu_idx = epid ? PPC_TLB_EPID_STORE : cpu_mmu_index(env, false); > > #if defined(TARGET_PPC64) > /* Check for dcbz vs dcbzl on 970 */ > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 5e629291d3..a53463b9b8 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -7658,8 +7658,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags) > cs->cpu_index); > qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx " HF " > "%08x iidx %d didx %d\n", > - env->msr, env->spr[SPR_HID0], > - env->hflags, env->immu_idx, env->dmmu_idx); > + env->msr, env->spr[SPR_HID0], env->hflags, > + cpu_mmu_index(env, true), cpu_mmu_index(env, false)); > #if !defined(NO_TIMER_DUMP) > qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64 > #if !defined(CONFIG_USER_ONLY) > @@ -7885,7 +7885,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > ctx->exception = POWERPC_EXCP_NONE; > ctx->spr_cb = env->spr_cb; > ctx->pr = (hflags >> HFLAGS_PR) & 1; > - ctx->mem_idx = env->dmmu_idx; > + ctx->mem_idx = (hflags >> HFLAGS_DMMU_IDX) & 7; > ctx->dr = (hflags >> HFLAGS_DR) & 1; > ctx->hv = (hflags >> HFLAGS_HV) & 1; > ctx->insns_flags = env->insns_flags; -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 3/21/21 10:26 PM, David Gibson wrote: > On Mon, Mar 15, 2021 at 12:46:11PM -0600, Richard Henderson wrote: >> We weren't recording MSR_GS in hflags, which means that BookE >> memory accesses were essentially random vs Guest State. >> >> Instead of adding this bit directly, record the completed mmu >> indexes instead. This makes it obvious that we are recording >> exactly the information that we need. >> >> This also means that we can stop directly recording MSR_IR. > > What still uses MSR_DR, that you can't also drop it? #define CHK_HVRM \ do { \ if (unlikely(ctx->pr || !ctx->hv || ctx->dr)) { \ I have this notion that this (and CHK_HV and CHK_SV) could be a test against mmu_idx instead, but was reluctant to make that change. r~
On Mon, Mar 22, 2021 at 11:27:49AM -0600, Richard Henderson wrote: > On 3/21/21 10:26 PM, David Gibson wrote: > > On Mon, Mar 15, 2021 at 12:46:11PM -0600, Richard Henderson wrote: > > > We weren't recording MSR_GS in hflags, which means that BookE > > > memory accesses were essentially random vs Guest State. > > > > > > Instead of adding this bit directly, record the completed mmu > > > indexes instead. This makes it obvious that we are recording > > > exactly the information that we need. > > > > > > This also means that we can stop directly recording MSR_IR. > > > > What still uses MSR_DR, that you can't also drop it? > > #define CHK_HVRM \ > do { \ > if (unlikely(ctx->pr || !ctx->hv || ctx->dr)) { \ > > I have this notion that this (and CHK_HV and CHK_SV) could be a test against > mmu_idx instead, but was reluctant to make that change. Yeah, that's checking for hypervisor real mode (hence "HVRM") for ldcix and friends, so it should be equivalent to (mmu_idx != 7). -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 23ff16c154..2f8d7fa13c 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -597,7 +597,6 @@ enum { HFLAGS_64 = 2, /* computed from MSR_CE and MSR_SF */ HFLAGS_PR = 3, /* MSR_PR */ HFLAGS_DR = 4, /* MSR_DR */ - HFLAGS_IR = 5, /* MSR_IR */ HFLAGS_SPE = 6, /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */ HFLAGS_TM = 8, /* computed from MSR_TM */ HFLAGS_BE = 9, /* MSR_BE -- from elsewhere on embedded ppc */ @@ -606,6 +605,9 @@ enum { HFLAGS_FP = 13, /* MSR_FP */ HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */ HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */ + + HFLAGS_IMMU_IDX = 26, /* 26..28 -- the composite immu_idx */ + HFLAGS_DMMU_IDX = 29, /* 29..31 -- the composite dmmu_idx */ }; /*****************************************************************************/ @@ -1130,8 +1132,6 @@ struct CPUPPCState { /* These resources are used only in TCG */ uint32_t hflags; target_ulong hflags_compat_nmsr; /* for migration compatibility */ - int immu_idx; /* precomputed MMU index to speed up insn accesses */ - int dmmu_idx; /* precomputed MMU index to speed up data accesses */ /* Power management */ int (*check_pow)(CPUPPCState *env); @@ -1367,7 +1367,11 @@ int ppc_dcr_write(ppc_dcr_t *dcr_env, int dcrn, uint32_t val); #define MMU_USER_IDX 0 static inline int cpu_mmu_index(CPUPPCState *env, bool ifetch) { - return ifetch ? env->immu_idx : env->dmmu_idx; +#ifdef CONFIG_USER_ONLY + return MMU_USER_IDX; +#else + return (env->hflags >> (ifetch ? HFLAGS_IMMU_IDX : HFLAGS_DMMU_IDX)) & 7; +#endif } /* Compatibility modes */ diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h index 4148a442b3..42f26870b9 100644 --- a/target/ppc/helper_regs.h +++ b/target/ppc/helper_regs.h @@ -21,7 +21,6 @@ #define HELPER_REGS_H void hreg_swap_gpr_tgpr(CPUPPCState *env); -void hreg_compute_mem_idx(CPUPPCState *env); void hreg_compute_hflags(CPUPPCState *env); void cpu_interrupt_exittb(CPUState *cs); int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv); diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c index d62921c322..b28037ca24 100644 --- a/target/ppc/helper_regs.c +++ b/target/ppc/helper_regs.c @@ -43,49 +43,6 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env) env->tgpr[3] = tmp; } -void hreg_compute_mem_idx(CPUPPCState *env) -{ - /* - * This is our encoding for server processors. The architecture - * specifies that there is no such thing as userspace with - * translation off, however it appears that MacOS does it and some - * 32-bit CPUs support it. Weird... - * - * 0 = Guest User space virtual mode - * 1 = Guest Kernel space virtual mode - * 2 = Guest User space real mode - * 3 = Guest Kernel space real mode - * 4 = HV User space virtual mode - * 5 = HV Kernel space virtual mode - * 6 = HV User space real mode - * 7 = HV Kernel space real mode - * - * For BookE, we need 8 MMU modes as follow: - * - * 0 = AS 0 HV User space - * 1 = AS 0 HV Kernel space - * 2 = AS 1 HV User space - * 3 = AS 1 HV Kernel space - * 4 = AS 0 Guest User space - * 5 = AS 0 Guest Kernel space - * 6 = AS 1 Guest User space - * 7 = AS 1 Guest Kernel space - */ - if (env->mmu_model & POWERPC_MMU_BOOKE) { - env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1; - env->immu_idx += msr_is ? 2 : 0; - env->dmmu_idx += msr_ds ? 2 : 0; - env->immu_idx += msr_gs ? 4 : 0; - env->dmmu_idx += msr_gs ? 4 : 0; - } else { - env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1; - env->immu_idx += msr_ir ? 0 : 2; - env->dmmu_idx += msr_dr ? 0 : 2; - env->immu_idx += msr_hv ? 4 : 0; - env->dmmu_idx += msr_hv ? 4 : 0; - } -} - void hreg_compute_hflags(CPUPPCState *env) { target_ulong msr = env->msr; @@ -95,7 +52,7 @@ void hreg_compute_hflags(CPUPPCState *env) /* Some bits come straight across from MSR. */ msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) | - (1 << MSR_DR) | (1 << MSR_IR) | (1 << MSR_FP)); + (1 << MSR_DR) | (1 << MSR_FP)); if (ppc_flags & POWERPC_FLAG_HID0_LE) { /* @@ -146,10 +103,51 @@ void hreg_compute_hflags(CPUPPCState *env) if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) { hflags |= 1 << HFLAGS_HV; } + + /* + * This is our encoding for server processors. The architecture + * specifies that there is no such thing as userspace with + * translation off, however it appears that MacOS does it and some + * 32-bit CPUs support it. Weird... + * + * 0 = Guest User space virtual mode + * 1 = Guest Kernel space virtual mode + * 2 = Guest User space real mode + * 3 = Guest Kernel space real mode + * 4 = HV User space virtual mode + * 5 = HV Kernel space virtual mode + * 6 = HV User space real mode + * 7 = HV Kernel space real mode + * + * For BookE, we need 8 MMU modes as follow: + * + * 0 = AS 0 HV User space + * 1 = AS 0 HV Kernel space + * 2 = AS 1 HV User space + * 3 = AS 1 HV Kernel space + * 4 = AS 0 Guest User space + * 5 = AS 0 Guest Kernel space + * 6 = AS 1 Guest User space + * 7 = AS 1 Guest Kernel space + */ + unsigned immu_idx, dmmu_idx; + dmmu_idx = msr & (1 << MSR_PR) ? 0 : 1; + if (env->mmu_model & POWERPC_MMU_BOOKE) { + dmmu_idx |= msr & (1 << MSR_GS) ? 4 : 0; + immu_idx = dmmu_idx; + immu_idx |= msr & (1 << MSR_IS) ? 2 : 0; + dmmu_idx |= msr & (1 << MSR_DS) ? 2 : 0; + } else { + dmmu_idx |= msr & (1ull << MSR_HV) ? 4 : 0; + immu_idx = dmmu_idx; + immu_idx |= msr & (1 << MSR_IR) ? 0 : 2; + dmmu_idx |= msr & (1 << MSR_DR) ? 0 : 2; + } + hflags |= immu_idx << HFLAGS_IMMU_IDX; + hflags |= dmmu_idx << HFLAGS_DMMU_IDX; #endif env->hflags = hflags | (msr & msr_mask); - hreg_compute_mem_idx(env); } void cpu_interrupt_exittb(CPUState *cs) diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c index f4f7e730de..444b2a30ef 100644 --- a/target/ppc/mem_helper.c +++ b/target/ppc/mem_helper.c @@ -278,7 +278,7 @@ static void dcbz_common(CPUPPCState *env, target_ulong addr, target_ulong mask, dcbz_size = env->dcache_line_size; uint32_t i; void *haddr; - int mmu_idx = epid ? PPC_TLB_EPID_STORE : env->dmmu_idx; + int mmu_idx = epid ? PPC_TLB_EPID_STORE : cpu_mmu_index(env, false); #if defined(TARGET_PPC64) /* Check for dcbz vs dcbzl on 970 */ diff --git a/target/ppc/translate.c b/target/ppc/translate.c index 5e629291d3..a53463b9b8 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -7658,8 +7658,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags) cs->cpu_index); qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx " HF " "%08x iidx %d didx %d\n", - env->msr, env->spr[SPR_HID0], - env->hflags, env->immu_idx, env->dmmu_idx); + env->msr, env->spr[SPR_HID0], env->hflags, + cpu_mmu_index(env, true), cpu_mmu_index(env, false)); #if !defined(NO_TIMER_DUMP) qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64 #if !defined(CONFIG_USER_ONLY) @@ -7885,7 +7885,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) ctx->exception = POWERPC_EXCP_NONE; ctx->spr_cb = env->spr_cb; ctx->pr = (hflags >> HFLAGS_PR) & 1; - ctx->mem_idx = env->dmmu_idx; + ctx->mem_idx = (hflags >> HFLAGS_DMMU_IDX) & 7; ctx->dr = (hflags >> HFLAGS_DR) & 1; ctx->hv = (hflags >> HFLAGS_HV) & 1; ctx->insns_flags = env->insns_flags;
We weren't recording MSR_GS in hflags, which means that BookE memory accesses were essentially random vs Guest State. Instead of adding this bit directly, record the completed mmu indexes instead. This makes it obvious that we are recording exactly the information that we need. This also means that we can stop directly recording MSR_IR. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/ppc/cpu.h | 12 ++++-- target/ppc/helper_regs.h | 1 - target/ppc/helper_regs.c | 88 ++++++++++++++++++++-------------------- target/ppc/mem_helper.c | 2 +- target/ppc/translate.c | 6 +-- 5 files changed, 55 insertions(+), 54 deletions(-) -- 2.25.1