[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/3] target/ppc: Add SPRs XML gene
From: |
Fabiano Rosas |
Subject: |
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/3] target/ppc: Add SPRs XML generation code for gdbstub |
Date: |
Thu, 17 Jan 2019 14:11:38 -0200 |
Greg Kurz <address@hidden> writes:
> On Tue, 15 Jan 2019 17:37:48 -0200
> Fabiano Rosas <address@hidden> wrote:
>
>> A following patch will add support for handling the Special Purpose
>> Registers (SPR) in GDB via gdbstub. For that purpose, GDB needs to be
>> provided with an XML description of the registers (see gdb-xml
>> directory).
>>
>> This patch adds the code that generates the XML dynamically based on
>> the SPRs already defined in the machine. This eliminates the need for
>> several XML files to match each possible ppc machine.
>>
>> A "group" is defined so that the GDB command `info registers spr` can
>> be used.
>>
>> Signed-off-by: Fabiano Rosas <address@hidden>
>> ---
>> target/ppc/cpu.h | 8 +++++++
>> target/ppc/gdbstub.c | 54 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 62 insertions(+)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 486abaf99b..34f0d2d419 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -230,6 +230,7 @@ struct ppc_spr_t {
>> void (*oea_write)(DisasContext *ctx, int spr_num, int gpr_num);
>> void (*hea_read)(DisasContext *ctx, int gpr_num, int spr_num);
>> void (*hea_write)(DisasContext *ctx, int spr_num, int gpr_num);
>> + unsigned int gdb_id;
>> #endif
>> const char *name;
>> target_ulong default_value;
>> @@ -1053,6 +1054,9 @@ struct CPUPPCState {
>> /* Special purpose registers */
>> target_ulong spr[1024];
>> ppc_spr_t spr_cb[1024];
>> +#if !defined(CONFIG_USER_ONLY)
>> + const char *gdb_spr_xml;
>> +#endif
>> /* Vector status and control register */
>> uint32_t vscr;
>> /* VSX registers (including FP and AVR) */
>> @@ -1267,6 +1271,10 @@ int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t
>> *buf, int reg);
>> int ppc_cpu_gdb_read_register_apple(CPUState *cpu, uint8_t *buf, int reg);
>> int ppc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>> int ppc_cpu_gdb_write_register_apple(CPUState *cpu, uint8_t *buf, int reg);
>> +#ifndef CONFIG_USER_ONLY
>> +int ppc_gdb_gen_spr_xml(CPUState *cpu);
>> +const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
>> +#endif
>> int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>> int cpuid, void *opaque);
>> int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
>> index 19565b584d..ce4b728028 100644
>> --- a/target/ppc/gdbstub.c
>> +++ b/target/ppc/gdbstub.c
>> @@ -319,3 +319,57 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs,
>> uint8_t *mem_buf, int n)
>> }
>> return r;
>> }
>> +
>> +#ifndef CONFIG_USER_ONLY
>> +int ppc_gdb_gen_spr_xml(CPUState *cs)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> + GString *s = g_string_new(NULL);
>> + unsigned int num_regs = 0;
>> + int i;
>> +
>> + 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.power.spr\">");
>> +
>> + for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
>> + ppc_spr_t *spr = &env->spr_cb[i];
>> +
>> + if (!spr->name) {
>> + continue;
>> + }
>> +
>> + g_string_append_printf(s, "<reg name=\"%s\"",
>> + g_ascii_strdown(spr->name, -1));
>> + g_string_append_printf(s, " bitsize=\"%d\"", TARGET_LONG_BITS);
>> + g_string_append_printf(s, " group=\"spr\"/>");
>> +
>> + /*
>> + * GDB identifies registers based on the order they are
>> + * presented in the XML. These ids will not match QEMU's
>> + * representation (which follows the PowerISA).
>> + *
>> + * Store the position of the current register description so
>> + * we can make the correspondence later.
>> + */
>> + spr->gdb_id = num_regs;
>> + num_regs++;
>> + }
>> +
>> + g_string_append_printf(s, "</feature>");
>> + env->gdb_spr_xml = g_string_free(s, false);
>
> The g_string_free() documentation says that its up to the caller to g_free()
> the character data in this case. If the CPU gets hot-unplugged at some point,
> gdb_spr_xml is leaked since I see no g_free(env->gdb_spr_xml) in this patch.
>
> This makes me think that all CPUs are supposed to have the same set of SPRs.
> What about moving gdb_spr_xml to PowerPCCPUClass and have the first CPU
> to set it once and far all ?
Good idea. I'll do that.
Thank you!
>> + return num_regs;
>> +}
>> +
>> +const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> +
>> + if (strcmp(xml_name, "power-spr.xml") == 0) {
>> + return env->gdb_spr_xml;
>> + }
>> + return NULL;
>> +}
>> +#endif