[Qemu-devel] [PATCH] gdb: detect target architecture via target.xml

[Qemu-devel] [PATCH] gdb: detect target architecture via target.xml

[Jens Freimann]

Conny, Alex, Christian,

this patch has been sent before as part of a gdb series a few weeks ago.

It contains only names for architectures that are also defined 
in the xml files that are shipped with gdb.

Discussion revealed that the 32bit arm case can easily be added as an 
follow-up patch. Therefore this patch is left unchanged. 

Adding Emanuel and Richard to Cc since they were involved in the discussion last time

David Hildenbrand (1):
  gdb: provide the name of the architecture in the target.xml

 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(-)

-- 
1.8.5.5

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

[Jens Freimann]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

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*".

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.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Cc: Andrzej Zaborowski <balrogg@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Vassili Karpov (malc) <av1474@comtv.ru>
CC: Edgar Iglesias <edgar.iglesias@gmail.com>
CC: Richard Henderson <rth@twiddle.net>
---
 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 71aaa23..21d78f8 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 f576b47..0ceb99d 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -99,6 +99,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.
  * @cpu_exec_enter: Callback for cpu_exec preparation.
  * @cpu_exec_exit: Callback for cpu_exec cleanup.
  * @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec.
@@ -152,6 +153,7 @@ typedef struct CPUClass {
     const struct VMStateDescription *vmsd;
     int gdb_num_core_regs;
     const char *gdb_core_xml_file;
+    const char *gdb_arch_name;
 
     void (*cpu_exec_enter)(CPUState *cpu);
     void (*cpu_exec_exit)(CPUState *cpu);
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index c30f47e..a2b855c 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -203,6 +203,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 65b840d..bd9a26f 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9659,8 +9659,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 c9c237f..8447701 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -385,6 +385,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
 #endif
     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

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

[Cornelia Huck]

On Tue, 30 Sep 2014 17:23:47 +0200
Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:

> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> 
> 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*".
> 
> 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.
> 
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> Cc: Andrzej Zaborowski <balrogg@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Vassili Karpov (malc) <av1474@comtv.ru>
> CC: Edgar Iglesias <edgar.iglesias@gmail.com>
> CC: Richard Henderson <rth@twiddle.net>
> ---
>  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(-)
> 
I will send this with the next pile of s390x updates, unless someone on
cc: has any objections.

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

[Peter Maydell]

On 6 October 2014 16:08, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Tue, 30 Sep 2014 17:23:47 +0200
> Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
>
>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>
>> 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*".
>>
>> 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.
>>
>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> Cc: Andrzej Zaborowski <balrogg@gmail.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Vassili Karpov (malc) <av1474@comtv.ru>
>> CC: Edgar Iglesias <edgar.iglesias@gmail.com>
>> CC: Richard Henderson <rth@twiddle.net>
>> ---
>>  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(-)
>>
> I will send this with the next pile of s390x updates, unless someone on
> cc: has any objections.

I'm still hoping for an answer about why this is setting
the name for 64 bit ARM and not 32 bit ARM, and whether
there are any cases which need to actually be able to set
the architecture name in a more complicated name than
simply a string. I raised those in the last lot of review
and there doesn't seem to have been any answer.

thanks
-- PMM

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

[David Hildenbrand]

> On 6 October 2014 16:08, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > On Tue, 30 Sep 2014 17:23:47 +0200
> > Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
> >
> >> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >>
> >> 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*".
> >>
> >> 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.
> >>
> >> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> >> Reviewed-by: Andreas Färber <afaerber@suse.de>
> >> Cc: Andrzej Zaborowski <balrogg@gmail.com>
> >> Cc: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: Vassili Karpov (malc) <av1474@comtv.ru>
> >> CC: Edgar Iglesias <edgar.iglesias@gmail.com>
> >> CC: Richard Henderson <rth@twiddle.net>
> >> ---
> >>  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(-)
> >>
> > I will send this with the next pile of s390x updates, unless someone on
> > cc: has any objections.
> 
> I'm still hoping for an answer about why this is setting
> the name for 64 bit ARM and not 32 bit ARM, and whether
> there are any cases which need to actually be able to set
> the architecture name in a more complicated name than
> simply a string. I raised those in the last lot of review
> and there doesn't seem to have been any answer.
> 
> thanks
> -- PMM
> 

Hi Peter,

actually the questions were addressed in the last review. Haven't received any
answer from you to my reply. Maybe some mails got lost in the system.

32bit arm:
-"On my way through the possible architecture names
  (binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right name
  for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
  adapts to the xml files from gdb."
- Not included in this patch as Edgar provided a patch in the previous thread
  (that's why he is on cc) that can easily be adopted. I don't want to simply
  include his effort in my patch :) And we have to make sure that this name is
  the right one.

More complicated names:
- "The architecture should be known at the same point when specifying the xml
  file. So if anyone can come up with the proper arm name in the future (or even some
  kind of detection algorithm), it can simply be set in target-arm/cpu.c (after
  "arm-core.xml")."
- The same should apply for all architectures. So we can set (or even build)
  the proper string when also specifying the core xml file.

Do you have something in mind like your "powerpc:common" and "powerpc:e500"
example? To build the names based on some pattern?

I don't think that we can generalize the name format here (at least "aarch64"
makes me assume that :) ). I think it would be enough to set the complete
strings in the class init functions.

What do you think? Any suggestions?

Thanks!

David

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

[Peter Maydell]

On 6 October 2014 20:14, David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:
> actually the questions were addressed in the last review. Haven't received any
> answer from you to my reply. Maybe some mails got lost in the system.
>
> 32bit arm:
> -"On my way through the possible architecture names
>   (binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right name
>   for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
>   adapts to the xml files from gdb."

But can we specify it for 32 bit ARM? Saying "set arch arm" at
a gdb prompt works OK, so can we pass "arm" through in the XML too?
The gdb documentation says that anything that works as an arch
string at the gdb prompt is OK to pass through in the XML.

> - Not included in this patch as Edgar provided a patch in the previous thread
>   (that's why he is on cc) that can easily be adopted. I don't want to simply
>   include his effort in my patch :) And we have to make sure that this name is
>   the right one.
>
> More complicated names:
> - "The architecture should be known at the same point when specifying the xml
>   file. So if anyone can come up with the proper arm name in the future (or even some
>   kind of detection algorithm), it can simply be set in target-arm/cpu.c (after
>   "arm-core.xml")."

At the moment we specify the XML file in the CPU class init
function, whereas specific CPU subfeatures may not be known
until rather later when we actually instantiate a CPU
object. Are there any cases where we'd actually need to care?
I can't tell, because you seem to be taking the "we're going
to ignore all the CPU types where the answer isn't obvious"
approach. The ARM "iwmmxt" architecture would be an interesting
one to check, since if we really need to use that string rather
than a generic "arm" string we need to postpone choosing the
name til CPU init time when we set up the feature bits.

> - The same should apply for all architectures. So we can set (or even build)
>   the proper string when also specifying the core xml file.
>
> Do you have something in mind like your "powerpc:common" and "powerpc:e500"
> example? To build the names based on some pattern?
>
> I don't think that we can generalize the name format here (at least "aarch64"
> makes me assume that :) ). I think it would be enough to set the complete
> strings in the class init functions.

Only if the complete string doesn't depend on things we don't
know at that point. Maybe we can always pick a generic
name for each architecture, in which case we should do that
(and do it for all architectures now, not only three).
What does gdb do differently if you specify "powerpc:common"
versus "powerpc:e500", for instance? Is it anything we care
about?

thanks
-- PMM

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

[David Hildenbrand]

> On 6 October 2014 20:14, David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:
> > actually the questions were addressed in the last review. Haven't received any
> > answer from you to my reply. Maybe some mails got lost in the system.
> >
> > 32bit arm:
> > -"On my way through the possible architecture names
> >   (binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right name
> >   for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
> >   adapts to the xml files from gdb."
> 
> But can we specify it for 32 bit ARM? Saying "set arch arm" at
> a gdb prompt works OK, so can we pass "arm" through in the XML too?
> The gdb documentation says that anything that works as an arch
> string at the gdb prompt is OK to pass through in the XML.

Yes, arm should work. But this whole armvX thing (see my other mail) makes me
wonder which is the right one. And if there is any difference (at least for
our purpose).

> 
> > - Not included in this patch as Edgar provided a patch in the previous thread
> >   (that's why he is on cc) that can easily be adopted. I don't want to simply
> >   include his effort in my patch :) And we have to make sure that this name is
> >   the right one.
> >
> > More complicated names:
> > - "The architecture should be known at the same point when specifying the xml
> >   file. So if anyone can come up with the proper arm name in the future (or even some
> >   kind of detection algorithm), it can simply be set in target-arm/cpu.c (after
> >   "arm-core.xml")."
> 
> At the moment we specify the XML file in the CPU class init
> function, whereas specific CPU subfeatures may not be known
> until rather later when we actually instantiate a CPU
> object. Are there any cases where we'd actually need to care?
> I can't tell, because you seem to be taking the "we're going
> to ignore all the CPU types where the answer isn't obvious"
> approach. The ARM "iwmmxt" architecture would be an interesting
> one to check, since if we really need to use that string rather
> than a generic "arm" string we need to postpone choosing the
> name til CPU init time when we set up the feature bits.

You're right, I was most likely only thinking about the kvm case :) But we of
course chose to emulate (almost) every cpu version/feature we want.

Even if the e500 might work with the common definition, you identified one
potential case that won't work, so we can't go with this generic approach.

So should each architecture rather provide a function that gives the name back?
Like
- NULL if not known
- Generic name if not further specified
- Complex name if specific version was initialized?

> 
> > - The same should apply for all architectures. So we can set (or even build)
> >   the proper string when also specifying the core xml file.
> >
> > Do you have something in mind like your "powerpc:common" and "powerpc:e500"
> > example? To build the names based on some pattern?
> >
> > I don't think that we can generalize the name format here (at least "aarch64"
> > makes me assume that :) ). I think it would be enough to set the complete
> > strings in the class init functions.
> 
> Only if the complete string doesn't depend on things we don't
> know at that point. Maybe we can always pick a generic
> name for each architecture, in which case we should do that
> (and do it for all architectures now, not only three).
> What does gdb do differently if you specify "powerpc:common"
> versus "powerpc:e500", for instance? Is it anything we care
> about?

I would have thought that the generic one would have worked for us (especially
because the gdb supplied xml files don't contain most of the special names). But
you point was right.

We should rather focus on the really supported architectures than on the ones
given in the xml files.

Thanks!

David

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

[Peter Maydell]

On 30 September 2014 16:23, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>
> 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*".
>
> 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.

gdb seems to support more than one powerpc architecture
name. Do we need to report "powerpc:e500" for
our e500 cpu models, for instance?

-- PMM

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

[David Hildenbrand]

> On 30 September 2014 16:23, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
> > From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >
> > 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*".
> >
> > 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.
> 
> gdb seems to support more than one powerpc architecture
> name. Do we need to report "powerpc:e500" for
> our e500 cpu models, for instance?
> 
> -- PMM
> 

Hi Peter,

good point. I was hoping for more feedback from the powerpc folks.

My gdb multi-arch seems to support the following architectures:

(gdb) set arch
Requires an argument. Valid arguments are i386, i386:x86-64, i386:x64-32, i8086, i386:intel, i386:x86-64:intel, i386:x64-32:intel, i386:nacl, i386:x86-64:nacl, i386:x64-32:nacl, s390:64-bit, rs6000:6000, rs6000:rs1, rs6000:rsc, rs6000:rs2, powerpc:common64, powerpc:common, powerpc:603, powerpc:EC603e, powerpc:604, powerpc:403, powerpc:601, powerpc:620, powerpc:630, powerpc:a35, powerpc:rs64ii, powerpc:rs64iii, powerpc:7400, powerpc:e500, powerpc:e500mc, powerpc:e500mc64, powerpc:MPC8XX, powerpc:750, powerpc:titan, powerpc:vle, powerpc:e5500, powerpc:e6500, arm, armv2, armv2a, armv3, armv3m, armv4, armv4t, armv5, armv5t, armv5te, xscale, ep9312, iwmmxt, iwmmxt2, aarch64, aarch64:ilp32, auto.

However I am not sure if there are duplicates / compatible ones among them. The
available registers are all defined in the XML supplied by the gdbserver - so
I am not sure if they are "part" of the more specific architecture names.

Maybe it makes sense to leave powerpc and arm out of this patch. So I would
just set s390:64-bit in the first shot (unless there are any experts saying
that e.g. powerpc:common always works). At least for s390:64-bit I am very
sure :)

Of course, the mechanism to set the name should be flexible enough (if we find
the existing one to be too strict).

Thanks!

David