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

[Stuart Brady]

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 <address@hidden>
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
> 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?

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'.

Cheers,
Stuart