Thanks for reviewing !
On Mon, 2015-11-16 at 15:49 +1100, David Gibson wrote:
> > static inline void hreg_compute_mem_idx(CPUPPCState *env)
> > {
> > - /* Precompute MMU index */
> > - if (msr_pr == 0 && msr_hv != 0) {
> > - env->mmu_idx = 2;
> > + /* This is our encoding for server processors
> > + *
> > + * 0 = Guest User space virtual mode
> > + * 1 = Guest Kernel space virtual mode
> > + * 2 = Guest Kernel space real mode
> > + * 3 = HV User space virtual mode
> > + * 4 = HV Kernel space virtual mode
> > + * 5 = HV Kernel space real mode
> > + *
> > + * The combination PR=1 IR&DR=0 is invalid, we will treat
> > + * it as IR=DR=1
>
> Hmm.. so being in problem state with translation off would certainly
> be a bad idea, but would it actually behave this way on CPU hardware?
No, it's not allowed in HW. I think (maybe in another patch) I enforce
it. Architecturally, setting PR=1 will fore IR, DR and EE to 1
> > + *
> > + * 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
> > + */
>
> I'm wondering if it might be simpler to unify these and allow all 8
> theoretical possibilities (hv/guest * user/kernel * translationmode)
> for both server and BookE.
I don't see the point. Server doesn't have "AS" and the distinction
only appears in that single function...
> > + 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->mmu_idx = 1 - msr_pr;
> > + /* First calucalte a base value independent of HV */
> > + if (msr_pr != 0) {
> > + /* User space, ignore IR and DR */
> > + env->immu_idx = env->dmmu_idx = 0;
> > + } else {
> > + /* Kernel, setup a base I/D value */
> > + env->immu_idx = msr_ir ? 1 : 2;
> > + env->dmmu_idx = msr_dr ? 1 : 2;
> > + }
> > + /* Then offset it for HV */
> > + if (msr_hv) {
> > + env->immu_idx += 3;
> > + env->dmmu_idx += 3;
> > + }
> > }
> > }
> >
> > @@ -82,9 +121,10 @@ static inline int hreg_store_msr(CPUPPCState
> > *env, target_ulong value,
> > }
> > if (((value >> MSR_IR) & 1) != msr_ir ||
> > ((value >> MSR_DR) & 1) != msr_dr) {
> > - /* Flush all tlb when changing translation mode */
> > - tlb_flush(cs, 1);
> > - excp = POWERPC_EXCP_NONE;
> > + cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> > + }
> > + if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
> > + ((value >> MSR_GS) & 1) != msr_gs) {
> > cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> > }
> > if (unlikely((env->flags & POWERPC_FLAG_TGPR) &&
> > diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> > index f4ac761..b969492 100644
> > --- a/target-ppc/machine.c
> > +++ b/target-ppc/machine.c
> > @@ -90,9 +90,11 @@ static int cpu_load_old(QEMUFile *f, void
> > *opaque, int version_id)
> > qemu_get_betls(f, &env->nip);
> > qemu_get_betls(f, &env->hflags);
> > qemu_get_betls(f, &env->hflags_nmsr);
> > - qemu_get_sbe32s(f, &env->mmu_idx);
>
> Have I missed something, or do you still need a read here to read the
> mmux_idx, even though you'll ignore it, otherwise you'll get out of
> sync and break migration from an old stream.
I am not completely cognizant of the migration stuff, that's very
possible yes. Do I need to read into a dummy local ? Or is there
a way to just say "drop 4 bytes from stream" ?
Note that I have generally completely overlooked the migration impact
of my patches, this is something that I need to do but I wouldn't mind
your help identifying the parts.
> > qemu_get_sbe32(f); /* Discard unused power_mode */
> >
> > + /* Ignore saved mmu_idx, recompute */
> > + hreg_compute_mem_idx(env);
> > +
> > return 0;
> > }
> >
> > diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> > index 308ad68..6d9f252 100644
> > --- a/target-ppc/translate.c
> > +++ b/target-ppc/translate.c
> > @@ -11220,8 +11220,9 @@ void ppc_cpu_dump_state(CPUState *cs, FILE
> > *f, fprintf_function cpu_fprintf,
> > env->nip, env->lr, env->ctr, cpu_read_xer(env),
> > cs->cpu_index);
> > cpu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx
> > " HF "
> > - TARGET_FMT_lx " idx %d\n", env->msr, env-
> > >spr[SPR_HID0],
> > - env->hflags, env->mmu_idx);
> > + TARGET_FMT_lx " iidx %d didx %d\n",
> > + env->msr, env->spr[SPR_HID0],
> > + env->hflags, env->immu_idx, env->dmmu_idx);
> > #if !defined(NO_TIMER_DUMP)
> > cpu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> > #if !defined(CONFIG_USER_ONLY)
> > @@ -11426,7 +11427,7 @@ void gen_intermediate_code(CPUPPCState
> > *env, struct TranslationBlock *tb)
> > ctx.spr_cb = env->spr_cb;
> > ctx.pr = msr_pr;
> > ctx.hv = !msr_pr && msr_hv;
> > - ctx.mem_idx = env->mmu_idx;
> > + ctx.mem_idx = env->dmmu_idx;
> > ctx.insns_flags = env->insns_flags;
> > ctx.insns_flags2 = env->insns_flags2;
> > ctx.access_type = -1;
>
