Re: [Qemu-devel] [Qemu-arm] [PATCH v2 18/23] target/arm: Move CPU state dumping routines to helper.c

["Alex Bennée" <alex.bennee@linaro.org>]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> From: Samuel Ortiz <sameo@linux.intel.com>
>
> They're not TCG specific and should be living the generic helper file
> instead.
>
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> Reviewed-by: Robert Bradford <robert.bradford@intel.com>
> [PMD: Rebased]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  target/arm/helper.c        | 214
> +++++++++++++++++++++++++++++++++++++

Hmm so helper is a mix of non-TCG and TCG bits whereas helper-a64.c is
basically just TCG helpers. It makes me wonder if we are breaking a
convention here as helper.c is traditionally only TCG helpers.

It feels like there should be different file that is unambiguously used
for both TCG and KVM based workloads where things like the cpu dump code
can live.

>  target/arm/internals.h     |   8 ++
>  target/arm/translate-a64.c | 127 ----------------------
>  target/arm/translate.c     |  87 ---------------
>  target/arm/translate.h     |   5 -
>  5 files changed, 222 insertions(+), 219 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a4af02c984..8c32b2bc0d 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11293,4 +11293,218 @@ void aarch64_sve_change_el(CPUARMState *env, int old_el,
>          aarch64_sve_narrow_vq(env, new_len + 1);
>      }
>  }
> +
> +void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    uint32_t psr = pstate_read(env);
> +    int i;
> +    int el = arm_current_el(env);
> +    const char *ns_status;
> +
> +    qemu_fprintf(f, " PC=%016" PRIx64 " ", env->pc);
> +    for (i = 0; i < 32; i++) {
> +        if (i == 31) {
> +            qemu_fprintf(f, " SP=%016" PRIx64 "\n", env->xregs[i]);
> +        } else {
> +            qemu_fprintf(f, "X%02d=%016" PRIx64 "%s", i, env->xregs[i],
> +                         (i + 2) % 3 ? " " : "\n");
> +        }
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_EL3) && el != 3) {
> +        ns_status = env->cp15.scr_el3 & SCR_NS ? "NS " : "S ";
> +    } else {
> +        ns_status = "";
> +    }
> +    qemu_fprintf(f, "PSTATE=%08x %c%c%c%c %sEL%d%c",
> +                 psr,
> +                 psr & PSTATE_N ? 'N' : '-',
> +                 psr & PSTATE_Z ? 'Z' : '-',
> +                 psr & PSTATE_C ? 'C' : '-',
> +                 psr & PSTATE_V ? 'V' : '-',
> +                 ns_status,
> +                 el,
> +                 psr & PSTATE_SP ? 'h' : 't');
> +
> +    if (cpu_isar_feature(aa64_bti, cpu)) {
> +        qemu_fprintf(f, "  BTYPE=%d", (psr & PSTATE_BTYPE) >> 10);
> +    }
> +    if (!(flags & CPU_DUMP_FPU)) {
> +        qemu_fprintf(f, "\n");
> +        return;
> +    }
> +    if (fp_exception_el(env, el) != 0) {
> +        qemu_fprintf(f, "    FPU disabled\n");
> +        return;
> +    }
> +    qemu_fprintf(f, "     FPCR=%08x FPSR=%08x\n",
> +                 vfp_get_fpcr(env), vfp_get_fpsr(env));
> +
> +    if (cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {
> +        int j, zcr_len = sve_zcr_len_for_el(env, el);
> +
> +        for (i = 0; i <= FFR_PRED_NUM; i++) {
> +            bool eol;
> +            if (i == FFR_PRED_NUM) {
> +                qemu_fprintf(f, "FFR=");
> +                /* It's last, so end the line.  */
> +                eol = true;
> +            } else {
> +                qemu_fprintf(f, "P%02d=", i);
> +                switch (zcr_len) {
> +                case 0:
> +                    eol = i % 8 == 7;
> +                    break;
> +                case 1:
> +                    eol = i % 6 == 5;
> +                    break;
> +                case 2:
> +                case 3:
> +                    eol = i % 3 == 2;
> +                    break;
> +                default:
> +                    /* More than one quadword per predicate.  */
> +                    eol = true;
> +                    break;
> +                }
> +            }
> +            for (j = zcr_len / 4; j >= 0; j--) {
> +                int digits;
> +                if (j * 4 + 4 <= zcr_len + 1) {
> +                    digits = 16;
> +                } else {
> +                    digits = (zcr_len % 4 + 1) * 4;
> +                }
> +                qemu_fprintf(f, "%0*" PRIx64 "%s", digits,
> +                             env->vfp.pregs[i].p[j],
> +                             j ? ":" : eol ? "\n" : " ");
> +            }
> +        }
> +
> +        for (i = 0; i < 32; i++) {
> +            if (zcr_len == 0) {
> +                qemu_fprintf(f, "Z%02d=%016" PRIx64 ":%016" PRIx64 "%s",
> +                             i, env->vfp.zregs[i].d[1],
> +                             env->vfp.zregs[i].d[0], i & 1 ? "\n" : " ");
> +            } else if (zcr_len == 1) {
> +                qemu_fprintf(f, "Z%02d=%016" PRIx64 ":%016" PRIx64
> +                             ":%016" PRIx64 ":%016" PRIx64 "\n",
> +                             i, env->vfp.zregs[i].d[3], env->vfp.zregs[i].d[2],
> +                             env->vfp.zregs[i].d[1], env->vfp.zregs[i].d[0]);
> +            } else {
> +                for (j = zcr_len; j >= 0; j--) {
> +                    bool odd = (zcr_len - j) % 2 != 0;
> +                    if (j == zcr_len) {
> +                        qemu_fprintf(f, "Z%02d[%x-%x]=", i, j, j - 1);
> +                    } else if (!odd) {
> +                        if (j > 0) {
> +                            qemu_fprintf(f, "   [%x-%x]=", j, j - 1);
> +                        } else {
> +                            qemu_fprintf(f, "     [%x]=", j);
> +                        }
> +                    }
> +                    qemu_fprintf(f, "%016" PRIx64 ":%016" PRIx64 "%s",
> +                                 env->vfp.zregs[i].d[j * 2 + 1],
> +                                 env->vfp.zregs[i].d[j * 2],
> +                                 odd || j == 0 ? "\n" : ":");
> +                }
> +            }
> +        }
> +    } else {
> +        for (i = 0; i < 32; i++) {
> +            uint64_t *q = aa64_vfp_qreg(env, i);
> +            qemu_fprintf(f, "Q%02d=%016" PRIx64 ":%016" PRIx64 "%s",
> +                         i, q[1], q[0], (i & 1 ? "\n" : " "));
> +        }
> +    }
> +}
>  #endif
> +
> +void arm_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    int i;
> +
> +    if (is_a64(env)) {
> +        aarch64_cpu_dump_state(cs, f, flags);
> +        return;
> +    }
> +
> +    for (i = 0; i < 16; i++) {
> +        qemu_fprintf(f, "R%02d=%08x", i, env->regs[i]);
> +        if ((i % 4) == 3) {
> +            qemu_fprintf(f, "\n");
> +        } else {
> +            qemu_fprintf(f, " ");
> +        }
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_M)) {
> +        uint32_t xpsr = xpsr_read(env);
> +        const char *mode;
> +        const char *ns_status = "";
> +
> +        if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> +            ns_status = env->v7m.secure ? "S " : "NS ";
> +        }
> +
> +        if (xpsr & XPSR_EXCP) {
> +            mode = "handler";
> +        } else {
> +            if (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_NPRIV_MASK) {
> +                mode = "unpriv-thread";
> +            } else {
> +                mode = "priv-thread";
> +            }
> +        }
> +
> +        qemu_fprintf(f, "XPSR=%08x %c%c%c%c %c %s%s\n",
> +                     xpsr,
> +                     xpsr & XPSR_N ? 'N' : '-',
> +                     xpsr & XPSR_Z ? 'Z' : '-',
> +                     xpsr & XPSR_C ? 'C' : '-',
> +                     xpsr & XPSR_V ? 'V' : '-',
> +                     xpsr & XPSR_T ? 'T' : 'A',
> +                     ns_status,
> +                     mode);
> +    } else {
> +        uint32_t psr = cpsr_read(env);
> +        const char *ns_status = "";
> +
> +        if (arm_feature(env, ARM_FEATURE_EL3) &&
> +            (psr & CPSR_M) != ARM_CPU_MODE_MON) {
> +            ns_status = env->cp15.scr_el3 & SCR_NS ? "NS " : "S ";
> +        }
> +
> +        qemu_fprintf(f, "PSR=%08x %c%c%c%c %c %s%s%d\n",
> +                     psr,
> +                     psr & CPSR_N ? 'N' : '-',
> +                     psr & CPSR_Z ? 'Z' : '-',
> +                     psr & CPSR_C ? 'C' : '-',
> +                     psr & CPSR_V ? 'V' : '-',
> +                     psr & CPSR_T ? 'T' : 'A',
> +                     ns_status,
> +                     aarch32_mode_name(psr), (psr & 0x10) ? 32 : 26);
> +    }
> +
> +    if (flags & CPU_DUMP_FPU) {
> +        int numvfpregs = 0;
> +        if (arm_feature(env, ARM_FEATURE_VFP)) {
> +            numvfpregs += 16;
> +        }
> +        if (arm_feature(env, ARM_FEATURE_VFP3)) {
> +            numvfpregs += 16;
> +        }
> +        for (i = 0; i < numvfpregs; i++) {
> +            uint64_t v = *aa32_vfp_dreg(env, i);
> +            qemu_fprintf(f, "s%02d=%08x s%02d=%08x d%02d=%016" PRIx64 "\n",
> +                         i * 2, (uint32_t)v,
> +                         i * 2 + 1, (uint32_t)(v >> 32),
> +                         i, v);
> +        }
> +        qemu_fprintf(f, "FPSCR: %08x\n", vfp_get_fpscr(env));
> +    }
> +}
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 06e676bf62..56281d8ece 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1042,4 +1042,12 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
>                         int *prot, bool *is_subpage,
>                         ARMMMUFaultInfo *fi, uint32_t *mregion);
>
> +#ifdef TARGET_AARCH64
> +void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags);
> +#else
> +static inline void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> +{
> +}
> +#endif
> +
>  #endif
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index ae739f6575..8abe1f0e4f 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -152,133 +152,6 @@ static void set_btype(DisasContext *s, int val)
>      s->btype = -1;
>  }
>
> -void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> -{
> -    ARMCPU *cpu = ARM_CPU(cs);
> -    CPUARMState *env = &cpu->env;
> -    uint32_t psr = pstate_read(env);
> -    int i;
> -    int el = arm_current_el(env);
> -    const char *ns_status;
> -
> -    qemu_fprintf(f, " PC=%016" PRIx64 " ", env->pc);
> -    for (i = 0; i < 32; i++) {
> -        if (i == 31) {
> -            qemu_fprintf(f, " SP=%016" PRIx64 "\n", env->xregs[i]);
> -        } else {
> -            qemu_fprintf(f, "X%02d=%016" PRIx64 "%s", i, env->xregs[i],
> -                         (i + 2) % 3 ? " " : "\n");
> -        }
> -    }
> -
> -    if (arm_feature(env, ARM_FEATURE_EL3) && el != 3) {
> -        ns_status = env->cp15.scr_el3 & SCR_NS ? "NS " : "S ";
> -    } else {
> -        ns_status = "";
> -    }
> -    qemu_fprintf(f, "PSTATE=%08x %c%c%c%c %sEL%d%c",
> -                 psr,
> -                 psr & PSTATE_N ? 'N' : '-',
> -                 psr & PSTATE_Z ? 'Z' : '-',
> -                 psr & PSTATE_C ? 'C' : '-',
> -                 psr & PSTATE_V ? 'V' : '-',
> -                 ns_status,
> -                 el,
> -                 psr & PSTATE_SP ? 'h' : 't');
> -
> -    if (cpu_isar_feature(aa64_bti, cpu)) {
> -        qemu_fprintf(f, "  BTYPE=%d", (psr & PSTATE_BTYPE) >> 10);
> -    }
> -    if (!(flags & CPU_DUMP_FPU)) {
> -        qemu_fprintf(f, "\n");
> -        return;
> -    }
> -    if (fp_exception_el(env, el) != 0) {
> -        qemu_fprintf(f, "    FPU disabled\n");
> -        return;
> -    }
> -    qemu_fprintf(f, "     FPCR=%08x FPSR=%08x\n",
> -                 vfp_get_fpcr(env), vfp_get_fpsr(env));
> -
> -    if (cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {
> -        int j, zcr_len = sve_zcr_len_for_el(env, el);
> -
> -        for (i = 0; i <= FFR_PRED_NUM; i++) {
> -            bool eol;
> -            if (i == FFR_PRED_NUM) {
> -                qemu_fprintf(f, "FFR=");
> -                /* It's last, so end the line.  */
> -                eol = true;
> -            } else {
> -                qemu_fprintf(f, "P%02d=", i);
> -                switch (zcr_len) {
> -                case 0:
> -                    eol = i % 8 == 7;
> -                    break;
> -                case 1:
> -                    eol = i % 6 == 5;
> -                    break;
> -                case 2:
> -                case 3:
> -                    eol = i % 3 == 2;
> -                    break;
> -                default:
> -                    /* More than one quadword per predicate.  */
> -                    eol = true;
> -                    break;
> -                }
> -            }
> -            for (j = zcr_len / 4; j >= 0; j--) {
> -                int digits;
> -                if (j * 4 + 4 <= zcr_len + 1) {
> -                    digits = 16;
> -                } else {
> -                    digits = (zcr_len % 4 + 1) * 4;
> -                }
> -                qemu_fprintf(f, "%0*" PRIx64 "%s", digits,
> -                             env->vfp.pregs[i].p[j],
> -                             j ? ":" : eol ? "\n" : " ");
> -            }
> -        }
> -
> -        for (i = 0; i < 32; i++) {
> -            if (zcr_len == 0) {
> -                qemu_fprintf(f, "Z%02d=%016" PRIx64 ":%016" PRIx64 "%s",
> -                             i, env->vfp.zregs[i].d[1],
> -                             env->vfp.zregs[i].d[0], i & 1 ? "\n" : " ");
> -            } else if (zcr_len == 1) {
> -                qemu_fprintf(f, "Z%02d=%016" PRIx64 ":%016" PRIx64
> -                             ":%016" PRIx64 ":%016" PRIx64 "\n",
> -                             i, env->vfp.zregs[i].d[3], env->vfp.zregs[i].d[2],
> -                             env->vfp.zregs[i].d[1], env->vfp.zregs[i].d[0]);
> -            } else {
> -                for (j = zcr_len; j >= 0; j--) {
> -                    bool odd = (zcr_len - j) % 2 != 0;
> -                    if (j == zcr_len) {
> -                        qemu_fprintf(f, "Z%02d[%x-%x]=", i, j, j - 1);
> -                    } else if (!odd) {
> -                        if (j > 0) {
> -                            qemu_fprintf(f, "   [%x-%x]=", j, j - 1);
> -                        } else {
> -                            qemu_fprintf(f, "     [%x]=", j);
> -                        }
> -                    }
> -                    qemu_fprintf(f, "%016" PRIx64 ":%016" PRIx64 "%s",
> -                                 env->vfp.zregs[i].d[j * 2 + 1],
> -                                 env->vfp.zregs[i].d[j * 2],
> -                                 odd || j == 0 ? "\n" : ":");
> -                }
> -            }
> -        }
> -    } else {
> -        for (i = 0; i < 32; i++) {
> -            uint64_t *q = aa64_vfp_qreg(env, i);
> -            qemu_fprintf(f, "Q%02d=%016" PRIx64 ":%016" PRIx64 "%s",
> -                         i, q[1], q[0], (i & 1 ? "\n" : " "));
> -        }
> -    }
> -}
> -
>  void gen_a64_set_pc_im(uint64_t val)
>  {
>      tcg_gen_movi_i64(cpu_pc, val);
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index d0ab3e27e6..1e50627690 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -12416,93 +12416,6 @@ void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb, int max_insns)
>      translator_loop(ops, &dc.base, cpu, tb, max_insns);
>  }
>
> -void arm_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> -{
> -    ARMCPU *cpu = ARM_CPU(cs);
> -    CPUARMState *env = &cpu->env;
> -    int i;
> -
> -    if (is_a64(env)) {
> -        aarch64_cpu_dump_state(cs, f, flags);
> -        return;
> -    }
> -
> -    for (i = 0; i < 16; i++) {
> -        qemu_fprintf(f, "R%02d=%08x", i, env->regs[i]);
> -        if ((i % 4) == 3) {
> -            qemu_fprintf(f, "\n");
> -        } else {
> -            qemu_fprintf(f, " ");
> -        }
> -    }
> -
> -    if (arm_feature(env, ARM_FEATURE_M)) {
> -        uint32_t xpsr = xpsr_read(env);
> -        const char *mode;
> -        const char *ns_status = "";
> -
> -        if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> -            ns_status = env->v7m.secure ? "S " : "NS ";
> -        }
> -
> -        if (xpsr & XPSR_EXCP) {
> -            mode = "handler";
> -        } else {
> -            if (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_NPRIV_MASK) {
> -                mode = "unpriv-thread";
> -            } else {
> -                mode = "priv-thread";
> -            }
> -        }
> -
> -        qemu_fprintf(f, "XPSR=%08x %c%c%c%c %c %s%s\n",
> -                     xpsr,
> -                     xpsr & XPSR_N ? 'N' : '-',
> -                     xpsr & XPSR_Z ? 'Z' : '-',
> -                     xpsr & XPSR_C ? 'C' : '-',
> -                     xpsr & XPSR_V ? 'V' : '-',
> -                     xpsr & XPSR_T ? 'T' : 'A',
> -                     ns_status,
> -                     mode);
> -    } else {
> -        uint32_t psr = cpsr_read(env);
> -        const char *ns_status = "";
> -
> -        if (arm_feature(env, ARM_FEATURE_EL3) &&
> -            (psr & CPSR_M) != ARM_CPU_MODE_MON) {
> -            ns_status = env->cp15.scr_el3 & SCR_NS ? "NS " : "S ";
> -        }
> -
> -        qemu_fprintf(f, "PSR=%08x %c%c%c%c %c %s%s%d\n",
> -                     psr,
> -                     psr & CPSR_N ? 'N' : '-',
> -                     psr & CPSR_Z ? 'Z' : '-',
> -                     psr & CPSR_C ? 'C' : '-',
> -                     psr & CPSR_V ? 'V' : '-',
> -                     psr & CPSR_T ? 'T' : 'A',
> -                     ns_status,
> -                     aarch32_mode_name(psr), (psr & 0x10) ? 32 : 26);
> -    }
> -
> -    if (flags & CPU_DUMP_FPU) {
> -        int numvfpregs = 0;
> -        if (arm_feature(env, ARM_FEATURE_VFP)) {
> -            numvfpregs += 16;
> -        }
> -        if (arm_feature(env, ARM_FEATURE_VFP3)) {
> -            numvfpregs += 16;
> -        }
> -        for (i = 0; i < numvfpregs; i++) {
> -            uint64_t v = *aa32_vfp_dreg(env, i);
> -            qemu_fprintf(f, "s%02d=%08x s%02d=%08x d%02d=%016" PRIx64 "\n",
> -                         i * 2, (uint32_t)v,
> -                         i * 2 + 1, (uint32_t)(v >> 32),
> -                         i, v);
> -        }
> -        qemu_fprintf(f, "FPSCR: %08x\n", vfp_get_fpscr(env));
> -    }
> -}
> -
>  void restore_state_to_opc(CPUARMState *env, TranslationBlock *tb,
>                            target_ulong *data)
>  {
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index dc06dce767..1dd3ac0a41 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -169,7 +169,6 @@ static inline void disas_set_insn_syndrome(DisasContext *s, uint32_t syn)
>  #ifdef TARGET_AARCH64
>  void a64_translate_init(void);
>  void gen_a64_set_pc_im(uint64_t val);
> -void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags);
>  extern const TranslatorOps aarch64_translator_ops;
>  #else
>  static inline void a64_translate_init(void)
> @@ -179,10 +178,6 @@ static inline void a64_translate_init(void)
>  static inline void gen_a64_set_pc_im(uint64_t val)
>  {
>  }
> -
> -static inline void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> -{
> -}
>  #endif
>
>  void arm_test_cc(DisasCompare *cmp, int cc);


--
Alex Bennée