Re: [Qemu-devel] [PATCH qemu 2/2] target-ppc: Define get_monitor_def

[Alexey Kardashevskiy]

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 <address@hidden>
> > ---
> >   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