[Qemu-devel] [PATCH v5 0/1] ppc/gdbstub: Expose SPRs to GDB

[Qemu-devel] [PATCH v5 0/1] ppc/gdbstub: Expose SPRs to GDB

[Fabiano Rosas]

This series implements the reading and writing of Special Purpose
Registers in PPC's gdbstub.

How it works generally [1]:

GDB asks for the target.xml file which contains the target description
along with the list of available feature XMLs. GDB then asks for each
of the XML files in sequence.

The XML files contain a list of registers descriptions:

  <reg name="msr" bitsize="64" type="uint64"/>

When the user tries to access a register, GDB reads the XML file in
sequence and sends QEMU the number of the register. This number is
sequential across all feature files.

The index provided by GDB must be converted by QEMU to match QEMU's
internal representation.

A set of callbacks are implemented to read/write the register.

Note about endianness:

GDB remote protocol defines that registers should be sent in target
(guest) byte order. It will store the value in memory without regard
to endianness and later use what it thinks is the target endianness to
choose a byte order for printing. This means that the `show endian`
option in GDB should match the guest's current endianness for the
values to display properly. Notice that GDB and QEMU could be running
in machines of opposite endianness.

https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Descriptions.html

v1 -> v2:
 - patch 1: explicitly store the gdb_id and add comment explaining why
            we need it
 - patch 2: use gdb_id to find the correct env->spr array's index

 https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00648.html

v2 -> v3:
 - patch 2: move gdb_register_coprocessor call from patch 3 so that
            all patches build independently

 https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02939.html

v3 -> v4:
 - patch 1: move gdb_spr_xml into PowerPCCPUClass so that it is
            generated only once for all CPUs

 http://lists.nongnu.org/archive/html/qemu-ppc/2019-01/msg00357.html

v4 -> v5:
 - squash all 3 patches
 - use g_string_append() instead of g_string_append_printf() where
 possible
 - free the g_ascii_strdown() string
 - fix uninitialized variable
 - use POWERPC_CPU_GET_CLASS with CPUState

 Regarding the endianness discussion, continue providing the registers
 in the target endianness as GBD currently requires this.

 http://lists.nongnu.org/archive/html/qemu-ppc/2019-01/msg00498.html


Fabiano Rosas (1):
  target/ppc: Enable reporting of SPRs to GDB

 target/ppc/cpu-qom.h            |  4 +++
 target/ppc/cpu.h                |  5 +++
 target/ppc/gdbstub.c            | 61 ++++++++++++++++++++++++++++++++
 target/ppc/translate_init.inc.c | 62 +++++++++++++++++++++++++++++++--
 4 files changed, 130 insertions(+), 2 deletions(-)

--
2.17.1

[Qemu-devel] [PATCH v5 1/1] target/ppc: Enable reporting of SPRs to GDB

[Fabiano Rosas]

This allows reading and writing of SPRs via GDB:

(gdb) p/x $srr1
$1 = 0x8000000002803033

(gdb) p/x $pvr
$2 = 0x4b0201
(gdb) set $pvr=0x4b0000
(gdb) p/x $pvr
$3 = 0x4b0000

The `info` command can also be used:
(gdb) info registers spr

For this purpose, GDB needs to be provided with an XML description of
the registers (see the gdb-xml directory for examples) and a set of
callbacks for reading and writing the registers must be defined.

The XML file in this case is created dynamically, based on the SPRs
already defined in the machine. This way we avoid the need for several
XML files to suit each possible ppc machine.

The gdb_{get,set}_spr_reg callbacks take an index based on the order
the registers appear in the XML file. This index does not match the
actual location of the registers in the env->spr array so the
gdb_find_spr_idx function does that conversion.

Note: GDB currently needs to know the guest endianness in order to
properly print the registers values. This is done automatically by GDB
when provided with the ELF file or explicitly with the `set endian
<big|little>` command.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 target/ppc/cpu-qom.h            |  4 +++
 target/ppc/cpu.h                |  5 +++
 target/ppc/gdbstub.c            | 61 ++++++++++++++++++++++++++++++++
 target/ppc/translate_init.inc.c | 62 +++++++++++++++++++++++++++++++--
 4 files changed, 130 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
index 4ea67692e2..3130802304 100644
--- a/target/ppc/cpu-qom.h
+++ b/target/ppc/cpu-qom.h
@@ -179,6 +179,10 @@ typedef struct PowerPCCPUClass {
     uint32_t flags;
     int bfd_mach;
     uint32_t l1_dcache_size, l1_icache_size;
+#ifndef CONFIG_USER_ONLY
+    unsigned int gdb_num_sprs;
+    const char *gdb_spr_xml;
+#endif
     const PPCHash64Options *hash64_opts;
     struct ppc_radix_page_info *radix_page_info;
     void (*init_proc)(CPUPPCState *env);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2c22292e7f..78af7e4608 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;
@@ -1263,6 +1264,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
+void ppc_gdb_gen_spr_xml(PowerPCCPU *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..fbf3821f4b 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -319,3 +319,64 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
     }
     return r;
 }
+
+#ifndef CONFIG_USER_ONLY
+void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    CPUPPCState *env = &cpu->env;
+    GString *xml;
+    char *spr_name;
+    unsigned int num_regs = 0;
+    int i;
+
+    if (pcc->gdb_spr_xml) {
+        return;
+    }
+
+    xml = g_string_new("<?xml version=\"1.0\"?>");
+    g_string_append(xml, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
+    g_string_append(xml, "<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;
+        }
+
+        spr_name = g_ascii_strdown(spr->name, -1);
+        g_string_append_printf(xml, "<reg name=\"%s\"", spr_name);
+        g_free(spr_name);
+
+        g_string_append_printf(xml, " bitsize=\"%d\"", TARGET_LONG_BITS);
+        g_string_append(xml, " 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(xml, "</feature>");
+
+    pcc->gdb_num_sprs = num_regs;
+    pcc->gdb_spr_xml = g_string_free(xml, false);
+}
+
+const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
+
+    if (strcmp(xml_name, "power-spr.xml") == 0) {
+        return pcc->gdb_spr_xml;
+    }
+    return NULL;
+}
+#endif
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 59e0b86762..9295f78d5f 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8979,6 +8979,10 @@ static void init_ppc_proc(PowerPCCPU *cpu)
     /* PowerPC implementation specific initialisations (SPRs, timers, ...) */
     (*pcc->init_proc)(env);
 
+#if !defined(CONFIG_USER_ONLY)
+    ppc_gdb_gen_spr_xml(cpu);
+#endif
+
     /* MSR bits & flags consistency checks */
     if (env->msr_mask & (1 << 25)) {
         switch (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
@@ -9475,6 +9479,55 @@ static bool avr_need_swap(CPUPPCState *env)
 #endif
 }
 
+#if !defined(CONFIG_USER_ONLY)
+static int gdb_find_spr_idx(CPUPPCState *env, int n)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
+        ppc_spr_t *spr = &env->spr_cb[i];
+
+        if (spr->name && spr->gdb_id == n) {
+            return i;
+        }
+    }
+    return -1;
+}
+
+static int gdb_get_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+{
+    int reg;
+    int len;
+
+    reg = gdb_find_spr_idx(env, n);
+    if (reg < 0) {
+        return 0;
+    }
+
+    len = TARGET_LONG_SIZE;
+    stn_p(mem_buf, len, env->spr[reg]);
+    ppc_maybe_bswap_register(env, mem_buf, len);
+    return len;
+}
+
+static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+{
+    int reg;
+    int len;
+
+    reg = gdb_find_spr_idx(env, n);
+    if (reg < 0) {
+        return 0;
+    }
+
+    len = TARGET_LONG_SIZE;
+    ppc_maybe_bswap_register(env, mem_buf, len);
+    env->spr[reg] = ldn_p(mem_buf, len);
+
+    return len;
+}
+#endif
+
 static int gdb_get_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
@@ -9704,7 +9757,10 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
         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
     qemu_init_vcpu(cs);
 
     pcc->parent_realize(dev, errp);
@@ -10467,7 +10523,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 #endif
 
     cc->gdb_num_core_regs = 71;
-
+#ifndef CONFIG_USER_ONLY
+    cc->gdb_get_dynamic_xml = ppc_gdb_get_dynamic_xml;
+#endif
 #ifdef USE_APPLE_GDB
     cc->gdb_read_register = ppc_cpu_gdb_read_register_apple;
     cc->gdb_write_register = ppc_cpu_gdb_write_register_apple;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v5 1/1] target/ppc: Enable reporting of SPRs to GDB

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8836 bytes --]

On Wed, Feb 06, 2019 at 02:51:33PM -0200, Fabiano Rosas wrote:
> This allows reading and writing of SPRs via GDB:
> 
> (gdb) p/x $srr1
> $1 = 0x8000000002803033
> 
> (gdb) p/x $pvr
> $2 = 0x4b0201
> (gdb) set $pvr=0x4b0000
> (gdb) p/x $pvr
> $3 = 0x4b0000
> 
> The `info` command can also be used:
> (gdb) info registers spr
> 
> For this purpose, GDB needs to be provided with an XML description of
> the registers (see the gdb-xml directory for examples) and a set of
> callbacks for reading and writing the registers must be defined.
> 
> The XML file in this case is created dynamically, based on the SPRs
> already defined in the machine. This way we avoid the need for several
> XML files to suit each possible ppc machine.
> 
> The gdb_{get,set}_spr_reg callbacks take an index based on the order
> the registers appear in the XML file. This index does not match the
> actual location of the registers in the env->spr array so the
> gdb_find_spr_idx function does that conversion.
> 
> Note: GDB currently needs to know the guest endianness in order to
> properly print the registers values. This is done automatically by GDB
> when provided with the ELF file or explicitly with the `set endian
> <big|little>` command.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>

I've applied this to ppc-for-4.0 because it does something we don't
currently do.  I'm still a bit baffled by the endian handling, but we
can fix that later if necessary.

> ---
>  target/ppc/cpu-qom.h            |  4 +++
>  target/ppc/cpu.h                |  5 +++
>  target/ppc/gdbstub.c            | 61 ++++++++++++++++++++++++++++++++
>  target/ppc/translate_init.inc.c | 62 +++++++++++++++++++++++++++++++--
>  4 files changed, 130 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index 4ea67692e2..3130802304 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -179,6 +179,10 @@ typedef struct PowerPCCPUClass {
>      uint32_t flags;
>      int bfd_mach;
>      uint32_t l1_dcache_size, l1_icache_size;
> +#ifndef CONFIG_USER_ONLY
> +    unsigned int gdb_num_sprs;
> +    const char *gdb_spr_xml;
> +#endif
>      const PPCHash64Options *hash64_opts;
>      struct ppc_radix_page_info *radix_page_info;
>      void (*init_proc)(CPUPPCState *env);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2c22292e7f..78af7e4608 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;
> @@ -1263,6 +1264,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
> +void ppc_gdb_gen_spr_xml(PowerPCCPU *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..fbf3821f4b 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -319,3 +319,64 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
>      }
>      return r;
>  }
> +
> +#ifndef CONFIG_USER_ONLY
> +void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    GString *xml;
> +    char *spr_name;
> +    unsigned int num_regs = 0;
> +    int i;
> +
> +    if (pcc->gdb_spr_xml) {
> +        return;
> +    }
> +
> +    xml = g_string_new("<?xml version=\"1.0\"?>");
> +    g_string_append(xml, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
> +    g_string_append(xml, "<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;
> +        }
> +
> +        spr_name = g_ascii_strdown(spr->name, -1);
> +        g_string_append_printf(xml, "<reg name=\"%s\"", spr_name);
> +        g_free(spr_name);
> +
> +        g_string_append_printf(xml, " bitsize=\"%d\"", TARGET_LONG_BITS);
> +        g_string_append(xml, " 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(xml, "</feature>");
> +
> +    pcc->gdb_num_sprs = num_regs;
> +    pcc->gdb_spr_xml = g_string_free(xml, false);
> +}
> +
> +const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> +
> +    if (strcmp(xml_name, "power-spr.xml") == 0) {
> +        return pcc->gdb_spr_xml;
> +    }
> +    return NULL;
> +}
> +#endif
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 59e0b86762..9295f78d5f 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -8979,6 +8979,10 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>      /* PowerPC implementation specific initialisations (SPRs, timers, ...) */
>      (*pcc->init_proc)(env);
>  
> +#if !defined(CONFIG_USER_ONLY)
> +    ppc_gdb_gen_spr_xml(cpu);
> +#endif
> +
>      /* MSR bits & flags consistency checks */
>      if (env->msr_mask & (1 << 25)) {
>          switch (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
> @@ -9475,6 +9479,55 @@ static bool avr_need_swap(CPUPPCState *env)
>  #endif
>  }
>  
> +#if !defined(CONFIG_USER_ONLY)
> +static int gdb_find_spr_idx(CPUPPCState *env, int n)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
> +        ppc_spr_t *spr = &env->spr_cb[i];
> +
> +        if (spr->name && spr->gdb_id == n) {
> +            return i;
> +        }
> +    }
> +    return -1;
> +}
> +
> +static int gdb_get_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +{
> +    int reg;
> +    int len;
> +
> +    reg = gdb_find_spr_idx(env, n);
> +    if (reg < 0) {
> +        return 0;
> +    }
> +
> +    len = TARGET_LONG_SIZE;
> +    stn_p(mem_buf, len, env->spr[reg]);
> +    ppc_maybe_bswap_register(env, mem_buf, len);
> +    return len;
> +}
> +
> +static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +{
> +    int reg;
> +    int len;
> +
> +    reg = gdb_find_spr_idx(env, n);
> +    if (reg < 0) {
> +        return 0;
> +    }
> +
> +    len = TARGET_LONG_SIZE;
> +    ppc_maybe_bswap_register(env, mem_buf, len);
> +    env->spr[reg] = ldn_p(mem_buf, len);
> +
> +    return len;
> +}
> +#endif
> +
>  static int gdb_get_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>  {
>      if (n < 32) {
> @@ -9704,7 +9757,10 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
>          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
>      qemu_init_vcpu(cs);
>  
>      pcc->parent_realize(dev, errp);
> @@ -10467,7 +10523,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  #endif
>  
>      cc->gdb_num_core_regs = 71;
> -
> +#ifndef CONFIG_USER_ONLY
> +    cc->gdb_get_dynamic_xml = ppc_gdb_get_dynamic_xml;
> +#endif
>  #ifdef USE_APPLE_GDB
>      cc->gdb_read_register = ppc_cpu_gdb_read_register_apple;
>      cc->gdb_write_register = ppc_cpu_gdb_write_register_apple;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]