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 <farosas@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);
+    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
-- 
2.17.1

On Tue, 15 Jan 2019 17:37:48 -0200
Fabiano Rosas <farosas@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 <farosas@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

Greg Kurz <groug@kaod.org> writes:

> On Tue, 15 Jan 2019 17:37:48 -0200
> Fabiano Rosas <farosas@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 <farosas@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 ?

Good idea. I'll do that.

Thank you!

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