Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[Andreas Färber]

Am 01.09.2014 12:19, schrieb Peter Maydell:
> [ccing Andreas in case he wants to review the QOM aspects of this,
> though they're fairly straightforward I think.]
> 
> On 29 August 2014 14:52, Jens Freimann <address@hidden> wrote:
>> From: David Hildenbrand <address@hidden>
>>
>> This patch provides the name of the architecture in the target.xml if 
>> available.
>>
>> This allows the remote gdb to detect the target architecture on its own - so
>> there is no need to specify it manually (e.g. if gdb is started without a
>> binary) using "set arch *arch_name*".
> 
> This is neat; I didn't realise gdb let you do this.
> 
>> The name of the architecture has been added to all archs that provide a
>> target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
>> name in gdb's feature xml files.
> 
> What about 32-bit ARM? You set the architecture name for AArch64
> but not the 32 bit case.
> 
> Are there architectures that might need to specify something
> more complicated than "always the same string"? (ie is there
> a case for having the target provide a "return architecture name"
> method rather than a constant string?)
> 
>> Signed-off-by: David Hildenbrand <address@hidden>
>> Acked-by: Cornelia Huck <address@hidden>
>> Acked-by: Christian Borntraeger <address@hidden>
>> Signed-off-by: Jens Freimann <address@hidden>
>> Cc: Andrzej Zaborowski <address@hidden>
>> Cc: Peter Maydell <address@hidden>
>> Cc: Vassili Karpov (malc) <address@hidden>
>> ---
>>  gdbstub.c                   | 19 ++++++++++++-------
>>  include/qom/cpu.h           |  2 ++
>>  target-arm/cpu64.c          |  1 +
>>  target-ppc/translate_init.c |  2 ++
>>  target-s390x/cpu.c          |  1 +
>>  5 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 8afe0b7..af82259 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -523,13 +523,18 @@ static const char *get_feature_xml(const char *p, 
>> const char **newp,
>>              GDBRegisterState *r;
>>              CPUState *cpu = first_cpu;
>>
>> -            snprintf(target_xml, sizeof(target_xml),
>> -                     "<?xml version=\"1.0\"?>"
>> -                     "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
>> -                     "<target>"
>> -                     "<xi:include href=\"%s\"/>",
>> -                     cc->gdb_core_xml_file);
>> -
>> +            pstrcat(target_xml, sizeof(target_xml),
>> +                    "<?xml version=\"1.0\"?>"
>> +                    "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
>> +                    "<target>");
>> +            if (cc->gdb_arch_name) {
>> +                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
>> +                pstrcat(target_xml, sizeof(target_xml), cc->gdb_arch_name);
>> +                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
>> +            }
>> +            pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
>> +            pstrcat(target_xml, sizeof(target_xml), cc->gdb_core_xml_file);
>> +            pstrcat(target_xml, sizeof(target_xml), "\"/>");
>>              for (r = cpu->gdb_regs; r; r = r->next) {
>>                  pstrcat(target_xml, sizeof(target_xml), "<xi:include 
>> href=\"");
>>                  pstrcat(target_xml, sizeof(target_xml), r->xml);
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 1aafbf5..8828b16 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -98,6 +98,7 @@ struct TranslationBlock;
>>   * @vmsd: State description for migration.
>>   * @gdb_num_core_regs: Number of core registers accessible to GDB.
>>   * @gdb_core_xml_file: File name for core registers GDB XML description.
>> + * @gdb_arch_name: Architecture name known to GDB.
>>   *
>>   * Represents a CPU family or model.
>>   */
>> @@ -147,6 +148,7 @@ typedef struct CPUClass {
>>      const struct VMStateDescription *vmsd;
>>      int gdb_num_core_regs;
>>      const char *gdb_core_xml_file;
>> +    const char *gdb_arch_name;
>>  } CPUClass;
>>
>>  #ifdef HOST_WORDS_BIGENDIAN
>> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
>> index 38d2b84..9df7492 100644
>> --- a/target-arm/cpu64.c
>> +++ b/target-arm/cpu64.c
>> @@ -201,6 +201,7 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void 
>> *data)
>>      cc->gdb_write_register = aarch64_cpu_gdb_write_register;
>>      cc->gdb_num_core_regs = 34;
>>      cc->gdb_core_xml_file = "aarch64-core.xml";
>> +    cc->gdb_arch_name = "aarch64";
>>  }
>>
>>  static void aarch64_cpu_register(const ARMCPUInfo *info)
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 48177ed..7165347 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -9649,8 +9649,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void 
>> *data)
>>
>>  #if defined(TARGET_PPC64)
>>      cc->gdb_core_xml_file = "power64-core.xml";
>> +    cc->gdb_arch_name = "powerpc:common64";
>>  #else
>>      cc->gdb_core_xml_file = "power-core.xml";
>> +    cc->gdb_arch_name = "powerpc:common";
>>  #endif
>>  #ifndef CONFIG_USER_ONLY
>>      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 4b03e42..5dae93c 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -262,6 +262,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void 
>> *data)
>>      dc->vmsd = &vmstate_s390_cpu;
>>      cc->gdb_num_core_regs = S390_NUM_CORE_REGS;
>>      cc->gdb_core_xml_file = "s390x-core64.xml";
>> +    cc->gdb_arch_name = "s390:64-bit";
>>  }
>>
>>  static const TypeInfo s390_cpu_type_info = {
>> --
>> 1.8.5.5

Looks good to me and even got documented,

Reviewed-by: Andreas Färber <address@hidden>

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg