Re: [PATCH v1 0/1] target/microblaze: Add GDB XML files

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

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

On Thu, May 21, 2020 at 09:02:14PM -0700, Joe Komlodi wrote:
> Hi all,
> 
> This adds GDB XML files for Microblaze CPUs.
> For Microblaze, it's split up into core and stack protect XML files.

Hi Joe,

Can you please add a license header to microblaze-core.xml?
I think you wrote this on your own but in case you took it from
GDB (or if they actually are identical) we can keep GDB's
permissive license?

We also need to register the stackprot xml with
gdb_register_coprocessor().

I think you were right about lowering gdb_num_core_regs back to:
    cc->gdb_num_core_regs = 32 + 25;

Things work even if we keep 32 + 27 but I think it's a little
confusing. I've tried a version that uses:

    cc->gdb_num_core_regs = 32 + 25;

And registers stack-prot:
    gdb_register_coprocessor(cs, mb_sp_gdb_get_reg, mb_sp_gdb_set_reg,
                             2, "microblaze-stack-protect.xml", 32 + 25);

That works with GDB from upstream and with Xilinx XSDB.
Vitis 2019.2 microblaze gdb thinks SLR/SHR are 64bit for some
reason and doesn't handle them well with any QEMU version.
We should probably talk to the Vitis folks about changing their
version to match upstream GDB and XSDB.

I'm attaching the diff I used as reference.

Thanks,
Edgar

[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 3315 bytes --]

diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
index a31134b65c..faa48493f6 100644
--- a/target/microblaze/cpu.h
+++ b/target/microblaze/cpu.h
@@ -321,6 +321,8 @@ void mb_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 hwaddr mb_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int mb_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int mb_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+int mb_sp_gdb_get_reg(CPUMBState *env, GByteArray *buf, int n);
+int mb_sp_gdb_set_reg(CPUMBState *env, uint8_t *buf, int n);
 
 void mb_tcg_init(void);
 /* you can call this signal handler from your SIGBUS and SIGSEGV
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index faa88e5865..c87257d928 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -28,6 +28,7 @@
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "exec/exec-all.h"
+#include "exec/gdbstub.h"
 #include "fpu/softfloat-helpers.h"
 
 static const struct {
@@ -226,6 +227,9 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
     env->pvr.regs[11] = (cpu->cfg.use_mmu ? PVR11_USE_MMU : 0) |
                         16 << 17;
 
+    gdb_register_coprocessor(cs, mb_sp_gdb_get_reg, mb_sp_gdb_set_reg,
+                             2, "microblaze-stack-protect.xml", 32 + 25);
+
     mcc->parent_realize(dev, errp);
 }
 
@@ -329,7 +333,7 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
 #endif
     dc->vmsd = &vmstate_mb_cpu;
     device_class_set_props(dc, mb_properties);
-    cc->gdb_num_core_regs = 32 + 27;
+    cc->gdb_num_core_regs = 32 + 25;
     cc->gdb_core_xml_file = "microblaze-core.xml";
 
     cc->disas_set_info = mb_disas_set_info;
diff --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c
index 73e8973597..6eeccb9201 100644
--- a/target/microblaze/gdbstub.c
+++ b/target/microblaze/gdbstub.c
@@ -65,10 +65,12 @@ int mb_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         /* Other SRegs aren't modeled, so report a value of 0 */
         case 19 ... 24:
             return gdb_get_reg32(mem_buf, 0);
+#if 0
         case 25:
             return gdb_get_reg32(mem_buf, env->slr);
         case 26:
             return gdb_get_reg32(mem_buf, env->shr);
+#endif
         default:
             return 0;
         }
@@ -129,13 +131,47 @@ int mb_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         case 18:
             env->sregs[SR_EDR] = tmp;
             break;
+#if 0
         case 25:
             env->slr = tmp;
             break;
         case 26:
             env->shr = tmp;
             break;
+#endif
         }
     }
     return 4;
 }
+
+int mb_sp_gdb_get_reg(CPUMBState *env, GByteArray *buf, int n)
+{
+    printf("%s: n=%d\n", __func__, n);
+    switch (n) {
+    case 0:
+        return gdb_get_reg32(buf, env->slr);
+    case 1:
+        return gdb_get_reg32(buf, env->shr);
+    default:
+        return 0;
+    }
+    return 0;
+}
+
+int mb_sp_gdb_set_reg(CPUMBState *env, uint8_t *buf, int n)
+{
+    uint32_t tmp;
+
+    tmp = ldl_p(buf);
+
+    printf("%s: n=%d\n", __func__, n);
+    switch (n) {
+    case 25:
+        env->slr = tmp;
+        break;
+    case 26:
+        env->shr = tmp;
+        break;
+    }
+    return 4;
+}