From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42066) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RL30B-0004aB-9L for qemu-devel@nongnu.org; Mon, 31 Oct 2011 21:18:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RL309-00061z-Q1 for qemu-devel@nongnu.org; Mon, 31 Oct 2011 21:18:55 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:43788) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RL309-0005zT-Ju for qemu-devel@nongnu.org; Mon, 31 Oct 2011 21:18:53 -0400 Received: from /spool/local by e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 31 Oct 2011 19:18:49 -0600 Date: Mon, 31 Oct 2011 18:18:28 -0700 From: Nishanth Aravamudan Message-ID: <20111101011828.GH4078@us.ibm.com> References: <1320031007-25884-1-git-send-email-david@gibson.dropbear.id.au> <1320031007-25884-4-git-send-email-david@gibson.dropbear.id.au> <3CCDA61A-2DAE-416F-879B-ABE3104EED2B@suse.de> <20111031041412.GG9698@truffala.fritz.box> <20111031225340.GC4078@us.ibm.com> <477A2C7C-C5B9-409F-849C-A6F09E608899@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <477A2C7C-C5B9-409F-849C-A6F09E608899@suse.de> Subject: Re: [Qemu-devel] [PATCH 3/3] monitor: add ability to dump SLB entries List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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 > >>>> > >>>> 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 > >>>> Signed-off-by: David Gibson > >>>> --- > >>>> 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 IBM Linux Technology Center