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 <n...@us.ibm.com>
> > >
> > > 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 <n...@us.ibm.com>
> > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> > > ---
> > > 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)?
> > > + 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)?
Basically, asking if #ifdefs in the middle of functions are ok :)
Thanks,
Nish
--
Nishanth Aravamudan <n...@us.ibm.com>
IBM Linux Technology Center
