Hi Edgar, Comments marked with [Joe]
-----Original Message----- From: Edgar E. Iglesias <edgar.igles...@xilinx.com> Sent: Thursday, May 14, 2020 6:41 AM To: Joe Komlodi <koml...@xilinx.com> Cc: qemu-devel@nongnu.org Subject: Re: [PATCH V2 1/4] target/microblaze: gdb: Add dynamic GDB XML register support On Wed, May 13, 2020 at 11:08:45AM -0700, Joe Komlodi wrote: > Add dynamic GDB register XML for Microblaze, and modify the config > file to use XML when building for Microblaze. > For the dynamic XML to be read, there still needs to be a core XML file. Hi Joe, I was looking a little closer at this and got a bit confused with this approach. So we're adding microblaze-core.xml but we're actually at runtime dynamically generating and providing the contents for it. So the static builtin file does not get used. [Joe] If I recall correctly, the GDB stub wouldn't use any dynamic XML files without a static XML file present. This might have changed since then, since this was written on an older version of QEMU. We should do either (not both): 1. Keep the dynamic generation of the XML file and remove the addintion of gdb_xml_files= and microblaze-core.xml. or 2. Keep the addition of static microblaze-core.xml and remove the dynamic generation of it. Since we're not yet using the dynamic aspects for anything relevant (only r17 code_ptr) my preference would be to use the static files for now. [Joe] That sounds good to me. Also, it's probably a good idea to move this patch to after the patches that fix the register ordering. A few more comments inline. > > Signed-off-by: Joe Komlodi <koml...@xilinx.com> > --- > configure | 1 + > gdb-xml/microblaze-core.xml | 64 +++++++++++++++++++++++ > target/microblaze/cpu.c | 4 ++ > target/microblaze/cpu.h | 9 ++++ > target/microblaze/gdbstub.c | 123 > ++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 201 insertions(+) > create mode 100644 gdb-xml/microblaze-core.xml > > diff --git a/configure b/configure > index 0d69c36..5a099b6 100755 > --- a/configure > +++ b/configure > @@ -7832,6 +7832,7 @@ case "$target_name" in > TARGET_ARCH=microblaze > TARGET_SYSTBL_ABI=common > bflt="yes" > + gdb_xml_files="microblaze-core.xml" > echo "TARGET_ABI32=y" >> $config_target_mak > ;; > mips|mipsel) > diff --git a/gdb-xml/microblaze-core.xml b/gdb-xml/microblaze-core.xml > new file mode 100644 index 0000000..13e2c08 > --- /dev/null > +++ b/gdb-xml/microblaze-core.xml > @@ -0,0 +1,64 @@ > +<?xml version="1.0"?> > + > +<!DOCTYPE feature SYSTEM "gdb-target.dtd"> <feature > +name="org.gnu.gdb.microblaze.core"> > + <reg name="r0" bitsize="32"/> > + <reg name="r1" bitsize="32" type="data_ptr"/> > + <reg name="r2" bitsize="32"/> > + <reg name="r3" bitsize="32"/> > + <reg name="r4" bitsize="32"/> > + <reg name="r5" bitsize="32"/> > + <reg name="r6" bitsize="32"/> > + <reg name="r7" bitsize="32"/> > + <reg name="r8" bitsize="32"/> > + <reg name="r9" bitsize="32"/> > + <reg name="r10" bitsize="32"/> > + <reg name="r11" bitsize="32"/> > + <reg name="r12" bitsize="32"/> > + <reg name="r13" bitsize="32"/> > + <reg name="r14" bitsize="32" type="code_ptr"/> > + <reg name="r15" bitsize="32" type="code_ptr"/> > + <reg name="r16" bitsize="32" type="code_ptr"/> > + <reg name="r17" bitsize="32"/> > + <reg name="r18" bitsize="32"/> > + <reg name="r19" bitsize="32"/> > + <reg name="r20" bitsize="32"/> > + <reg name="r21" bitsize="32"/> > + <reg name="r22" bitsize="32"/> > + <reg name="r23" bitsize="32"/> > + <reg name="r24" bitsize="32"/> > + <reg name="r25" bitsize="32"/> > + <reg name="r26" bitsize="32"/> > + <reg name="r27" bitsize="32"/> > + <reg name="r28" bitsize="32"/> > + <reg name="r29" bitsize="32"/> > + <reg name="r30" bitsize="32"/> > + <reg name="r31" bitsize="32"/> > + <reg name="rpc" bitsize="32" type="code_ptr"/> > + <reg name="rmsr" bitsize="32"/> > + <reg name="rear" bitsize="32"/> > + <reg name="resr" bitsize="32"/> > + <reg name="rfsr" bitsize="32"/> > + <reg name="rbtr" bitsize="32"/> > + <reg name="rpvr0" bitsize="32"/> > + <reg name="rpvr1" bitsize="32"/> > + <reg name="rpvr2" bitsize="32"/> > + <reg name="rpvr3" bitsize="32"/> > + <reg name="rpvr4" bitsize="32"/> > + <reg name="rpvr5" bitsize="32"/> > + <reg name="rpvr6" bitsize="32"/> > + <reg name="rpvr7" bitsize="32"/> > + <reg name="rpvr8" bitsize="32"/> > + <reg name="rpvr9" bitsize="32"/> > + <reg name="rpvr10" bitsize="32"/> > + <reg name="rpvr11" bitsize="32"/> > + <reg name="redr" bitsize="32"/> > + <reg name="rpid" bitsize="32"/> > + <reg name="rzpr" bitsize="32"/> > + <reg name="rtlbx" bitsize="32"/> > + <reg name="rtlbsx" bitsize="32"/> > + <reg name="rtlblo" bitsize="32"/> > + <reg name="rtlbhi" bitsize="32"/> > + <reg name="rslr" bitsize="32"/> > + <reg name="rshr" bitsize="32"/> This last part doesn't look right. slr and shr are optional and should only be presented when the core supports stack protection. I think it would be easier if we simply copied both these files from GDB: https://github.com/bminor/binutils-gdb/blob/master/gdb/features/microblaze-core.xml https://github.com/bminor/binutils-gdb/blob/master/gdb/features/microblaze-stack-protect.xml Add both to gdb_xml_files= and register the stack protect XML file with gdb_register_coprocessor() if stack protection is enabled. [Joe] Agreed. > +</feature> > diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c index > aa99830..41cac1b 100644 > --- a/target/microblaze/cpu.c > +++ b/target/microblaze/cpu.c > @@ -226,6 +226,8 @@ static void mb_cpu_realizefn(DeviceState *dev, Error > **errp) > env->pvr.regs[11] = (cpu->cfg.use_mmu ? PVR11_USE_MMU : 0) | > 16 << 17; > > + mb_gen_dynamic_xml(cpu); > + > mcc->parent_realize(dev, errp); > } > > @@ -330,6 +332,8 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data) > dc->vmsd = &vmstate_mb_cpu; > device_class_set_props(dc, mb_properties); > cc->gdb_num_core_regs = 32 + 5; > + cc->gdb_get_dynamic_xml = mb_gdb_get_dynamic_xml; > + cc->gdb_core_xml_file = "microblaze-core.xml"; > > cc->disas_set_info = mb_disas_set_info; > cc->tcg_initialize = mb_tcg_init; diff --git > a/target/microblaze/cpu.h b/target/microblaze/cpu.h index > a31134b..074a18e 100644 > --- a/target/microblaze/cpu.h > +++ b/target/microblaze/cpu.h > @@ -25,6 +25,8 @@ > #include "fpu/softfloat-types.h" > > typedef struct CPUMBState CPUMBState; > +typedef struct DynamicMBGDBXMLInfo DynamicMBGDBXMLInfo; > + > #if !defined(CONFIG_USER_ONLY) > #include "mmu.h" > #endif > @@ -272,6 +274,10 @@ struct CPUMBState { > } pvr; > }; > > +struct DynamicMBGDBXMLInfo { > + char *xml; > +}; > + > /** > * MicroBlazeCPU: > * @env: #CPUMBState > @@ -286,6 +292,7 @@ struct MicroBlazeCPU { > > CPUNegativeOffsetState neg; > CPUMBState env; > + DynamicMBGDBXMLInfo dyn_xml; > > /* Microblaze Configuration Settings */ > struct { > @@ -321,6 +328,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); > +void mb_gen_dynamic_xml(MicroBlazeCPU *cpu); const char > +*mb_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname); > > void mb_tcg_init(void); > /* you can call this signal handler from your SIGBUS and SIGSEGV diff > --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c > index f41ebf1..cdca434 100644 > --- a/target/microblaze/gdbstub.c > +++ b/target/microblaze/gdbstub.c > @@ -54,3 +54,126 @@ int mb_cpu_gdb_write_register(CPUState *cs, uint8_t > *mem_buf, int n) > } > return 4; > } > + > +static void mb_gen_xml_reg_tag(const MicroBlazeCPU *cpu, GString *s, > + const char *name, uint8_t bitsize, > + const char *type) { > + g_string_append_printf(s, "<reg name=\"%s\" bitsize=\"%d\"", > + name, bitsize); > + if (type) { > + g_string_append_printf(s, " type=\"%s\"", type); > + } > + g_string_append_printf(s, "/>\n"); } > + > +static uint8_t mb_cpu_sreg_size(const MicroBlazeCPU *cpu, uint8_t > +index) { > + /* > + * NOTE: mb-gdb will refuse to connect if we say registers are > + * larger then 32-bits. > + * For now, say none of our registers are dynamically sized, and are > + * therefore only 32-bits. > + */ > + > + return 32; > +} > + > +static void mb_gen_xml_reg_tags(const MicroBlazeCPU *cpu, GString *s) > +{ > + uint8_t i; > + const char *type; > + char reg_name[4]; > + bool has_hw_exception = cpu->cfg.dopb_bus_exception || > + cpu->cfg.iopb_bus_exception || > + cpu->cfg.illegal_opcode_exception || > + cpu->cfg.opcode_0_illegal || > + cpu->cfg.div_zero_exception || > + cpu->cfg.unaligned_exceptions; > + > + static const char *reg_types[32] = { > + [1] = "data_ptr", > + [14] = "code_ptr", > + [15] = "code_ptr", > + [16] = "code_ptr", > + [17] = "code_ptr" > + }; > + > + for (i = 0; i < 32; ++i) { > + type = reg_types[i]; > + /* r17 only has a code_ptr tag if we have HW exceptions */ > + if (i == 17 && !has_hw_exception) { > + type = NULL; > + } > + > + sprintf(reg_name, "r%d", i); > + mb_gen_xml_reg_tag(cpu, s, reg_name, 32, type); > + } > +} > + > +static void mb_gen_xml_sreg_tags(const MicroBlazeCPU *cpu, GString > +*s) { > + uint8_t i; > + > + static const char *sreg_names[] = { > + "rpc", > + "rmsr", > + "rear", > + "resr", > + "rfsr", > + "rbtr", > + "rpvr0", > + "rpvr1", > + "rpvr2", > + "rpvr3", > + "rpvr4", > + "rpvr5", > + "rpvr6", > + "rpvr7", > + "rpvr8", > + "rpvr9", > + "rpvr10", > + "rpvr11", > + "redr", > + "rpid", > + "rzpr", > + "rtlblo", > + "rtlbhi", > + "rtlbx", > + "rtlbsx", In case we decide to keep this dynamic approach, tlbx and tlbsx should be before tlblo and tlbhi. > + "rslr", > + "rshr" These need to be optional and in a separate XML description with org.gnu.gdb.microblaze.stack-protect. > + }; > + > + static const char *sreg_types[ARRAY_SIZE(sreg_names)] = { > + [SR_PC] = "code_ptr" > + }; > + > + for (i = 0; i < ARRAY_SIZE(sreg_names); ++i) { > + mb_gen_xml_reg_tag(cpu, s, sreg_names[i], mb_cpu_sreg_size(cpu, i), > + sreg_types[i]); > + } > +} > + > +void mb_gen_dynamic_xml(MicroBlazeCPU *cpu) { > + GString *s = g_string_new(NULL); > + > + g_string_printf(s, "<?xml version=\"1.0\"?>\n" > + "<!DOCTYPE feature SYSTEM \"gdb-target.dtd\">\n" > + "<feature > + name=\"org.gnu.gdb.microblaze.core\">\n"); > + > + mb_gen_xml_reg_tags(cpu, s); > + mb_gen_xml_sreg_tags(cpu, s); > + > + g_string_append_printf(s, "</feature>"); > + > + cpu->dyn_xml.xml = g_string_free(s, false); } > + > +const char *mb_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname) > +{ > + MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs); > + > + return cpu->dyn_xml.xml; > +} > -- > 2.7.4 >