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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]

On Fri, Aug 14, 2015 at 01:34:30PM +1000, 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 <aik@ozlabs.ru>

Sorry it's taken me so long to review this.

[snip]
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 84c5cea..2c6f772 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -11401,6 +11401,86 @@ void ppc_cpu_dump_statistics(CPUState *cs, FILE*f,
>  #endif
>  }
>  
> +static bool ppc_cpu_get_reg(target_ulong *regs, const char *numstr, int maxnum,
> +                            uint64_t *pval)
> +{
> +    char *endptr = NULL;
> +    int regnum;
> +
> +    if (!*numstr) {
> +        return false;
> +    }
> +
> +    regnum = strtoul(numstr, &endptr, 10);
> +    if (*endptr || (regnum >= maxnum)) {
> +        return false;
> +    }
> +    *pval = regs[regnum];
> +
> +    return true;
> +}
> +
> +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)) { \
> +        *pval = (f); \
> +        return 0; \
> +    }
> +    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) || (strcasecmp("cr", name) == 0)) {
> +        unsigned int u = 0;
> +
> +        for (i = 0; i < 8; i++)
> +            u |= env->crf[i] << (32 - (4 * (i + 1)));
> +
> +        return u;

The other branches here set *pval and return 0, this one is returning
the register contents directly.  Is that a bug?

Apart from that the patch looks good.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]