Re: [Qemu-ppc] [PATCH 3/3] monitor: add ability to dump SLB entries

[David Gibson]

Good points below.  I forgot to CC Nish, the original patch author on
my post, so I've added him to the list now.

Nish, can you correct these problems and resend the patch please?

On Mon, Oct 31, 2011 at 04:35:54AM +0100, Alexander Graf wrote:
> 
> On 31.10.2011, at 04:16, David Gibson wrote:
> 
> > From: Nishanth Aravamudan <address@hidden>
> > 
> > When run with a PPC Book3S (server) CPU Currently 'info tlb' in the
> > qemu monitor reports "dump_mmu: unimplemented".  However, during
> > bringup work, it can be quite handy to have the SLB entries, which are
> > available in the CPUPPCState.  This patch adds an implementation of
> > info tlb for book3s, which dumps the SLB.
> > 
> > Signed-off-by: Nishanth Aravamudan <address@hidden>
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> > target-ppc/helper.c |   32 +++++++++++++++++++++++++++-----
> > 1 files changed, 27 insertions(+), 5 deletions(-)
> > 
> > diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> > index 137a494..29c7050 100644
> > --- a/target-ppc/helper.c
> > +++ b/target-ppc/helper.c
> > @@ -1545,14 +1545,36 @@ static void mmubooke206_dump_mmu(FILE *f, 
> > fprintf_function cpu_fprintf,
> >     }
> > }
> > 
> > +static void mmubooks_dump_mmu(FILE *f, fprintf_function cpu_fprintf,
> > +                              CPUState *env)
> > +{
> > +    int i;
> > +    uint64_t slbe, slbv;
> > +
> > +    cpu_synchronize_state(env);
> > +
> > +    cpu_fprintf(f, "SLB\tESID\t\t\tVSID\n");
> > +    for (i = 0; i < env->slb_nr; i++) {
> > +        slbe = env->slb[i].esid;
> > +        slbv = env->slb[i].vsid;
> 
> From cpu.h:
> 
> #if defined(TARGET_PPC64)
>     /* Address space register */
>     target_ulong asr;
>     /* PowerPC 64 SLB area */
>     ppc_slb_t slb[64];
>     int slb_nr;
> #endif
> 
> 
> > +        if (slbe == 0 && slbv == 0) {
> > +            continue;
> > +        }
> > +        cpu_fprintf(f, "%d\t0x%016" PRIx64 "\t0x%016" PRIx64 "\n",
> > +                    i, slbe, slbv);
> > +    }
> > +}
> > +
> > void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUState *env)
> > {
> > -    switch (env->mmu_model) {
> > -    case POWERPC_MMU_BOOKE206:
> > +    if (env->mmu_model == POWERPC_MMU_BOOKE206) {
> >         mmubooke206_dump_mmu(f, cpu_fprintf, env);
> > -        break;
> > -    default:
> > -        cpu_fprintf(f, "%s: unimplemented\n", __func__);
> > +    } else {
> > +        if ((env->mmu_model & POWERPC_MMU_64B) != 0) {
> 
> I would actually prefer to explicitly keep the switch and match on all 
> implementations explicitly. Also, have you verified this works without 
> CONFIG_PPC64 set? In cpu.h I see the following:
> 
> #if defined(TARGET_PPC64)
> #define POWERPC_MMU_64       0x00010000
> #define POWERPC_MMU_1TSEG    0x00020000
>     /* 64 bits PowerPC MMU                                     */
>     POWERPC_MMU_64B        = POWERPC_MMU_64 | 0x00000001,
>     /* 620 variant (no segment exceptions)                     */
>     POWERPC_MMU_620        = POWERPC_MMU_64 | 0x00000002,
>     /* Architecture 2.06 variant                               */
>     POWERPC_MMU_2_06       = POWERPC_MMU_64 | POWERPC_MMU_1TSEG | 0x00000003,
> #endif /* defined(TARGET_PPC64) */
> 
> So POWERPC_MMU_64B shouldn't be defined for qemu-system-ppc.
> 
> 
> Alex
> 

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