From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54708) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNEjr-0002I6-JP for qemu-devel@nongnu.org; Thu, 06 Aug 2015 02:33:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZNEjn-0003wH-Er for qemu-devel@nongnu.org; Thu, 06 Aug 2015 02:33:15 -0400 Message-ID: <55C2FF9F.3000607@redhat.com> Date: Thu, 06 Aug 2015 08:33:03 +0200 From: Thomas Huth MIME-Version: 1.0 References: <1438838757-32352-1-git-send-email-aik@ozlabs.ru> <1438838757-32352-3-git-send-email-aik@ozlabs.ru> In-Reply-To: <1438838757-32352-3-git-send-email-aik@ozlabs.ru> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: Alexey Kardashevskiy , qemu-devel@nongnu.org Cc: Alexander Graf , Markus Armbruster , Luiz Capitulino , qemu-ppc@nongnu.org, =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= , David Gibson 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. >=20 > 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. >=20 > 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, FIL= E*f, > #endif > } > =20 > +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 =3D 32 bi= t ? > +{ > + char *endptr =3D NULL; > + int regnum =3D strtoul(numstr, &endptr, 10); > + > + if ((endptr && *endptr) || (regnum >=3D maxnum)) { I'll never get used to your bracketism... > + return -EINVAL; > + } > + *pval =3D regs[regnum]; > + > + return 0; > +} > + > +int ppc_cpu_get_monitor_def(CPUState *cs, const char *name, uint64_t *= pval) > +{ > + int i; > + PowerPCCPU *cpu =3D POWERPC_CPU(cs); > + CPUPPCState *env =3D &cpu->env; > + > +#define MONREG(s, f) \ > + if ((strcasecmp((s), name) =3D=3D 0)) { \ Remove at least here the outermost round brackets? > + *pval =3D (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? > + 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) =3D=3D 0) { > + unsigned int u =3D 0; > + > + for (i =3D 0; i < 8; i++) > + u |=3D env->crf[i] << (32 - (4 * (i + 1))); > + > + return u; > + } > + > + /* General purpose registers */ > + if (name[0] =3D=3D 'r') { > + return ppc_cpu_get_reg(env->gpr, name + 1, ARRAY_SIZE(env->gpr= ), pval); > + } > + > + /* Floating point registers */ > + if (name[0] =3D=3D 'f') { > + return ppc_cpu_get_reg(env->fpr, name + 1, ARRAY_SIZE(env->fpr= ), pval); > + } > + > + /* Segment registers */ > + if (strncmp(name, "sr", 2) =3D=3D 0) { > + return ppc_cpu_get_reg(env->sr, name + 2, ARRAY_SIZE(env->sr),= pval); > + } > + > + /* Special purpose registers */ > + for (i =3D 0; i < ARRAY_SIZE(env->spr_cb); ++i) { > + ppc_spr_t *spr =3D &env->spr_cb[i]; > + > + if (spr->name && (strcasecmp(name, spr->name) =3D=3D 0)) { > + *pval =3D 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 ? Thomas