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

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

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

On Thu, Aug 06, 2015 at 03:25:57PM +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>

The idea looks sound, but..

[snip]
> 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)
> +{
> +    char *endptr = NULL;
> +    int regnum = strtoul(numstr, &endptr, 10);
> +
> +    if ((endptr && *endptr) || (regnum >= maxnum)) {
> +        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)) { \
> +        *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) {

Probably want to recognize "cr" here as well as "ccr".

> +        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);

This means you won't be able to display SPRs whose names start with
"r" (there aren't many, but there are a few).

Well.. ok, you can by using a capital "R".  But being able to see some
SPRs with lower-case names, but not others definitely violates the
least surprise principle.

> +    }
> +
> +    /* Floating point registers */
> +    if (name[0] == 'f') {
> +        return ppc_cpu_get_reg(env->fpr, name + 1,
> ARRAY_SIZE(env->fpr), pval);

Likewise SPRs beginning with "f" (e.g. fscr).

> +    }
> +
> +    /* Segment registers */
> +    if (strncmp(name, "sr", 2) == 0) {
> +        return ppc_cpu_get_reg(env->sr, name + 2, ARRAY_SIZE(env->sr), pval);

And "sr.." such as the rather important srr0 and srr1.

> +    }
> +
> +    /* 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;
> +}
> +
>  /*****************************************************************************/
>  static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
>                                                    TranslationBlock *tb,
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 16d7b16..038674a 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9706,6 +9706,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
>      cc->dump_state = ppc_cpu_dump_state;
>      cc->dump_statistics = ppc_cpu_dump_statistics;
> +    cc->get_monitor_def = ppc_cpu_get_monitor_def;
>      cc->set_pc = ppc_cpu_set_pc;
>      cc->gdb_read_register = ppc_cpu_gdb_read_register;
>      cc->gdb_write_register = ppc_cpu_gdb_write_register;

-- 
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 --]