Re: [Qemu-devel] [PATCH 3/7] target-alpha: Add support for -cpu ?

[Eduardo Habkost]

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 <address@hidden>
> >>> ---
> >>>  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

-- 
Eduardo