Patchwork [3/7] target-alpha: Add support for -cpu ?

login
register
mail settings
Submitter Andreas Färber
Date Oct. 31, 2012, 3:04 a.m.
Message ID <1351652644-18687-4-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/195724/
State New
Headers show

Comments

Andreas Färber - Oct. 31, 2012, 3:04 a.m.
Implement alphabetical listing of CPU subclasses.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-alpha/cpu.c |   41 +++++++++++++++++++++++++++++++++++++++++
 target-alpha/cpu.h |    4 +++-
 2 Dateien geändert, 44 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
Richard Henderson - Oct. 31, 2012, 5:01 a.m.
On 2012-10-31 14:04, Andreas Färber wrote:
> Implement alphabetical listing of CPU subclasses.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>

There doesn't seem to be anything alpha-specific about this.
Does it really need to be replicated?  That said,

Acked-by: Richard Henderson <rth@twiddle.net>


r~
Eduardo Habkost - Dec. 6, 2012, 3:37 p.m.
On Wed, Oct 31, 2012 at 04:04:00AM +0100, Andreas Färber wrote:
> Implement alphabetical listing of CPU subclasses.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  target-alpha/cpu.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  target-alpha/cpu.h |    4 +++-
>  2 Dateien geändert, 44 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
> 
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index e1a5739..ab25c44 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -23,6 +23,47 @@
>  #include "qemu-common.h"
>  
>  
> +typedef struct AlphaCPUListState {
> +    fprintf_function cpu_fprintf;
> +    FILE *file;
> +} AlphaCPUListState;
> +
> +/* Sort alphabetically by type name. */
> +static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
> +{
> +    ObjectClass *class_a = (ObjectClass *)a;
> +    ObjectClass *class_b = (ObjectClass *)b;
> +    const char *name_a, *name_b;
> +
> +    name_a = object_class_get_name(class_a);
> +    name_b = object_class_get_name(class_b);
> +    return strcmp(name_a, name_b);
> +}
> +
> +static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
> +{
> +    ObjectClass *oc = data;
> +    AlphaCPUListState *s = user_data;
> +
> +    (*s->cpu_fprintf)(s->file, "  %s\n",
> +                      object_class_get_name(oc));
> +}
> +
> +void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> +{
> +    AlphaCPUListState s = {
> +        .file = f,
> +        .cpu_fprintf = cpu_fprintf,
> +    };
> +    GSList *list;
> +
> +    list = object_class_get_list(TYPE_ALPHA_CPU, false);
> +    list = g_slist_sort(list, alpha_cpu_list_compare);
> +    (*cpu_fprintf)(f, "Available CPUs:\n");
> +    g_slist_foreach(list, alpha_cpu_list_entry, &s);
> +    g_slist_free(list);
> +}

target-arm has very similar code. Isn't it better to first write a
common reusable function to list CPU models using the list of
subclasses, instead of adding very similar functions to all
architectures?
Andreas Färber - Dec. 6, 2012, 3:42 p.m.
Am 06.12.2012 16:37, schrieb Eduardo Habkost:
> On Wed, Oct 31, 2012 at 04:04:00AM +0100, Andreas Färber wrote:
>> Implement alphabetical listing of CPU subclasses.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  target-alpha/cpu.c |   41 +++++++++++++++++++++++++++++++++++++++++
>>  target-alpha/cpu.h |    4 +++-
>>  2 Dateien geändert, 44 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
>>
>> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
>> index e1a5739..ab25c44 100644
>> --- a/target-alpha/cpu.c
>> +++ b/target-alpha/cpu.c
>> @@ -23,6 +23,47 @@
>>  #include "qemu-common.h"
>>  
>>  
>> +typedef struct AlphaCPUListState {
>> +    fprintf_function cpu_fprintf;
>> +    FILE *file;
>> +} AlphaCPUListState;
>> +
>> +/* Sort alphabetically by type name. */
>> +static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
>> +{
>> +    ObjectClass *class_a = (ObjectClass *)a;
>> +    ObjectClass *class_b = (ObjectClass *)b;
>> +    const char *name_a, *name_b;
>> +
>> +    name_a = object_class_get_name(class_a);
>> +    name_b = object_class_get_name(class_b);
>> +    return strcmp(name_a, name_b);
>> +}
>> +
>> +static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
>> +{
>> +    ObjectClass *oc = data;
>> +    AlphaCPUListState *s = user_data;
>> +
>> +    (*s->cpu_fprintf)(s->file, "  %s\n",
>> +                      object_class_get_name(oc));
>> +}
>> +
>> +void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>> +{
>> +    AlphaCPUListState s = {
>> +        .file = f,
>> +        .cpu_fprintf = cpu_fprintf,
>> +    };
>> +    GSList *list;
>> +
>> +    list = object_class_get_list(TYPE_ALPHA_CPU, false);
>> +    list = g_slist_sort(list, alpha_cpu_list_compare);
>> +    (*cpu_fprintf)(f, "Available CPUs:\n");
>> +    g_slist_foreach(list, alpha_cpu_list_entry, &s);
>> +    g_slist_free(list);
>> +}
> 
> target-arm has very similar code. Isn't it better to first write a
> common reusable function to list CPU models using the list of
> subclasses, instead of adding very similar functions to all
> architectures?

Most ordering functions vary slightly (target-arm for "any"). It would
be possible to generalize the struct and provide a wrapper with type and
callback arguments, but then again some functions add a header line like
here, some don't, and some even hardcode some options like "host". For
the targets that already had -cpu ? support before QOM I tried to keep
output identical apart from possibly the order.

Andreas
Peter Maydell - Dec. 6, 2012, 3:59 p.m.
On 6 December 2012 15:37, Eduardo Habkost <ehabkost@redhat.com> wrote:
> target-arm has very similar code. Isn't it better to first write a
> common reusable function to list CPU models using the list of
> subclasses, instead of adding very similar functions to all
> architectures?

What would be particularly useful to have as a common
utility routine is support for x86-style +feature,-feature
syntax. At the moment we don't implement that on most
other targets but it would be good to have that on ARM
at some point.

-- PMM
Andreas Färber - Dec. 6, 2012, 4:02 p.m.
Am 06.12.2012 16:42, schrieb Andreas Färber:
> Am 06.12.2012 16:37, schrieb Eduardo Habkost:
>> On Wed, Oct 31, 2012 at 04:04:00AM +0100, Andreas Färber wrote:
>>> Implement alphabetical listing of CPU subclasses.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>>  target-alpha/cpu.c |   41 +++++++++++++++++++++++++++++++++++++++++
>>>  target-alpha/cpu.h |    4 +++-
>>>  2 Dateien geändert, 44 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
>>>
>>> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
>>> index e1a5739..ab25c44 100644
>>> --- a/target-alpha/cpu.c
>>> +++ b/target-alpha/cpu.c
>>> @@ -23,6 +23,47 @@
>>>  #include "qemu-common.h"
>>>  
>>>  
>>> +typedef struct AlphaCPUListState {
>>> +    fprintf_function cpu_fprintf;
>>> +    FILE *file;
>>> +} AlphaCPUListState;
>>> +
>>> +/* Sort alphabetically by type name. */
>>> +static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
>>> +{
>>> +    ObjectClass *class_a = (ObjectClass *)a;
>>> +    ObjectClass *class_b = (ObjectClass *)b;
>>> +    const char *name_a, *name_b;
>>> +
>>> +    name_a = object_class_get_name(class_a);
>>> +    name_b = object_class_get_name(class_b);
>>> +    return strcmp(name_a, name_b);
>>> +}
>>> +
>>> +static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
>>> +{
>>> +    ObjectClass *oc = data;
>>> +    AlphaCPUListState *s = user_data;
>>> +
>>> +    (*s->cpu_fprintf)(s->file, "  %s\n",
>>> +                      object_class_get_name(oc));
>>> +}
>>> +
>>> +void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>>> +{
>>> +    AlphaCPUListState s = {
>>> +        .file = f,
>>> +        .cpu_fprintf = cpu_fprintf,
>>> +    };
>>> +    GSList *list;
>>> +
>>> +    list = object_class_get_list(TYPE_ALPHA_CPU, false);
>>> +    list = g_slist_sort(list, alpha_cpu_list_compare);
>>> +    (*cpu_fprintf)(f, "Available CPUs:\n");
>>> +    g_slist_foreach(list, alpha_cpu_list_entry, &s);
>>> +    g_slist_free(list);
>>> +}
>>
>> target-arm has very similar code. Isn't it better to first write a
>> common reusable function to list CPU models using the list of
>> subclasses, instead of adding very similar functions to all
>> architectures?
> 
> Most ordering functions vary slightly (target-arm for "any"). It would
> be possible to generalize the struct and provide a wrapper with type and
> callback arguments,

Just remembered Anthony being against callbacks in this context:

http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02944.html

The RFC was for specifically for implementing the CPU lists. So I used
g_slist_* instead as suggested, which duplicates a few lines FWIW.
If someone has suggestions how else to share more code, I'm all ears.

Andreas

> but then again some functions add a header line like
> here, some don't, and some even hardcode some options like "host". For
> the targets that already had -cpu ? support before QOM I tried to keep
> output identical apart from possibly the order.
> 
> Andreas
Eduardo Habkost - Dec. 6, 2012, 4:10 p.m.
On Thu, Dec 06, 2012 at 03:59:46PM +0000, Peter Maydell wrote:
> On 6 December 2012 15:37, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > target-arm has very similar code. Isn't it better to first write a
> > common reusable function to list CPU models using the list of
> > subclasses, instead of adding very similar functions to all
> > architectures?
> 
> What would be particularly useful to have as a common
> utility routine is support for x86-style +feature,-feature
> syntax. At the moment we don't implement that on most
> other targets but it would be good to have that on ARM
> at some point.

Once we finish the work on x86, I plan to make the code reusable by
other architectures.

That would include the +feature,-feature parsing and the CPU model ->
CPU class lookup (this one may be more complicated to make reusable, but
I think it's doable).
Eduardo Habkost - Dec. 6, 2012, 5:45 p.m.
On Thu, Dec 06, 2012 at 05:02:52PM +0100, Andreas Färber wrote:
> Am 06.12.2012 16:42, schrieb Andreas Färber:
> > Am 06.12.2012 16:37, schrieb Eduardo Habkost:
> >> On Wed, Oct 31, 2012 at 04:04:00AM +0100, Andreas Färber wrote:
> >>> Implement alphabetical listing of CPU subclasses.
> >>>
> >>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >>> ---
> >>>  target-alpha/cpu.c |   41 +++++++++++++++++++++++++++++++++++++++++
> >>>  target-alpha/cpu.h |    4 +++-
> >>>  2 Dateien geändert, 44 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
> >>>
> >>> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> >>> index e1a5739..ab25c44 100644
> >>> --- a/target-alpha/cpu.c
> >>> +++ b/target-alpha/cpu.c
> >>> @@ -23,6 +23,47 @@
> >>>  #include "qemu-common.h"
> >>>  
> >>>  
> >>> +typedef struct AlphaCPUListState {
> >>> +    fprintf_function cpu_fprintf;
> >>> +    FILE *file;
> >>> +} AlphaCPUListState;
> >>> +
> >>> +/* Sort alphabetically by type name. */
> >>> +static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
> >>> +{
> >>> +    ObjectClass *class_a = (ObjectClass *)a;
> >>> +    ObjectClass *class_b = (ObjectClass *)b;
> >>> +    const char *name_a, *name_b;
> >>> +
> >>> +    name_a = object_class_get_name(class_a);
> >>> +    name_b = object_class_get_name(class_b);
> >>> +    return strcmp(name_a, name_b);
> >>> +}
> >>> +
> >>> +static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
> >>> +{
> >>> +    ObjectClass *oc = data;
> >>> +    AlphaCPUListState *s = user_data;
> >>> +
> >>> +    (*s->cpu_fprintf)(s->file, "  %s\n",
> >>> +                      object_class_get_name(oc));
> >>> +}
> >>> +
> >>> +void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> >>> +{
> >>> +    AlphaCPUListState s = {
> >>> +        .file = f,
> >>> +        .cpu_fprintf = cpu_fprintf,
> >>> +    };
> >>> +    GSList *list;
> >>> +
> >>> +    list = object_class_get_list(TYPE_ALPHA_CPU, false);
> >>> +    list = g_slist_sort(list, alpha_cpu_list_compare);
> >>> +    (*cpu_fprintf)(f, "Available CPUs:\n");
> >>> +    g_slist_foreach(list, alpha_cpu_list_entry, &s);
> >>> +    g_slist_free(list);
> >>> +}
> >>
> >> target-arm has very similar code. Isn't it better to first write a
> >> common reusable function to list CPU models using the list of
> >> subclasses, instead of adding very similar functions to all
> >> architectures?
> > 
> > Most ordering functions vary slightly (target-arm for "any"). It would
> > be possible to generalize the struct and provide a wrapper with type and
> > callback arguments,
> 
> Just remembered Anthony being against callbacks in this context:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02944.html
> 
> The RFC was for specifically for implementing the CPU lists. So I used
> g_slist_* instead as suggested, which duplicates a few lines FWIW.
> If someone has suggestions how else to share more code, I'm all ears.

We could simply reuse the existing arch_query_cpu_definitions() interface to
implement cpu_list(), and the target-specific arch_query_cpu_definitions()
could reorder the list any way it wants. The list could then be used for both
cpu_list() and the the QMP query-cpu-definitions command.

If necessary, we can add a "description" field to CpuDefinitionInfo, so targets
can optionally return a description of each CPU model, too (that's the case for
the current x86 cpu_list() output).

 
> Andreas
> 
> > but then again some functions add a header line like
> > here, some don't, and some even hardcode some options like "host". For
> > the targets that already had -cpu ? support before QOM I tried to keep
> > output identical apart from possibly the order.
> > 
> > Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Patch

diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
index e1a5739..ab25c44 100644
--- a/target-alpha/cpu.c
+++ b/target-alpha/cpu.c
@@ -23,6 +23,47 @@ 
 #include "qemu-common.h"
 
 
+typedef struct AlphaCPUListState {
+    fprintf_function cpu_fprintf;
+    FILE *file;
+} AlphaCPUListState;
+
+/* Sort alphabetically by type name. */
+static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+    ObjectClass *class_a = (ObjectClass *)a;
+    ObjectClass *class_b = (ObjectClass *)b;
+    const char *name_a, *name_b;
+
+    name_a = object_class_get_name(class_a);
+    name_b = object_class_get_name(class_b);
+    return strcmp(name_a, name_b);
+}
+
+static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    AlphaCPUListState *s = user_data;
+
+    (*s->cpu_fprintf)(s->file, "  %s\n",
+                      object_class_get_name(oc));
+}
+
+void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf)
+{
+    AlphaCPUListState s = {
+        .file = f,
+        .cpu_fprintf = cpu_fprintf,
+    };
+    GSList *list;
+
+    list = object_class_get_list(TYPE_ALPHA_CPU, false);
+    list = g_slist_sort(list, alpha_cpu_list_compare);
+    (*cpu_fprintf)(f, "Available CPUs:\n");
+    g_slist_foreach(list, alpha_cpu_list_entry, &s);
+    g_slist_free(list);
+}
+
 /* Models */
 
 static void ev4_cpu_initfn(Object *obj)
diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
index 8f131b7..28999ab 100644
--- a/target-alpha/cpu.h
+++ b/target-alpha/cpu.h
@@ -298,6 +298,7 @@  struct CPUAlphaState {
 };
 
 #define cpu_init cpu_alpha_init
+#define cpu_list alpha_cpu_list
 #define cpu_exec cpu_alpha_exec
 #define cpu_gen_code cpu_alpha_gen_code
 #define cpu_signal_handler cpu_alpha_signal_handler
@@ -434,7 +435,8 @@  enum {
     IR_ZERO = 31,
 };
 
-CPUAlphaState * cpu_alpha_init (const char *cpu_model);
+CPUAlphaState *cpu_alpha_init(const char *cpu_model);
+void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 int cpu_alpha_exec(CPUAlphaState *s);
 /* you can call this signal handler from your SIGBUS and SIGSEGV
    signal handlers to inform the virtual CPU of exceptions. non zero