| Message ID | 20231129042012.277831-3-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unified CPU type check | expand |
Hi Gavin, 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> > + warn_report("CPU model %s is deprecated -- %s", > + machine->cpu_type, cc->deprecation_note); > + } > + > + return true; > +}
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
On 4/12/23 00:13, Gavin Shan wrote: > 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] cpu_type and cc an be NULL with the 'none' machine. > >>> + warn_report("CPU model %s is deprecated -- %s", >>> + machine->cpu_type, cc->deprecation_note); >>> + } >>> + >>> + return true; >>> +} >> > > Thanks, > Gavin >
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) { + warn_report("CPU model %s is deprecated -- %s", + machine->cpu_type, cc->deprecation_note); + } + + return true; +} 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; /* This checkpoint is required by replay to separate prior clock reading from the other reads, because timer polling functions query @@ -1448,42 +1488,9 @@ 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(errp, "Invalid CPU type: %s", machine->cpu_type); - error_append_hint(errp, "The valid types are: %s", - machine_class->valid_cpu_types[0]); - for (i = 1; machine_class->valid_cpu_types[i]; i++) { - error_append_hint(errp, ", %s", - machine_class->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); + /* Check if the CPU type is supported */ + if (!is_cpu_type_supported(machine, errp)) { + return; } if (machine->cgs) {
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(-)