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

2014-10-06 Thread Cornelia Huck 
On Tue, 30 Sep 2014 17:23:47 +0200
Jens Freimann jf...@linux.vnet.ibm.com wrote:

 From: David Hildenbrand d...@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 d...@linux.vnet.ibm.com
 Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
 Acked-by: Christian Borntraeger borntrae...@de.ibm.com
 Signed-off-by: Jens Freimann jf...@linux.vnet.ibm.com
 Reviewed-by: Andreas Färber afaer...@suse.de
 Cc: Andrzej Zaborowski balr...@gmail.com
 Cc: Peter Maydell peter.mayd...@linaro.org
 Cc: Vassili Karpov (malc) av1...@comtv.ru
 CC: Edgar Iglesias edgar.igles...@gmail.com
 CC: Richard Henderson r...@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

2014-10-06 Thread Peter Maydell 
On 6 October 2014 16:08, Cornelia Huck cornelia.h...@de.ibm.com wrote:
 On Tue, 30 Sep 2014 17:23:47 +0200
 Jens Freimann jf...@linux.vnet.ibm.com wrote:

 From: David Hildenbrand d...@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 d...@linux.vnet.ibm.com
 Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
 Acked-by: Christian Borntraeger borntrae...@de.ibm.com
 Signed-off-by: Jens Freimann jf...@linux.vnet.ibm.com
 Reviewed-by: Andreas Färber afaer...@suse.de
 Cc: Andrzej Zaborowski balr...@gmail.com
 Cc: Peter Maydell peter.mayd...@linaro.org
 Cc: Vassili Karpov (malc) av1...@comtv.ru
 CC: Edgar Iglesias edgar.igles...@gmail.com
 CC: Richard Henderson r...@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

2014-10-06 Thread Peter Maydell 
On 30 September 2014 16:23, Jens Freimann jf...@linux.vnet.ibm.com wrote:
 From: David Hildenbrand d...@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

2014-10-06 Thread David Hildenbrand 
 On 6 October 2014 16:08, Cornelia Huck cornelia.h...@de.ibm.com wrote:
  On Tue, 30 Sep 2014 17:23:47 +0200
  Jens Freimann jf...@linux.vnet.ibm.com wrote:
 
  From: David Hildenbrand d...@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 d...@linux.vnet.ibm.com
  Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
  Acked-by: Christian Borntraeger borntrae...@de.ibm.com
  Signed-off-by: Jens Freimann jf...@linux.vnet.ibm.com
  Reviewed-by: Andreas Färber afaer...@suse.de
  Cc: Andrzej Zaborowski balr...@gmail.com
  Cc: Peter Maydell peter.mayd...@linaro.org
  Cc: Vassili Karpov (malc) av1...@comtv.ru
  CC: Edgar Iglesias edgar.igles...@gmail.com
  CC: Richard Henderson r...@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

2014-10-06 Thread Peter Maydell 
On 6 October 2014 20:14, David Hildenbrand d...@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

2014-10-06 Thread David Hildenbrand 
 On 30 September 2014 16:23, Jens Freimann jf...@linux.vnet.ibm.com wrote:
  From: David Hildenbrand d...@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

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

2014-10-06 Thread David Hildenbrand 
 On 6 October 2014 20:14, David Hildenbrand d...@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

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

2014-09-30 Thread Jens Freimann 
From: David Hildenbrand d...@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 d...@linux.vnet.ibm.com
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
Acked-by: Christian Borntraeger borntrae...@de.ibm.com
Signed-off-by: Jens Freimann jf...@linux.vnet.ibm.com
Reviewed-by: Andreas Färber afaer...@suse.de
Cc: Andrzej Zaborowski balr...@gmail.com
Cc: Peter Maydell peter.mayd...@linaro.org
Cc: Vassili Karpov (malc) av1...@comtv.ru
CC: Edgar Iglesias edgar.igles...@gmail.com
CC: Richard Henderson r...@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