On Tue, 15 Jan 2019 17:37:48 -0200
Fabiano Rosas <faro...@linux.ibm.com> wrote:
> A following patch will add support for handling the Special Purpose
> Registers (SPR) in GDB via gdbstub. For that purpose, GDB needs to be
> provided with an XML description of the registers (see gdb-xml
> directory).
>
> This patch adds the code that generates the XML dynamically based on
> the SPRs already defined in the machine. This eliminates the need for
> several XML files to match each possible ppc machine.
>
> A "group" is defined so that the GDB command `info registers spr` can
> be used.
>
> Signed-off-by: Fabiano Rosas <faro...@linux.ibm.com>
> ---
> target/ppc/cpu.h | 8 +++++++
> target/ppc/gdbstub.c | 54 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 62 insertions(+)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 486abaf99b..34f0d2d419 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -230,6 +230,7 @@ struct ppc_spr_t {
> void (*oea_write)(DisasContext *ctx, int spr_num, int gpr_num);
> void (*hea_read)(DisasContext *ctx, int gpr_num, int spr_num);
> void (*hea_write)(DisasContext *ctx, int spr_num, int gpr_num);
> + unsigned int gdb_id;
> #endif
> const char *name;
> target_ulong default_value;
> @@ -1053,6 +1054,9 @@ struct CPUPPCState {
> /* Special purpose registers */
> target_ulong spr[1024];
> ppc_spr_t spr_cb[1024];
> +#if !defined(CONFIG_USER_ONLY)
> + const char *gdb_spr_xml;
> +#endif
> /* Vector status and control register */
> uint32_t vscr;
> /* VSX registers (including FP and AVR) */
> @@ -1267,6 +1271,10 @@ int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t
> *buf, int reg);
> int ppc_cpu_gdb_read_register_apple(CPUState *cpu, uint8_t *buf, int reg);
> int ppc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> int ppc_cpu_gdb_write_register_apple(CPUState *cpu, uint8_t *buf, int reg);
> +#ifndef CONFIG_USER_ONLY
> +int ppc_gdb_gen_spr_xml(CPUState *cpu);
> +const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
> +#endif
> int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> int cpuid, void *opaque);
> int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 19565b584d..ce4b728028 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -319,3 +319,57 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs,
> uint8_t *mem_buf, int n)
> }
> return r;
> }
> +
> +#ifndef CONFIG_USER_ONLY
> +int ppc_gdb_gen_spr_xml(CPUState *cs)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + CPUPPCState *env = &cpu->env;
> + GString *s = g_string_new(NULL);
> + unsigned int num_regs = 0;
> + int i;
> +
> + g_string_printf(s, "<?xml version=\"1.0\"?>");
> + g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
> + g_string_append_printf(s, "<feature name=\"org.qemu.power.spr\">");
> +
> + for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
> + ppc_spr_t *spr = &env->spr_cb[i];
> +
> + if (!spr->name) {
> + continue;
> + }
> +
> + g_string_append_printf(s, "<reg name=\"%s\"",
> + g_ascii_strdown(spr->name, -1));
> + g_string_append_printf(s, " bitsize=\"%d\"", TARGET_LONG_BITS);
> + g_string_append_printf(s, " group=\"spr\"/>");
> +
> + /*
> + * GDB identifies registers based on the order they are
> + * presented in the XML. These ids will not match QEMU's
> + * representation (which follows the PowerISA).
> + *
> + * Store the position of the current register description so
> + * we can make the correspondence later.
> + */
> + spr->gdb_id = num_regs;
> + num_regs++;
> + }
> +
> + g_string_append_printf(s, "</feature>");
> + env->gdb_spr_xml = g_string_free(s, false);
The g_string_free() documentation says that its up to the caller to g_free()
the character data in this case. If the CPU gets hot-unplugged at some point,
gdb_spr_xml is leaked since I see no g_free(env->gdb_spr_xml) in this patch.
This makes me think that all CPUs are supposed to have the same set of SPRs.
What about moving gdb_spr_xml to PowerPCCPUClass and have the first CPU
to set it once and far all ?
> + return num_regs;
> +}
> +
> +const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + CPUPPCState *env = &cpu->env;
> +
> + if (strcmp(xml_name, "power-spr.xml") == 0) {
> + return env->gdb_spr_xml;
> + }
> + return NULL;
> +}
> +#endif
