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) {
+        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) {
-- 
2.42.0

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
>