Re: [PATCH v5 06/31] cpu: Add generic cpu_list()

[Philippe Mathieu-Daudé]

On 16/11/23 11:34, Gavin Shan wrote:
> Hi Phil,
>
> On 11/16/23 17:51, Philippe Mathieu-Daudé wrote:
> > On 16/11/23 08:39, Philippe Mathieu-Daudé wrote:
> > > On 15/11/23 00:56, Gavin Shan wrote:
> > > > Add generic cpu_list() to replace the individual target's 
> > > > implementation
> > > > in the subsequent commits. Currently, there are 3 targets with no 
> > > > cpu_list()
> > > > implementation: microblaze and nios2. With this applied, those two 
> > > > targets
> > > > switch to the generic cpu_list().
> > > >
> > > > [gshan@gshan q]$ ./build/qemu-system-microblaze -cpu ?
> > > > Available CPUs:
> > > >    microblaze-cpu
> > > >
> > > > [gshan@gshan q]$ ./build/qemu-system-nios2 -cpu ?
> > > > Available CPUs:
> > > >    nios2-cpu
> > > >
> > > > Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> > > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > > > ---
> > > >   bsd-user/main.c |  5 +----
> > > >   cpu-target.c    | 29 ++++++++++++++++++++++++++---
> > > >   2 files changed, 27 insertions(+), 7 deletions(-)
> > >
> > >
> > > > diff --git a/cpu-target.c b/cpu-target.c
> > > > index c078c0e91b..acfc654b95 100644
> > > > --- a/cpu-target.c
> > > > +++ b/cpu-target.c
> > > > @@ -24,6 +24,7 @@
> > > >   #include "hw/qdev-core.h"
> > > >   #include "hw/qdev-properties.h"
> > > >   #include "qemu/error-report.h"
> > > > +#include "qemu/qemu-print.h"
> > > >   #include "migration/vmstate.h"
> > > >   #ifdef CONFIG_USER_ONLY
> > > >   #include "qemu.h"
> > > > @@ -283,12 +284,34 @@ const char *parse_cpu_option(const char 
> > > > *cpu_option)
> > > >       return cpu_type;
> > > >   }
> > > > +#ifndef cpu_list
> > > > +static void cpu_list_entry(gpointer data, gpointer user_data)
> > > > +{
> > > > +    CPUClass *cc = CPU_CLASS(OBJECT_CLASS(data));
> > > > +    const char *typename = object_class_get_name(OBJECT_CLASS(data));
> > > > +    g_autofree char *model = cpu_model_from_type(typename);
> > > > +
> > > > +    if (cc->deprecation_note) {
> > > > +        qemu_printf("  %s (deprecated)\n", model);
> > > > +    } else {
> > > > +        qemu_printf("  %s\n", model);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void cpu_list(void)
> > > > +{
> > > > +    GSList *list;
> > > > +
> > > > +    list = object_class_get_list_sorted(TYPE_CPU, false);
> > > > +    qemu_printf("Available CPUs:\n");
> > >
> > > Since this output will likely be displayed a lot, IMHO it is worth
> > > doing a first pass to get the number of available CPUs. If it is 1,
> > > print using singular but even better smth like:
> > >
> > >         "This machine can only be used with the following CPU:"
> >
> > Hmm I missed this code is common to user/system emulation.
> >
> > System helper could be clever by using the intersection of cpu_list()
> > and MachineClass::valid_cpu_types[] sets.
>
> When cpu_list() is called, it's possible the machine type option isn't
> parsed yet. Besides, this function is usually used by "qemu-system-arm 
> -cpu ?".

Not sure this is a good example :)

  $ qemu-system-arm -cpu ?
  qemu-system-arm: No machine specified, and there is no default
  Use -machine help to list supported machines

> So I wouldn't connect to MachineClass::valid_cpu_types[] here if you agree.

Agreed.