Re: [PATCH] target/ppc: code motion from translate_init.c.inc to gdbstub.c

[Fabiano Rosas <farosas@linux.ibm.com>]

"Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:

Please send ppc patches to both qemu-devel and qemu-ppc.

> As suggested by Fabiano Rosas,

In these situations you can just add along with your signed-off-by:

Suggested-by: Fabiano Rosas <farosas@linux.ibm.com>

> all the code related to gdb has been moved
> from translate_init.c.inc file to the gdbstub.c file, where it makes more
> sense
>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  target/ppc/cpu.h                |  11 ++
>  target/ppc/gdbstub.c            | 261 ++++++++++++++++++++++++++++++++
>  target/ppc/translate_init.c.inc | 224 ---------------------------
>  3 files changed, 272 insertions(+), 224 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e73416da68..795b121e04 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2612,4 +2612,15 @@ static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
>  void dump_mmu(CPUPPCState *env);
>  
>  void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
> +
> +/*
> + * functions used by cpu_ppc_realize, but that dont necessarily make sense
> + * to be added to cpu.c, because they seem very related to TCG or gdb
> + */

This comment has served its purpose for the RFC I think. You can drop it.

> +
> +/* gdbstub.c */
> +void ppc_cpu_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);

I don't think "cpu" adds much here. ppc_gdb_init gets the meaning across
just fine.

> +gchar *ppc_gdb_arch_name(CPUState *cs);
> +
> +
>  #endif /* PPC_CPU_H */
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index c28319fb97..0c016b8483 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -20,6 +20,10 @@
>  #include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "exec/gdbstub.h"
> +#ifdef CONFIG_TCG
> +#include "exec/helper-proto.h"
> +#endif

We still need to figure out where to move the vscr helpers so that both
TCG and !TCG code can see them. But we cannot build without TCG
currently anyway so I guess it's ok to leave the ifdef.

> +#include "kvm_ppc.h"

This one is not being used.

>  
>  static int ppc_gdb_register_len_apple(int n)
>  {
> @@ -387,3 +391,260 @@ const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
>      return NULL;
>  }
>  #endif

<snip>

> +gchar *ppc_gdb_arch_name(CPUState *cs)
> +{
> +#if defined(TARGET_PPC64)
> +    return g_strdup("powerpc:common64");
> +#else
> +    return g_strdup("powerpc:common");
> +#endif
> +}

Where is the removal of this from translate_init.inc.c? You must have
left it unstaged in your tree.

> +
> +void ppc_cpu_gdb_init(CPUState *cs, PowerPCCPUClass *pcc)
> +{
> +
> +    if (pcc->insns_flags & PPC_FLOAT) {
> +        gdb_register_coprocessor(cs, gdb_get_float_reg, gdb_set_float_reg,
> +                                 33, "power-fpu.xml", 0);
> +    }
> +    if (pcc->insns_flags & PPC_ALTIVEC) {
> +        gdb_register_coprocessor(cs, gdb_get_avr_reg, gdb_set_avr_reg,
> +                                 34, "power-altivec.xml", 0);
> +    }
> +    if (pcc->insns_flags & PPC_SPE) {
> +        gdb_register_coprocessor(cs, gdb_get_spe_reg, gdb_set_spe_reg,
> +                                 34, "power-spe.xml", 0);
> +    }
> +    if (pcc->insns_flags2 & PPC2_VSX) {
> +        gdb_register_coprocessor(cs, gdb_get_vsx_reg, gdb_set_vsx_reg,
> +                                 32, "power-vsx.xml", 0);
> +    }
> +#ifndef CONFIG_USER_ONLY
> +    gdb_register_coprocessor(cs, gdb_get_spr_reg, gdb_set_spr_reg,
> +                             pcc->gdb_num_sprs, "power-spr.xml", 0);
> +#endif
> +}

Same here.