RE: [PATCH V2 1/4] target/microblaze: gdb: Add dynamic GDB XML register support

[Joe Komlodi]

Hi Edgar,

Comments marked with [Joe]

-----Original Message-----
From: Edgar E. Iglesias <address@hidden> 
Sent: Thursday, May 14, 2020 6:41 AM
To: Joe Komlodi <address@hidden>
Cc: address@hidden
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 <address@hidden>
> ---
>  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
>