[Qemu-arm] [PATCH v3 0/4] target/arm: Add a dynamic XML-description of the cp-registers to GDB

[Qemu-arm] [PATCH v3 0/4] target/arm: Add a dynamic XML-description of the cp-registers to GDB

[Abdallah Bouassida]

The last exchange:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg03618.html

- Add a new "ARM_CP_NO_GDB" bit field and enable it when creating CP_ANY
  wildcard aliases.
- Add "_S" suffix to the secure version of a sysreg and fix the reg names that
  were manually containing (S) or (NS).
- Add the XML dynamic generation and add a callback to read these sysregs.
- Add a callback to set these sysregs (I put this as a separate patch as
  we still discussing if we'll give a write access for these sysregs from GDB).

@Peter: Intresting advises from your last review, Thanks a lot ;)


Abdallah Bouassida (4):
  target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo
    type
  target/arm: Add "_S" suffix to the secure version of a sysreg
  target/arm: Add the XML dynamic generation
  target/arm: Add arm_gdb_set_sysreg() callback

 gdbstub.c            |   7 ++++
 include/qom/cpu.h    |   9 +++-
 target/arm/cpu.c     |   3 ++
 target/arm/cpu.h     |  22 +++++++++-
 target/arm/gdbstub.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++
 target/arm/helper.c  |  35 ++++++++++------
 6 files changed, 177 insertions(+), 15 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/4] target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo type

[Abdallah Bouassida]

This is a preparation for the coming feature of creating dynamically an XML
description for the ARM sysregs.
A register has ARM_CP_NO_GDB enabled will not be shown in the dynamic XML.
This bit is enabled automatically when creating CP_ANY wildcard aliases.
This bit could be enabled manually for any register we want to remove from the
dynamic XML description.

Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
---
 target/arm/cpu.h    | 3 ++-
 target/arm/helper.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8c839fa..92cfe4c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1776,10 +1776,11 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
 #define ARM_LAST_SPECIAL         ARM_CP_DC_ZVA
 #define ARM_CP_FPU               0x1000
 #define ARM_CP_SVE               0x2000
+#define ARM_CP_NO_GDB            0x4000
 /* Used only as a terminator for ARMCPRegInfo lists */
 #define ARM_CP_SENTINEL          0xffff
 /* Mask of only the flag bits in a type field */
-#define ARM_CP_FLAG_MASK         0x30ff
+#define ARM_CP_FLAG_MASK         0x70ff
 
 /* Valid values for ARMCPRegInfo state field, indicating which of
  * the AArch32 and AArch64 execution states this register is visible in.
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c5bc69b..bdd212f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5663,7 +5663,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
     if (((r->crm == CP_ANY) && crm != 0) ||
         ((r->opc1 == CP_ANY) && opc1 != 0) ||
         ((r->opc2 == CP_ANY) && opc2 != 0)) {
-        r2->type |= ARM_CP_ALIAS;
+        r2->type |= ARM_CP_ALIAS | ARM_CP_NO_GDB;
     }
 
     /* Check that raw accesses are either forbidden or handled. Note that
-- 
2.7.4

[Qemu-arm] [PATCH v3 2/4] target/arm: Add "_S" suffix to the secure version of a sysreg

[Abdallah Bouassida]

This is a preparation for the coming feature of creating dynamically an XML
description for the ARM sysregs.
Add "_S" suffix to the secure version of sysregs that have both S and NS views
Replace (S) and (NS) by _S and _NS for the register that are manually defined,
so all the registers follow the same convention.

Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
---
 target/arm/helper.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index bdd212f..1594ec45 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -694,12 +694,12 @@ static const ARMCPRegInfo cp_reginfo[] = {
      * the secure register to be properly reset and migrated. There is also no
      * v8 EL1 version of the register so the non-secure instance stands alone.
      */
-    { .name = "FCSEIDR(NS)",
+    { .name = "FCSEIDR_NS",
       .cp = 15, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 0,
       .access = PL1_RW, .secure = ARM_CP_SECSTATE_NS,
       .fieldoffset = offsetof(CPUARMState, cp15.fcseidr_ns),
       .resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
-    { .name = "FCSEIDR(S)",
+    { .name = "FCSEIDR_S",
       .cp = 15, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 0,
       .access = PL1_RW, .secure = ARM_CP_SECSTATE_S,
       .fieldoffset = offsetof(CPUARMState, cp15.fcseidr_s),
@@ -715,7 +715,7 @@ static const ARMCPRegInfo cp_reginfo[] = {
       .access = PL1_RW, .secure = ARM_CP_SECSTATE_NS,
       .fieldoffset = offsetof(CPUARMState, cp15.contextidr_el[1]),
       .resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write, },
-    { .name = "CONTEXTIDR(S)", .state = ARM_CP_STATE_AA32,
+    { .name = "CONTEXTIDR_S", .state = ARM_CP_STATE_AA32,
       .cp = 15, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 1,
       .access = PL1_RW, .secure = ARM_CP_SECSTATE_S,
       .fieldoffset = offsetof(CPUARMState, cp15.contextidr_s),
@@ -1966,7 +1966,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
                                    cp15.c14_timer[GTIMER_PHYS].ctl),
       .writefn = gt_phys_ctl_write, .raw_writefn = raw_write,
     },
-    { .name = "CNTP_CTL(S)",
+    { .name = "CNTP_CTL_S",
       .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 1,
       .secure = ARM_CP_SECSTATE_S,
       .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_R,
@@ -2005,7 +2005,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .accessfn = gt_ptimer_access,
       .readfn = gt_phys_tval_read, .writefn = gt_phys_tval_write,
     },
-    { .name = "CNTP_TVAL(S)",
+    { .name = "CNTP_TVAL_S",
       .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0,
       .secure = ARM_CP_SECSTATE_S,
       .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
@@ -2059,7 +2059,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .accessfn = gt_ptimer_access,
       .writefn = gt_phys_cval_write, .raw_writefn = raw_write,
     },
-    { .name = "CNTP_CVAL(S)", .cp = 15, .crm = 14, .opc1 = 2,
+    { .name = "CNTP_CVAL_S", .cp = 15, .crm = 14, .opc1 = 2,
       .secure = ARM_CP_SECSTATE_S,
       .access = PL1_RW | PL0_R,
       .type = ARM_CP_64BIT | ARM_CP_IO | ARM_CP_ALIAS,
@@ -5562,7 +5562,8 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 
 static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
                                    void *opaque, int state, int secstate,
-                                   int crm, int opc1, int opc2)
+                                   int crm, int opc1, int opc2,
+                                   const char *name)
 {
     /* Private utility function for define_one_arm_cp_reg_with_opaque():
      * add a single reginfo struct to the hash table.
@@ -5572,6 +5573,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
     int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
     int ns = (secstate & ARM_CP_SECSTATE_NS) ? 1 : 0;
 
+    r2->name = name;
     /* Reset the secure state to the specific incoming state.  This is
      * necessary as the register may have been defined with both states.
      */
@@ -5803,19 +5805,26 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
                         /* Under AArch32 CP registers can be common
                          * (same for secure and non-secure world) or banked.
                          */
+                        const char *name;
+                        GString *s;
+
                         switch (r->secure) {
                         case ARM_CP_SECSTATE_S:
                         case ARM_CP_SECSTATE_NS:
                             add_cpreg_to_hashtable(cpu, r, opaque, state,
-                                                   r->secure, crm, opc1, opc2);
+                                                   r->secure, crm, opc1, opc2,
+                                                   r->name);
                             break;
                         default:
+                            s = g_string_new(r->name);
+                            g_string_append_printf(s, "_S");
+                            name = g_string_free(s, false);
                             add_cpreg_to_hashtable(cpu, r, opaque, state,
                                                    ARM_CP_SECSTATE_S,
-                                                   crm, opc1, opc2);
+                                                   crm, opc1, opc2, name);
                             add_cpreg_to_hashtable(cpu, r, opaque, state,
                                                    ARM_CP_SECSTATE_NS,
-                                                   crm, opc1, opc2);
+                                                   crm, opc1, opc2, r->name);
                             break;
                         }
                     } else {
@@ -5823,7 +5832,7 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
                          * of AArch32 */
                         add_cpreg_to_hashtable(cpu, r, opaque, state,
                                                ARM_CP_SECSTATE_NS,
-                                               crm, opc1, opc2);
+                                               crm, opc1, opc2, r->name);
                     }
                 }
             }
-- 
2.7.4

[Qemu-devel] [PATCH v3 3/4] target/arm: Add the XML dynamic generation

[Abdallah Bouassida]

Generate an XML description for the cp-regs.
Register these regs with the gdb_register_coprocessor().
Add arm_gdb_get_sysreg() to use it as a callback to read those regs.

Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
---
 gdbstub.c            |  7 ++++
 include/qom/cpu.h    |  9 ++++-
 target/arm/cpu.c     |  3 ++
 target/arm/cpu.h     | 17 +++++++++
 target/arm/gdbstub.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 132 insertions(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index f1d5148..ffab30b 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -665,6 +665,9 @@ static const char *get_feature_xml(const char *p, const char **newp,
             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), "\"/>");
+            if (cc->gdb_has_dynamic_xml) {
+                cc->register_gdb_regs_for_features(cpu);
+            }
             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);
@@ -674,6 +677,10 @@ static const char *get_feature_xml(const char *p, const char **newp,
         }
         return target_xml;
     }
+    if (strncmp(p, "system-registers.xml", len) == 0) {
+        CPUState *cpu = first_cpu;
+        return cc->gdb_get_dynamic_xml(cpu);
+    }
     for (i = 0; ; i++) {
         name = xml_builtin[i][0];
         if (!name || (strncmp(name, p, len) == 0 && strlen(name) == len))
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index aff88fa..04771b3 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -131,6 +131,11 @@ struct TranslationBlock;
  *           before the insn which triggers a watchpoint rather than after it.
  * @gdb_arch_name: Optional callback that returns the architecture name known
  * to GDB. The caller must free the returned string with g_free.
+ * @gdb_has_dynamic_xml: Indicates if GDB supports generating a dynamic XML
+ *                   description for the sysregs of this CPU.
+ * @register_gdb_regs_for_features: Callback for letting GDB create a dynamic
+ *       XML description for the sysregs and register those sysregs afterwards.
+ * @gdb_get_dynamic_xml: Callback for letting GDB get the dynamic XML.
  * @cpu_exec_enter: Callback for cpu_exec preparation.
  * @cpu_exec_exit: Callback for cpu_exec cleanup.
  * @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec.
@@ -197,7 +202,9 @@ typedef struct CPUClass {
     const struct VMStateDescription *vmsd;
     const char *gdb_core_xml_file;
     gchar * (*gdb_arch_name)(CPUState *cpu);
-
+    bool gdb_has_dynamic_xml;
+    void (*register_gdb_regs_for_features)(CPUState *cpu);
+    char *(*gdb_get_dynamic_xml)(CPUState *cpu);
     void (*cpu_exec_enter)(CPUState *cpu);
     void (*cpu_exec_exit)(CPUState *cpu);
     bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 1b3ae62..04c4d04 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1781,6 +1781,9 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_num_core_regs = 26;
     cc->gdb_core_xml_file = "arm-core.xml";
     cc->gdb_arch_name = arm_gdb_arch_name;
+    cc->gdb_has_dynamic_xml = true;
+    cc->register_gdb_regs_for_features = arm_register_gdb_regs_for_features;
+    cc->gdb_get_dynamic_xml = arm_gdb_get_dynamic_xml;
     cc->gdb_stop_before_watchpoint = true;
     cc->debug_excp_handler = arm_debug_excp_handler;
     cc->debug_check_watchpoint = arm_debug_check_watchpoint;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 92cfe4c..0e35f64 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -133,6 +133,19 @@ enum {
    s<2n+1> maps to the most significant half of d<n>
  */
 
+/**
+ * DynamicGDBXMLInfo:
+ * @desc: Contains the XML descriptions.
+ * @num_cpregs: Number of the Coprocessor registers seen by GDB.
+ * @cpregs_keys: Array that contains the corresponding Key of
+ * a given cpreg with the same order of the cpreg in the XML description.
+ */
+typedef struct DynamicGDBXMLInfo {
+    char *desc;
+    int num_cpregs;
+    uint32_t *cpregs_keys;
+} DynamicGDBXMLInfo;
+
 /* CPU state for each instance of a generic timer (in cp15 c14) */
 typedef struct ARMGenericTimer {
     uint64_t cval; /* Timer CompareValue register */
@@ -671,6 +684,8 @@ struct ARMCPU {
     uint64_t *cpreg_vmstate_values;
     int32_t cpreg_vmstate_array_len;
 
+    DynamicGDBXMLInfo dyn_xml;
+
     /* Timers used by the generic (architected) timer */
     QEMUTimer *gt_timer[NUM_GTIMERS];
     /* GPIO outputs for generic timer */
@@ -835,6 +850,8 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
 
 int arm_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+void arm_register_gdb_regs_for_features(CPUState *cpu);
+char *arm_gdb_get_dynamic_xml(CPUState *cpu);
 
 int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
                              int cpuid, void *opaque);
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 04c1208..e08ad79 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -101,3 +101,100 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     /* Unknown register.  */
     return 0;
 }
+
+static void arm_gen_one_xml_reg_tag(DynamicGDBXMLInfo *dyn_xml,
+                                    ARMCPRegInfo *ri, uint32_t ri_key,
+                                    bool is64)
+{
+    GString *s = g_string_new(dyn_xml->desc);
+
+    g_string_append_printf(s, "<reg name=\"%s\"", ri->name);
+    g_string_append_printf(s, " bitsize=\"%s\"", is64 ? "64" : "32");
+    g_string_append_printf(s, " group=\"cp_regs\"/>");
+    dyn_xml->desc = g_string_free(s, false);
+    dyn_xml->num_cpregs++;
+    dyn_xml->cpregs_keys = g_renew(uint32_t,
+                                       dyn_xml->cpregs_keys,
+                                       dyn_xml->num_cpregs);
+    dyn_xml->cpregs_keys[dyn_xml->num_cpregs - 1] = ri_key;
+}
+
+static void arm_register_sysreg_for_xml(gpointer key, gpointer value,
+                                        gpointer cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    ARMCPRegInfo *ri = value;
+    uint32_t ri_key = *(uint32_t *)key;
+
+    if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_NO_GDB))) {
+        if (env->aarch64) {
+            if (ri->state == ARM_CP_STATE_AA64) {
+                arm_gen_one_xml_reg_tag(&cpu->dyn_xml, ri, ri_key, 1);
+            }
+        } else {
+            if (ri->state == ARM_CP_STATE_AA32) {
+                if (!arm_feature(env, ARM_FEATURE_EL3) &&
+                    (ri->secure & ARM_CP_SECSTATE_S)) {
+                    return;
+                }
+                if (ri->type & ARM_CP_64BIT) {
+                    arm_gen_one_xml_reg_tag(&cpu->dyn_xml, ri, ri_key, 1);
+                } else {
+                    arm_gen_one_xml_reg_tag(&cpu->dyn_xml, ri, ri_key, 0);
+                }
+            }
+        }
+    }
+}
+
+static int arm_gen_dynamic_xml(CPUState *cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    GString *s = g_string_new(NULL);
+
+    cpu->dyn_xml.num_cpregs = 0;
+    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.gnu.gdb.sys.regs\">");
+    cpu->dyn_xml.desc = g_string_free(s, false);
+    g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_xml, cs);
+    s = g_string_new(cpu->dyn_xml.desc);
+    g_string_append_printf(s, "</feature>");
+    cpu->dyn_xml.desc = g_string_free(s, false);
+    return  cpu->dyn_xml.num_cpregs;
+}
+
+static int arm_gdb_get_sysreg(CPUARMState *env, uint8_t *buf, int reg)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    const ARMCPRegInfo *ri;
+    uint32_t key;
+
+    key = cpu->dyn_xml.cpregs_keys[reg];
+    ri = get_arm_cp_reginfo(cpu->cp_regs, key);
+    if (ri) {
+        if (cpreg_field_is_64bit(ri)) {
+            return gdb_get_reg64(buf, (uint64_t)read_raw_cp_reg(env, ri));
+        } else {
+            return gdb_get_reg32(buf, (uint32_t)read_raw_cp_reg(env, ri));
+        }
+    }
+    return 0;
+}
+
+void arm_register_gdb_regs_for_features(CPUState *cs)
+{
+    int n;
+
+    n = arm_gen_dynamic_xml(cs);
+    gdb_register_coprocessor(cs, arm_gdb_get_sysreg, NULL,
+                             n, "system-registers.xml", 0);
+
+}
+
+char *arm_gdb_get_dynamic_xml(CPUState *cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    return cpu->dyn_xml.desc;
+}
-- 
2.7.4

[Qemu-devel] [PATCH v3 4/4] target/arm: Add arm_gdb_set_sysreg() callback

[Abdallah Bouassida]

This is a callback to set the cp-regs registered by the dynamic XML.

Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
---
>> Some of our customers need to connect to Qemu using our tool TRACE32® 
>> via GDB,
>> and for some use case they need to have write access to some particular 
>> cpregs.
>> So, it will be nice to have this capability!
>> Usually, a user won't modify these registers unless he knows what he is 
>> doing!

> I also still don't really like using write_raw_cp_reg() here --
> it will bypass some behaviour you want and in some cases will
> just break the emulation because invariants we assume will
> hold no longer hold. It would be a lot lot safer to not
> provide write access at all, only read access.

Adding to that our customers may need this write access, our tool TRACE32®
needs this also in some particular cases. For example: temporary disabling MMU
to do a physical memory access.

 target/arm/cpu.h     |  2 ++
 target/arm/gdbstub.c | 21 ++++++++++++++++++++-
 target/arm/helper.c  |  2 +-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 0e35f64..f4fea98 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2120,6 +2120,8 @@ static inline bool cp_access_ok(int current_el,
 /* Raw read of a coprocessor register (as needed for migration, etc) */
 uint64_t read_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri);
 
+void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t v);
+
 /**
  * write_list_to_cpustate
  * @cpu: ARMCPU
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index e08ad79..57bd418 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -183,12 +183,31 @@ static int arm_gdb_get_sysreg(CPUARMState *env, uint8_t *buf, int reg)
     return 0;
 }
 
+static int arm_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    const ARMCPRegInfo *ri;
+    uint32_t key;
+    uint32_t tmp;
+
+    tmp = ldl_p(buf);
+    key = cpu->dyn_xml.cpregs_keys[reg];
+    ri = get_arm_cp_reginfo(arm_env_get_cpu(env)->cp_regs, key);
+    if (ri) {
+        if (!(ri->type & ARM_CP_CONST)) {
+            write_raw_cp_reg(env, ri, tmp);
+            return cpreg_field_is_64bit(ri) ? 8 : 4;
+        }
+    }
+    return 0;
+}
+
 void arm_register_gdb_regs_for_features(CPUState *cs)
 {
     int n;
 
     n = arm_gen_dynamic_xml(cs);
-    gdb_register_coprocessor(cs, arm_gdb_get_sysreg, NULL,
+    gdb_register_coprocessor(cs, arm_gdb_get_sysreg, arm_gdb_set_sysreg,
                              n, "system-registers.xml", 0);
 
 }
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1594ec45..4a4afbf 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -200,7 +200,7 @@ uint64_t read_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri)
     }
 }
 
-static void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
+void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t v)
 {
     /* Raw write of a coprocessor register (as needed for migration, etc).
-- 
2.7.4

Re: [Qemu-arm] [PATCH v3 4/4] target/arm: Add arm_gdb_set_sysreg() callback

[Peter Maydell]

On 28 February 2018 at 11:01, Abdallah Bouassida
<abdallah.bouassida@lauterbach.com> wrote:
> This is a callback to set the cp-regs registered by the dynamic XML.
>
> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
> ---
>>> Some of our customers need to connect to Qemu using our tool TRACE32®
>>> via GDB,
>>> and for some use case they need to have write access to some particular
>>> cpregs.
>>> So, it will be nice to have this capability!
>>> Usually, a user won't modify these registers unless he knows what he is
>>> doing!
>
>> I also still don't really like using write_raw_cp_reg() here --
>> it will bypass some behaviour you want and in some cases will
>> just break the emulation because invariants we assume will
>> hold no longer hold. It would be a lot lot safer to not
>> provide write access at all, only read access.
>
> Adding to that our customers may need this write access, our tool TRACE32®
> needs this also in some particular cases. For example: temporary disabling MMU
> to do a physical memory access.

By clearing the SCTLR bit? That's a good example of a case that
won't work reliably. If you clear the SCTLR.M bit via raw_write
this will not perform the tlb_flush() that it needs to, which
means that if anything does a memory access via the QEMU TLB
it may get the wrong cached results. If you always clear the
bit, do one gdb memory access then set the bit then it will
probably not run into problems but you're walking on thin ice.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/4] target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo type

[Peter Maydell]

On 28 February 2018 at 11:01, Abdallah Bouassida
<abdallah.bouassida@lauterbach.com> wrote:
> This is a preparation for the coming feature of creating dynamically an XML
> description for the ARM sysregs.
> A register has ARM_CP_NO_GDB enabled will not be shown in the dynamic XML.
> This bit is enabled automatically when creating CP_ANY wildcard aliases.
> This bit could be enabled manually for any register we want to remove from the
> dynamic XML description.
>
> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
> ---
>  target/arm/cpu.h    | 3 ++-
>  target/arm/helper.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8c839fa..92cfe4c 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1776,10 +1776,11 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
>  #define ARM_LAST_SPECIAL         ARM_CP_DC_ZVA
>  #define ARM_CP_FPU               0x1000
>  #define ARM_CP_SVE               0x2000
> +#define ARM_CP_NO_GDB            0x4000
>  /* Used only as a terminator for ARMCPRegInfo lists */
>  #define ARM_CP_SENTINEL          0xffff
>  /* Mask of only the flag bits in a type field */
> -#define ARM_CP_FLAG_MASK         0x30ff
> +#define ARM_CP_FLAG_MASK         0x70ff
>
>  /* Valid values for ARMCPRegInfo state field, indicating which of
>   * the AArch32 and AArch64 execution states this register is visible in.
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index c5bc69b..bdd212f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5663,7 +5663,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
>      if (((r->crm == CP_ANY) && crm != 0) ||
>          ((r->opc1 == CP_ANY) && opc1 != 0) ||
>          ((r->opc2 == CP_ANY) && opc2 != 0)) {
> -        r2->type |= ARM_CP_ALIAS;
> +        r2->type |= ARM_CP_ALIAS | ARM_CP_NO_GDB;
>      }
>
>      /* Check that raw accesses are either forbidden or handled. Note that
> --
> 2.7.4

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/4] target/arm: Add "_S" suffix to the secure version of a sysreg

[Peter Maydell]

On 28 February 2018 at 11:01, Abdallah Bouassida
<abdallah.bouassida@lauterbach.com> wrote:
> This is a preparation for the coming feature of creating dynamically an XML
> description for the ARM sysregs.
> Add "_S" suffix to the secure version of sysregs that have both S and NS views
> Replace (S) and (NS) by _S and _NS for the register that are manually defined,
> so all the registers follow the same convention.
>
> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
> ---
>  target/arm/helper.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index bdd212f..1594ec45 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -694,12 +694,12 @@ static const ARMCPRegInfo cp_reginfo[] = {
>       * the secure register to be properly reset and migrated. There is also no
>       * v8 EL1 version of the register so the non-secure instance stands alone.
>       */
> -    { .name = "FCSEIDR(NS)",
> +    { .name = "FCSEIDR_NS",

I think this should just be "FCSEIDR", because the convention we
seem to be going with is "_S" for the secure banked register, and
no suffix for the non-secure banked register.

> @@ -5562,7 +5562,8 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>
>  static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
>                                     void *opaque, int state, int secstate,
> -                                   int crm, int opc1, int opc2)
> +                                   int crm, int opc1, int opc2,
> +                                   const char *name)
>  {
>      /* Private utility function for define_one_arm_cp_reg_with_opaque():
>       * add a single reginfo struct to the hash table.
> @@ -5572,6 +5573,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
>      int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
>      int ns = (secstate & ARM_CP_SECSTATE_NS) ? 1 : 0;
>
> +    r2->name = name;
>      /* Reset the secure state to the specific incoming state.  This is
>       * necessary as the register may have been defined with both states.
>       */
> @@ -5803,19 +5805,26 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
>                          /* Under AArch32 CP registers can be common
>                           * (same for secure and non-secure world) or banked.
>                           */
> +                        const char *name;
> +                        GString *s;
> +
>                          switch (r->secure) {
>                          case ARM_CP_SECSTATE_S:
>                          case ARM_CP_SECSTATE_NS:
>                              add_cpreg_to_hashtable(cpu, r, opaque, state,
> -                                                   r->secure, crm, opc1, opc2);
> +                                                   r->secure, crm, opc1, opc2,
> +                                                   r->name);
>                              break;
>                          default:
> +                            s = g_string_new(r->name);
> +                            g_string_append_printf(s, "_S");
> +                            name = g_string_free(s, false);

You can do this more simply with just
     name = g_strdup_printf("%s_S", r->name);

You only need to use the g_string APIs when you're appending to the
same string multiple times; if you're creating the entire string in
one go this is simpler.

>                              add_cpreg_to_hashtable(cpu, r, opaque, state,
>                                                     ARM_CP_SECSTATE_S,
> -                                                   crm, opc1, opc2);
> +                                                   crm, opc1, opc2, name);
>                              add_cpreg_to_hashtable(cpu, r, opaque, state,
>                                                     ARM_CP_SECSTATE_NS,
> -                                                   crm, opc1, opc2);
> +                                                   crm, opc1, opc2, r->name);
>                              break;
>                          }
>                      } else {

This means we're going to have the hashtable entries be a mix of
structs that point to constant strings and structs that point to
dynamically allocated strings. That's OK right now, because you
can't dynamically construct and delete Arm CPU objects. But if/when
we add support for that (eg for cpu hotplug) it'll mean memory leaks.
I think the simplest approach is that:
 * in add_cpreg_to_hashtable() instead of r2->name = name we should
   r2->name = g_strdup(name);
 * the code here which allocates memory for the _S variant name
   should then do a g_free(name) once it's called add_cpreg_to_hashtable()

Then disposal of hashtable entries (when we write it) will be simple:
always free r->name.

thanks
-- PMM

Re: [Qemu-arm] [PATCH v3 3/4] target/arm: Add the XML dynamic generation

[Peter Maydell]

On 28 February 2018 at 11:01, Abdallah Bouassida
<abdallah.bouassida@lauterbach.com> wrote:
> Generate an XML description for the cp-regs.
> Register these regs with the gdb_register_coprocessor().
> Add arm_gdb_get_sysreg() to use it as a callback to read those regs.
>
> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
> ---
>  gdbstub.c            |  7 ++++
>  include/qom/cpu.h    |  9 ++++-
>  target/arm/cpu.c     |  3 ++
>  target/arm/cpu.h     | 17 +++++++++
>  target/arm/gdbstub.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 132 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index f1d5148..ffab30b 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -665,6 +665,9 @@ static const char *get_feature_xml(const char *p, const char **newp,
>              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), "\"/>");
> +            if (cc->gdb_has_dynamic_xml) {
> +                cc->register_gdb_regs_for_features(cpu);
> +            }

I don't think we need a callback hook for this. You could just assume
the target code has already done whatever it needs. Specifically for
arm you can just make arm_cpu_register_gdb_regs_for_features() in helper.c
call gdb_register_coprocessor(..., "system-registers.xml",...) the
same way it already does for other xml files.

>              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);
> @@ -674,6 +677,10 @@ static const char *get_feature_xml(const char *p, const char **newp,
>          }
>          return target_xml;
>      }
> +    if (strncmp(p, "system-registers.xml", len) == 0) {
> +        CPUState *cpu = first_cpu;
> +        return cc->gdb_get_dynamic_xml(cpu);
> +    }

Rather than hardcoding that "system-registers.xml" is special, just
have the hook pass the xml filename to the CPU hook, and let it
decide whether it wants to handle the filename or not.

Also you must check whether the hook function is NULL before calling it,
or this will crash on CPUs that haven't implemented it.

>      for (i = 0; ; i++) {
>          name = xml_builtin[i][0];
>          if (!name || (strncmp(name, p, len) == 0 && strlen(name) == len))
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index aff88fa..04771b3 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -131,6 +131,11 @@ struct TranslationBlock;
>   *           before the insn which triggers a watchpoint rather than after it.
>   * @gdb_arch_name: Optional callback that returns the architecture name known
>   * to GDB. The caller must free the returned string with g_free.
> + * @gdb_has_dynamic_xml: Indicates if GDB supports generating a dynamic XML
> + *                   description for the sysregs of this CPU.
> + * @register_gdb_regs_for_features: Callback for letting GDB create a dynamic
> + *       XML description for the sysregs and register those sysregs afterwards.
> + * @gdb_get_dynamic_xml: Callback for letting GDB get the dynamic XML.
>   * @cpu_exec_enter: Callback for cpu_exec preparation.
>   * @cpu_exec_exit: Callback for cpu_exec cleanup.
>   * @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec.
> @@ -197,7 +202,9 @@ typedef struct CPUClass {
>      const struct VMStateDescription *vmsd;
>      const char *gdb_core_xml_file;
>      gchar * (*gdb_arch_name)(CPUState *cpu);
> -
> +    bool gdb_has_dynamic_xml;
> +    void (*register_gdb_regs_for_features)(CPUState *cpu);
> +    char *(*gdb_get_dynamic_xml)(CPUState *cpu);

You should only need one new field, something like
       char *(*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
doc comment something like
    * gdb_get_dynamic_xml: Callback to return dynamically generated XML
      for the gdb stub. Returns a pointer to the XML contents for the
      specified XML file, or NULL if the CPU doesn't have dynamically
      generated contents for it.

>      void (*cpu_exec_enter)(CPUState *cpu);
>      void (*cpu_exec_exit)(CPUState *cpu);
>      bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 1b3ae62..04c4d04 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1781,6 +1781,9 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_num_core_regs = 26;
>      cc->gdb_core_xml_file = "arm-core.xml";
>      cc->gdb_arch_name = arm_gdb_arch_name;
> +    cc->gdb_has_dynamic_xml = true;
> +    cc->register_gdb_regs_for_features = arm_register_gdb_regs_for_features;
> +    cc->gdb_get_dynamic_xml = arm_gdb_get_dynamic_xml;
>      cc->gdb_stop_before_watchpoint = true;
>      cc->debug_excp_handler = arm_debug_excp_handler;
>      cc->debug_check_watchpoint = arm_debug_check_watchpoint;
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 92cfe4c..0e35f64 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -133,6 +133,19 @@ enum {
>     s<2n+1> maps to the most significant half of d<n>
>   */
>
> +/**
> + * DynamicGDBXMLInfo:
> + * @desc: Contains the XML descriptions.
> + * @num_cpregs: Number of the Coprocessor registers seen by GDB.
> + * @cpregs_keys: Array that contains the corresponding Key of
> + * a given cpreg with the same order of the cpreg in the XML description.
> + */
> +typedef struct DynamicGDBXMLInfo {
> +    char *desc;
> +    int num_cpregs;
> +    uint32_t *cpregs_keys;
> +} DynamicGDBXMLInfo;
> +
>  /* CPU state for each instance of a generic timer (in cp15 c14) */
>  typedef struct ARMGenericTimer {
>      uint64_t cval; /* Timer CompareValue register */
> @@ -671,6 +684,8 @@ struct ARMCPU {
>      uint64_t *cpreg_vmstate_values;
>      int32_t cpreg_vmstate_array_len;
>
> +    DynamicGDBXMLInfo dyn_xml;
> +
>      /* Timers used by the generic (architected) timer */
>      QEMUTimer *gt_timer[NUM_GTIMERS];
>      /* GPIO outputs for generic timer */
> @@ -835,6 +850,8 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
>
>  int arm_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>  int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> +void arm_register_gdb_regs_for_features(CPUState *cpu);
> +char *arm_gdb_get_dynamic_xml(CPUState *cpu);
>
>  int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>                               int cpuid, void *opaque);
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 04c1208..e08ad79 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -101,3 +101,100 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>      /* Unknown register.  */
>      return 0;
>  }
> +
> +static void arm_gen_one_xml_reg_tag(DynamicGDBXMLInfo *dyn_xml,
> +                                    ARMCPRegInfo *ri, uint32_t ri_key,
> +                                    bool is64)
> +{
> +    GString *s = g_string_new(dyn_xml->desc);

Rather than creating a new GString here, use the one you
already had in arm_gen_dynamic_xml() to do all the appending,
and only convert it to an actual C string at the very end.

> +
> +    g_string_append_printf(s, "<reg name=\"%s\"", ri->name);
> +    g_string_append_printf(s, " bitsize=\"%s\"", is64 ? "64" : "32");
> +    g_string_append_printf(s, " group=\"cp_regs\"/>");
> +    dyn_xml->desc = g_string_free(s, false);
> +    dyn_xml->num_cpregs++;
> +    dyn_xml->cpregs_keys = g_renew(uint32_t,
> +                                       dyn_xml->cpregs_keys,
> +                                       dyn_xml->num_cpregs);
> +    dyn_xml->cpregs_keys[dyn_xml->num_cpregs - 1] = ri_key;
> +}
> +
> +static void arm_register_sysreg_for_xml(gpointer key, gpointer value,
> +                                        gpointer cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    ARMCPRegInfo *ri = value;
> +    uint32_t ri_key = *(uint32_t *)key;
> +
> +    if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_NO_GDB))) {
> +        if (env->aarch64) {
> +            if (ri->state == ARM_CP_STATE_AA64) {
> +                arm_gen_one_xml_reg_tag(&cpu->dyn_xml, ri, ri_key, 1);
> +            }
> +        } else {
> +            if (ri->state == ARM_CP_STATE_AA32) {
> +                if (!arm_feature(env, ARM_FEATURE_EL3) &&
> +                    (ri->secure & ARM_CP_SECSTATE_S)) {
> +                    return;
> +                }
> +                if (ri->type & ARM_CP_64BIT) {
> +                    arm_gen_one_xml_reg_tag(&cpu->dyn_xml, ri, ri_key, 1);
> +                } else {
> +                    arm_gen_one_xml_reg_tag(&cpu->dyn_xml, ri, ri_key, 0);
> +                }
> +            }
> +        }
> +    }
> +}
> +
> +static int arm_gen_dynamic_xml(CPUState *cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    GString *s = g_string_new(NULL);
> +
> +    cpu->dyn_xml.num_cpregs = 0;
> +    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.gnu.gdb.sys.regs\">");
> +    cpu->dyn_xml.desc = g_string_free(s, false);
> +    g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_xml, cs);
> +    s = g_string_new(cpu->dyn_xml.desc);
> +    g_string_append_printf(s, "</feature>");
> +    cpu->dyn_xml.desc = g_string_free(s, false);
> +    return  cpu->dyn_xml.num_cpregs;
> +}
> +
> +static int arm_gdb_get_sysreg(CPUARMState *env, uint8_t *buf, int reg)
> +{
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    const ARMCPRegInfo *ri;
> +    uint32_t key;
> +
> +    key = cpu->dyn_xml.cpregs_keys[reg];
> +    ri = get_arm_cp_reginfo(cpu->cp_regs, key);
> +    if (ri) {
> +        if (cpreg_field_is_64bit(ri)) {
> +            return gdb_get_reg64(buf, (uint64_t)read_raw_cp_reg(env, ri));
> +        } else {
> +            return gdb_get_reg32(buf, (uint32_t)read_raw_cp_reg(env, ri));
> +        }
> +    }
> +    return 0;
> +}
> +
> +void arm_register_gdb_regs_for_features(CPUState *cs)
> +{
> +    int n;
> +
> +    n = arm_gen_dynamic_xml(cs);
> +    gdb_register_coprocessor(cs, arm_gdb_get_sysreg, NULL,
> +                             n, "system-registers.xml", 0);
> +
> +}
> +
> +char *arm_gdb_get_dynamic_xml(CPUState *cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    return cpu->dyn_xml.desc;
> +}
> --

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/4] target/arm: Add the XML dynamic generation

[Peter Maydell]

On 28 February 2018 at 11:01, Abdallah Bouassida
<abdallah.bouassida@lauterbach.com> wrote:
> Generate an XML description for the cp-regs.
> Register these regs with the gdb_register_coprocessor().
> Add arm_gdb_get_sysreg() to use it as a callback to read those regs.
>
> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
> ---

> +void arm_register_gdb_regs_for_features(CPUState *cs)
> +{
> +    int n;
> +
> +    n = arm_gen_dynamic_xml(cs);
> +    gdb_register_coprocessor(cs, arm_gdb_get_sysreg, NULL,
> +                             n, "system-registers.xml", 0);
> +
> +}

Have you tried writing to a sysreg from the debugger without
your patch 4 applied? I suspect you'll find it crashes because
of the NULL pointer you're passing here, and you'll need a
dummy write function.

thanks
-- PMM

Re: [Qemu-arm] [PATCH v3 3/4] target/arm: Add the XML dynamic generation

[Abdallah Bouassida]

Hi Peter,
>> diff --git a/gdbstub.c b/gdbstub.c
>> index f1d5148..ffab30b 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -665,6 +665,9 @@ static const char *get_feature_xml(const char *p, const char **newp,
>>              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), "\"/>");
>> +            if (cc->gdb_has_dynamic_xml) {
>> +                cc->register_gdb_regs_for_features(cpu);
>> +            }
> I don't think we need a callback hook for this. You could just assume
> the target code has already done whatever it needs. Specifically for
> arm you can just make arm_cpu_register_gdb_regs_for_features() in helper.c
> call gdb_register_coprocessor(..., "system-registers.xml",...) the
> same way it already does for other xml files.
I think in this case, for ARM64, if the execution mode has changed (to AARCH32) the time we connect
to gdbstub then we will get the registers view of AARCH64 instead of AARCH32, as we have created
our XML at the beginning (on arm_cpu_register_gdb_regs_for_feature()) where the CPU is
on AARCH64.

Best regards,
Abdallah

Re: [Qemu-devel] [PATCH v3 3/4] target/arm: Add the XML dynamic generation

[Peter Maydell]

On 6 March 2018 at 15:29, Abdallah Bouassida
<abdallah.bouassida@lauterbach.com> wrote:
> Hi Peter,
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index f1d5148..ffab30b 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -665,6 +665,9 @@ static const char *get_feature_xml(const char *p, const char **newp,
>>>              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), "\"/>");
>>> +            if (cc->gdb_has_dynamic_xml) {
>>> +                cc->register_gdb_regs_for_features(cpu);
>>> +            }
>> I don't think we need a callback hook for this. You could just assume
>> the target code has already done whatever it needs. Specifically for
>> arm you can just make arm_cpu_register_gdb_regs_for_features() in helper.c
>> call gdb_register_coprocessor(..., "system-registers.xml",...) the
>> same way it already does for other xml files.
> I think in this case, for ARM64, if the execution mode has changed (to AARCH32) the time we connect
> to gdbstub then we will get the registers view of AARCH64 instead of AARCH32, as we have created
> our XML at the beginning (on arm_cpu_register_gdb_regs_for_feature()) where the CPU is
> on AARCH64.

The gdbstub code does not currently support guests flipping between
AArch32 and AArch64 -- it always exposes the AArch64 view of an
AArch64 CPU even if it happens to be in AArch32. This is obviously
unfortunate, but traditionally gdb has not supported the target
flipping architecture like this. I think that recent gdb changes
have enhanced the protocol to permit this, but adapting QEMU
to use them is a big job that you don't really want to make this
series be dependent on.

thanks
-- PMM