| Message ID | 1351652644-18687-4-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
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~
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?
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
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
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
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).
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
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
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(-)