[Qemu-devel] [PATCH v4 0/3] ppc/gdbstub: Expose SPRs to GDB

[Qemu-devel] [PATCH v4 0/3] 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.

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.

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


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-qom.h            |  4 +++
 target/ppc/cpu.h                |  5 +++
 target/ppc/gdbstub.c            | 60 +++++++++++++++++++++++++++++++
 target/ppc/translate_init.inc.c | 62 +++++++++++++++++++++++++++++++--
 4 files changed, 129 insertions(+), 2 deletions(-)

--
2.17.1

[Qemu-devel] [PATCH v4 1/3] target/ppc: Add SPRs XML generation code for gdbstub

[Fabiano Rosas]

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-qom.h            |  4 +++
 target/ppc/cpu.h                |  5 +++
 target/ppc/gdbstub.c            | 60 +++++++++++++++++++++++++++++++++
 target/ppc/translate_init.inc.c |  4 +++
 4 files changed, 73 insertions(+)

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 a62ff60414..850c5ba278 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;
@@ -1268,6 +1269,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..b2bb765506 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -319,3 +319,63 @@ 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 *s;
+    unsigned int num_regs;
+    int i;
+
+    if (pcc->gdb_spr_xml) {
+        return;
+    }
+
+    s = g_string_new(NULL);
+    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>");
+
+    pcc->gdb_num_sprs = num_regs;
+    pcc->gdb_spr_xml = g_string_free(s, false);
+}
+
+const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    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 ade06cc773..710064a25d 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8987,6 +8987,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)) {
-- 
2.17.1

[Qemu-devel] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Fabiano Rosas]

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 710064a25d..f29ac3558a 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -9487,6 +9487,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) {
@@ -9716,7 +9765,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);
-- 
2.17.1

[Qemu-devel] [PATCH v4 3/3] 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

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 f29ac3558a..cba0303be2 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10531,7 +10531,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 v4 1/3] target/ppc: Add SPRs XML generation code for gdbstub

[Alexey Kardashevskiy]

On 23/01/2019 04:01, Fabiano Rosas 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-qom.h            |  4 +++
>  target/ppc/cpu.h                |  5 +++
>  target/ppc/gdbstub.c            | 60 +++++++++++++++++++++++++++++++++
>  target/ppc/translate_init.inc.c |  4 +++
>  4 files changed, 73 insertions(+)
> 
> 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 a62ff60414..850c5ba278 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;
> @@ -1268,6 +1269,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..b2bb765506 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -319,3 +319,63 @@ 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 *s;
> +    unsigned int num_regs;
> +    int i;
> +
> +    if (pcc->gdb_spr_xml) {
> +        return;
> +    }
> +
> +    s = g_string_new(NULL);
> +    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\">");


nit: most of g_string_append_printf() in this patch can easily be
g_string_append().


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

This leaks memory as g_ascii_strdown() returns a newly-allocated string:
https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-ascii-strdown


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



/home/aik/p/qemu/target/ppc/gdbstub.c: In function ‘ppc_gdb_gen_spr_xml’:
/home/aik/p/qemu/target/ppc/gdbstub.c:362:17: warning: ‘num_regs’ may be
used uninitialized in this function [-Wmaybe-uninitialized]
         num_regs++;
                 ^

as num_regs is uninitialized indeed.


> +    }
> +
> +    g_string_append_printf(s, "</feature>");
> +
> +    pcc->gdb_num_sprs = num_regs;
> +    pcc->gdb_spr_xml = g_string_free(s, false);
> +}
> +
> +const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);

nit: you could do POWERPC_CPU_GET_CLASS(cs) and ditch @cpu.


> +
> +    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 ade06cc773..710064a25d 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -8987,6 +8987,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)) {
> 

-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Alexey Kardashevskiy]

On 23/01/2019 04:01, Fabiano Rosas wrote:
> 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 710064a25d..f29ac3558a 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -9487,6 +9487,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);


I am confused by this as it produces different results depending on the
guest mode:

(gdb) p $pvr
$1 = 0x14c0000000000
(gdb) c
Continuing.
Program received signal SIGINT, Interrupt.
(gdb) p $pvr
$2 = 0x4c0100


First print is when I stopped the guest in the SLOF firmware (so it is
big-endian) and then I continued and stopped gdb when the guest booted a
little-endian system; the KVM host is little endian, the machine running
gdb is LE too.

QEMU monitor prints the same 0x4c0100 in both cases.

I am adding the inventor of maybe_bswap_register() in cc: for
assistance. Swapping happens:
- once for BE: after stn_p()
	*(unsigned long *)mem_buf is 0x14c0000000000
- twice for LE.





> +    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) {
> @@ -9716,7 +9765,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);
> 

-- 
Alexey

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

[Alexey Kardashevskiy]

On 23/01/2019 04:01, 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
> 
> They can also be shown as a group:
> (gdb) info reg spr


This does not make much sense as 3 patches, one would do it; otherwise
if there is a problem with 1/3 or 2/3 - bisect will point to 3/3
although it does not actually add new code but enables other bits.


> 
> 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 f29ac3558a..cba0303be2 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10531,7 +10531,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;
> 

-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[David Gibson]

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

On Thu, Jan 24, 2019 at 06:20:02PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 23/01/2019 04:01, Fabiano Rosas wrote:
> > 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 710064a25d..f29ac3558a 100644
> > --- a/target/ppc/translate_init.inc.c
> > +++ b/target/ppc/translate_init.inc.c
> > @@ -9487,6 +9487,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);
> 
> 
> I am confused by this as it produces different results depending on the
> guest mode:


Hm, yeah, I thought the bswap here looked odd, but it wasn't obvious
to me if it was bogus here, or just a bogus gdb interface we have to
work around.

> (gdb) p $pvr
> $1 = 0x14c0000000000
> (gdb) c
> Continuing.
> Program received signal SIGINT, Interrupt.
> (gdb) p $pvr
> $2 = 0x4c0100

But that behaviour definitely looks wrong.

> First print is when I stopped the guest in the SLOF firmware (so it is
> big-endian) and then I continued and stopped gdb when the guest booted a
> little-endian system; the KVM host is little endian, the machine running
> gdb is LE too.
> 
> QEMU monitor prints the same 0x4c0100 in both cases.
> 
> I am adding the inventor of maybe_bswap_register() in cc: for
> assistance. Swapping happens:
> - once for BE: after stn_p()
> 	*(unsigned long *)mem_buf is 0x14c0000000000
> - twice for LE.
> 
> 
> 
> 
> 
> > +    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) {
> > @@ -9716,7 +9765,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);
> > 
> 

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Thu, Jan 24, 2019 at 06:20:02PM +1100, Alexey Kardashevskiy wrote:
>> 
>> 
>> On 23/01/2019 04:01, Fabiano Rosas wrote:
>> > 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 710064a25d..f29ac3558a 100644
>> > --- a/target/ppc/translate_init.inc.c
>> > +++ b/target/ppc/translate_init.inc.c
>> > @@ -9487,6 +9487,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);
>> 
>> 
>> I am confused by this as it produces different results depending on the
>> guest mode:
>
>
> Hm, yeah, I thought the bswap here looked odd, but it wasn't obvious
> to me if it was bogus here, or just a bogus gdb interface we have to
> work around.
>
>> (gdb) p $pvr
>> $1 = 0x14c0000000000
>> (gdb) c
>> Continuing.
>> Program received signal SIGINT, Interrupt.
>> (gdb) p $pvr
>> $2 = 0x4c0100
>
> But that behaviour definitely looks wrong.

GDB detects the endianness by looking at the ELF headers:

(gdb) p/x $pvr
$1 = 0x1024b0000000000
(gdb) file ~/qemu/roms/SLOF/board-qemu/llfw/stage1.elf
Reading symbols from ~/qemu/roms/SLOF/board-qemu/llfw/stage1.elf...done.
(gdb) show endian
The target endianness is set automatically (currently big endian)
(gdb) p/x $pvr
$2 = 0x4b0201
(gdb) c
Continuing.

(gdb) ^C
Program received signal SIGINT, Interrupt.
0x74a70c00000000c0 in ?? ()
(gdb) file vmlinux
Reading symbols from vmlinux...done.
(gdb) show endian
The target endianness is set automatically (currently little endian)
(gdb) p/x $pvr
$3 = 0x4b0201

The maybe_bswap_register is done due to QEMU having TARGET_WORDS_BIGENDIAN set
even after we have changed into LE mode.

>> First print is when I stopped the guest in the SLOF firmware (so it is
>> big-endian) and then I continued and stopped gdb when the guest booted a
>> little-endian system; the KVM host is little endian, the machine running
>> gdb is LE too.
>> 
>> QEMU monitor prints the same 0x4c0100 in both cases.
>> 
>> I am adding the inventor of maybe_bswap_register() in cc: for
>> assistance. Swapping happens:
>> - once for BE: after stn_p()
>> 	*(unsigned long *)mem_buf is 0x14c0000000000
>> - twice for LE.
>> 
>> 
>> 
>> 
>> 
>> > +    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) {
>> > @@ -9716,7 +9765,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);
>> > 
>> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Alexey Kardashevskiy]

On 29/01/2019 07:00, Fabiano Rosas wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
>> On Thu, Jan 24, 2019 at 06:20:02PM +1100, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 23/01/2019 04:01, Fabiano Rosas wrote:
>>>> 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 710064a25d..f29ac3558a 100644
>>>> --- a/target/ppc/translate_init.inc.c
>>>> +++ b/target/ppc/translate_init.inc.c
>>>> @@ -9487,6 +9487,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);
>>>
>>>
>>> I am confused by this as it produces different results depending on the
>>> guest mode:
>>
>>
>> Hm, yeah, I thought the bswap here looked odd, but it wasn't obvious
>> to me if it was bogus here, or just a bogus gdb interface we have to
>> work around.
>>
>>> (gdb) p $pvr
>>> $1 = 0x14c0000000000
>>> (gdb) c
>>> Continuing.
>>> Program received signal SIGINT, Interrupt.
>>> (gdb) p $pvr
>>> $2 = 0x4c0100
>>
>> But that behaviour definitely looks wrong.
> 
> GDB detects the endianness by looking at the ELF headers:


but this is a register which does not have endianness, the endianness
appears here because the interface between gdb and qemu is
uint8_t*==bytestream but this interface should have fixed endianness
imho (now it is bigendian afaict).

Something is not right here...

> 
> (gdb) p/x $pvr
> $1 = 0x1024b0000000000
> (gdb) file ~/qemu/roms/SLOF/board-qemu/llfw/stage1.elf
> Reading symbols from ~/qemu/roms/SLOF/board-qemu/llfw/stage1.elf...done.
> (gdb) show endian
> The target endianness is set automatically (currently big endian)
> (gdb) p/x $pvr
> $2 = 0x4b0201
> (gdb) c
> Continuing.
> 
> (gdb) ^C
> Program received signal SIGINT, Interrupt.
> 0x74a70c00000000c0 in ?? ()
> (gdb) file vmlinux
> Reading symbols from vmlinux...done.
> (gdb) show endian
> The target endianness is set automatically (currently little endian)
> (gdb) p/x $pvr
> $3 = 0x4b0201
> 
> The maybe_bswap_register is done due to QEMU having TARGET_WORDS_BIGENDIAN set
> even after we have changed into LE mode.
> 
>>> First print is when I stopped the guest in the SLOF firmware (so it is
>>> big-endian) and then I continued and stopped gdb when the guest booted a
>>> little-endian system; the KVM host is little endian, the machine running
>>> gdb is LE too.
>>>
>>> QEMU monitor prints the same 0x4c0100 in both cases.
>>>
>>> I am adding the inventor of maybe_bswap_register() in cc: for
>>> assistance. Swapping happens:
>>> - once for BE: after stn_p()
>>> 	*(unsigned long *)mem_buf is 0x14c0000000000
>>> - twice for LE.
>>>
>>>
>>>
>>>
>>>
>>>> +    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) {
>>>> @@ -9716,7 +9765,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);
>>>>
>>>
> 

-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Fabiano Rosas]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

>
> but this is a register which does not have endianness, the endianness
> appears here because the interface between gdb and qemu is
> uint8_t*==bytestream but this interface should have fixed endianness
> imho (now it is bigendian afaict).
>
> Something is not right here...

Having a fixed endianness would not work because GDB have no way of
knowing how to represent what comes from the remote end. It will
always check the target endianness before printing a value, even if it
refers to a register:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/findvar.c;hb=HEAD#l49

So in our case the contents of mem_buf need to match both the guest
endianness *and* what GDB has set for 'show endian' because it will
detect it automatically from the ELF. If it guesses incorrectly because
there is no ELF, we need to use the 'set endian' command.

By the way, this is already the behavior for the registers that are
already implemented (e.g. $msr). Here's the commit that introduced
that:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8a286ce4502356ce0b97a2424a2cb7

Now, what might be a source of confusion here is the fact that we
*always* do a bswap when the host is LE because QEMU thinks that the ppc
guest is always BE. That requires the maybe_bswap function to make
things right in the end.

What I could do is try to improve this by only swapping when the
guest's actual endianness (msr_le) is different from the host's. That
is not entirely within the scope of this patch, though.

Cheers

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Alexey Kardashevskiy]

On 31/01/2019 03:30, Fabiano Rosas wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>>
>> but this is a register which does not have endianness, the endianness
>> appears here because the interface between gdb and qemu is
>> uint8_t*==bytestream but this interface should have fixed endianness
>> imho (now it is bigendian afaict).
>>
>> Something is not right here...
> 
> Having a fixed endianness would not work because GDB have no way of
> knowing how to represent what comes from the remote end.

It definitely would. A register is stored as "unsigned long" in QEMU and
all gdb has to do is printf("%lx") and that is it. The problem is that
we want to pass it as a byte stream from the gdb_read_register() hook
all the way to gdb and for some reason gdb does not define endianness of
that stream but rather tries guessing the endianness which is broken.

Today I was debugging rtas/clientinterface calls which a little endian
kernel makes and these calls need to switch to the big endian first. And
gdb goes nuts when this switch happens (note that I did not give an ELF
to gdb this time so it picked LE itself). Even if it could fetch the
endianness from QEMU, it would fail as it is an LE bit in MSR which is a
register which is parsed according to the gdb's idea of endianness :)


> It will
> always check the target endianness before printing a value, even if it
> refers to a register:
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/findvar.c;hb=HEAD#l49
> 
> So in our case the contents of mem_buf need to match both the guest
> endianness *and* what GDB has set for 'show endian' because it will
> detect it automatically from the ELF. If it guesses incorrectly because
> there is no ELF, we need to use the 'set endian' command.
> 
> By the way, this is already the behavior for the registers that are
> already implemented (e.g. $msr). Here's the commit that introduced
> that:
> 
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8a286ce4502356ce0b97a2424a2cb7
> 
> Now, what might be a source of confusion here is the fact that we
> *always* do a bswap when the host is LE because QEMU thinks that the ppc
> guest is always BE. That requires the maybe_bswap function to make
> things right in the end.
> 
> What I could do is try to improve this by only swapping when the
> guest's actual endianness (msr_le) is different from the host's.

The bytestream for registers should have fixed endianness. But looking
at the gdb code makes me think it is not going to happen :(


> That
> is not entirely within the scope of this patch, though.

True. But since you are touching this, may be you could fix gdb too :)

Does gdb tell QEMU about what endianness it thinks that QEMU is using?
Or can it read it from QEMU? I cannot easily spot this in QEMU...


-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Fabiano Rosas]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 31/01/2019 03:30, Fabiano Rosas wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>> 
>>>
>>> but this is a register which does not have endianness, the endianness
>>> appears here because the interface between gdb and qemu is
>>> uint8_t*==bytestream but this interface should have fixed endianness
>>> imho (now it is bigendian afaict).
>>>
>>> Something is not right here...
>> 
>> Having a fixed endianness would not work because GDB have no way of
>> knowing how to represent what comes from the remote end.
>
> It definitely would. A register is stored as "unsigned long" in QEMU and
> all gdb has to do is printf("%lx") and that is it. 

OK, but something is not clear to me. Even if GDB just printf("%lx") the
value, we would still have to bswap when the host is LE, right?

QEMU BE:
 (gdb) x/8xb &env->spr[287]
 0x11391760: 0x00    0x00    0x00    0x00    0x00    0x4e    0x12    0x00

QEMU LE:
 (gdb) x/8xb &env->spr[287]
 0x7ffff5bd98c0: 0x01    0x02    0x4b    0x00    0x00    0x00    0x00    0x00

> The problem is that
> we want to pass it as a byte stream from the gdb_read_register() hook
> all the way to gdb and for some reason gdb does not define endianness of
> that stream but rather tries guessing the endianness which is broken.

GDB does define the endianness of the stream (in a way):

 "Each byte of register data is described by two hex digits. The bytes
 with the register are transmitted in target byte order."

https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#Packets

> Today I was debugging rtas/clientinterface calls which a little endian
> kernel makes and these calls need to switch to the big endian first. And
> gdb goes nuts when this switch happens (note that I did not give an ELF
> to gdb this time so it picked LE itself). Even if it could fetch the
> endianness from QEMU, it would fail as it is an LE bit in MSR which is a
> register which is parsed according to the gdb's idea of endianness :)

I think it would be possible to define another packet for the remote
protocol that informs the endianness explicitly at each time the guest
stops. If you provide more info on how to reproduce that issue I could
put in my list or go bug GDB folks about it.

>> It will
>> always check the target endianness before printing a value, even if it
>> refers to a register:
>> 
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/findvar.c;hb=HEAD#l49
>> 
>> So in our case the contents of mem_buf need to match both the guest
>> endianness *and* what GDB has set for 'show endian' because it will
>> detect it automatically from the ELF. If it guesses incorrectly because
>> there is no ELF, we need to use the 'set endian' command.
>> 
>> By the way, this is already the behavior for the registers that are
>> already implemented (e.g. $msr). Here's the commit that introduced
>> that:
>> 
>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8a286ce4502356ce0b97a2424a2cb7
>> 
>> Now, what might be a source of confusion here is the fact that we
>> *always* do a bswap when the host is LE because QEMU thinks that the ppc
>> guest is always BE. That requires the maybe_bswap function to make
>> things right in the end.
>> 
>> What I could do is try to improve this by only swapping when the
>> guest's actual endianness (msr_le) is different from the host's.
>
> The bytestream for registers should have fixed endianness. But looking
> at the gdb code makes me think it is not going to happen :(

Yes, I can't think of a way to fix that without changing the way GDB
exhibits the values or the remote protocol.

May I proceed with this series as it is with the bswaps?

>> That
>> is not entirely within the scope of this patch, though.
>
> True. But since you are touching this, may be you could fix gdb too :)
>
> Does gdb tell QEMU about what endianness it thinks that QEMU is using?
> Or can it read it from QEMU? I cannot easily spot this in QEMU...

GDB currently does not tell and does not ask about endianness. So I
believe there is room for improvement here. I could not find it in the
documentation but I think that GDB supports file transfers and it asks
for the ELF in some scenarios. This approach could be one way of
informing it about the endianness, although it has its own shortcomings.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Alexey Kardashevskiy]

On 01/02/2019 08:57, Fabiano Rosas wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> On 31/01/2019 03:30, Fabiano Rosas wrote:
>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>
>>>>
>>>> but this is a register which does not have endianness, the endianness
>>>> appears here because the interface between gdb and qemu is
>>>> uint8_t*==bytestream but this interface should have fixed endianness
>>>> imho (now it is bigendian afaict).
>>>>
>>>> Something is not right here...
>>>
>>> Having a fixed endianness would not work because GDB have no way of
>>> knowing how to represent what comes from the remote end.
>>
>> It definitely would. A register is stored as "unsigned long" in QEMU and
>> all gdb has to do is printf("%lx") and that is it. 
> 
> OK, but something is not clear to me. Even if GDB just printf("%lx") the
> value, we would still have to bswap when the host is LE, right?

Not for %lx, this should just print a correct value.

> QEMU BE:
>  (gdb) x/8xb &env->spr[287]
>  0x11391760: 0x00    0x00    0x00    0x00    0x00    0x4e    0x12    0x00
> 
> QEMU LE:
>  (gdb) x/8xb &env->spr[287]
>  0x7ffff5bd98c0: 0x01    0x02    0x4b    0x00    0x00    0x00    0x00    0x00
> 
>> The problem is that
>> we want to pass it as a byte stream from the gdb_read_register() hook
>> all the way to gdb and for some reason gdb does not define endianness of
>> that stream but rather tries guessing the endianness which is broken.
> 
> GDB does define the endianness of the stream (in a way):
> 
>  "Each byte of register data is described by two hex digits. The bytes
>  with the register are transmitted in target byte order."
> 
> https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#Packets


Target byte order changes all the time so we need a endian-agnostic way
of knowing the current endianness.


> 
>> Today I was debugging rtas/clientinterface calls which a little endian
>> kernel makes and these calls need to switch to the big endian first. And
>> gdb goes nuts when this switch happens (note that I did not give an ELF
>> to gdb this time so it picked LE itself). Even if it could fetch the
>> endianness from QEMU, it would fail as it is an LE bit in MSR which is a
>> register which is parsed according to the gdb's idea of endianness :)
> 
> I think it would be possible to define another packet for the remote
> protocol that informs the endianness explicitly at each time the guest
> stops. If you provide more info on how to reproduce that issue I could
> put in my list or go bug GDB folks about it.
> 
>>> It will
>>> always check the target endianness before printing a value, even if it
>>> refers to a register:
>>>
>>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/findvar.c;hb=HEAD#l49
>>>
>>> So in our case the contents of mem_buf need to match both the guest
>>> endianness *and* what GDB has set for 'show endian' because it will
>>> detect it automatically from the ELF. If it guesses incorrectly because
>>> there is no ELF, we need to use the 'set endian' command.
>>>
>>> By the way, this is already the behavior for the registers that are
>>> already implemented (e.g. $msr). Here's the commit that introduced
>>> that:
>>>
>>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8a286ce4502356ce0b97a2424a2cb7
>>>
>>> Now, what might be a source of confusion here is the fact that we
>>> *always* do a bswap when the host is LE because QEMU thinks that the ppc
>>> guest is always BE. That requires the maybe_bswap function to make
>>> things right in the end.
>>>
>>> What I could do is try to improve this by only swapping when the
>>> guest's actual endianness (msr_le) is different from the host's.
>>
>> The bytestream for registers should have fixed endianness. But looking
>> at the gdb code makes me think it is not going to happen :(
> 
> Yes, I can't think of a way to fix that without changing the way GDB
> exhibits the values or the remote protocol.
> 
> May I proceed with this series as it is with the bswaps?
> 
>>> That
>>> is not entirely within the scope of this patch, though.
>>
>> True. But since you are touching this, may be you could fix gdb too :)
>>
>> Does gdb tell QEMU about what endianness it thinks that QEMU is using?
>> Or can it read it from QEMU? I cannot easily spot this in QEMU...
> 
> GDB currently does not tell and does not ask about endianness. So I
> believe there is room for improvement here. I could not find it in the
> documentation but I think that GDB supports file transfers and it asks
> for the ELF in some scenarios. This approach could be one way of
> informing it about the endianness, although it has its own shortcomings.


There is no ELF in my scenario really. What I did was:

1. hack qemu to not load slof.bin but load vmlinux instead and changed
starting pc to where I loaded the kernel (I did not have to but it is a
lot easier to ditch slof and set breakpoint in gdb before starting the
guest).

2. start a guest, connect external gdb and observe garbage in $pc and
disassembly until the guest switched to LE.

3. trace till here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/entry_64.S?h=v4.20#n1299
and set breakpoint to whatever $r4 was at this point; $r4 points to RTAS
which executes in BE as we remove LE bit from SRR1 (lines 1296..1298).

When we stop at this new breakpoint - we get same biteswapped garbage as
in step2. Well, this is SLOF and there is an ELF somewhere but do we
really want to load ELF and switch endianness every time the guest
changes it and jumps to different chunks of code? :)


It also seems like we cannot really debug slof via the gdb stub as it is
big endian so it will be executing instructions but the UI will look
byteswapped.


-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Fabiano Rosas]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 01/02/2019 08:57, Fabiano Rosas wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>> 
>>> On 31/01/2019 03:30, Fabiano Rosas wrote:
>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>>
>>>>>
>>>>> but this is a register which does not have endianness, the endianness
>>>>> appears here because the interface between gdb and qemu is
>>>>> uint8_t*==bytestream but this interface should have fixed endianness
>>>>> imho (now it is bigendian afaict).
>>>>>
>>>>> Something is not right here...
>>>>
>>>> Having a fixed endianness would not work because GDB have no way of
>>>> knowing how to represent what comes from the remote end.
>>>
>>> It definitely would. A register is stored as "unsigned long" in QEMU and
>>> all gdb has to do is printf("%lx") and that is it. 
>> 
>> OK, but something is not clear to me. Even if GDB just printf("%lx") the
>> value, we would still have to bswap when the host is LE, right?
>
> Not for %lx, this should just print a correct value.
>
>> QEMU BE:
>>  (gdb) x/8xb &env->spr[287]
>>  0x11391760: 0x00    0x00    0x00    0x00    0x00    0x4e    0x12    0x00
>> 
>> QEMU LE:
>>  (gdb) x/8xb &env->spr[287]
>>  0x7ffff5bd98c0: 0x01    0x02    0x4b    0x00    0x00    0x00    0x00    0x00
>> 
>>> The problem is that
>>> we want to pass it as a byte stream from the gdb_read_register() hook
>>> all the way to gdb and for some reason gdb does not define endianness of
>>> that stream but rather tries guessing the endianness which is broken.
>> 
>> GDB does define the endianness of the stream (in a way):
>> 
>>  "Each byte of register data is described by two hex digits. The bytes
>>  with the register are transmitted in target byte order."
>> 
>> https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#Packets
>
>
> Target byte order changes all the time so we need a endian-agnostic way
> of knowing the current endianness.
>
>
>> 
>>> Today I was debugging rtas/clientinterface calls which a little endian
>>> kernel makes and these calls need to switch to the big endian first. And
>>> gdb goes nuts when this switch happens (note that I did not give an ELF
>>> to gdb this time so it picked LE itself). Even if it could fetch the
>>> endianness from QEMU, it would fail as it is an LE bit in MSR which is a
>>> register which is parsed according to the gdb's idea of endianness :)
>> 
>> I think it would be possible to define another packet for the remote
>> protocol that informs the endianness explicitly at each time the guest
>> stops. If you provide more info on how to reproduce that issue I could
>> put in my list or go bug GDB folks about it.
>> 
>>>> It will
>>>> always check the target endianness before printing a value, even if it
>>>> refers to a register:
>>>>
>>>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/findvar.c;hb=HEAD#l49
>>>>
>>>> So in our case the contents of mem_buf need to match both the guest
>>>> endianness *and* what GDB has set for 'show endian' because it will
>>>> detect it automatically from the ELF. If it guesses incorrectly because
>>>> there is no ELF, we need to use the 'set endian' command.
>>>>
>>>> By the way, this is already the behavior for the registers that are
>>>> already implemented (e.g. $msr). Here's the commit that introduced
>>>> that:
>>>>
>>>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8a286ce4502356ce0b97a2424a2cb7
>>>>
>>>> Now, what might be a source of confusion here is the fact that we
>>>> *always* do a bswap when the host is LE because QEMU thinks that the ppc
>>>> guest is always BE. That requires the maybe_bswap function to make
>>>> things right in the end.
>>>>
>>>> What I could do is try to improve this by only swapping when the
>>>> guest's actual endianness (msr_le) is different from the host's.
>>>
>>> The bytestream for registers should have fixed endianness. But looking
>>> at the gdb code makes me think it is not going to happen :(
>> 
>> Yes, I can't think of a way to fix that without changing the way GDB
>> exhibits the values or the remote protocol.
>> 
>> May I proceed with this series as it is with the bswaps?
>> 
>>>> That
>>>> is not entirely within the scope of this patch, though.
>>>
>>> True. But since you are touching this, may be you could fix gdb too :)
>>>
>>> Does gdb tell QEMU about what endianness it thinks that QEMU is using?
>>> Or can it read it from QEMU? I cannot easily spot this in QEMU...
>> 
>> GDB currently does not tell and does not ask about endianness. So I
>> believe there is room for improvement here. I could not find it in the
>> documentation but I think that GDB supports file transfers and it asks
>> for the ELF in some scenarios. This approach could be one way of
>> informing it about the endianness, although it has its own shortcomings.
>
>
> There is no ELF in my scenario really. What I did was:
>
> 1. hack qemu to not load slof.bin but load vmlinux instead and changed
> starting pc to where I loaded the kernel (I did not have to but it is a
> lot easier to ditch slof and set breakpoint in gdb before starting the
> guest).

Not sure if it improves your situation but the hack I do in this case is
to add a:

+    if (runstate_check(6))
+           addr += 0x400000;

in kvm_arch_{insert,remove}_sw_breakpoint.

Fyi, this is other thing I have been trying to fix. I am currently
trying to figure out a way of going out to QEMU right before the kernel
starts so we could then apply the breakpoints correctly. But this gets
more complicated when the kernel is loaded from disk.

> 2. start a guest, connect external gdb and observe garbage in $pc and
> disassembly until the guest switched to LE.
>
> 3. trace till here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/entry_64.S?h=v4.20#n1299
> and set breakpoint to whatever $r4 was at this point; $r4 points to RTAS
> which executes in BE as we remove LE bit from SRR1 (lines 1296..1298).
>
> When we stop at this new breakpoint - we get same biteswapped garbage as
> in step2. Well, this is SLOF and there is an ELF somewhere but do we
> really want to load ELF and switch endianness every time the guest
> changes it and jumps to different chunks of code? :)

You're right, we need something more versatile than that.

Perhaps for the general scenario of debugging same endianness code it
could work well. I think that might be a good thing anyway because it
would provide the symbols and so on. But I get your point.

For the case where the code switches back and forth, a new, fixed
endianness packet that tells GDB what is the current target endianness
should work just fine I believe. Analogous to calling `set endian
little|big` on gdb cmd line.

I'll reproduce the issue as you mentioned and will try to come up with
something along these lines.

> It also seems like we cannot really debug slof via the gdb stub as it is
> big endian so it will be executing instructions but the UI will look
> byteswapped.

As a temporary workaround I believe you could use `set endian`.


Thanks for the discussion so far. It's been really helpful.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/3] target/ppc: Add GDB callbacks for SPRs

[Alexey Kardashevskiy]

On 01/02/2019 23:01, Fabiano Rosas wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> On 01/02/2019 08:57, Fabiano Rosas wrote:
>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>
>>>> On 31/01/2019 03:30, Fabiano Rosas wrote:
>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>>>
>>>>>>
>>>>>> but this is a register which does not have endianness, the endianness
>>>>>> appears here because the interface between gdb and qemu is
>>>>>> uint8_t*==bytestream but this interface should have fixed endianness
>>>>>> imho (now it is bigendian afaict).
>>>>>>
>>>>>> Something is not right here...
>>>>>
>>>>> Having a fixed endianness would not work because GDB have no way of
>>>>> knowing how to represent what comes from the remote end.
>>>>
>>>> It definitely would. A register is stored as "unsigned long" in QEMU and
>>>> all gdb has to do is printf("%lx") and that is it. 
>>>
>>> OK, but something is not clear to me. Even if GDB just printf("%lx") the
>>> value, we would still have to bswap when the host is LE, right?
>>
>> Not for %lx, this should just print a correct value.
>>
>>> QEMU BE:
>>>  (gdb) x/8xb &env->spr[287]
>>>  0x11391760: 0x00    0x00    0x00    0x00    0x00    0x4e    0x12    0x00
>>>
>>> QEMU LE:
>>>  (gdb) x/8xb &env->spr[287]
>>>  0x7ffff5bd98c0: 0x01    0x02    0x4b    0x00    0x00    0x00    0x00    0x00
>>>
>>>> The problem is that
>>>> we want to pass it as a byte stream from the gdb_read_register() hook
>>>> all the way to gdb and for some reason gdb does not define endianness of
>>>> that stream but rather tries guessing the endianness which is broken.
>>>
>>> GDB does define the endianness of the stream (in a way):
>>>
>>>  "Each byte of register data is described by two hex digits. The bytes
>>>  with the register are transmitted in target byte order."
>>>
>>> https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#Packets
>>
>>
>> Target byte order changes all the time so we need a endian-agnostic way
>> of knowing the current endianness.
>>
>>
>>>
>>>> Today I was debugging rtas/clientinterface calls which a little endian
>>>> kernel makes and these calls need to switch to the big endian first. And
>>>> gdb goes nuts when this switch happens (note that I did not give an ELF
>>>> to gdb this time so it picked LE itself). Even if it could fetch the
>>>> endianness from QEMU, it would fail as it is an LE bit in MSR which is a
>>>> register which is parsed according to the gdb's idea of endianness :)
>>>
>>> I think it would be possible to define another packet for the remote
>>> protocol that informs the endianness explicitly at each time the guest
>>> stops. If you provide more info on how to reproduce that issue I could
>>> put in my list or go bug GDB folks about it.
>>>
>>>>> It will
>>>>> always check the target endianness before printing a value, even if it
>>>>> refers to a register:
>>>>>
>>>>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/findvar.c;hb=HEAD#l49
>>>>>
>>>>> So in our case the contents of mem_buf need to match both the guest
>>>>> endianness *and* what GDB has set for 'show endian' because it will
>>>>> detect it automatically from the ELF. If it guesses incorrectly because
>>>>> there is no ELF, we need to use the 'set endian' command.
>>>>>
>>>>> By the way, this is already the behavior for the registers that are
>>>>> already implemented (e.g. $msr). Here's the commit that introduced
>>>>> that:
>>>>>
>>>>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=8a286ce4502356ce0b97a2424a2cb7
>>>>>
>>>>> Now, what might be a source of confusion here is the fact that we
>>>>> *always* do a bswap when the host is LE because QEMU thinks that the ppc
>>>>> guest is always BE. That requires the maybe_bswap function to make
>>>>> things right in the end.
>>>>>
>>>>> What I could do is try to improve this by only swapping when the
>>>>> guest's actual endianness (msr_le) is different from the host's.
>>>>
>>>> The bytestream for registers should have fixed endianness. But looking
>>>> at the gdb code makes me think it is not going to happen :(
>>>
>>> Yes, I can't think of a way to fix that without changing the way GDB
>>> exhibits the values or the remote protocol.
>>>
>>> May I proceed with this series as it is with the bswaps?
>>>
>>>>> That
>>>>> is not entirely within the scope of this patch, though.
>>>>
>>>> True. But since you are touching this, may be you could fix gdb too :)
>>>>
>>>> Does gdb tell QEMU about what endianness it thinks that QEMU is using?
>>>> Or can it read it from QEMU? I cannot easily spot this in QEMU...
>>>
>>> GDB currently does not tell and does not ask about endianness. So I
>>> believe there is room for improvement here. I could not find it in the
>>> documentation but I think that GDB supports file transfers and it asks
>>> for the ELF in some scenarios. This approach could be one way of
>>> informing it about the endianness, although it has its own shortcomings.
>>
>>
>> There is no ELF in my scenario really. What I did was:
>>
>> 1. hack qemu to not load slof.bin but load vmlinux instead and changed
>> starting pc to where I loaded the kernel (I did not have to but it is a
>> lot easier to ditch slof and set breakpoint in gdb before starting the
>> guest).
> 
> Not sure if it improves your situation but the hack I do in this case is
> to add a:
> 
> +    if (runstate_check(6))
> +           addr += 0x400000;


I assume this 0x400000 is from FW_MAX_SIZE, right?

In my example the vmlinux is loaded at zero and changing 0x400000 is
just setting a random breakpoint.


> 
> in kvm_arch_{insert,remove}_sw_breakpoint.
> 
> Fyi, this is other thing I have been trying to fix. I am currently
> trying to figure out a way of going out to QEMU right before the kernel
> starts so we could then apply the breakpoints correctly.

For the starter (to proceed with these patches) you can just make sure
that setting breakpoints still works even if the guest has not started yet.


> But this gets
> more complicated when the kernel is loaded from disk.

True...


>> 2. start a guest, connect external gdb and observe garbage in $pc and
>> disassembly until the guest switched to LE.
>>
>> 3. trace till here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/entry_64.S?h=v4.20#n1299
>> and set breakpoint to whatever $r4 was at this point; $r4 points to RTAS
>> which executes in BE as we remove LE bit from SRR1 (lines 1296..1298).
>>
>> When we stop at this new breakpoint - we get same biteswapped garbage as
>> in step2. Well, this is SLOF and there is an ELF somewhere but do we
>> really want to load ELF and switch endianness every time the guest
>> changes it and jumps to different chunks of code? :)
> 
> You're right, we need something more versatile than that.
> 
> Perhaps for the general scenario of debugging same endianness code it
> could work well. I think that might be a good thing anyway because it
> would provide the symbols and so on. But I get your point.
> 
> For the case where the code switches back and forth, a new, fixed
> endianness packet that tells GDB what is the current target endianness
> should work just fine I believe. Analogous to calling `set endian
> little|big` on gdb cmd line.
> 
> I'll reproduce the issue as you mentioned and will try to come up with
> something along these lines.
> 
>> It also seems like we cannot really debug slof via the gdb stub as it is
>> big endian so it will be executing instructions but the UI will look
>> byteswapped.
> 
> As a temporary workaround I believe you could use `set endian`.
> 
> 
> Thanks for the discussion so far. It's been really helpful.



-- 
Alexey