Re: [PATCH v8 2/9] machine: Introduce helper is_cpu_type_supported()

[Gavin Shan]

Hi Phil,

On 12/1/23 20:53, Philippe Mathieu-Daudé wrote:
> On 29/11/23 05:20, 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 comments are tweaked a bit
> > either.
> >
> > No functional change intended.
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> > v8: Move the precise message hint to PATCH[v8 3/9]        (Gavin)
> > ---
> >   hw/core/machine.c | 83 +++++++++++++++++++++++++----------------------
> >   1 file changed, 45 insertions(+), 38 deletions(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index bde7f4af6d..1797e002f9 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1387,13 +1387,53 @@ out:
> >       return r;
> >   }
> > +static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
> > +{
> > +    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);
> > +            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 false;
> > +        }
> > +    }
> > +
> > +    /* Check if CPU type is deprecated and warn if so */
> > +    cc = CPU_CLASS(oc);
> > +    if (cc && cc->deprecation_note) {
>
> cc can't be NULL, right? Otherwise,
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

machine->cpu_type is either mc->default_cpu_type or returned from 
parse_cpu_option().
It can be NULL if mc->default_cpu_type is invalid, which is a program error. So
assert(cc != NULL) should be used instead. I will fold the change to PATCH[v9 
3/9]

> > +        warn_report("CPU model %s is deprecated -- %s",
> > +                    machine->cpu_type, cc->deprecation_note);
> > +    }
> > +
> > +    return true;
> > +}

Thanks,
Gavin