[Qemu-devel] [PATCH 0/5] s390x/gdb: various fixes

[Qemu-devel] [PATCH 0/5] s390x/gdb: various fixes

[Jens Freimann]

Conny, Alex, Christian,

here are some patches improving our gdb support. 

* Patch 1 fixes a bug where the cc was changed accidentally. 
* Patch 2 adds the gdb feature XML files for s390x
* Patch 3 Define acr and fpr registers as coprocessor registers. This allows us
   to reuse the feature XML files. 
* Patch 4 whitespace fixes
* Patch 5 changes common code and other architectures with gdb target.xml support.
   It adds a field gdb_arch_name to the XML description of the CPU and to struct
   CPUClass.  It allows the remote gdb to detect the target architecture
   in cases where it can't tell otherwise.

David Hildenbrand (5):
  s390x/gdb: don't touch the cc if tcg is not enabled
  s390x/gdb: add the feature xml files for s390x
  s390x/gdb: generate target.xml and handle fp/ac as coprocessors
  s390x/gdb: coding style fixes
  gdb: provide the name of the architecture in the target.xml

 configure                   |   1 +
 gdb-xml/s390-acr.xml        |  26 +++++++++++
 gdb-xml/s390-fpr.xml        |  27 +++++++++++
 gdb-xml/s390x-core64.xml    |  28 ++++++++++++
 gdbstub.c                   |  19 +++++---
 include/qom/cpu.h           |   2 +
 target-arm/cpu64.c          |   1 +
 target-ppc/translate_init.c |   2 +
 target-s390x/cpu-qom.h      |   1 +
 target-s390x/cpu.c          |   5 +-
 target-s390x/cpu.h          |  40 +---------------
 target-s390x/gdbstub.c      | 109 +++++++++++++++++++++++++++++++++-----------
 12 files changed, 188 insertions(+), 73 deletions(-)
 create mode 100644 gdb-xml/s390-acr.xml
 create mode 100644 gdb-xml/s390-fpr.xml
 create mode 100644 gdb-xml/s390x-core64.xml

-- 
1.8.5.5

[Qemu-devel] [PATCH 1/5] s390x/gdb: don't touch the cc if tcg is not enabled

[Jens Freimann]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

When reading/writing the psw mask, the condition code may only be touched if
running on tcg.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 target-s390x/gdbstub.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/target-s390x/gdbstub.c b/target-s390x/gdbstub.c
index a129742..8d55006 100644
--- a/target-s390x/gdbstub.c
+++ b/target-s390x/gdbstub.c
@@ -31,9 +31,13 @@ int s390_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
 
     switch (n) {
     case S390_PSWM_REGNUM:
-        cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst, env->cc_vr);
-        val = deposit64(env->psw.mask, 44, 2, cc_op);
-        return gdb_get_regl(mem_buf, val);
+        if (tcg_enabled()) {
+            cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst,
+                            env->cc_vr);
+            val = deposit64(env->psw.mask, 44, 2, cc_op);
+            return gdb_get_regl(mem_buf, val);
+        }
+        return gdb_get_regl(mem_buf, env->psw.mask);
     case S390_PSWA_REGNUM:
         return gdb_get_regl(mem_buf, env->psw.addr);
     case S390_R0_REGNUM ... S390_R15_REGNUM:
@@ -62,7 +66,9 @@ int s390_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     switch (n) {
     case S390_PSWM_REGNUM:
         env->psw.mask = tmpl;
-        env->cc_op = extract64(tmpl, 44, 2);
+        if (tcg_enabled()) {
+            env->cc_op = extract64(tmpl, 44, 2);
+        }
         break;
     case S390_PSWA_REGNUM:
         env->psw.addr = tmpl;
-- 
1.8.5.5

[Qemu-devel] [PATCH 2/5] s390x/gdb: add the feature xml files for s390x

[Jens Freimann]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

This patch adds the relevant s390x feature xml files taken from gdb.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 configure                |  1 +
 gdb-xml/s390-acr.xml     | 26 ++++++++++++++++++++++++++
 gdb-xml/s390-fpr.xml     | 27 +++++++++++++++++++++++++++
 gdb-xml/s390x-core64.xml | 28 ++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+)
 create mode 100644 gdb-xml/s390-acr.xml
 create mode 100644 gdb-xml/s390-fpr.xml
 create mode 100644 gdb-xml/s390x-core64.xml

diff --git a/configure b/configure
index 2063cf6..15201f9 100755
--- a/configure
+++ b/configure
@@ -5093,6 +5093,7 @@ case "$target_name" in
     echo "TARGET_ABI32=y" >> $config_target_mak
   ;;
   s390x)
+    gdb_xml_files="s390x-core64.xml s390-acr.xml s390-fpr.xml"
   ;;
   unicore32)
   ;;
diff --git a/gdb-xml/s390-acr.xml b/gdb-xml/s390-acr.xml
new file mode 100644
index 0000000..71dfb20
--- /dev/null
+++ b/gdb-xml/s390-acr.xml
@@ -0,0 +1,26 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2010-2014 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.s390.acr">
+  <reg name="acr0" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr1" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr2" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr3" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr4" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr5" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr6" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr7" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr8" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr9" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr10" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr11" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr12" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr13" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr14" bitsize="32" type="uint32" group="access"/>
+  <reg name="acr15" bitsize="32" type="uint32" group="access"/>
+</feature>
diff --git a/gdb-xml/s390-fpr.xml b/gdb-xml/s390-fpr.xml
new file mode 100644
index 0000000..7de0c13
--- /dev/null
+++ b/gdb-xml/s390-fpr.xml
@@ -0,0 +1,27 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2010-2014 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.s390.fpr">
+  <reg name="fpc" bitsize="32" type="uint32" group="float"/>
+  <reg name="f0" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f1" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f2" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f3" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f4" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f5" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f6" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f7" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f8" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f9" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f10" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f11" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f12" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f13" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f14" bitsize="64" type="ieee_double" group="float"/>
+  <reg name="f15" bitsize="64" type="ieee_double" group="float"/>
+</feature>
diff --git a/gdb-xml/s390x-core64.xml b/gdb-xml/s390x-core64.xml
new file mode 100644
index 0000000..1523437
--- /dev/null
+++ b/gdb-xml/s390x-core64.xml
@@ -0,0 +1,28 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2010-2014 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.s390.core">
+  <reg name="pswm" bitsize="64" type="uint64" group="psw"/>
+  <reg name="pswa" bitsize="64" type="uint64" group="psw"/>
+  <reg name="r0" bitsize="64" type="uint64" group="general"/>
+  <reg name="r1" bitsize="64" type="uint64" group="general"/>
+  <reg name="r2" bitsize="64" type="uint64" group="general"/>
+  <reg name="r3" bitsize="64" type="uint64" group="general"/>
+  <reg name="r4" bitsize="64" type="uint64" group="general"/>
+  <reg name="r5" bitsize="64" type="uint64" group="general"/>
+  <reg name="r6" bitsize="64" type="uint64" group="general"/>
+  <reg name="r7" bitsize="64" type="uint64" group="general"/>
+  <reg name="r8" bitsize="64" type="uint64" group="general"/>
+  <reg name="r9" bitsize="64" type="uint64" group="general"/>
+  <reg name="r10" bitsize="64" type="uint64" group="general"/>
+  <reg name="r11" bitsize="64" type="uint64" group="general"/>
+  <reg name="r12" bitsize="64" type="uint64" group="general"/>
+  <reg name="r13" bitsize="64" type="uint64" group="general"/>
+  <reg name="r14" bitsize="64" type="uint64" group="general"/>
+  <reg name="r15" bitsize="64" type="uint64" group="general"/>
+</feature>
-- 
1.8.5.5

[Qemu-devel] [PATCH 3/5] s390x/gdb: generate target.xml and handle fp/ac as coprocessors

[Jens Freimann]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

This patch reduces the core registers to the psw and the general purpose
registers. The fpc and ac registers are handled as coprocessors registers by gdb.
This allows to reuse the feature xml files taken from gdb without further
modification and is what other architectures do.

The target.xml is now generated and provided to the gdb client. Therefore, the
client doesn't have to guess which registers are available at which logical
register number.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 target-s390x/cpu-qom.h |  1 +
 target-s390x/cpu.c     |  4 ++-
 target-s390x/cpu.h     | 40 ++--------------------
 target-s390x/gdbstub.c | 91 ++++++++++++++++++++++++++++++++++++++------------
 4 files changed, 76 insertions(+), 60 deletions(-)

diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index f9c96d1..80dd741 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -89,5 +89,6 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 hwaddr s390_cpu_get_phys_addr_debug(CPUState *cpu, vaddr addr);
 int s390_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int s390_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+void s390_cpu_gdb_init(CPUState *cs);
 
 #endif
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index c3082b7..4b03e42 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -174,6 +174,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUState *cs = CPU(dev);
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
 
+    s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
     cpu_reset(cs);
 
@@ -259,7 +260,8 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     cc->write_elf64_qemunote = s390_cpu_write_elf64_qemunote;
 #endif
     dc->vmsd = &vmstate_s390_cpu;
-    cc->gdb_num_core_regs = S390_NUM_REGS;
+    cc->gdb_num_core_regs = S390_NUM_CORE_REGS;
+    cc->gdb_core_xml_file = "s390x-core64.xml";
 }
 
 static const TypeInfo s390_cpu_type_info = {
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index b13761d..525f969 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -551,44 +551,8 @@ void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 #define S390_R13_REGNUM 15
 #define S390_R14_REGNUM 16
 #define S390_R15_REGNUM 17
-/* Access Registers.  */
-#define S390_A0_REGNUM 18
-#define S390_A1_REGNUM 19
-#define S390_A2_REGNUM 20
-#define S390_A3_REGNUM 21
-#define S390_A4_REGNUM 22
-#define S390_A5_REGNUM 23
-#define S390_A6_REGNUM 24
-#define S390_A7_REGNUM 25
-#define S390_A8_REGNUM 26
-#define S390_A9_REGNUM 27
-#define S390_A10_REGNUM 28
-#define S390_A11_REGNUM 29
-#define S390_A12_REGNUM 30
-#define S390_A13_REGNUM 31
-#define S390_A14_REGNUM 32
-#define S390_A15_REGNUM 33
-/* Floating Point Control Word.  */
-#define S390_FPC_REGNUM 34
-/* Floating Point Registers.  */
-#define S390_F0_REGNUM 35
-#define S390_F1_REGNUM 36
-#define S390_F2_REGNUM 37
-#define S390_F3_REGNUM 38
-#define S390_F4_REGNUM 39
-#define S390_F5_REGNUM 40
-#define S390_F6_REGNUM 41
-#define S390_F7_REGNUM 42
-#define S390_F8_REGNUM 43
-#define S390_F9_REGNUM 44
-#define S390_F10_REGNUM 45
-#define S390_F11_REGNUM 46
-#define S390_F12_REGNUM 47
-#define S390_F13_REGNUM 48
-#define S390_F14_REGNUM 49
-#define S390_F15_REGNUM 50
-/* Total.  */
-#define S390_NUM_REGS 51
+/* Total Core Registers. */
+#define S390_NUM_CORE_REGS 18
 
 /* CC optimization */
 
diff --git a/target-s390x/gdbstub.c b/target-s390x/gdbstub.c
index 8d55006..cda8053 100644
--- a/target-s390x/gdbstub.c
+++ b/target-s390x/gdbstub.c
@@ -42,14 +42,7 @@ int s390_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
         return gdb_get_regl(mem_buf, env->psw.addr);
     case S390_R0_REGNUM ... S390_R15_REGNUM:
         return gdb_get_regl(mem_buf, env->regs[n-S390_R0_REGNUM]);
-    case S390_A0_REGNUM ... S390_A15_REGNUM:
-        return gdb_get_reg32(mem_buf, env->aregs[n-S390_A0_REGNUM]);
-    case S390_FPC_REGNUM:
-        return gdb_get_reg32(mem_buf, env->fpc);
-    case S390_F0_REGNUM ... S390_F15_REGNUM:
-        return gdb_get_reg64(mem_buf, env->fregs[n-S390_F0_REGNUM].ll);
     }
-
     return 0;
 }
 
@@ -57,11 +50,7 @@ int s390_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 {
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
-    target_ulong tmpl;
-    uint32_t tmp32;
-    int r = 8;
-    tmpl = ldtul_p(mem_buf);
-    tmp32 = ldl_p(mem_buf);
+    target_ulong tmpl = ldtul_p(mem_buf);
 
     switch (n) {
     case S390_PSWM_REGNUM:
@@ -76,19 +65,79 @@ int s390_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     case S390_R0_REGNUM ... S390_R15_REGNUM:
         env->regs[n-S390_R0_REGNUM] = tmpl;
         break;
+    default:
+        return 0;
+    }
+    return 8;
+}
+
+/* the values represent the positions in s390-acr.xml */
+#define S390_A0_REGNUM 0
+#define S390_A15_REGNUM 15
+/* total number of registers in s390-acr.xml */
+#define S390_NUM_AC_REGS 16
+
+static int cpu_read_ac_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+{
+    switch (n) {
     case S390_A0_REGNUM ... S390_A15_REGNUM:
-        env->aregs[n-S390_A0_REGNUM] = tmp32;
-        r = 4;
-        break;
+        return gdb_get_reg32(mem_buf, env->aregs[n]);
+    default:
+        return 0;
+    }
+}
+
+static int cpu_write_ac_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+{
+    switch (n) {
+    case S390_A0_REGNUM ... S390_A15_REGNUM:
+        env->aregs[n] = ldl_p(mem_buf);
+        return 4;
+    default:
+        return 0;
+    }
+}
+
+/* the values represent the positions in s390-fpr.xml */
+#define S390_FPC_REGNUM 0
+#define S390_F0_REGNUM 1
+#define S390_F15_REGNUM 16
+/* total number of registers in s390-fpr.xml */
+#define S390_NUM_FP_REGS 17
+
+static int cpu_read_fp_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+{
+    switch (n) {
     case S390_FPC_REGNUM:
-        env->fpc = tmp32;
-        r = 4;
-        break;
+        return gdb_get_reg32(mem_buf, env->fpc);
     case S390_F0_REGNUM ... S390_F15_REGNUM:
-        env->fregs[n-S390_F0_REGNUM].ll = tmpl;
-        break;
+        return gdb_get_reg64(mem_buf, env->fregs[n - S390_F0_REGNUM].ll);
     default:
         return 0;
     }
-    return r;
+}
+
+static int cpu_write_fp_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+{
+    switch (n) {
+    case S390_FPC_REGNUM:
+        env->fpc = ldl_p(mem_buf);
+        return 4;
+    case S390_F0_REGNUM ... S390_F15_REGNUM:
+        env->fregs[n - S390_F0_REGNUM].ll = ldtul_p(mem_buf);
+        return 8;
+    default:
+        return 0;
+    }
+}
+
+void s390_cpu_gdb_init(CPUState *cs)
+{
+    gdb_register_coprocessor(cs, cpu_read_ac_reg,
+                             cpu_write_ac_reg,
+                             S390_NUM_AC_REGS, "s390-acr.xml", 0);
+
+    gdb_register_coprocessor(cs, cpu_read_fp_reg,
+                             cpu_write_fp_reg,
+                             S390_NUM_FP_REGS, "s390-fpr.xml", 0);
 }
-- 
1.8.5.5

[Qemu-devel] [PATCH 4/5] s390x/gdb: coding style fixes

[Jens Freimann]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

This patch cleanes up two coding style issues (missing whitespaces).

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 target-s390x/gdbstub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-s390x/gdbstub.c b/target-s390x/gdbstub.c
index cda8053..8945f02 100644
--- a/target-s390x/gdbstub.c
+++ b/target-s390x/gdbstub.c
@@ -41,7 +41,7 @@ int s390_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
     case S390_PSWA_REGNUM:
         return gdb_get_regl(mem_buf, env->psw.addr);
     case S390_R0_REGNUM ... S390_R15_REGNUM:
-        return gdb_get_regl(mem_buf, env->regs[n-S390_R0_REGNUM]);
+        return gdb_get_regl(mem_buf, env->regs[n - S390_R0_REGNUM]);
     }
     return 0;
 }
@@ -63,7 +63,7 @@ int s390_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         env->psw.addr = tmpl;
         break;
     case S390_R0_REGNUM ... S390_R15_REGNUM:
-        env->regs[n-S390_R0_REGNUM] = tmpl;
+        env->regs[n - S390_R0_REGNUM] = tmpl;
         break;
     default:
         return 0;
-- 
1.8.5.5

[Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[Jens Freimann]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

This patch provides the name of the architecture in the target.xml if available.

This allows the remote gdb to detect the target architecture on its own - so
there is no need to specify it manually (e.g. if gdb is started without a
binary) using "set arch *arch_name*".

The name of the architecture has been added to all archs that provide a
target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
name in gdb's feature xml files.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Cc: Andrzej Zaborowski <balrogg@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Vassili Karpov (malc) <av1474@comtv.ru>
---
 gdbstub.c                   | 19 ++++++++++++-------
 include/qom/cpu.h           |  2 ++
 target-arm/cpu64.c          |  1 +
 target-ppc/translate_init.c |  2 ++
 target-s390x/cpu.c          |  1 +
 5 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 8afe0b7..af82259 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -523,13 +523,18 @@ static const char *get_feature_xml(const char *p, const char **newp,
             GDBRegisterState *r;
             CPUState *cpu = first_cpu;
 
-            snprintf(target_xml, sizeof(target_xml),
-                     "<?xml version=\"1.0\"?>"
-                     "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
-                     "<target>"
-                     "<xi:include href=\"%s\"/>",
-                     cc->gdb_core_xml_file);
-
+            pstrcat(target_xml, sizeof(target_xml),
+                    "<?xml version=\"1.0\"?>"
+                    "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
+                    "<target>");
+            if (cc->gdb_arch_name) {
+                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
+                pstrcat(target_xml, sizeof(target_xml), cc->gdb_arch_name);
+                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
+            }
+            pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
+            pstrcat(target_xml, sizeof(target_xml), cc->gdb_core_xml_file);
+            pstrcat(target_xml, sizeof(target_xml), "\"/>");
             for (r = cpu->gdb_regs; r; r = r->next) {
                 pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
                 pstrcat(target_xml, sizeof(target_xml), r->xml);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 1aafbf5..8828b16 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -98,6 +98,7 @@ struct TranslationBlock;
  * @vmsd: State description for migration.
  * @gdb_num_core_regs: Number of core registers accessible to GDB.
  * @gdb_core_xml_file: File name for core registers GDB XML description.
+ * @gdb_arch_name: Architecture name known to GDB.
  *
  * Represents a CPU family or model.
  */
@@ -147,6 +148,7 @@ typedef struct CPUClass {
     const struct VMStateDescription *vmsd;
     int gdb_num_core_regs;
     const char *gdb_core_xml_file;
+    const char *gdb_arch_name;
 } CPUClass;
 
 #ifdef HOST_WORDS_BIGENDIAN
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index 38d2b84..9df7492 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -201,6 +201,7 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_write_register = aarch64_cpu_gdb_write_register;
     cc->gdb_num_core_regs = 34;
     cc->gdb_core_xml_file = "aarch64-core.xml";
+    cc->gdb_arch_name = "aarch64";
 }
 
 static void aarch64_cpu_register(const ARMCPUInfo *info)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 48177ed..7165347 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9649,8 +9649,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 
 #if defined(TARGET_PPC64)
     cc->gdb_core_xml_file = "power64-core.xml";
+    cc->gdb_arch_name = "powerpc:common64";
 #else
     cc->gdb_core_xml_file = "power-core.xml";
+    cc->gdb_arch_name = "powerpc:common";
 #endif
 #ifndef CONFIG_USER_ONLY
     cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 4b03e42..5dae93c 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -262,6 +262,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     dc->vmsd = &vmstate_s390_cpu;
     cc->gdb_num_core_regs = S390_NUM_CORE_REGS;
     cc->gdb_core_xml_file = "s390x-core64.xml";
+    cc->gdb_arch_name = "s390:64-bit";
 }
 
 static const TypeInfo s390_cpu_type_info = {
-- 
1.8.5.5

Re: [Qemu-devel] [PATCH 0/5] s390x/gdb: various fixes

[Christian Borntraeger]

On 29/08/14 15:52, Jens Freimann wrote:
> Conny, Alex, Christian,
> 
> here are some patches improving our gdb support. 
> 
> * Patch 1 fixes a bug where the cc was changed accidentally. 
> * Patch 2 adds the gdb feature XML files for s390x
> * Patch 3 Define acr and fpr registers as coprocessor registers. This allows us
>    to reuse the feature XML files. 
> * Patch 4 whitespace fixes
> * Patch 5 changes common code and other architectures with gdb target.xml support.
>    It adds a field gdb_arch_name to the XML description of the CPU and to struct
>    CPUClass.  It allows the remote gdb to detect the target architecture
>    in cases where it can't tell otherwise.
> 
> David Hildenbrand (5):
>   s390x/gdb: don't touch the cc if tcg is not enabled
>   s390x/gdb: add the feature xml files for s390x
>   s390x/gdb: generate target.xml and handle fp/ac as coprocessors
>   s390x/gdb: coding style fixes
>   gdb: provide the name of the architecture in the target.xml
> 
>  configure                   |   1 +
>  gdb-xml/s390-acr.xml        |  26 +++++++++++
>  gdb-xml/s390-fpr.xml        |  27 +++++++++++
>  gdb-xml/s390x-core64.xml    |  28 ++++++++++++
>  gdbstub.c                   |  19 +++++---
>  include/qom/cpu.h           |   2 +
>  target-arm/cpu64.c          |   1 +
>  target-ppc/translate_init.c |   2 +
>  target-s390x/cpu-qom.h      |   1 +
>  target-s390x/cpu.c          |   5 +-
>  target-s390x/cpu.h          |  40 +---------------
>  target-s390x/gdbstub.c      | 109 +++++++++++++++++++++++++++++++++-----------
>  12 files changed, 188 insertions(+), 73 deletions(-)
>  create mode 100644 gdb-xml/s390-acr.xml
>  create mode 100644 gdb-xml/s390-fpr.xml
>  create mode 100644 gdb-xml/s390x-core64.xml
> 

Applied 1-4.

Peter,
do you want to push patch5 yourself?
As an alternative I can push it via the s390 tree, I need your ACK in that case.

Alex, (or Alexey?) can you ACK/NACK patch 5 from the power perspective? 

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[Peter Maydell]

[ccing Andreas in case he wants to review the QOM aspects of this,
though they're fairly straightforward I think.]

On 29 August 2014 14:52, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>
> This patch provides the name of the architecture in the target.xml if available.
>
> This allows the remote gdb to detect the target architecture on its own - so
> there is no need to specify it manually (e.g. if gdb is started without a
> binary) using "set arch *arch_name*".

This is neat; I didn't realise gdb let you do this.

> The name of the architecture has been added to all archs that provide a
> target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
> name in gdb's feature xml files.

What about 32-bit ARM? You set the architecture name for AArch64
but not the 32 bit case.

Are there architectures that might need to specify something
more complicated than "always the same string"? (ie is there
a case for having the target provide a "return architecture name"
method rather than a constant string?)

> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Cc: Andrzej Zaborowski <balrogg@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Vassili Karpov (malc) <av1474@comtv.ru>
> ---
>  gdbstub.c                   | 19 ++++++++++++-------
>  include/qom/cpu.h           |  2 ++
>  target-arm/cpu64.c          |  1 +
>  target-ppc/translate_init.c |  2 ++
>  target-s390x/cpu.c          |  1 +
>  5 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 8afe0b7..af82259 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -523,13 +523,18 @@ static const char *get_feature_xml(const char *p, const char **newp,
>              GDBRegisterState *r;
>              CPUState *cpu = first_cpu;
>
> -            snprintf(target_xml, sizeof(target_xml),
> -                     "<?xml version=\"1.0\"?>"
> -                     "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
> -                     "<target>"
> -                     "<xi:include href=\"%s\"/>",
> -                     cc->gdb_core_xml_file);
> -
> +            pstrcat(target_xml, sizeof(target_xml),
> +                    "<?xml version=\"1.0\"?>"
> +                    "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
> +                    "<target>");
> +            if (cc->gdb_arch_name) {
> +                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
> +                pstrcat(target_xml, sizeof(target_xml), cc->gdb_arch_name);
> +                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
> +            }
> +            pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
> +            pstrcat(target_xml, sizeof(target_xml), cc->gdb_core_xml_file);
> +            pstrcat(target_xml, sizeof(target_xml), "\"/>");
>              for (r = cpu->gdb_regs; r; r = r->next) {
>                  pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
>                  pstrcat(target_xml, sizeof(target_xml), r->xml);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 1aafbf5..8828b16 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -98,6 +98,7 @@ struct TranslationBlock;
>   * @vmsd: State description for migration.
>   * @gdb_num_core_regs: Number of core registers accessible to GDB.
>   * @gdb_core_xml_file: File name for core registers GDB XML description.
> + * @gdb_arch_name: Architecture name known to GDB.
>   *
>   * Represents a CPU family or model.
>   */
> @@ -147,6 +148,7 @@ typedef struct CPUClass {
>      const struct VMStateDescription *vmsd;
>      int gdb_num_core_regs;
>      const char *gdb_core_xml_file;
> +    const char *gdb_arch_name;
>  } CPUClass;
>
>  #ifdef HOST_WORDS_BIGENDIAN
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index 38d2b84..9df7492 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -201,6 +201,7 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_write_register = aarch64_cpu_gdb_write_register;
>      cc->gdb_num_core_regs = 34;
>      cc->gdb_core_xml_file = "aarch64-core.xml";
> +    cc->gdb_arch_name = "aarch64";
>  }
>
>  static void aarch64_cpu_register(const ARMCPUInfo *info)
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 48177ed..7165347 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9649,8 +9649,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>
>  #if defined(TARGET_PPC64)
>      cc->gdb_core_xml_file = "power64-core.xml";
> +    cc->gdb_arch_name = "powerpc:common64";
>  #else
>      cc->gdb_core_xml_file = "power-core.xml";
> +    cc->gdb_arch_name = "powerpc:common";
>  #endif
>  #ifndef CONFIG_USER_ONLY
>      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 4b03e42..5dae93c 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -262,6 +262,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      dc->vmsd = &vmstate_s390_cpu;
>      cc->gdb_num_core_regs = S390_NUM_CORE_REGS;
>      cc->gdb_core_xml_file = "s390x-core64.xml";
> +    cc->gdb_arch_name = "s390:64-bit";
>  }
>
>  static const TypeInfo s390_cpu_type_info = {
> --
> 1.8.5.5

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[Andreas Färber]

Am 01.09.2014 12:19, schrieb Peter Maydell:
> [ccing Andreas in case he wants to review the QOM aspects of this,
> though they're fairly straightforward I think.]
> 
> On 29 August 2014 14:52, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>
>> This patch provides the name of the architecture in the target.xml if available.
>>
>> This allows the remote gdb to detect the target architecture on its own - so
>> there is no need to specify it manually (e.g. if gdb is started without a
>> binary) using "set arch *arch_name*".
> 
> This is neat; I didn't realise gdb let you do this.
> 
>> The name of the architecture has been added to all archs that provide a
>> target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
>> name in gdb's feature xml files.
> 
> What about 32-bit ARM? You set the architecture name for AArch64
> but not the 32 bit case.
> 
> Are there architectures that might need to specify something
> more complicated than "always the same string"? (ie is there
> a case for having the target provide a "return architecture name"
> method rather than a constant string?)
> 
>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>> Cc: Andrzej Zaborowski <balrogg@gmail.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Vassili Karpov (malc) <av1474@comtv.ru>
>> ---
>>  gdbstub.c                   | 19 ++++++++++++-------
>>  include/qom/cpu.h           |  2 ++
>>  target-arm/cpu64.c          |  1 +
>>  target-ppc/translate_init.c |  2 ++
>>  target-s390x/cpu.c          |  1 +
>>  5 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 8afe0b7..af82259 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -523,13 +523,18 @@ static const char *get_feature_xml(const char *p, const char **newp,
>>              GDBRegisterState *r;
>>              CPUState *cpu = first_cpu;
>>
>> -            snprintf(target_xml, sizeof(target_xml),
>> -                     "<?xml version=\"1.0\"?>"
>> -                     "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
>> -                     "<target>"
>> -                     "<xi:include href=\"%s\"/>",
>> -                     cc->gdb_core_xml_file);
>> -
>> +            pstrcat(target_xml, sizeof(target_xml),
>> +                    "<?xml version=\"1.0\"?>"
>> +                    "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
>> +                    "<target>");
>> +            if (cc->gdb_arch_name) {
>> +                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
>> +                pstrcat(target_xml, sizeof(target_xml), cc->gdb_arch_name);
>> +                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
>> +            }
>> +            pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
>> +            pstrcat(target_xml, sizeof(target_xml), cc->gdb_core_xml_file);
>> +            pstrcat(target_xml, sizeof(target_xml), "\"/>");
>>              for (r = cpu->gdb_regs; r; r = r->next) {
>>                  pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
>>                  pstrcat(target_xml, sizeof(target_xml), r->xml);
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 1aafbf5..8828b16 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -98,6 +98,7 @@ struct TranslationBlock;
>>   * @vmsd: State description for migration.
>>   * @gdb_num_core_regs: Number of core registers accessible to GDB.
>>   * @gdb_core_xml_file: File name for core registers GDB XML description.
>> + * @gdb_arch_name: Architecture name known to GDB.
>>   *
>>   * Represents a CPU family or model.
>>   */
>> @@ -147,6 +148,7 @@ typedef struct CPUClass {
>>      const struct VMStateDescription *vmsd;
>>      int gdb_num_core_regs;
>>      const char *gdb_core_xml_file;
>> +    const char *gdb_arch_name;
>>  } CPUClass;
>>
>>  #ifdef HOST_WORDS_BIGENDIAN
>> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
>> index 38d2b84..9df7492 100644
>> --- a/target-arm/cpu64.c
>> +++ b/target-arm/cpu64.c
>> @@ -201,6 +201,7 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->gdb_write_register = aarch64_cpu_gdb_write_register;
>>      cc->gdb_num_core_regs = 34;
>>      cc->gdb_core_xml_file = "aarch64-core.xml";
>> +    cc->gdb_arch_name = "aarch64";
>>  }
>>
>>  static void aarch64_cpu_register(const ARMCPUInfo *info)
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 48177ed..7165347 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -9649,8 +9649,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>>
>>  #if defined(TARGET_PPC64)
>>      cc->gdb_core_xml_file = "power64-core.xml";
>> +    cc->gdb_arch_name = "powerpc:common64";
>>  #else
>>      cc->gdb_core_xml_file = "power-core.xml";
>> +    cc->gdb_arch_name = "powerpc:common";
>>  #endif
>>  #ifndef CONFIG_USER_ONLY
>>      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 4b03e42..5dae93c 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -262,6 +262,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>      dc->vmsd = &vmstate_s390_cpu;
>>      cc->gdb_num_core_regs = S390_NUM_CORE_REGS;
>>      cc->gdb_core_xml_file = "s390x-core64.xml";
>> +    cc->gdb_arch_name = "s390:64-bit";
>>  }
>>
>>  static const TypeInfo s390_cpu_type_info = {
>> --
>> 1.8.5.5

Looks good to me and even got documented,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[Christian Borntraeger]

On 01/09/14 12:19, Peter Maydell wrote:
> [ccing Andreas in case he wants to review the QOM aspects of this,
> though they're fairly straightforward I think.]
> 
> On 29 August 2014 14:52, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>
>> This patch provides the name of the architecture in the target.xml if available.
>>
>> This allows the remote gdb to detect the target architecture on its own - so
>> there is no need to specify it manually (e.g. if gdb is started without a
>> binary) using "set arch *arch_name*".
> 
> This is neat; I didn't realise gdb let you do this.
> 
>> The name of the architecture has been added to all archs that provide a
>> target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
>> name in gdb's feature xml files.
> 
> What about 32-bit ARM? You set the architecture name for AArch64
> but not the 32 bit case.
> 
> Are there architectures that might need to specify something
> more complicated than "always the same string"? (ie is there
> a case for having the target provide a "return architecture name"
> method rather than a constant string?)

I dont know and David is on vacation.
Would it make sense to do the other architectures as addon patches or shall we wait with this patch until we know what is necessary for all supported platforms?


> 
>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>> Cc: Andrzej Zaborowski <balrogg@gmail.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Vassili Karpov (malc) <av1474@comtv.ru>
>> ---
>>  gdbstub.c                   | 19 ++++++++++++-------
>>  include/qom/cpu.h           |  2 ++
>>  target-arm/cpu64.c          |  1 +
>>  target-ppc/translate_init.c |  2 ++
>>  target-s390x/cpu.c          |  1 +
>>  5 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 8afe0b7..af82259 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -523,13 +523,18 @@ static const char *get_feature_xml(const char *p, const char **newp,
>>              GDBRegisterState *r;
>>              CPUState *cpu = first_cpu;
>>
>> -            snprintf(target_xml, sizeof(target_xml),
>> -                     "<?xml version=\"1.0\"?>"
>> -                     "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
>> -                     "<target>"
>> -                     "<xi:include href=\"%s\"/>",
>> -                     cc->gdb_core_xml_file);
>> -
>> +            pstrcat(target_xml, sizeof(target_xml),
>> +                    "<?xml version=\"1.0\"?>"
>> +                    "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
>> +                    "<target>");
>> +            if (cc->gdb_arch_name) {
>> +                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
>> +                pstrcat(target_xml, sizeof(target_xml), cc->gdb_arch_name);
>> +                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
>> +            }
>> +            pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
>> +            pstrcat(target_xml, sizeof(target_xml), cc->gdb_core_xml_file);
>> +            pstrcat(target_xml, sizeof(target_xml), "\"/>");
>>              for (r = cpu->gdb_regs; r; r = r->next) {
>>                  pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
>>                  pstrcat(target_xml, sizeof(target_xml), r->xml);
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 1aafbf5..8828b16 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -98,6 +98,7 @@ struct TranslationBlock;
>>   * @vmsd: State description for migration.
>>   * @gdb_num_core_regs: Number of core registers accessible to GDB.
>>   * @gdb_core_xml_file: File name for core registers GDB XML description.
>> + * @gdb_arch_name: Architecture name known to GDB.
>>   *
>>   * Represents a CPU family or model.
>>   */
>> @@ -147,6 +148,7 @@ typedef struct CPUClass {
>>      const struct VMStateDescription *vmsd;
>>      int gdb_num_core_regs;
>>      const char *gdb_core_xml_file;
>> +    const char *gdb_arch_name;
>>  } CPUClass;
>>
>>  #ifdef HOST_WORDS_BIGENDIAN
>> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
>> index 38d2b84..9df7492 100644
>> --- a/target-arm/cpu64.c
>> +++ b/target-arm/cpu64.c
>> @@ -201,6 +201,7 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->gdb_write_register = aarch64_cpu_gdb_write_register;
>>      cc->gdb_num_core_regs = 34;
>>      cc->gdb_core_xml_file = "aarch64-core.xml";
>> +    cc->gdb_arch_name = "aarch64";
>>  }
>>
>>  static void aarch64_cpu_register(const ARMCPUInfo *info)
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 48177ed..7165347 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -9649,8 +9649,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>>
>>  #if defined(TARGET_PPC64)
>>      cc->gdb_core_xml_file = "power64-core.xml";
>> +    cc->gdb_arch_name = "powerpc:common64";
>>  #else
>>      cc->gdb_core_xml_file = "power-core.xml";
>> +    cc->gdb_arch_name = "powerpc:common";
>>  #endif
>>  #ifndef CONFIG_USER_ONLY
>>      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 4b03e42..5dae93c 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -262,6 +262,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>      dc->vmsd = &vmstate_s390_cpu;
>>      cc->gdb_num_core_regs = S390_NUM_CORE_REGS;
>>      cc->gdb_core_xml_file = "s390x-core64.xml";
>> +    cc->gdb_arch_name = "s390:64-bit";
>>  }
>>
>>  static const TypeInfo s390_cpu_type_info = {
>> --
>> 1.8.5.5
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[Peter Maydell]

On 1 September 2014 11:31, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> On 01/09/14 12:19, Peter Maydell wrote:
>> [ccing Andreas in case he wants to review the QOM aspects of this,
>> though they're fairly straightforward I think.]
>> On 29 August 2014 14:52, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
>>> The name of the architecture has been added to all archs that provide a
>>> target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
>>> name in gdb's feature xml files.
>>
>> What about 32-bit ARM? You set the architecture name for AArch64
>> but not the 32 bit case.
>>
>> Are there architectures that might need to specify something
>> more complicated than "always the same string"? (ie is there
>> a case for having the target provide a "return architecture name"
>> method rather than a constant string?)
>
> I dont know and David is on vacation.
> Would it make sense to do the other architectures as addon patches
> or shall we wait with this patch until we know what is necessary for
> all supported platforms?

We don't have to implement all the architectures at once, but
I would prefer it if we can get the QOM API for it right from
the start rather than having to change it (and all the targets
which implemented it the old way) later.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/5] s390x/gdb: don't touch the cc if tcg is not enabled

[Alexander Graf]

On 29.08.14 15:52, Jens Freimann wrote:
> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> 
> When reading/writing the psw mask, the condition code may only be touched if
> running on tcg.

Why? Shouldn't we be able to set CC from gdb as well?


Alex

Re: [Qemu-devel] [PATCH 0/5] s390x/gdb: various fixes

[Alexander Graf]

On 01.09.14 12:06, Christian Borntraeger wrote:
> On 29/08/14 15:52, Jens Freimann wrote:
>> Conny, Alex, Christian,
>>
>> here are some patches improving our gdb support. 
>>
>> * Patch 1 fixes a bug where the cc was changed accidentally. 
>> * Patch 2 adds the gdb feature XML files for s390x
>> * Patch 3 Define acr and fpr registers as coprocessor registers. This allows us
>>    to reuse the feature XML files. 
>> * Patch 4 whitespace fixes
>> * Patch 5 changes common code and other architectures with gdb target.xml support.
>>    It adds a field gdb_arch_name to the XML description of the CPU and to struct
>>    CPUClass.  It allows the remote gdb to detect the target architecture
>>    in cases where it can't tell otherwise.
>>
>> David Hildenbrand (5):
>>   s390x/gdb: don't touch the cc if tcg is not enabled
>>   s390x/gdb: add the feature xml files for s390x
>>   s390x/gdb: generate target.xml and handle fp/ac as coprocessors
>>   s390x/gdb: coding style fixes
>>   gdb: provide the name of the architecture in the target.xml
>>
>>  configure                   |   1 +
>>  gdb-xml/s390-acr.xml        |  26 +++++++++++
>>  gdb-xml/s390-fpr.xml        |  27 +++++++++++
>>  gdb-xml/s390x-core64.xml    |  28 ++++++++++++
>>  gdbstub.c                   |  19 +++++---
>>  include/qom/cpu.h           |   2 +
>>  target-arm/cpu64.c          |   1 +
>>  target-ppc/translate_init.c |   2 +
>>  target-s390x/cpu-qom.h      |   1 +
>>  target-s390x/cpu.c          |   5 +-
>>  target-s390x/cpu.h          |  40 +---------------
>>  target-s390x/gdbstub.c      | 109 +++++++++++++++++++++++++++++++++-----------
>>  12 files changed, 188 insertions(+), 73 deletions(-)
>>  create mode 100644 gdb-xml/s390-acr.xml
>>  create mode 100644 gdb-xml/s390-fpr.xml
>>  create mode 100644 gdb-xml/s390x-core64.xml
>>
> 
> Applied 1-4.
> 
> Peter,
> do you want to push patch5 yourself?
> As an alternative I can push it via the s390 tree, I need your ACK in that case.
> 
> Alex, (or Alexey?) can you ACK/NACK patch 5 from the power perspective? 

The ppc side looks reasonable I think. But I guess Richard is the person
to really ask here - FWIW he should know his way around gdb the best.


Alex

Re: [Qemu-devel] [PATCH 1/5] s390x/gdb: don't touch the cc if tcg is not enabled

[Christian Borntraeger]

On 02/09/14 00:39, Alexander Graf wrote:
> 
> 
> On 29.08.14 15:52, Jens Freimann wrote:
>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>
>> When reading/writing the psw mask, the condition code may only be touched if
>> running on tcg.
> 
> Why? Shouldn't we be able to set CC from gdb as well?
> 

You can. What this patch does (and the patch description is a bit vague here) is to not modify the PSW it gets from KVM when passing it to gdb:
The qemu core gets the PSW from KVM. Without this patch, we use cc_op to calculate the current CC of the PSW (No idea of TCG, I guess its evaluated lazy - at least this is how valgrind works). This is wrong for the KVM case, as cc_op does not contain any useful data for the KVM case. With this patch we simply pass the psw from KVM to gdb and back.

The symptom was that the cc was always shown as zero.

Christian

Re: [Qemu-devel] [PATCH 1/5] s390x/gdb: don't touch the cc if tcg is not enabled

[Alexander Graf]

On 02.09.14 09:07, Christian Borntraeger wrote:
> On 02/09/14 00:39, Alexander Graf wrote:
>>
>>
>> On 29.08.14 15:52, Jens Freimann wrote:
>>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>>
>>> When reading/writing the psw mask, the condition code may only be touched if
>>> running on tcg.
>>
>> Why? Shouldn't we be able to set CC from gdb as well?
>>
> 
> You can. What this patch does (and the patch description is a bit vague here) is to not modify the PSW it gets from KVM when passing it to gdb:
> The qemu core gets the PSW from KVM. Without this patch, we use cc_op to calculate the current CC of the PSW (No idea of TCG, I guess its evaluated lazy - at least this is how valgrind works). This is wrong for the KVM case, as cc_op does not contain any useful data for the KVM case. With this patch we simply pass the psw from KVM to gdb and back.
> 
> The symptom was that the cc was always shown as zero.

Ah, I see. I agree with the patch, but the patch description does not
actually describe what the patch does. Please rework it.

I also wouldn't mind if instead of hard coding this logic in the
gdbstub, we'd extract it as helper functions to read and write the
PSW.MASK in cpu.h.


Alex

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[David Hildenbrand]

> [ccing Andreas in case he wants to review the QOM aspects of this,
> though they're fairly straightforward I think.]
> 
> On 29 August 2014 14:52, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
> > From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >
> > This patch provides the name of the architecture in the target.xml if available.
> >
> > This allows the remote gdb to detect the target architecture on its own - so
> > there is no need to specify it manually (e.g. if gdb is started without a
> > binary) using "set arch *arch_name*".
> 
> This is neat; I didn't realise gdb let you do this.
> 
> > The name of the architecture has been added to all archs that provide a
> > target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
> > name in gdb's feature xml files.
> 
> What about 32-bit ARM? You set the architecture name for AArch64
> but not the 32 bit case.
> 

Well, my point was to not break anything :)

On my way through the possible architecture names
(binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right name
for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
adapts to the xml files from gdb.

The architecture should be known at the same point when specifying the xml file.
So if anyone can come up with the proper arm name in the future (or even some
kind of detection algorithm), it can simply be set in target-arm/cpu.c (after
"arm-core.xml").

David

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[Edgar E. Iglesias]

On Wed, Sep 03, 2014 at 11:37:24AM +0200, David Hildenbrand wrote:
> > [ccing Andreas in case he wants to review the QOM aspects of this,
> > though they're fairly straightforward I think.]
> > 
> > On 29 August 2014 14:52, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
> > > From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > >
> > > This patch provides the name of the architecture in the target.xml if available.
> > >
> > > This allows the remote gdb to detect the target architecture on its own - so
> > > there is no need to specify it manually (e.g. if gdb is started without a
> > > binary) using "set arch *arch_name*".
> > 
> > This is neat; I didn't realise gdb let you do this.
> > 
> > > The name of the architecture has been added to all archs that provide a
> > > target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
> > > name in gdb's feature xml files.
> > 
> > What about 32-bit ARM? You set the architecture name for AArch64
> > but not the 32 bit case.
> > 
> 
> Well, my point was to not break anything :)
> 
> On my way through the possible architecture names
> (binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right name
> for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
> adapts to the xml files from gdb.
> 
> The architecture should be known at the same point when specifying the xml file.
> So if anyone can come up with the proper arm name in the future (or even some
> kind of detection algorithm), it can simply be set in target-arm/cpu.c (after
> "arm-core.xml").

Hi,

I've got some similar patches in my tree. I used the following:


commit 26932a453da466d111b67c37b93dec71fb3ae111
Author: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Date:   Wed Aug 20 19:22:10 2014 +1000

    gdbstub: Emit the CPUs GDB architecture if available
    
    Allows GDB to autodetect the architecture.
    
    Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

diff --git a/gdbstub.c b/gdbstub.c
index 7f82186..5b62c50 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -604,6 +604,11 @@ static const char *get_feature_xml(const char *p, const char **newp,
                 pstrcat(target_xml, sizeof(target_xml), r->xml);
                 pstrcat(target_xml, sizeof(target_xml), "\"/>");
             }
+            if (cc->gdb_arch) {
+                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
+                pstrcat(target_xml, sizeof(target_xml), cc->gdb_arch);
+                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
+            }
             pstrcat(target_xml, sizeof(target_xml), "</target>");
         }
         return target_xml;

commit a0b166c59491b154c05b963649f561c1e48aa706
Author: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Date:   Wed Aug 20 19:21:28 2014 +1000

    target-arm: Provide GDB arch names
    
    Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index b726b6a..d1704f5 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1163,6 +1163,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->get_phys_page_debug = arm_cpu_get_phys_page_debug;
     cc->vmsd = &vmstate_arm_cpu;
 #endif
+    cc->gdb_arch = "arm";
     cc->gdb_num_core_regs = 26;
     cc->gdb_core_xml_file = "arm-core.xml";
 }
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index fc9c991..c099d13 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -215,6 +215,7 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_write_register = aarch64_cpu_gdb_write_register;
     cc->gdb_num_core_regs = 34;
     cc->gdb_core_xml_file = "aarch64-core.xml";
+    cc->gdb_arch = "aarch64";
 }
 
 static void aarch64_cpu_register(const ARMCPUInfo *info)

commit 0aca15f7829e6e8a94639ddd75bcfdfd3b034d2e
Author: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Date:   Wed Aug 20 19:20:35 2014 +1000

    qom/cpu: Add a gdb_arch field to the CPUClass
    
    Used to optionally set the GDB architecture of the CPU.
    
    Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 4d0908b..46b72e7 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -147,6 +147,7 @@ typedef struct CPUClass {
                                 void *opaque);
 
     const struct VMStateDescription *vmsd;
+    const char *gdb_arch;
     int gdb_num_core_regs;
     const char *gdb_core_xml_file;
 } CPUClass;

commit eaa8bac08e300c9516d04c3425c3794a1bd893b8
Author: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Date:   Wed Aug 20 11:26:52 2014 +1000

    configure: gdbstub: Include ARM xml descritions in AArch64 builds
    
    Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

diff --git a/configure b/configure
index fce5801..3a95948 100755
--- a/configure
+++ b/configure
@@ -4976,7 +4976,9 @@ case "$target_name" in
   aarch64)
     TARGET_BASE_ARCH=arm
     bflt="yes"
-    gdb_xml_files="aarch64-core.xml aarch64-fpu.xml"
+    arm_gdb_xml_files="arm-core.xml arm-vfp.xml arm-vfp3.xml arm-neon.xml"
+    gdb_xml_files="aarch64-core.xml aarch64-fpu.xml $arm_gdb_xml_files"
+    echo $gdb_xml_files
   ;;
   cris)
   ;;

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[David Hildenbrand]

> On Wed, Sep 03, 2014 at 11:37:24AM +0200, David Hildenbrand wrote:
> > > [ccing Andreas in case he wants to review the QOM aspects of this,
> > > though they're fairly straightforward I think.]
> > > 
> > > On 29 August 2014 14:52, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
> > > > From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > > >
> > > > This patch provides the name of the architecture in the target.xml if available.
> > > >
> > > > This allows the remote gdb to detect the target architecture on its own - so
> > > > there is no need to specify it manually (e.g. if gdb is started without a
> > > > binary) using "set arch *arch_name*".
> > > 
> > > This is neat; I didn't realise gdb let you do this.
> > > 
> > > > The name of the architecture has been added to all archs that provide a
> > > > target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
> > > > name in gdb's feature xml files.
> > > 
> > > What about 32-bit ARM? You set the architecture name for AArch64
> > > but not the 32 bit case.
> > > 
> > 
> > Well, my point was to not break anything :)
> > 
> > On my way through the possible architecture names
> > (binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right name
> > for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
> > adapts to the xml files from gdb.
> > 
> > The architecture should be known at the same point when specifying the xml file.
> > So if anyone can come up with the proper arm name in the future (or even some
> > kind of detection algorithm), it can simply be set in target-arm/cpu.c (after
> > "arm-core.xml").
> 
> Hi,
> 
> I've got some similar patches in my tree. I used the following:
> 

Thanks! So "arm" seems to be the proper name for arm32, right?

> 
> commit 26932a453da466d111b67c37b93dec71fb3ae111
> Author: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Date:   Wed Aug 20 19:22:10 2014 +1000
> 
>     gdbstub: Emit the CPUs GDB architecture if available
>     
>     Allows GDB to autodetect the architecture.
>     
>     Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 7f82186..5b62c50 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -604,6 +604,11 @@ static const char *get_feature_xml(const char *p, const char **newp,
>                  pstrcat(target_xml, sizeof(target_xml), r->xml);
>                  pstrcat(target_xml, sizeof(target_xml), "\"/>");
>              }
> +            if (cc->gdb_arch) {
> +                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
> +                pstrcat(target_xml, sizeof(target_xml), cc->gdb_arch);
> +                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
> +            }

Please not that "gdb-target.dtd" specifies the architecture to come directly at
the beginning of the target "section".

Putting it after the xml-includes, to the end of the target section makes the
whole XML failing to be recognized on my tests with s390x.

David

>              pstrcat(target_xml, sizeof(target_xml), "</target>");
>          }
>          return target_xml;
> 

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[Edgar E. Iglesias]

On Wed, Sep 03, 2014 at 11:59:45AM +0200, David Hildenbrand wrote:
> > On Wed, Sep 03, 2014 at 11:37:24AM +0200, David Hildenbrand wrote:
> > > > [ccing Andreas in case he wants to review the QOM aspects of this,
> > > > though they're fairly straightforward I think.]
> > > > 
> > > > On 29 August 2014 14:52, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
> > > > > From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > > > >
> > > > > This patch provides the name of the architecture in the target.xml if available.
> > > > >
> > > > > This allows the remote gdb to detect the target architecture on its own - so
> > > > > there is no need to specify it manually (e.g. if gdb is started without a
> > > > > binary) using "set arch *arch_name*".
> > > > 
> > > > This is neat; I didn't realise gdb let you do this.
> > > > 
> > > > > The name of the architecture has been added to all archs that provide a
> > > > > target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
> > > > > name in gdb's feature xml files.
> > > > 
> > > > What about 32-bit ARM? You set the architecture name for AArch64
> > > > but not the 32 bit case.
> > > > 
> > > 
> > > Well, my point was to not break anything :)
> > > 
> > > On my way through the possible architecture names
> > > (binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right name
> > > for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
> > > adapts to the xml files from gdb.
> > > 
> > > The architecture should be known at the same point when specifying the xml file.
> > > So if anyone can come up with the proper arm name in the future (or even some
> > > kind of detection algorithm), it can simply be set in target-arm/cpu.c (after
> > > "arm-core.xml").
> > 
> > Hi,
> > 
> > I've got some similar patches in my tree. I used the following:
> > 
> 
> Thanks! So "arm" seems to be the proper name for arm32, right?
> 
> > 
> > commit 26932a453da466d111b67c37b93dec71fb3ae111
> > Author: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Date:   Wed Aug 20 19:22:10 2014 +1000
> > 
> >     gdbstub: Emit the CPUs GDB architecture if available
> >     
> >     Allows GDB to autodetect the architecture.
> >     
> >     Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > 
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 7f82186..5b62c50 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -604,6 +604,11 @@ static const char *get_feature_xml(const char *p, const char **newp,
> >                  pstrcat(target_xml, sizeof(target_xml), r->xml);
> >                  pstrcat(target_xml, sizeof(target_xml), "\"/>");
> >              }
> > +            if (cc->gdb_arch) {
> > +                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
> > +                pstrcat(target_xml, sizeof(target_xml), cc->gdb_arch);
> > +                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
> > +            }
> 
> Please not that "gdb-target.dtd" specifies the architecture to come directly at
> the beginning of the target "section".
> 
> Putting it after the xml-includes, to the end of the target section makes the
> whole XML failing to be recognized on my tests with s390x.

Hmm, interesting. It worked here with a multi-arch gdb.

Cheers,
Edgar

> 
> David
> 
> >              pstrcat(target_xml, sizeof(target_xml), "</target>");
> >          }
> >          return target_xml;
> > 
> 

Re: [Qemu-devel] [PATCH 1/5] s390x/gdb: don't touch the cc if tcg is not enabled

[Christian Borntraeger]

On 03/09/14 11:27, Alexander Graf wrote:
> 
> 
> On 02.09.14 09:07, Christian Borntraeger wrote:
>> On 02/09/14 00:39, Alexander Graf wrote:
>>>
>>>
>>> On 29.08.14 15:52, Jens Freimann wrote:
>>>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>>>
>>>> When reading/writing the psw mask, the condition code may only be touched if
>>>> running on tcg.
>>>
>>> Why? Shouldn't we be able to set CC from gdb as well?
>>>
>>
>> You can. What this patch does (and the patch description is a bit vague here) is to not modify the PSW it gets from KVM when passing it to gdb:
>> The qemu core gets the PSW from KVM. Without this patch, we use cc_op to calculate the current CC of the PSW (No idea of TCG, I guess its evaluated lazy - at least this is how valgrind works). This is wrong for the KVM case, as cc_op does not contain any useful data for the KVM case. With this patch we simply pass the psw from KVM to gdb and back.
>>
>> The symptom was that the cc was always shown as zero.
> 
> Ah, I see. I agree with the patch, but the patch description does not
> actually describe what the patch does. Please rework it.

The patch was already pulled...Well the description is misleaded but with some interpretion still correct ;-).
But your point is well taken. We will try to make sure that all patch description are descriptive and not ambiguous. Your question is a good indication that this was not the case in this patch.

Christian

 
> I also wouldn't mind if instead of hard coding this logic in the
> gdbstub, we'd extract it as helper functions to read and write the
> PSW.MASK in cpu.h.