Re: [Qemu-devel] [Qemu-ppc] [PATCH 02/77] ppc: Use split I/D mmu modes to avoid flushes on interrupts

[Benjamin Herrenschmidt]

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;
>