From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36646) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNFAN-0005c3-6u for qemu-devel@nongnu.org; Thu, 06 Aug 2015 03:00:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZNFAJ-0002Ia-B5 for qemu-devel@nongnu.org; Thu, 06 Aug 2015 03:00:39 -0400 Received: from mail-pa0-f48.google.com ([209.85.220.48]:36512) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNFAI-0002HJ-Nu for qemu-devel@nongnu.org; Thu, 06 Aug 2015 03:00:35 -0400 Received: by pacrr5 with SMTP id rr5so20460797pac.3 for ; Thu, 06 Aug 2015 00:00:34 -0700 (PDT) References: <1438838757-32352-1-git-send-email-aik@ozlabs.ru> <1438838757-32352-3-git-send-email-aik@ozlabs.ru> <55C2FF9F.3000607@redhat.com> From: Alexey Kardashevskiy Message-ID: <55C3060C.8010108@ozlabs.ru> Date: Thu, 6 Aug 2015 17:00:28 +1000 MIME-Version: 1.0 In-Reply-To: <55C2FF9F.3000607@redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu 2/2] target-ppc: Define get_monitor_def List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth , qemu-devel@nongnu.org Cc: Alexander Graf , Markus Armbruster , Luiz Capitulino , qemu-ppc@nongnu.org, =?UTF-8?Q?Andreas_F=c3=a4rber?= , David Gibson On 08/06/2015 04:33 PM, Thomas Huth wrote: > On 06/08/15 07:25, Alexey Kardashevskiy wrote: >> At the moment get_monitor_def() prints only registers from monitor_defs. >> However there is a lot of BOOK3S SPRs which are not in the list and >> cannot be printed. >> >> This makes use of the new get_monitor_def() callback and prints all >> registered SPRs and fails on unregistered ones proving the user >> information on what is actually supported in the running CPU. >> >> Signed-off-by: Alexey Kardashevskiy >> --- >> monitor.c | 215 +------------------------------------------- >> target-ppc/cpu-qom.h | 2 + >> target-ppc/translate.c | 72 +++++++++++++++ >> target-ppc/translate_init.c | 1 + >> 4 files changed, 76 insertions(+), 214 deletions(-) > ... >> diff --git a/target-ppc/translate.c b/target-ppc/translate.c >> index 84c5cea..f4acafb 100644 >> --- a/target-ppc/translate.c >> +++ b/target-ppc/translate.c >> @@ -11401,6 +11401,78 @@ void ppc_cpu_dump_statistics(CPUState *cs, FILE*f, >> #endif >> } >> >> +static int ppc_cpu_get_reg(target_ulong *regs, const char *numstr, int maxnum, >> + uint64_t *pval) > > Don't you break the 32-bit QEMU (ppc-softmmu instead of ppc64-softmmu) > here? Since pval is uint64_t but the registers are target_ulong = 32 bit ? I cannot see how I break it - 64bit is enough for both, 32bit will just have upper bits set to zero. > >> +{ >> + char *endptr = NULL; >> + int regnum = strtoul(numstr, &endptr, 10); >> + >> + if ((endptr && *endptr) || (regnum >= maxnum)) { > > I'll never get used to your bracketism... > >> + return -EINVAL; >> + } >> + *pval = regs[regnum]; >> + >> + return 0; >> +} >> + >> +int ppc_cpu_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval) >> +{ >> + int i; >> + PowerPCCPU *cpu = POWERPC_CPU(cs); >> + CPUPPCState *env = &cpu->env; >> + >> +#define MONREG(s, f) \ >> + if ((strcasecmp((s), name) == 0)) { \ > > Remove at least here the outermost round brackets? Oh, leftovers... >> + *pval = (f); \ >> + return 0; \ >> + } > > ... also defining a macro with parameters and code within a function > looks somewhat strange to me. Maybe you could consider moving this in > front of the function? Why? It is very very local and not intended to be used anywhere else, why to push people to look for their definitions somewhere else? >> + MONREG("pc", env->nip) >> + MONREG("nip", env->nip) >> + MONREG("lr", env->lr) >> + MONREG("ctr", env->ctr) >> + MONREG("xer", env->xer) >> + MONREG("decr", cpu_ppc_load_decr(env)) >> + MONREG("msr", env->msr) >> + MONREG("tbu", cpu_ppc_load_tbu(env)) >> + MONREG("tbl", cpu_ppc_load_tbl(env)) >> + >> + if (strcasecmp("ccr", name) == 0) { >> + unsigned int u = 0; >> + >> + for (i = 0; i < 8; i++) >> + u |= env->crf[i] << (32 - (4 * (i + 1))); >> + >> + return u; >> + } >> + >> + /* General purpose registers */ >> + if (name[0] == 'r') { >> + return ppc_cpu_get_reg(env->gpr, name + 1, ARRAY_SIZE(env->gpr), pval); >> + } >> + >> + /* Floating point registers */ >> + if (name[0] == 'f') { >> + return ppc_cpu_get_reg(env->fpr, name + 1, ARRAY_SIZE(env->fpr), pval); >> + } >> + >> + /* Segment registers */ >> + if (strncmp(name, "sr", 2) == 0) { >> + return ppc_cpu_get_reg(env->sr, name + 2, ARRAY_SIZE(env->sr), pval); >> + } >> + >> + /* Special purpose registers */ >> + for (i = 0; i < ARRAY_SIZE(env->spr_cb); ++i) { >> + ppc_spr_t *spr = &env->spr_cb[i]; >> + >> + if (spr->name && (strcasecmp(name, spr->name) == 0)) { >> + *pval = env->spr[i]; >> + return 0; >> + } >> + } >> + >> + return -EINVAL; >> +} > > Since translate.c is very overcrowded already ... maybe you could put > this code into a separate file instead? target-ppc/monitor.c ? Or maybe > target-ppc/cpu.c ? Well, I will do that as well (and move ppc_cpu_dump_state&co to this new file) if the whole approach will be ack'ed. -- Alexey