[Qemu-devel] Re: [PATCH 1/4] powerpc: Improve emulation of the BookE MMU

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Mon, Sep 20, 2010 at 12:35:02PM +0200, Alexander Graf wrote:
> Edgar E. Iglesias wrote:
> > Improve the emulation of the BookE MMU to be able to boot linux
> > on virtex5 boards.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > ---
> >  target-ppc/helper.c |   46 ++++++++++++++++++++++++++++++++++++++++------
> >  1 files changed, 40 insertions(+), 6 deletions(-)
> >
> > diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> > index a7ec1f4..4c810d1 100644
> > --- a/target-ppc/helper.c
> > +++ b/target-ppc/helper.c
> > @@ -1325,8 +1325,15 @@ int get_physical_address (CPUState *env, mmu_ctx_t *ctx, target_ulong eaddr,
> >  #endif
> >      if ((access_type == ACCESS_CODE && msr_ir == 0) ||
> >          (access_type != ACCESS_CODE && msr_dr == 0)) {
> > -        /* No address translation */
> > -        ret = check_physical(env, ctx, eaddr, rw);
> > +        if (env->mmu_model == POWERPC_MMU_BOOKE) {
> > +            /* The BookE MMU always performs address translation. The
> > +               IS and DS bits only affect the address space.  */
> > +            ret = mmubooke_get_physical_address(env, ctx, eaddr,
> > +                                                rw, access_type);
> > +        } else {
> > +            /* No address translation.  */
> > +            ret = check_physical(env, ctx, eaddr, rw);
> > +        }
> >      } else {
> >          ret = -1;
> >          switch (env->mmu_model) {
> > @@ -1445,7 +1452,10 @@ int cpu_ppc_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
> >                      break;
> >                  case POWERPC_MMU_BOOKE:
> >                      /* XXX: TODO */
> > -                    cpu_abort(env, "BookE MMU model is not implemented\n");
> > +                    env->exception_index = POWERPC_EXCP_ITLB;
> > +                    env->error_code = 0;
> > +                    env->spr[SPR_BOOKE_DEAR] = address;
> > +                    env->spr[SPR_BOOKE_ESR] = 0x00000000;
> >   
> 
> Uh - I don't see ESR be modified here in the spec.

Right, will fix that.


> 
> >                      return -1;
> >                  case POWERPC_MMU_BOOKE_FSL:
> >                      /* XXX: TODO */
> > @@ -1471,6 +1481,10 @@ int cpu_ppc_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
> >                  break;
> >              case -3:
> >                  /* No execute protection violation */
> > +                if (env->mmu_model == POWERPC_MMU_BOOKE) {
> > +                    env->spr[SPR_BOOKE_DEAR] = address;
> > +                    env->spr[SPR_BOOKE_ESR] = 0x00000000;
> > +                }
> >                  env->exception_index = POWERPC_EXCP_ISI;
> >   
> 
> ISIs don't set DEAR. Only data exceptions do.

OK, will fix that.

> 
> >                  env->error_code = 0x10000000;
> >   
> 
> Hrm. What is error_code? It's basically what later on becomes ESR, no?
> Shouldn't we rather have the error_code setter here be conditional and
> later on set ESR to error_code? SRR1 on BookE is unaffected by error codes.


It looks suspicious, but I didn't add that logic so I think it's fair
to leave that kind of investigation as future improvments.


> 
> >                  break;
> > @@ -1557,7 +1571,14 @@ int cpu_ppc_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
> >                      break;
> >                  case POWERPC_MMU_BOOKE:
> >                      /* XXX: TODO */
> > -                    cpu_abort(env, "BookE MMU model is not implemented\n");
> > +                    env->exception_index = POWERPC_EXCP_DTLB;
> > +                    env->error_code = 0;
> > +                    env->spr[SPR_BOOKE_DEAR] = address;
> > +                    if (rw) {
> > +                        env->spr[SPR_BOOKE_ESR] = 0x00800000;
> >   
> 
> I would really prefer a #define for ESR_ST. By then you can also just do
> 
>   env->spr[SPR_BOOKE_ESR] = rw ? 0 : ESR_ST;
> 
> That's more readable IMHO and doesn't clutter the code with braces.

Sounds good.

> 
> > +                    } else {
> > +                        env->spr[SPR_BOOKE_ESR] = 0x00000000;
> > +                    }
> >                      return -1;
> >                  case POWERPC_MMU_BOOKE_FSL:
> >                      /* XXX: TODO */
> > @@ -1582,6 +1603,13 @@ int cpu_ppc_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
> >                      if (rw) {
> >                          env->spr[SPR_40x_ESR] |= 0x00800000;
> >                      }
> > +                } else if (env->mmu_model == POWERPC_MMU_BOOKE) {
> > +                    env->spr[SPR_BOOKE_DEAR] = address;
> > +                    if (rw) {
> > +                        env->spr[SPR_BOOKE_ESR] = 0x00800000;
> > +                    } else {
> > +                        env->spr[SPR_BOOKE_ESR] = 0x00000000;
> > +                    }
> >                  } else {
> >                      env->spr[SPR_DAR] = address;
> >                      if (rw == 1) {
> > @@ -1848,8 +1876,7 @@ void ppc_tlb_invalidate_all (CPUPPCState *env)
> >          cpu_abort(env, "MPC8xx MMU model is not implemented\n");
> >          break;
> >      case POWERPC_MMU_BOOKE:
> > -        /* XXX: TODO */
> > -        cpu_abort(env, "BookE MMU model is not implemented\n");
> > +        tlb_flush(env, 1);
> >   
> 
> Shouldn't this also clear the entries from the TLB? tlb_flush only
> flushes the qemu TLB, no?

No, these helper calls are only for clearing the internal tables AFAICT.

> 
> >          break;
> >      case POWERPC_MMU_BOOKE_FSL:
> >          /* XXX: TODO */
> > @@ -2629,6 +2656,13 @@ static inline void powerpc_excp(CPUState *env, int excp_model, int excp)
> >      /* Reset exception state */
> >      env->exception_index = POWERPC_EXCP_NONE;
> >      env->error_code = 0;
> > +
> > +    if (env->mmu_model == POWERPC_MMU_BOOKE) {
> > +        /* XXX: The BookE changes address space when switching modes,
> > +                we should probably implement that as different MMU indexes,
> > +                but for the moment we do it the slow way and flush all.  */
> > +        tlb_flush(env, 1);
> >   
> 
> Ugh. Yeah, the Qemu internal TLB really needs some work to fit PPC well.

Well, the problem is not really with the internal qemu tlb, but more with
how we use it in the booke emulation (i.e lazyness from my side).

Thanks for the good review.

Cheers