Re: [Qemu-devel] [PATCH qemu v4] monitor/target-ppc: Define target_get_monitor_def

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

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

On Fri, Oct 02, 2015 at 04:16:13PM +1000, Alexey Kardashevskiy wrote:
> At the moment get_monitor_def() returns only registers from statically
> defined monitor_defs array. However there is a lot of BOOK3S SPRs
> which are not in the list and cannot be printed from the monitor.
> 
> This adds a new target platform hook - target_get_monitor_def().
> The hook is called if a register was not found in the static
> array returned by the target_monitor_defs() hook.
> 
> The hook is only defined for POWERPC, it returns registered
> SPRs and fails on unregistered ones providing the user with information
> on what is actually supported on the running CPU. The register value is
> saved as uint64_t as it is the biggest supported register size;
> target_ulong cannot be used because of the stub - it is in a "common"
> code and cannot include "cpu.h", etc; this is also why the hook prototype
> is redefined in the stub instead of being included from some header.
> 
> This replaces static descriptors for GPRs, FPRs, SRs with a helper which
> looks for a value in a corresponding array in the CPUPPCState.
> The immediate effect is that all 32 SRs can be printed now (instead of 16);
> later this can be reused for VSX or TM registers.
> 
> While we are here, this adds "cr" as a synonym of "ccr".

Sorry I've taken so long to look at this.

This is a big improvement over the current approach.

> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

(although I note one small nit below)

> ---
> 
> Does it make sense to split it into two patches?

Meaning one which adds the hook to the monitor, and another which
implements it for ppc?  Maybe.

I'd like to take this, but I'm not sure whether it's reasonable for
the small generic monitor change to go through the ppc tree.  Peter,
Paolo, opinion?


[snip]
> diff --git a/target-ppc/monitor.c b/target-ppc/monitor.c
> index 9cb1fe9..98417f0 100644
> --- a/target-ppc/monitor.c
> +++ b/target-ppc/monitor.c
> @@ -76,176 +76,21 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
>      dump_mmu((FILE*)mon, (fprintf_function)monitor_printf, env1);
>  }
>  
> -
>  const MonitorDef monitor_defs[] = {
> -    /* General purpose registers */
> -    { "r0", offsetof(CPUPPCState, gpr[0]) },
> -    { "r1", offsetof(CPUPPCState, gpr[1]) },
> -    { "r2", offsetof(CPUPPCState, gpr[2]) },
> -    { "r3", offsetof(CPUPPCState, gpr[3]) },
> -    { "r4", offsetof(CPUPPCState, gpr[4]) },
> -    { "r5", offsetof(CPUPPCState, gpr[5]) },
> -    { "r6", offsetof(CPUPPCState, gpr[6]) },
> -    { "r7", offsetof(CPUPPCState, gpr[7]) },
> -    { "r8", offsetof(CPUPPCState, gpr[8]) },
> -    { "r9", offsetof(CPUPPCState, gpr[9]) },
> -    { "r10", offsetof(CPUPPCState, gpr[10]) },
> -    { "r11", offsetof(CPUPPCState, gpr[11]) },
> -    { "r12", offsetof(CPUPPCState, gpr[12]) },
> -    { "r13", offsetof(CPUPPCState, gpr[13]) },
> -    { "r14", offsetof(CPUPPCState, gpr[14]) },
> -    { "r15", offsetof(CPUPPCState, gpr[15]) },
> -    { "r16", offsetof(CPUPPCState, gpr[16]) },
> -    { "r17", offsetof(CPUPPCState, gpr[17]) },
> -    { "r18", offsetof(CPUPPCState, gpr[18]) },
> -    { "r19", offsetof(CPUPPCState, gpr[19]) },
> -    { "r20", offsetof(CPUPPCState, gpr[20]) },
> -    { "r21", offsetof(CPUPPCState, gpr[21]) },
> -    { "r22", offsetof(CPUPPCState, gpr[22]) },
> -    { "r23", offsetof(CPUPPCState, gpr[23]) },
> -    { "r24", offsetof(CPUPPCState, gpr[24]) },
> -    { "r25", offsetof(CPUPPCState, gpr[25]) },
> -    { "r26", offsetof(CPUPPCState, gpr[26]) },
> -    { "r27", offsetof(CPUPPCState, gpr[27]) },
> -    { "r28", offsetof(CPUPPCState, gpr[28]) },
> -    { "r29", offsetof(CPUPPCState, gpr[29]) },
> -    { "r30", offsetof(CPUPPCState, gpr[30]) },
> -    { "r31", offsetof(CPUPPCState, gpr[31]) },
> -    /* Floating point registers */
> -    { "f0", offsetof(CPUPPCState, fpr[0]) },
> -    { "f1", offsetof(CPUPPCState, fpr[1]) },
> -    { "f2", offsetof(CPUPPCState, fpr[2]) },
> -    { "f3", offsetof(CPUPPCState, fpr[3]) },
> -    { "f4", offsetof(CPUPPCState, fpr[4]) },
> -    { "f5", offsetof(CPUPPCState, fpr[5]) },
> -    { "f6", offsetof(CPUPPCState, fpr[6]) },
> -    { "f7", offsetof(CPUPPCState, fpr[7]) },
> -    { "f8", offsetof(CPUPPCState, fpr[8]) },
> -    { "f9", offsetof(CPUPPCState, fpr[9]) },
> -    { "f10", offsetof(CPUPPCState, fpr[10]) },
> -    { "f11", offsetof(CPUPPCState, fpr[11]) },
> -    { "f12", offsetof(CPUPPCState, fpr[12]) },
> -    { "f13", offsetof(CPUPPCState, fpr[13]) },
> -    { "f14", offsetof(CPUPPCState, fpr[14]) },
> -    { "f15", offsetof(CPUPPCState, fpr[15]) },
> -    { "f16", offsetof(CPUPPCState, fpr[16]) },
> -    { "f17", offsetof(CPUPPCState, fpr[17]) },
> -    { "f18", offsetof(CPUPPCState, fpr[18]) },
> -    { "f19", offsetof(CPUPPCState, fpr[19]) },
> -    { "f20", offsetof(CPUPPCState, fpr[20]) },
> -    { "f21", offsetof(CPUPPCState, fpr[21]) },
> -    { "f22", offsetof(CPUPPCState, fpr[22]) },
> -    { "f23", offsetof(CPUPPCState, fpr[23]) },
> -    { "f24", offsetof(CPUPPCState, fpr[24]) },
> -    { "f25", offsetof(CPUPPCState, fpr[25]) },
> -    { "f26", offsetof(CPUPPCState, fpr[26]) },
> -    { "f27", offsetof(CPUPPCState, fpr[27]) },
> -    { "f28", offsetof(CPUPPCState, fpr[28]) },
> -    { "f29", offsetof(CPUPPCState, fpr[29]) },
> -    { "f30", offsetof(CPUPPCState, fpr[30]) },
> -    { "f31", offsetof(CPUPPCState, fpr[31]) },
>      { "fpscr", offsetof(CPUPPCState, fpscr) },
>      /* Next instruction pointer */
>      { "nip|pc", offsetof(CPUPPCState, nip) },
>      { "lr", offsetof(CPUPPCState, lr) },
>      { "ctr", offsetof(CPUPPCState, ctr) },
> +    { "xer", offsetof(CPUPPCState, xer) },
> +    { "msr", offsetof(CPUPPCState, msr) },

Nit: "msr"  and "xer" are already listed below, so I think they're
listed twice after this patch.

>      { "decr", 0, &monitor_get_decr, },
> -    { "ccr", 0, &monitor_get_ccr, },
> +    { "ccr|cr", 0, &monitor_get_ccr, },
>      /* Machine state register */
>      { "msr", 0, &monitor_get_msr, },
>      { "xer", 0, &monitor_get_xer, },

..here.

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]