Re: [Qemu-devel] [PATCH v2] target-ppc: improve "info registers" by printing SPRs

[Alexander Graf <agraf@suse.de>]

On 03/24/2014 07:24 AM, Alexey Kardashevskiy wrote:
> On 03/23/2014 01:43 AM, Stuart Brady wrote:
>> On Sat, Mar 22, 2014 at 11:25:49PM +1100, Alexey Kardashevskiy wrote:
>>> This adds printing of all SPR registers registered for a CPU.
>>>
>>> This removes "SPR_" prefix from SPR name to reduce the output.
>>>
>>> Cc: Fabien Chouteau <chouteau@adacore.com>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v2:
>>> * removed "switch (env->mmu_model)"
>>> * added "\n" if the last line has less than 4 registers
>>> ---
>>>   target-ppc/translate.c      | 96 +++++++--------------------------------------
>>>   target-ppc/translate_init.c | 40 +++++++++----------
>>>   2 files changed, 35 insertions(+), 101 deletions(-)
>>>
>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>> index e3fcb03..06f195a 100644
>>> --- a/target-ppc/translate.c
>>> +++ b/target-ppc/translate.c
>>> @@ -11116,7 +11116,7 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
>>>
>>>       PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>       CPUPPCState *env = &cpu->env;
>>> -    int i;
>>> +    int i, j;
>>>
>>>       cpu_fprintf(f, "NIP " TARGET_FMT_lx "   LR " TARGET_FMT_lx " CTR "
>>>                   TARGET_FMT_lx " XER " TARGET_FMT_lx "\n",
>>> @@ -11167,54 +11167,22 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
>>>               cpu_fprintf(f, "\n");
>>>       }
>>>       cpu_fprintf(f, "FPSCR " TARGET_FMT_lx "\n", env->fpscr);
>>> -#if !defined(CONFIG_USER_ONLY)
>>> -    cpu_fprintf(f, " SRR0 " TARGET_FMT_lx "  SRR1 " TARGET_FMT_lx
>>> -                   "    PVR " TARGET_FMT_lx " VRSAVE " TARGET_FMT_lx "\n",
>>> -                env->spr[SPR_SRR0], env->spr[SPR_SRR1],
>>> -                env->spr[SPR_PVR], env->spr[SPR_VRSAVE]);
>>>
>>> -    cpu_fprintf(f, "SPRG0 " TARGET_FMT_lx " SPRG1 " TARGET_FMT_lx
>>> -                   "  SPRG2 " TARGET_FMT_lx "  SPRG3 " TARGET_FMT_lx "\n",
>>> -                env->spr[SPR_SPRG0], env->spr[SPR_SPRG1],
>>> -                env->spr[SPR_SPRG2], env->spr[SPR_SPRG3]);
>>> -
>>> -    cpu_fprintf(f, "SPRG4 " TARGET_FMT_lx " SPRG5 " TARGET_FMT_lx
>>> -                   "  SPRG6 " TARGET_FMT_lx "  SPRG7 " TARGET_FMT_lx "\n",
>>> -                env->spr[SPR_SPRG4], env->spr[SPR_SPRG5],
>>> -                env->spr[SPR_SPRG6], env->spr[SPR_SPRG7]);
>>> -
>>> -    if (env->excp_model == POWERPC_EXCP_BOOKE) {
>>> -        cpu_fprintf(f, "CSRR0 " TARGET_FMT_lx " CSRR1 " TARGET_FMT_lx
>>> -                       " MCSRR0 " TARGET_FMT_lx " MCSRR1 " TARGET_FMT_lx "\n",
>>> -                    env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1],
>>> -                    env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
>>> -
>>> -        cpu_fprintf(f, "  TCR " TARGET_FMT_lx "   TSR " TARGET_FMT_lx
>>> -                       "    ESR " TARGET_FMT_lx "   DEAR " TARGET_FMT_lx "\n",
>>> -                    env->spr[SPR_BOOKE_TCR], env->spr[SPR_BOOKE_TSR],
>>> -                    env->spr[SPR_BOOKE_ESR], env->spr[SPR_BOOKE_DEAR]);
>>> -
>>> -        cpu_fprintf(f, "  PIR " TARGET_FMT_lx " DECAR " TARGET_FMT_lx
>>> -                       "   IVPR " TARGET_FMT_lx "   EPCR " TARGET_FMT_lx "\n",
>>> -                    env->spr[SPR_BOOKE_PIR], env->spr[SPR_BOOKE_DECAR],
>>> -                    env->spr[SPR_BOOKE_IVPR], env->spr[SPR_BOOKE_EPCR]);
>>> -
>>> -        cpu_fprintf(f, " MCSR " TARGET_FMT_lx " SPRG8 " TARGET_FMT_lx
>>> -                       "    EPR " TARGET_FMT_lx "\n",
>>> -                    env->spr[SPR_BOOKE_MCSR], env->spr[SPR_BOOKE_SPRG8],
>>> -                    env->spr[SPR_BOOKE_EPR]);
>>> -
>>> -        /* FSL-specific */
>>> -        cpu_fprintf(f, " MCAR " TARGET_FMT_lx "  PID1 " TARGET_FMT_lx
>>> -                       "   PID2 " TARGET_FMT_lx "    SVR " TARGET_FMT_lx "\n",
>>> -                    env->spr[SPR_Exxx_MCAR], env->spr[SPR_BOOKE_PID1],
>>> -                    env->spr[SPR_BOOKE_PID2], env->spr[SPR_E500_SVR]);
>>> -
>>> -        /*
>>> -         * IVORs are left out as they are large and do not change often --
>>> -         * they can be read with "p $ivor0", "p $ivor1", etc.
>>> -         */
>>> +    for (i = 0, j = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
>>> +        ppc_spr_t *spr = &env->spr_cb[i];
>>> +
>>> +        if (!spr->name) {
>>> +            continue;
>>> +        }
>> This would leave the output without a trailing newline if the last spr
>> doesn't have a name registered.  Is it necessary to handle unnamed sprs
>> at all (maybe add an assert to the registration function)? ... or would
>> we just want to warn about them here?
>
> In the current QEMU I do not see any place where SPR would be registered
> without a name so I would not bother.
>
>
>> FWIW, my approach is often to write an outer loop that process one item
>> of output at a time, with an inner loop to obtain the next item of data,
>> and with prefixing of separators, as you then have a far simpler special
>> case for 'j == 0' instead of 'i == ARRAY_SIZE(env->spr_cb) - 1'.  You can
>> then unconditionally finish on a '\n'.
> Oh. This is embarrassing, sorry :( This should do it, right? :)
>
> +    for (i = 0, j = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
> +        ppc_spr_t *spr = &env->spr_cb[i];
> +
> +        if (!spr->name) {
> +            continue;
> +        }
> +        if (j % 4) {
> +            cpu_fprintf(f, " ");
> +        } else if (j) {
> +            cpu_fprintf(f, "\n");
> +        }
> +        cpu_fprintf(f, "%-6s " TARGET_FMT_lx, spr->name, env->spr[i]);
> +        j++;
>       }
> +    cpu_fprintf(f, "\n");
>
>
>
> btw while grepping through the code, I found dump_ppc_sprs() which prints
> this (first chunk is what my patch adds and the second chunk is from
> dump_ppc_sprs()):
>
>
> XER    0000000000000000 LR     0000000000000000 CTR    0000000000000000

These are already listed, no?

> UAMR   0000000000000000
> DSCR   0000000000000000 DSISR  0000000000000000 DAR    0000000000000000
> DECR   0000000000000000
> SDR1   0000000000000005 SRR0   0000000000000000 SRR1   0000000000000000
> CFAR   0000000000000000
> AMR    0000000000000000 CTRLF  0000000080800000 CTRLT  0000000080800000
> UAMOR  0000000000000000
> VRSAVE 0000000000000000 TBL    0000000000000000 TBU    0000000000000000
> SPRG0  0000000000000000
> SPRG1  0000000000000000 SPRG2  0000000000000000 SPRG3  0000000000000000 EAR
>     0000000000000000
> TBL    0000000000000000 TBU    0000000000000000 PVR    00000000003f0201

TB is also explicitly printed.

> SPURR  0000000000000000
> PURR   0000000000000000 LPCR   0000000000007005 MMCRA  0000000000000000 PPR
>     0000000000000000
> UMMCR0 0000000000000000 UPMC1  0000000000000000 UPMC2  0000000000000000
> USIAR  0000000000000000
> UMMCR1 0000000000000000 UPMC3  0000000000000000 UPMC4  0000000000000000
> PMC5   0000000000000000
> PMC6   0000000000000000 MMCR0  0000000000000000 PMC1   0000000000000000
> PMC2   0000000000000000
> SIAR   0000000000000000 MMCR1  0000000000000000 PMC3   0000000000000000
> PMC4   0000000000000000
> IABR   0000000000000000 DABR   0000000000000000 ICTC   0000000000000000 PIR
>     0000000000000000

My main pain point with a cluttered SPR list in "info registers" is that 
it's also used for -d cpu. If we bloat it by too much, it becomes less 
useful that I'd like it to be.

Could we maybe add a flag to the SPR register function to indicate 
special treatment? We already have a _kvm version - how about we convert 
that one to a flag bitmap that can be SYNC_KVM and/or DUMP_SPR?



Alex

>
>
>
>
>
> Special purpose registers:
> SPR:    1 (001) XER      swr uwr
> SPR:    8 (008) LR       swr uwr
> SPR:    9 (009) CTR      swr uwr
> SPR:   12 (00c) UAMR     swr uwr
> SPR:   17 (011) DSCR     swr u--
> SPR:   18 (012) DSISR    swr u--
> SPR:   19 (013) DAR      swr u--
> SPR:   22 (016) DECR     swr u--
> SPR:   25 (019) SDR1     swr u--
> SPR:   26 (01a) SRR0     swr u--
> SPR:   27 (01b) SRR1     swr u--
> SPR:   28 (01c) CFAR     swr u--
> SPR:   29 (01d) AMR      swr u--
> SPR:  136 (088) CTRLF    s-r u--
> SPR:  152 (098) CTRLT    sw- u--
> SPR:  157 (09d) UAMOR    swr u--
> SPR:  256 (100) VRSAVE   swr uwr
> SPR:  268 (10c) TBL      s-r u-r
> SPR:  269 (10d) TBU      s-r u-r
> SPR:  272 (110) SPRG0    swr u--
> SPR:  273 (111) SPRG1    swr u--
> SPR:  274 (112) SPRG2    swr u--
> SPR:  275 (113) SPRG3    swr u--
> SPR:  282 (11a) EAR      swr u--
> SPR:  284 (11c) TBL      swr u-r
> SPR:  285 (11d) TBU      swr u-r
> SPR:  287 (11f) PVR      s-r u--
> SPR:  308 (134) SPURR    s-r u-r
> SPR:  309 (135) PURR     s-r u-r
> SPR:  318 (13e) LPCR     swr u--
> SPR:  770 (302) MMCRA    swr u--
> SPR:  896 (380) PPR      swr uwr
> SPR:  936 (3a8) UMMCR0   s-r u-r
> SPR:  937 (3a9) UPMC1    s-r u-r
> SPR:  938 (3aa) UPMC2    s-r u-r
> SPR:  939 (3ab) USIAR    s-r u-r
> SPR:  940 (3ac) UMMCR1   s-r u-r
> SPR:  941 (3ad) UPMC3    s-r u-r
> SPR:  942 (3ae) UPMC4    s-r u-r
> SPR:  945 (3b1) PMC5     swr u--
> SPR:  946 (3b2) PMC6     swr u--
> SPR:  952 (3b8) MMCR0    swr u--
> SPR:  953 (3b9) PMC1     swr u--
> SPR:  954 (3ba) PMC2     swr u--
> SPR:  955 (3bb) SIAR     s-r u--
> SPR:  956 (3bc) MMCR1    swr u--
> SPR:  957 (3bd) PMC3     swr u--
> SPR:  958 (3be) PMC4     swr u--
> SPR: 1010 (3f2) IABR     swr u--
> SPR: 1013 (3f5) DABR     swr u--
> SPR: 1019 (3fb) ICTC     swr u--
> SPR: 1023 (3ff) PIR      swr u--
>
>
> Which is nicer/more useful?

The list down here really is for debugging the configuration of 
available SPRs. I think it is useful, but it covers vastly different 
information and definitely does not belong to runtime CPU state - this 
is CPU configuration state (more along the lines of qom properties).


Alex