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

[Nishanth Aravamudan]

On 31.10.2011 [17:35:13 -0700], Alexander Graf wrote:
> 
> On 31.10.2011, at 15:53, Nishanth Aravamudan wrote:
> 
> > On 31.10.2011 [15:14:12 +1100], David Gibson wrote:
> >> 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?
> > 
> > Yep, I'll work on this shortly.
> > 
> >> 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
> > 
> > Being unfamiliar with qemu's coding style (and not immediately seeing it
> > in CODING_STYLE), would the right approach be to wrap this definition in
> > an #if defined(TARGET_PPC64)?
> 
> It's the only way to make it compile :)
> 
> > 
> >>>> +        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.
> > 
> > And similarly here, only have the MMU_2_06 and MMU_64B cases in the
> > switch be defined #if defined(TARGET_PPC64)?
> 
> Yup :).
> 
> > Basically, asking if #ifdefs in the middle of functions are ok :)
> 
> It's the only valid way of making it work, yes. The alternative would
> be to drop all the TARGET_PPC64 ifdefery, but let's keep that for
> later.

Great, thanks for the confirmation.

-Nish

-- 
Nishanth Aravamudan <address@hidden>
IBM Linux Technology Center