All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose SPRs to GDB
@ 2019-01-15 19:37 Fabiano Rosas
  2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 1/3] target/ppc: Add SPRs XML generation code for gdbstub Fabiano Rosas
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Fabiano Rosas @ 2019-01-15 19:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-ppc, david

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.

In this series:

The first patch implements the dynamic generation of the power-spr.xml
file. Making it dynamically facilitates converting the GDB index to an
index useful for addressing the env->spr array.

The second patch implements the gdb_{get,set}_spr_reg callbacks along
with the convertion from GDB index to QEMU index.

The third patch enables the functionality.

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


Fabiano Rosas (3):
  target/ppc: Add SPRs XML generation code for gdbstub
  target/ppc: Add GDB callbacks for SPRs
  target/ppc: Enable reporting of SPRs to GDB

 target/ppc/cpu.h                |  8 +++++
 target/ppc/gdbstub.c            | 54 ++++++++++++++++++++++++++++++
 target/ppc/translate_init.inc.c | 58 +++++++++++++++++++++++++++++++--
 3 files changed, 118 insertions(+), 2 deletions(-)

--
2.17.1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v3 1/3] target/ppc: Add SPRs XML generation code for gdbstub
  2019-01-15 19:37 [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose SPRs to GDB Fabiano Rosas
@ 2019-01-15 19:37 ` Fabiano Rosas
  2019-01-17 12:14   ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
  2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 2/3] target/ppc: Add GDB callbacks for SPRs Fabiano Rosas
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Fabiano Rosas @ 2019-01-15 19:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-ppc, david

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v3 2/3] target/ppc: Add GDB callbacks for SPRs
  2019-01-15 19:37 [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose SPRs to GDB Fabiano Rosas
  2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 1/3] target/ppc: Add SPRs XML generation code for gdbstub Fabiano Rosas
@ 2019-01-15 19:37 ` Fabiano Rosas
  2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 3/3] target/ppc: Enable reporting of SPRs to GDB Fabiano Rosas
  2019-01-21  5:51 ` [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose " no-reply
  3 siblings, 0 replies; 7+ messages in thread
From: Fabiano Rosas @ 2019-01-15 19:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-ppc, david

These will be used to let GDB know about PPC's Special Purpose
Registers (SPR).

They take an index based on the order the registers appear in the XML
file sent by QEMU to GDB. 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.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 target/ppc/translate_init.inc.c | 54 ++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index ade06cc773..9c6c935204 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -9483,6 +9483,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) {
@@ -9712,7 +9761,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,
+                             ppc_gdb_gen_spr_xml(cs), "power-spr.xml", 0);
+#endif
     qemu_init_vcpu(cs);
 
     pcc->parent_realize(dev, errp);
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v3 3/3] target/ppc: Enable reporting of SPRs to GDB
  2019-01-15 19:37 [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose SPRs to GDB Fabiano Rosas
  2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 1/3] target/ppc: Add SPRs XML generation code for gdbstub Fabiano Rosas
  2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 2/3] target/ppc: Add GDB callbacks for SPRs Fabiano Rosas
@ 2019-01-15 19:37 ` Fabiano Rosas
  2019-01-21  5:51 ` [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose " no-reply
  3 siblings, 0 replies; 7+ messages in thread
From: Fabiano Rosas @ 2019-01-15 19:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-ppc, david

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

They can also be shown as a group:
(gdb) info reg spr

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 target/ppc/translate_init.inc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 9c6c935204..ebf202e54d 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10527,7 +10527,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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/3] target/ppc: Add SPRs XML generation code for gdbstub
  2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 1/3] target/ppc: Add SPRs XML generation code for gdbstub Fabiano Rosas
@ 2019-01-17 12:14   ` Greg Kurz
  2019-01-17 16:11     ` Fabiano Rosas
  0 siblings, 1 reply; 7+ messages in thread
From: Greg Kurz @ 2019-01-17 12:14 UTC (permalink / raw)
  To: Fabiano Rosas; +Cc: qemu-devel, qemu-ppc, david

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/3] target/ppc: Add SPRs XML generation code for gdbstub
  2019-01-17 12:14   ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
@ 2019-01-17 16:11     ` Fabiano Rosas
  0 siblings, 0 replies; 7+ messages in thread
From: Fabiano Rosas @ 2019-01-17 16:11 UTC (permalink / raw)
  To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, david

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose SPRs to GDB
  2019-01-15 19:37 [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose SPRs to GDB Fabiano Rosas
                   ` (2 preceding siblings ...)
  2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 3/3] target/ppc: Enable reporting of SPRs to GDB Fabiano Rosas
@ 2019-01-21  5:51 ` no-reply
  3 siblings, 0 replies; 7+ messages in thread
From: no-reply @ 2019-01-21  5:51 UTC (permalink / raw)
  To: farosas; +Cc: fam, qemu-devel, qemu-ppc, david

Patchew URL: https://patchew.org/QEMU/20190115193750.17234-1-farosas@linux.ibm.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

  CC      hw/acpi/vmgenid.o
  CC      hw/acpi/acpi_interface.o
/tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
/tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
     strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
---
make: *** Waiting for unfinished jobs....
In function 'acpi_table_install',
    inlined from 'acpi_table_add' at /tmp/qemu-test/src/hw/acpi/core.c:296:5:
/tmp/qemu-test/src/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
         strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/acpi/core.c:203:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation]
         strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/acpi/core.c:207:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]
         strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                 sizeof ext_hdr->oem_table_id);
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/acpi/core.c:216:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
         strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                 sizeof ext_hdr->asl_compiler_id);


The full log is available at
http://patchew.org/logs/20190115193750.17234-1-farosas@linux.ibm.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-01-21  6:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-15 19:37 [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose SPRs to GDB Fabiano Rosas
2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 1/3] target/ppc: Add SPRs XML generation code for gdbstub Fabiano Rosas
2019-01-17 12:14   ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2019-01-17 16:11     ` Fabiano Rosas
2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 2/3] target/ppc: Add GDB callbacks for SPRs Fabiano Rosas
2019-01-15 19:37 ` [Qemu-devel] [PATCH v3 3/3] target/ppc: Enable reporting of SPRs to GDB Fabiano Rosas
2019-01-21  5:51 ` [Qemu-devel] [PATCH v3 0/3] ppc/gdbstub: Expose " no-reply

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.