Re: [PATCH v7 2/8] machine: Introduce helper is_cpu_type_supported()

[Gavin Shan]

Hi Phil,

On 11/28/23 20:38, Philippe Mathieu-Daudé wrote:
> On 27/11/23 00:12, Gavin Shan wrote:
> > The logic, to check if the specified CPU type is supported in
> > machine_run_board_init(), is independent enough. Factor it out into
> > helper is_cpu_type_supported(). machine_run_board_init() looks a bit
> > clean with this. Since we're here, @machine_class is renamed to @mc
> > to avoid multiple line spanning of code. The error messages and comments
> > are tweaked a bit either.
> >
> > No functional change intended.
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   hw/core/machine.c | 90 +++++++++++++++++++++++++++--------------------
> >   1 file changed, 51 insertions(+), 39 deletions(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index b3ef325936..05e1922b89 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1387,13 +1387,57 @@ out:
> >       return r;
> >   }
> > +static void is_cpu_type_supported(const MachineState *machine, Error **errp)
>
> Functions taking an Error** last argument should return a boolean value.

Correct, especially @errp instead of @local_err will be passed from
machine_run_board_init() to is_cpu_type_supported(). We needs an
indicator for machine_run_board_init() to bail immediately to avoid
calling mc->init() there in the failing cases.

> > +{
> > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > +    ObjectClass *oc = object_class_by_name(machine->cpu_type);
> > +    CPUClass *cc;
> > +    int i;
> > +
> > +    /*
> > +     * Check if the user specified CPU type is supported when the valid
> > +     * CPU types have been determined. Note that the user specified CPU
> > +     * type is provided through '-cpu' option.
> > +     */
> > +    if (mc->valid_cpu_types && machine->cpu_type) {
> > +        for (i = 0; mc->valid_cpu_types[i]; i++) {
> > +            if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
> > +                break;
> > +            }
> > +        }
> > +
> > +        /* The user specified CPU type isn't valid */
> > +        if (!mc->valid_cpu_types[i]) {
> > +            error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
> > +            if (!mc->valid_cpu_types[1]) {
> > +                error_append_hint(errp, "The only valid type is: %s",
> > +                                  mc->valid_cpu_types[0]);
> > +            } else {
> > +                error_append_hint(errp, "The valid types are: %s",
> > +                                  mc->valid_cpu_types[0]);
> > +            }
> > +
> > +            for (i = 1; mc->valid_cpu_types[i]; i++) {
> > +                error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
> > +            }
> > +
> > +            error_append_hint(errp, "\n");
> > +            return;
> > +        }
> > +    }
> > +
> > +    /* Check if CPU type is deprecated and warn if so */
> > +    cc = CPU_CLASS(oc);
> > +    if (cc && cc->deprecation_note) {
> > +        warn_report("CPU model %s is deprecated -- %s",
> > +                    machine->cpu_type, cc->deprecation_note);
>
> Why did you move the deprecation warning within the is_supported check?

This check is more relevant to CPU type, to check if the CPU type has
been deprecated. Besides, @oc and @cc can be dropped from 
machine_run_board_init().

> > +    }
> > +}
> >   void machine_run_board_init(MachineState *machine, const char *mem_path, 
> > Error **errp)
> >   {
> >       ERRP_GUARD();
> >       MachineClass *machine_class = MACHINE_GET_CLASS(machine);
> > -    ObjectClass *oc = object_class_by_name(machine->cpu_type);
> > -    CPUClass *cc;
> >       Error *local_err = NULL;
> >       /* This checkpoint is required by replay to separate prior clock
> > @@ -1449,43 +1493,11 @@ void machine_run_board_init(MachineState *machine, 
> > const char *mem_path, Error *
> >           machine->ram = machine_consume_memdev(machine, machine->memdev);
> >       }
> > -    /* If the machine supports the valid_cpu_types check and the user
> > -     * specified a CPU with -cpu check here that the user CPU is supported.
> > -     */
> > -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> > -        int i;
> > -
> > -        for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > -            if (object_class_dynamic_cast(oc,
> > -                                          machine_class->valid_cpu_types[i])) {
> > -                /* The user specified CPU is in the valid field, we are
> > -                 * good to go.
> > -                 */
> > -                break;
> > -            }
> > -        }
> > -
> > -        if (!machine_class->valid_cpu_types[i]) {
> > -            /* The user specified CPU is not valid */
> > -            error_setg(&local_err, "Invalid CPU type: %s", machine->cpu_type);
> > -            error_append_hint(&local_err, "The valid types are: %s",
> > -                              machine_class->valid_cpu_types[0]);
> > -            for (i = 1; machine_class->valid_cpu_types[i]; i++) {
> > -                error_append_hint(&local_err, ", %s",
> > -                                  machine_class->valid_cpu_types[i]);
> > -            }
> > -            error_append_hint(&local_err, "\n");
> > -
> > -            error_propagate(errp, local_err);
> > -            return;
> > -        }
> > -    }
> > -
> > -    /* Check if CPU type is deprecated and warn if so */
> > -    cc = CPU_CLASS(oc);
> > -    if (cc && cc->deprecation_note) {
> > -        warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
> > -                    cc->deprecation_note);
> > +    /* Check if the CPU type is supported */
> > +    is_cpu_type_supported(machine, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
>
> This becomes:
>
>         if (!is_cpu_type_supported(machine, errp)) {

Nod

> > +        return;
> >       }
> >       if (machine->cgs) {

Thanks,
Gavin