All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
@ 2018-04-19 15:56 Abdallah Bouassida
  2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 1/3] target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo type Abdallah Bouassida
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Abdallah Bouassida @ 2018-04-19 15:56 UTC (permalink / raw)
  To: qemu-devel, peter.maydell, alex.bennee
  Cc: qemu-arm, khaled.jmal, Abdallah Bouassida

The previous version:
http://patchwork.ozlabs.org/project/qemu-devel/list/?series=33714

Abdallah Bouassida (3):
  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

 gdbstub.c            | 10 +++++++
 include/qom/cpu.h    |  5 +++-
 target/arm/cpu.c     |  1 +
 target/arm/cpu.h     | 29 +++++++++++++++++++-
 target/arm/gdbstub.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 target/arm/helper.c  | 57 ++++++++++++++++++++++++++++++---------
 6 files changed, 164 insertions(+), 14 deletions(-)

-- 
2.7.4

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

* [Qemu-devel] [PATCH v6 1/3] target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo type
  2018-04-19 15:56 [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
@ 2018-04-19 15:56 ` Abdallah Bouassida
  2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 2/3] target/arm: Add "_S" suffix to the secure version of a sysreg Abdallah Bouassida
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Abdallah Bouassida @ 2018-04-19 15:56 UTC (permalink / raw)
  To: qemu-devel, peter.maydell, alex.bennee
  Cc: qemu-arm, khaled.jmal, 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 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 19a0c03..436f675 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1815,10 +1815,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 dcb8476..799e322 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5664,7 +5664,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

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

* [Qemu-devel] [PATCH v6 2/3] target/arm: Add "_S" suffix to the secure version of a sysreg
  2018-04-19 15:56 [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
  2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 1/3] target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo type Abdallah Bouassida
@ 2018-04-19 15:56 ` Abdallah Bouassida
  2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 3/3] target/arm: Add the XML dynamic generation Abdallah Bouassida
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Abdallah Bouassida @ 2018-04-19 15:56 UTC (permalink / raw)
  To: qemu-devel, peter.maydell, alex.bennee
  Cc: qemu-arm, khaled.jmal, 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/helper.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 799e322..858bda2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -695,12 +695,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",
       .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),
@@ -716,7 +716,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),
@@ -1967,7 +1967,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,
@@ -2006,7 +2006,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,
@@ -2060,7 +2060,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,
@@ -5563,7 +5563,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.
@@ -5573,6 +5574,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 = g_strdup(name);
     /* Reset the secure state to the specific incoming state.  This is
      * necessary as the register may have been defined with both states.
      */
@@ -5804,19 +5806,24 @@ 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.
                          */
+                        char *name;
+
                         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:
+                            name = g_strdup_printf("%s_S", r->name);
                             add_cpreg_to_hashtable(cpu, r, opaque, state,
                                                    ARM_CP_SECSTATE_S,
-                                                   crm, opc1, opc2);
+                                                   crm, opc1, opc2, name);
+                            g_free(name);
                             add_cpreg_to_hashtable(cpu, r, opaque, state,
                                                    ARM_CP_SECSTATE_NS,
-                                                   crm, opc1, opc2);
+                                                   crm, opc1, opc2, r->name);
                             break;
                         }
                     } else {
@@ -5824,7 +5831,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

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

* [Qemu-devel] [PATCH v6 3/3] target/arm: Add the XML dynamic generation
  2018-04-19 15:56 [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
  2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 1/3] target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo type Abdallah Bouassida
  2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 2/3] target/arm: Add "_S" suffix to the secure version of a sysreg Abdallah Bouassida
@ 2018-04-19 15:56 ` Abdallah Bouassida
  2018-05-03 10:19   ` Alex Bennée
  2018-05-02 13:31 ` [Qemu-devel] ping Re: [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Abdallah Bouassida @ 2018-04-19 15:56 UTC (permalink / raw)
  To: qemu-devel, peter.maydell, alex.bennee
  Cc: qemu-arm, khaled.jmal, 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.
Add a dummy arm_gdb_set_sysreg().

Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
---
 gdbstub.c            | 10 +++++++
 include/qom/cpu.h    |  5 +++-
 target/arm/cpu.c     |  1 +
 target/arm/cpu.h     | 26 ++++++++++++++++++
 target/arm/gdbstub.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 target/arm/helper.c  | 26 ++++++++++++++++++
 6 files changed, 143 insertions(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index a76b2fa..4b56a43 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -674,6 +674,16 @@ static const char *get_feature_xml(const char *p, const char **newp,
         }
         return target_xml;
     }
+    if (cc->gdb_get_dynamic_xml) {
+        CPUState *cpu = first_cpu;
+        char *xmlname = g_strndup(p, len);
+        const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);
+
+        g_free(xmlname);
+        if (xml) {
+            return xml;
+        }
+    }
     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 14e45c4..9d3afc6 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -132,6 +132,9 @@ 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_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 a dynamically generated content for it.
  * @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.
@@ -198,7 +201,7 @@ typedef struct CPUClass {
     const struct VMStateDescription *vmsd;
     const char *gdb_core_xml_file;
     gchar * (*gdb_arch_name)(CPUState *cpu);
-
+    const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
     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 022d8c5..38b8b1c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1879,6 +1879,7 @@ 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_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 436f675..9544043 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 */
@@ -682,6 +695,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 */
@@ -863,6 +878,17 @@ 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);
 
+/* Dynamically generates for gdb stub an XML description of the sysregs from
+ * the cp_regs hashtable. Returns the registered sysregs number.
+ */
+int arm_gen_dynamic_xml(CPUState *cpu);
+
+/* Returns the dynamically generated XML for the gdb stub.
+ * Returns a pointer to the XML contents for the specified XML file or NULL
+ * if the XML name doesn't match the predefined one.
+ */
+const char *arm_gdb_get_dynamic_xml(CPUState *cpu, const char *xmlname);
+
 int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
                              int cpuid, void *opaque);
 int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 04c1208..e80cfb4 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -22,6 +22,11 @@
 #include "cpu.h"
 #include "exec/gdbstub.h"
 
+typedef struct RegisterSysregXmlParam {
+    CPUState *cs;
+    GString *s;
+} RegisterSysregXmlParam;
+
 /* Old gdb always expect FPA registers.  Newer (xml-aware) gdb only expect
    whatever the target description contains.  Due to a historical mishap
    the FPA registers appear in between core integer regs and the CPSR.
@@ -101,3 +106,74 @@ 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(GString *s, DynamicGDBXMLInfo *dyn_xml,
+                                    ARMCPRegInfo *ri, uint32_t ri_key,
+                                    int bitsize)
+{
+    g_string_append_printf(s, "<reg name=\"%s\"", ri->name);
+    g_string_append_printf(s, " bitsize=\"%d\"", bitsize);
+    g_string_append_printf(s, " group=\"cp_regs\"/>");
+    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 p)
+{
+    uint32_t ri_key = *(uint32_t *)key;
+    ARMCPRegInfo *ri = value;
+    RegisterSysregXmlParam *param = (RegisterSysregXmlParam *)p;
+    GString *s = param->s;
+    ARMCPU *cpu = ARM_CPU(param->cs);
+    CPUARMState *env = &cpu->env;
+    DynamicGDBXMLInfo *dyn_xml = &cpu->dyn_xml;
+
+    if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_NO_GDB))) {
+        if (arm_feature(env, ARM_FEATURE_AARCH64)) {
+            if (ri->state == ARM_CP_STATE_AA64) {
+                arm_gen_one_xml_reg_tag(s , dyn_xml, ri, ri_key, 64);
+            }
+        } 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(s , dyn_xml, ri, ri_key, 64);
+                } else {
+                    arm_gen_one_xml_reg_tag(s , dyn_xml, ri, ri_key, 32);
+                }
+            }
+        }
+    }
+}
+
+int arm_gen_dynamic_xml(CPUState *cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    GString *s = g_string_new(NULL);
+    RegisterSysregXmlParam param = {cs, s};
+
+    cpu->dyn_xml.num_cpregs = 0;
+    cpu->dyn_xml.cpregs_keys = g_malloc(sizeof(uint32_t *) *
+                                        g_hash_table_size(cpu->cp_regs));
+    g_string_printf(s, "<?xml version=\"1.0\"?>");
+    g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
+    g_string_append_printf(s, "<feature name=\"org.qemu.gdb.arm.sys.regs\">");
+    g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_xml, &param);
+    g_string_append_printf(s, "</feature>");
+    cpu->dyn_xml.desc = g_string_free(s, false);
+    return cpu->dyn_xml.num_cpregs;
+}
+
+const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    if (strcmp(xmlname, "system-registers.xml") == 0) {
+        return cpu->dyn_xml.desc;
+    }
+    return NULL;
+}
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 858bda2..db77662 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -220,6 +220,29 @@ static void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 }
 
+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;
+}
+
+static int arm_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg)
+{
+    return 0;
+}
+
 static bool raw_accessors_invalid(const ARMCPRegInfo *ri)
 {
    /* Return true if the regdef would cause an assertion if you called
@@ -5474,6 +5497,9 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
         gdb_register_coprocessor(cs, vfp_gdb_get_reg, vfp_gdb_set_reg,
                                  19, "arm-vfp.xml", 0);
     }
+    gdb_register_coprocessor(cs, arm_gdb_get_sysreg, arm_gdb_set_sysreg,
+                             arm_gen_dynamic_xml(cs),
+                             "system-registers.xml", 0);
 }
 
 /* Sort alphabetically by type name, except for "any". */
-- 
2.7.4

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

* [Qemu-devel] ping Re: [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
  2018-04-19 15:56 [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
                   ` (2 preceding siblings ...)
  2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 3/3] target/arm: Add the XML dynamic generation Abdallah Bouassida
@ 2018-05-02 13:31 ` Abdallah Bouassida
  2018-05-03 19:48 ` [Qemu-devel] " Alex Bennée
  2018-05-10 13:12 ` Alex Bennée
  5 siblings, 0 replies; 16+ messages in thread
From: Abdallah Bouassida @ 2018-05-02 13:31 UTC (permalink / raw)
  To: qemu-devel, peter.maydell, alex.bennee; +Cc: qemu-arm, khaled.jmal

ping

Le 4/19/2018 à 4:56 PM, Abdallah Bouassida a écrit :
> The previous version:
> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=33714
>
> Abdallah Bouassida (3):
>   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
>
>  gdbstub.c            | 10 +++++++
>  include/qom/cpu.h    |  5 +++-
>  target/arm/cpu.c     |  1 +
>  target/arm/cpu.h     | 29 +++++++++++++++++++-
>  target/arm/gdbstub.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  target/arm/helper.c  | 57 ++++++++++++++++++++++++++++++---------
>  6 files changed, 164 insertions(+), 14 deletions(-)
>

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

* Re: [Qemu-devel] [PATCH v6 3/3] target/arm: Add the XML dynamic generation
  2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 3/3] target/arm: Add the XML dynamic generation Abdallah Bouassida
@ 2018-05-03 10:19   ` Alex Bennée
  2018-05-03 10:26     ` Peter Maydell
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2018-05-03 10:19 UTC (permalink / raw)
  To: Abdallah Bouassida; +Cc: qemu-devel, peter.maydell, qemu-arm, khaled.jmal


Abdallah Bouassida <abdallah.bouassida@lauterbach.com> writes:

> 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.
> Add a dummy arm_gdb_set_sysreg().
>
> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
> ---
>  gdbstub.c            | 10 +++++++
>  include/qom/cpu.h    |  5 +++-
>  target/arm/cpu.c     |  1 +
>  target/arm/cpu.h     | 26 ++++++++++++++++++
>  target/arm/gdbstub.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  target/arm/helper.c  | 26 ++++++++++++++++++
>  6 files changed, 143 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index a76b2fa..4b56a43 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -674,6 +674,16 @@ static const char *get_feature_xml(const char *p, const char **newp,
>          }
>          return target_xml;
>      }
> +    if (cc->gdb_get_dynamic_xml) {
> +        CPUState *cpu = first_cpu;
> +        char *xmlname = g_strndup(p, len);
> +        const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);

Last time I asked:

"although I'm confused as to why you need to g_strdup the string. You
already have p and its not like gdb_get_dynamic_xml couldn't dup the
string if it needed to (which it doesn't seem to)."

You could argue it as a bounding exercise from a potentially hostile
gdbstub packet but if that is the intention a comment should go here.

> +
> +        g_free(xmlname);
> +        if (xml) {
> +            return xml;
> +        }
> +    }
>      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 14e45c4..9d3afc6 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -132,6 +132,9 @@ 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_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 a dynamically generated content for it.
>   * @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.
> @@ -198,7 +201,7 @@ typedef struct CPUClass {
>      const struct VMStateDescription *vmsd;
>      const char *gdb_core_xml_file;
>      gchar * (*gdb_arch_name)(CPUState *cpu);
> -
> +    const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
>      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 022d8c5..38b8b1c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1879,6 +1879,7 @@ 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_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 436f675..9544043 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 */
> @@ -682,6 +695,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 */
> @@ -863,6 +878,17 @@ 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);
>
> +/* Dynamically generates for gdb stub an XML description of the sysregs from
> + * the cp_regs hashtable. Returns the registered sysregs number.
> + */
> +int arm_gen_dynamic_xml(CPUState *cpu);
> +
> +/* Returns the dynamically generated XML for the gdb stub.
> + * Returns a pointer to the XML contents for the specified XML file or NULL
> + * if the XML name doesn't match the predefined one.
> + */
> +const char *arm_gdb_get_dynamic_xml(CPUState *cpu, const char *xmlname);
> +
>  int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>                               int cpuid, void *opaque);
>  int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 04c1208..e80cfb4 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -22,6 +22,11 @@
>  #include "cpu.h"
>  #include "exec/gdbstub.h"
>
> +typedef struct RegisterSysregXmlParam {
> +    CPUState *cs;
> +    GString *s;
> +} RegisterSysregXmlParam;
> +
>  /* Old gdb always expect FPA registers.  Newer (xml-aware) gdb only expect
>     whatever the target description contains.  Due to a historical mishap
>     the FPA registers appear in between core integer regs and the CPSR.
> @@ -101,3 +106,74 @@ 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(GString *s, DynamicGDBXMLInfo *dyn_xml,
> +                                    ARMCPRegInfo *ri, uint32_t ri_key,
> +                                    int bitsize)
> +{
> +    g_string_append_printf(s, "<reg name=\"%s\"", ri->name);
> +    g_string_append_printf(s, " bitsize=\"%d\"", bitsize);
> +    g_string_append_printf(s, " group=\"cp_regs\"/>");
> +    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 p)
> +{
> +    uint32_t ri_key = *(uint32_t *)key;
> +    ARMCPRegInfo *ri = value;
> +    RegisterSysregXmlParam *param = (RegisterSysregXmlParam *)p;
> +    GString *s = param->s;
> +    ARMCPU *cpu = ARM_CPU(param->cs);
> +    CPUARMState *env = &cpu->env;
> +    DynamicGDBXMLInfo *dyn_xml = &cpu->dyn_xml;
> +
> +    if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_NO_GDB))) {
> +        if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> +            if (ri->state == ARM_CP_STATE_AA64) {
> +                arm_gen_one_xml_reg_tag(s , dyn_xml, ri, ri_key, 64);
> +            }
> +        } 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(s , dyn_xml, ri, ri_key, 64);
> +                } else {
> +                    arm_gen_one_xml_reg_tag(s , dyn_xml, ri, ri_key, 32);
> +                }
> +            }
> +        }
> +    }
> +}
> +
> +int arm_gen_dynamic_xml(CPUState *cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    GString *s = g_string_new(NULL);
> +    RegisterSysregXmlParam param = {cs, s};
> +
> +    cpu->dyn_xml.num_cpregs = 0;
> +    cpu->dyn_xml.cpregs_keys = g_malloc(sizeof(uint32_t *) *
> +                                        g_hash_table_size(cpu->cp_regs));
> +    g_string_printf(s, "<?xml version=\"1.0\"?>");
> +    g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
> +    g_string_append_printf(s, "<feature name=\"org.qemu.gdb.arm.sys.regs\">");
> +    g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_xml, &param);
> +    g_string_append_printf(s, "</feature>");
> +    cpu->dyn_xml.desc = g_string_free(s, false);
> +    return cpu->dyn_xml.num_cpregs;
> +}
> +
> +const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    if (strcmp(xmlname, "system-registers.xml") == 0) {
> +        return cpu->dyn_xml.desc;
> +    }
> +    return NULL;
> +}
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 858bda2..db77662 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -220,6 +220,29 @@ static void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
>      }
>  }
>
> +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;
> +}
> +
> +static int arm_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg)
> +{
> +    return 0;
> +}
> +
>  static bool raw_accessors_invalid(const ARMCPRegInfo *ri)
>  {
>     /* Return true if the regdef would cause an assertion if you called
> @@ -5474,6 +5497,9 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
>          gdb_register_coprocessor(cs, vfp_gdb_get_reg, vfp_gdb_set_reg,
>                                   19, "arm-vfp.xml", 0);
>      }
> +    gdb_register_coprocessor(cs, arm_gdb_get_sysreg, arm_gdb_set_sysreg,
> +                             arm_gen_dynamic_xml(cs),
> +                             "system-registers.xml", 0);
>  }
>
>  /* Sort alphabetically by type name, except for "any". */


--
Alex Bennée

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

* Re: [Qemu-devel] [PATCH v6 3/3] target/arm: Add the XML dynamic generation
  2018-05-03 10:19   ` Alex Bennée
@ 2018-05-03 10:26     ` Peter Maydell
  2018-05-03 11:54       ` Alex Bennée
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Maydell @ 2018-05-03 10:26 UTC (permalink / raw)
  To: Alex Bennée
  Cc: Abdallah Bouassida, QEMU Developers, qemu-arm, Khaled Jmal

On 3 May 2018 at 11:19, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Abdallah Bouassida <abdallah.bouassida@lauterbach.com> writes:
>
>> 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.
>> Add a dummy arm_gdb_set_sysreg().
>>
>> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
>> ---
>>  gdbstub.c            | 10 +++++++
>>  include/qom/cpu.h    |  5 +++-
>>  target/arm/cpu.c     |  1 +
>>  target/arm/cpu.h     | 26 ++++++++++++++++++
>>  target/arm/gdbstub.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  target/arm/helper.c  | 26 ++++++++++++++++++
>>  6 files changed, 143 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index a76b2fa..4b56a43 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -674,6 +674,16 @@ static const char *get_feature_xml(const char *p, const char **newp,
>>          }
>>          return target_xml;
>>      }
>> +    if (cc->gdb_get_dynamic_xml) {
>> +        CPUState *cpu = first_cpu;
>> +        char *xmlname = g_strndup(p, len);
>> +        const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);
>
> Last time I asked:
>
> "although I'm confused as to why you need to g_strdup the string. You
> already have p and its not like gdb_get_dynamic_xml couldn't dup the
> string if it needed to (which it doesn't seem to)."

...and the answer is still the same: p is not NUL terminated.

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH v6 3/3] target/arm: Add the XML dynamic generation
  2018-05-03 10:26     ` Peter Maydell
@ 2018-05-03 11:54       ` Alex Bennée
  0 siblings, 0 replies; 16+ messages in thread
From: Alex Bennée @ 2018-05-03 11:54 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Abdallah Bouassida, QEMU Developers, qemu-arm, Khaled Jmal


Peter Maydell <peter.maydell@linaro.org> writes:

> On 3 May 2018 at 11:19, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Abdallah Bouassida <abdallah.bouassida@lauterbach.com> writes:
>>
>>> 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.
>>> Add a dummy arm_gdb_set_sysreg().
>>>
>>> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
>>> ---
>>>  gdbstub.c            | 10 +++++++
>>>  include/qom/cpu.h    |  5 +++-
>>>  target/arm/cpu.c     |  1 +
>>>  target/arm/cpu.h     | 26 ++++++++++++++++++
>>>  target/arm/gdbstub.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  target/arm/helper.c  | 26 ++++++++++++++++++
>>>  6 files changed, 143 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index a76b2fa..4b56a43 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -674,6 +674,16 @@ static const char *get_feature_xml(const char *p, const char **newp,
>>>          }
>>>          return target_xml;
>>>      }
>>> +    if (cc->gdb_get_dynamic_xml) {
>>> +        CPUState *cpu = first_cpu;
>>> +        char *xmlname = g_strndup(p, len);
>>> +        const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);
>>
>> Last time I asked:
>>
>> "although I'm confused as to why you need to g_strdup the string. You
>> already have p and its not like gdb_get_dynamic_xml couldn't dup the
>> string if it needed to (which it doesn't seem to)."
>
> ...and the answer is still the same: p is not NUL terminated.

Ahh I missed that reply...

>
> thanks
> -- PMM


--
Alex Bennée

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

* Re: [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
  2018-04-19 15:56 [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
                   ` (3 preceding siblings ...)
  2018-05-02 13:31 ` [Qemu-devel] ping Re: [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
@ 2018-05-03 19:48 ` Alex Bennée
  2018-05-04  8:12   ` Abdallah Bouassida
  2018-05-04  8:22   ` Peter Maydell
  2018-05-10 13:12 ` Alex Bennée
  5 siblings, 2 replies; 16+ messages in thread
From: Alex Bennée @ 2018-05-03 19:48 UTC (permalink / raw)
  To: Abdallah Bouassida; +Cc: qemu-devel, peter.maydell, qemu-arm, khaled.jmal


Abdallah Bouassida <abdallah.bouassida@lauterbach.com> writes:

> The previous version:
> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=33714

I was trying to do some testing but I was finding it very hard to do at
gdb kept throwing up errors like:

(gdb) x/10i $pc
=> 0xffffff8008097760 <check_and_switch_context>:       Cannot access memory at address 0xffffff8008097760
Detaching from program: /home/alex/lsrc/qemu/kernel-v8-plain.build/vmlinux, Remote target
Ending remote debugging.

However this seems to be the same on master as well so I wonder there is
a regression somewhere.

What was the last QEMU master commit you've tested these patches on?
I'll do some more digging tomorrow.

--
Alex Bennée

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

* Re: [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
  2018-05-03 19:48 ` [Qemu-devel] " Alex Bennée
@ 2018-05-04  8:12   ` Abdallah Bouassida
  2018-05-04  8:22   ` Peter Maydell
  1 sibling, 0 replies; 16+ messages in thread
From: Abdallah Bouassida @ 2018-05-04  8:12 UTC (permalink / raw)
  To: Alex Bennée; +Cc: qemu-devel, peter.maydell, qemu-arm, khaled.jmal


> Abdallah Bouassida <abdallah.bouassida@lauterbach.com> writes:
>
>> The previous version:
>> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=33714
> I was trying to do some testing but I was finding it very hard to do at
> gdb kept throwing up errors like:
>
> (gdb) x/10i $pc
> => 0xffffff8008097760 <check_and_switch_context>:       Cannot access memory at address 0xffffff8008097760
> Detaching from program: /home/alex/lsrc/qemu/kernel-v8-plain.build/vmlinux, Remote target
> Ending remote debugging.
>
> However this seems to be the same on master as well so I wonder there is
> a regression somewhere.
>
> What was the last QEMU master commit you've tested these patches on?
> I'll do some more digging tomorrow.
It was:
    commit 0e87fdc966d05f4e5ad868034fcd8ee2a08ca62d
    Author: Peter Maydell <peter.maydell@linaro.org>
    Date:   Wed Apr 4 20:37:20 2018 +0100
        Update version for v2.12.0-rc2 release

Best regards,
Abdallah

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

* Re: [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
  2018-05-03 19:48 ` [Qemu-devel] " Alex Bennée
  2018-05-04  8:12   ` Abdallah Bouassida
@ 2018-05-04  8:22   ` Peter Maydell
  1 sibling, 0 replies; 16+ messages in thread
From: Peter Maydell @ 2018-05-04  8:22 UTC (permalink / raw)
  To: Alex Bennée
  Cc: Abdallah Bouassida, QEMU Developers, qemu-arm, Khaled Jmal

On 3 May 2018 at 20:48, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Abdallah Bouassida <abdallah.bouassida@lauterbach.com> writes:
>
>> The previous version:
>> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=33714
>
> I was trying to do some testing but I was finding it very hard to do at
> gdb kept throwing up errors like:
>
> (gdb) x/10i $pc
> => 0xffffff8008097760 <check_and_switch_context>:       Cannot access memory at address 0xffffff8008097760
> Detaching from program: /home/alex/lsrc/qemu/kernel-v8-plain.build/vmlinux, Remote target
> Ending remote debugging.
>
> However this seems to be the same on master as well so I wonder there is
> a regression somewhere.

What gdb version are you using? If it's a new one you may be running
into https://sourceware.org/bugzilla/show_bug.cgi?id=23127
(a regression in gdb).

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
  2018-04-19 15:56 [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
                   ` (4 preceding siblings ...)
  2018-05-03 19:48 ` [Qemu-devel] " Alex Bennée
@ 2018-05-10 13:12 ` Alex Bennée
  2018-05-15 12:03   ` Peter Maydell
  5 siblings, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2018-05-10 13:12 UTC (permalink / raw)
  To: Abdallah Bouassida; +Cc: qemu-devel, peter.maydell, qemu-arm, khaled.jmal


Abdallah Bouassida <abdallah.bouassida@lauterbach.com> writes:

> The previous version:
> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=33714
>
> Abdallah Bouassida (3):
>   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
>

So I got a fixed up gdb and I was testing the reading of the virtual
counter:

=> 0xffffff800854a118 <arch_counter_get_cntvct+16>:	mrs	x0, cntvct_el0
   0xffffff800854a11c <arch_counter_get_cntvct+20>:	b	0xffffff800854a148 <arch_counter_get_cntvct+64>
   0xffffff800854a120 <arch_counter_get_cntvct+24>:	adrp	x0, 0xffffff800896a000 <scsi_format_log+3568>
   0xffffff800854a124 <arch_counter_get_cntvct+28>:	add	x0, x0, #0x5a0
   0xffffff800854a128 <erratum_set_next_event_tval_virt+4294964112>:	mrs	x1, tpidr_el1

p/x $x0
 $6 = 0xffffff800854a108
p/x $cntvct_el0
 $7 = 0x0
stepi
0xffffff800854a11c	160		return arch_timer_reg_read_stable(cntvct_el0);
=> 0xffffff800854a11c <arch_counter_get_cntvct+20>:	b	0xffffff800854a148 <arch_counter_get_cntvct+64>
   0xffffff800854a120 <arch_counter_get_cntvct+24>:	adrp	x0, 0xffffff800896a000 <scsi_format_log+3568>
   0xffffff800854a124 <arch_counter_get_cntvct+28>:	add	x0, x0, #0x5a0
p/x $x0
  $8 = 0x7a5b32b
p/x $cntvct_el0
  $9 = 0x0

So I'm wondering why there is a disparity here?

--
Alex Bennée

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

* Re: [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
  2018-05-10 13:12 ` Alex Bennée
@ 2018-05-15 12:03   ` Peter Maydell
  2018-05-16  9:03     ` Alex Bennée
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Maydell @ 2018-05-15 12:03 UTC (permalink / raw)
  To: Alex Bennée
  Cc: Abdallah Bouassida, QEMU Developers, qemu-arm, Khaled Jmal

On 10 May 2018 at 14:12, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Abdallah Bouassida <abdallah.bouassida@lauterbach.com> writes:
>
>> The previous version:
>> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=33714
>>
>> Abdallah Bouassida (3):
>>   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
>>
>
> So I got a fixed up gdb and I was testing the reading of the virtual
> counter:
>
> => 0xffffff800854a118 <arch_counter_get_cntvct+16>:     mrs     x0, cntvct_el0
>    0xffffff800854a11c <arch_counter_get_cntvct+20>:     b       0xffffff800854a148 <arch_counter_get_cntvct+64>
>    0xffffff800854a120 <arch_counter_get_cntvct+24>:     adrp    x0, 0xffffff800896a000 <scsi_format_log+3568>
>    0xffffff800854a124 <arch_counter_get_cntvct+28>:     add     x0, x0, #0x5a0
>    0xffffff800854a128 <erratum_set_next_event_tval_virt+4294964112>:    mrs     x1, tpidr_el1
>
> p/x $x0
>  $6 = 0xffffff800854a108
> p/x $cntvct_el0
>  $7 = 0x0
> stepi
> 0xffffff800854a11c      160             return arch_timer_reg_read_stable(cntvct_el0);
> => 0xffffff800854a11c <arch_counter_get_cntvct+20>:     b       0xffffff800854a148 <arch_counter_get_cntvct+64>
>    0xffffff800854a120 <arch_counter_get_cntvct+24>:     adrp    x0, 0xffffff800896a000 <scsi_format_log+3568>
>    0xffffff800854a124 <arch_counter_get_cntvct+28>:     add     x0, x0, #0x5a0
> p/x $x0
>   $8 = 0x7a5b32b
> p/x $cntvct_el0
>   $9 = 0x0
>
> So I'm wondering why there is a disparity here?

CNTVCT_EL0 isn't in the set of registers we expose to gdb (it's
marked up as ARM_CP_NO_RAW), so I'm not sure why gdb is
giving you any value at all. Does it do that for any
random $no_such_thing strings ? Is CNTVCT_EL0 listed
if you ask gdb to display all registers?

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
  2018-05-15 12:03   ` Peter Maydell
@ 2018-05-16  9:03     ` Alex Bennée
  2018-05-17 15:23       ` Peter Maydell
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2018-05-16  9:03 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Abdallah Bouassida, QEMU Developers, qemu-arm, Khaled Jmal


Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 May 2018 at 14:12, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Abdallah Bouassida <abdallah.bouassida@lauterbach.com> writes:
>>
>>> The previous version:
>>> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=33714
>>>
>>> Abdallah Bouassida (3):
>>>   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
>>>
>>
>> So I got a fixed up gdb and I was testing the reading of the virtual
>> counter:
>>
>> => 0xffffff800854a118 <arch_counter_get_cntvct+16>:     mrs     x0, cntvct_el0
>>    0xffffff800854a11c <arch_counter_get_cntvct+20>:     b       0xffffff800854a148 <arch_counter_get_cntvct+64>
>>    0xffffff800854a120 <arch_counter_get_cntvct+24>:     adrp    x0, 0xffffff800896a000 <scsi_format_log+3568>
>>    0xffffff800854a124 <arch_counter_get_cntvct+28>:     add     x0, x0, #0x5a0
>>    0xffffff800854a128 <erratum_set_next_event_tval_virt+4294964112>:    mrs     x1, tpidr_el1
>>
>> p/x $x0
>>  $6 = 0xffffff800854a108
>> p/x $cntvct_el0
>>  $7 = 0x0
>> stepi
>> 0xffffff800854a11c      160             return arch_timer_reg_read_stable(cntvct_el0);
>> => 0xffffff800854a11c <arch_counter_get_cntvct+20>:     b       0xffffff800854a148 <arch_counter_get_cntvct+64>
>>    0xffffff800854a120 <arch_counter_get_cntvct+24>:     adrp    x0, 0xffffff800896a000 <scsi_format_log+3568>
>>    0xffffff800854a124 <arch_counter_get_cntvct+28>:     add     x0, x0, #0x5a0
>> p/x $x0
>>   $8 = 0x7a5b32b
>> p/x $cntvct_el0
>>   $9 = 0x0
>>
>> So I'm wondering why there is a disparity here?
>
> CNTVCT_EL0 isn't in the set of registers we expose to gdb (it's
> marked up as ARM_CP_NO_RAW), so I'm not sure why gdb is
> giving you any value at all. Does it do that for any
> random $no_such_thing strings ?

*sigh* yes....

> Is CNTVCT_EL0 listed
> if you ask gdb to display all registers?

I must have gotten confused parsing:

  CNTVOFF_EL2    0x0                 0
  CNTHP_CTL_EL2  0x0                 0
  CNTHP_CVAL_EL2 0x0                 0
  CNTHCTL_EL2    0x3                 3
  CNTV_CTL_EL0   0x0                 0
  CNTP_CVAL_EL0  0x13e39b327         5338936103
  CNTV_CVAL_EL0  0x0                 0
  CNTPS_CTL_EL1  0x0                 0
  CNTPS_CVAL_EL1 0x0                 0
  CNTP_CTL_EL0   0x1                 1

And of course case matters:

  (gdb) p/x $CNTP_CVAL_EL0
  $2 = 0x13e39b327
  (gdb) p/x $cntp_cval_el0
  $3 = 0x0

And gdb completion is broken so:

  p/x $CN<tab>
  completes to:
  p/x $CNTKCTL_EL1

So finally I tried another set of registers while tracing the early
bootcode:

  (gdb) p/x $SP_EL0
  $3 = 0x0
  (gdb) x/10i $pc
  => 0xffffff8008900290 <__primary_switched+16>:  msr     sp_el0, x5
     0xffffff8008900294 <__primary_switched+20>:  adrp    x8, 0xffffff8008082000 <vectors>
     0xffffff8008900298 <__primary_switched+24>:  add     x8, x8, #0x0
     0xffffff800890029c <__primary_switched+28>:  msr     vbar_el1, x8
     0xffffff80089002a0 <__primary_switched+32>:  isb
     0xffffff80089002a4 <__primary_switched+36>:  stp     xzr, x30, [sp, #-16]!
     0xffffff80089002a8 <__primary_switched+40>:  mov     x29, sp
     0xffffff80089002ac <__primary_switched+44>:  adrp    x5, 0xffffff800894f000 <tmp_cmdline.52085+2008>
     0xffffff80089002b0 <__primary_switched+48>:  str     x21, [x5, #280]
     0xffffff80089002b4 <__primary_switched+52>:  adrp    x4, 0xffffff80086b6000 <kimage_vaddr>
  (gdb) p/x $x5
  $4 = 0xffffff8008980780
  (gdb) i
  413             adr_l   x8, vectors                     // load VBAR_EL1 with virtual
  => 0xffffff8008900294 <__primary_switched+20>:  adrp    x8, 0xffffff8008082000 <vectors>
     0xffffff8008900298 <__primary_switched+24>:  add     x8, x8, #0x0
     0xffffff800890029c <__primary_switched+28>:  msr     vbar_el1, x8
  (gdb) p/x $SP_EL0
  $5 = 0xffffff8008980780
  (gdb) p/x $VBAR
  $6 = 0x0
  (gdb) i
  0xffffff8008900298      413             adr_l   x8, vectors                     // load VBAR_EL1 with virtual
  => 0xffffff8008900298 <__primary_switched+24>:  add     x8, x8, #0x0
     0xffffff800890029c <__primary_switched+28>:  msr     vbar_el1, x8
     0xffffff80089002a0 <__primary_switched+32>:  isb
  (gdb) i
  414             msr     vbar_el1, x8                    // vector table address
  => 0xffffff800890029c <__primary_switched+28>:  msr     vbar_el1, x8
     0xffffff80089002a0 <__primary_switched+32>:  isb
     0xffffff80089002a4 <__primary_switched+36>:  stp     xzr, x30, [sp, #-16]!
  (gdb) p/x $x8
  $7 = 0xffffff8008082000
  (gdb) p/x $VBAR
  $8 = 0x0
  (gdb) i
  415             isb
  => 0xffffff80089002a0 <__primary_switched+32>:  isb
     0xffffff80089002a4 <__primary_switched+36>:  stp     xzr, x30, [sp, #-16]!
     0xffffff80089002a8 <__primary_switched+40>:  mov     x29, sp
  (gdb) p/x $VBAR
  $9 = 0xffffff8008082000
  (gdb)

So finally:

Tested-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

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

* Re: [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
  2018-05-16  9:03     ` Alex Bennée
@ 2018-05-17 15:23       ` Peter Maydell
  2018-05-18 10:35         ` Abdallah Bouassida
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Maydell @ 2018-05-17 15:23 UTC (permalink / raw)
  To: Alex Bennée
  Cc: Abdallah Bouassida, QEMU Developers, qemu-arm, Khaled Jmal

On 16 May 2018 at 10:03, Alex Bennée <alex.bennee@linaro.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> CNTVCT_EL0 isn't in the set of registers we expose to gdb (it's
>> marked up as ARM_CP_NO_RAW), so I'm not sure why gdb is
>> giving you any value at all. Does it do that for any
>> random $no_such_thing strings ?
>
> *sigh* yes....

My gdb ("GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1") gives
(gdb) p/x $cntvct_el0
$3 = Value can't be converted to integer.
or if you don't ask for the hex conversion:
(gdb) print $cntvct_el0
$2 = void

so this is either something fixed in a newer gdb than you
have, or alternately it's a regression if you're using a
newer gdb, which you should probably report upstream...

> So finally:
>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>

Thanks for the testing.

Abdallah: I've added this series to my target-arm.next queue,
so it should reach QEMU master some time next week. Thanks for
your efforts in working through QEMU's review process, and
sorry this took us so long to deal with.

-- PMM

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

* Re: [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB
  2018-05-17 15:23       ` Peter Maydell
@ 2018-05-18 10:35         ` Abdallah Bouassida
  0 siblings, 0 replies; 16+ messages in thread
From: Abdallah Bouassida @ 2018-05-18 10:35 UTC (permalink / raw)
  To: Peter Maydell, Alex Bennée; +Cc: QEMU Developers, qemu-arm, Khaled Jmal


>> So finally:
>>
>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Thanks for the testing.
>
> Abdallah: I've added this series to my target-arm.next queue,
> so it should reach QEMU master some time next week. Thanks for
> your efforts in working through QEMU's review process, and
> sorry this took us so long to deal with.
>
> -- PMM
Happy to collaborate with you guys!
Thank you both for your reviews and your advises :)

Best regards,
Abdallah

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

end of thread, other threads:[~2018-05-18 10:36 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-19 15:56 [Qemu-devel] [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 1/3] target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo type Abdallah Bouassida
2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 2/3] target/arm: Add "_S" suffix to the secure version of a sysreg Abdallah Bouassida
2018-04-19 15:56 ` [Qemu-devel] [PATCH v6 3/3] target/arm: Add the XML dynamic generation Abdallah Bouassida
2018-05-03 10:19   ` Alex Bennée
2018-05-03 10:26     ` Peter Maydell
2018-05-03 11:54       ` Alex Bennée
2018-05-02 13:31 ` [Qemu-devel] ping Re: [PATCH v6 0/3] target/arm: Add a dynamic XML-description of the cp-registers to GDB Abdallah Bouassida
2018-05-03 19:48 ` [Qemu-devel] " Alex Bennée
2018-05-04  8:12   ` Abdallah Bouassida
2018-05-04  8:22   ` Peter Maydell
2018-05-10 13:12 ` Alex Bennée
2018-05-15 12:03   ` Peter Maydell
2018-05-16  9:03     ` Alex Bennée
2018-05-17 15:23       ` Peter Maydell
2018-05-18 10:35         ` Abdallah Bouassida

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