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

[Alexander Graf <agraf@suse.de>]

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.

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

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

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

> +                    } 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?

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


Alex