diff mbox series

[v2,18/23] target/arm: Move CPU state dumping routines to helper.c

Message ID 20190615154352.26824-19-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series Support disabling TCG on ARM | expand

Commit Message

Philippe Mathieu-Daudé June 15, 2019, 3:43 p.m. UTC
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 +++++++++++++++++++++++++++++++++++++
 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(-)

Comments

Alex Bennée June 17, 2019, 2:41 p.m. UTC | #1
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
Philippe Mathieu-Daudé June 17, 2019, 2:45 p.m. UTC | #2
On 6/17/19 4:41 PM, Alex Bennée wrote:
> 
> 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.

Good idea. What about target/arm/arch_dump.c?

>>  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(-)
Peter Maydell June 17, 2019, 2:52 p.m. UTC | #3
On Mon, 17 Jun 2019 at 15:41, Alex Bennée <alex.bennee@linaro.org> wrote:
> 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.

The original convention was:
 * op_helper.c has functions which need access to things which
   TCG puts in specific registers and which are accessed from
   C via gcc "register variables" (most notably the CPU env pointer)
 * helper.c has functions which don't need access to the register
   variables (a mishmash of stuff, usually including interrupt/exception
   handling)
 * some targets further split out some parts of the C code into other
   foo-helper.c files
This distinction became entirely moot when we reworked the TCG code
to no longer use register variables (but instead pass in the env
pointer and so on directly as a function argument). You can still
see the comments at the top of target/i386/{op_helper,helper}.c that
claim this is what the files are for, though :-)

So these days there is really no fixed convention, except that
when we've added new things we've sometimes put them in their
own .c file. helper-a64.c in particular is on its own because it's
code that's only compiled into aarch64-softmmu, not arm-softmmu.

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

Some sort of split like this seems like a good idea. I don't
know if any of the other target archs have already got a
good convention/naming scheme we could copy?

thanks
-- PMM
diff mbox series

Patch

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