[PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

[PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

[Philippe Mathieu-Daudé]

Per cpu_model_from_type() docstring (added in commit 445946f4dd):

  * Returns: CPU model name or NULL if the CPU class doesn't exist

We must check the return value in order to avoid surprises, i.e.:

 $ qemu-system-arm -machine virt -cpu cortex-a9
  qemu-system-arm: Invalid CPU model: cortex-a9
  The valid models are: cortex-a7, cortex-a15, (null), (null), (null), (null), (null), (null), (null), (null), (null), (null), (null), max

Add assertions when the call can not fail (because the CPU type
must be registered).

Fixes: 5422d2a8fa ("machine: Print CPU model name instead of CPU type")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 cpu-target.c          | 1 +
 hw/core/machine.c     | 5 +++++
 target/ppc/cpu_init.c | 1 +
 3 files changed, 7 insertions(+)

diff --git a/cpu-target.c b/cpu-target.c
index 5eecd7ea2d..b0f6deb13b 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -291,6 +291,7 @@ static void cpu_list_entry(gpointer data, gpointer user_data)
     const char *typename = object_class_get_name(OBJECT_CLASS(data));
     g_autofree char *model = cpu_model_from_type(typename);
 
+    assert(model);
     if (cc->deprecation_note) {
         qemu_printf("  %s (deprecated)\n", model);
     } else {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index fc239101f9..730ec10328 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1422,16 +1422,21 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
         /* The user specified CPU type isn't valid */
         if (!mc->valid_cpu_types[i]) {
             g_autofree char *requested = cpu_model_from_type(machine->cpu_type);
+            assert(requested);
             error_setg(errp, "Invalid CPU model: %s", requested);
             if (!mc->valid_cpu_types[1]) {
                 g_autofree char *model = cpu_model_from_type(
                                                  mc->valid_cpu_types[0]);
+                assert(model);
                 error_append_hint(errp, "The only valid type is: %s\n", model);
             } else {
                 error_append_hint(errp, "The valid models are: ");
                 for (i = 0; mc->valid_cpu_types[i]; i++) {
                     g_autofree char *model = cpu_model_from_type(
                                                  mc->valid_cpu_types[i]);
+                    if (!model) {
+                        continue;
+                    }
                     error_append_hint(errp, "%s%s",
                                       model,
                                       mc->valid_cpu_types[i + 1] ? ", " : "");
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 344196a8ce..58f0c1e30e 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7037,6 +7037,7 @@ static void ppc_cpu_list_entry(gpointer data, gpointer user_data)
     }
 
     name = cpu_model_from_type(typename);
+    assert(name);
     qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
     for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
         PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
-- 
2.41.0

Re: [PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

[Philippe Mathieu-Daudé]

On 11/1/24 07:47, Philippe Mathieu-Daudé wrote:
> Per cpu_model_from_type() docstring (added in commit 445946f4dd):
> 
>    * Returns: CPU model name or NULL if the CPU class doesn't exist
> 
> We must check the return value in order to avoid surprises, i.e.:
> 
>   $ qemu-system-arm -machine virt -cpu cortex-a9

Doh I missed one space before the '$' character when pasting.

>    qemu-system-arm: Invalid CPU model: cortex-a9
>    The valid models are: cortex-a7, cortex-a15, (null), (null), (null), (null), (null), (null), (null), (null), (null), (null), (null), max
> 
> Add assertions when the call can not fail (because the CPU type
> must be registered).
> 
> Fixes: 5422d2a8fa ("machine: Print CPU model name instead of CPU type")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   cpu-target.c          | 1 +
>   hw/core/machine.c     | 5 +++++
>   target/ppc/cpu_init.c | 1 +
>   3 files changed, 7 insertions(+)

Re: [PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

[Gavin Shan]

Hi Phil,

On 1/11/24 16:47, Philippe Mathieu-Daudé wrote:
> Per cpu_model_from_type() docstring (added in commit 445946f4dd):
> 
>    * Returns: CPU model name or NULL if the CPU class doesn't exist
> 
> We must check the return value in order to avoid surprises, i.e.:
> 
>   $ qemu-system-arm -machine virt -cpu cortex-a9
>    qemu-system-arm: Invalid CPU model: cortex-a9
>    The valid models are: cortex-a7, cortex-a15, (null), (null), (null), (null), (null), (null), (null), (null), (null), (null), (null), max
> 
> Add assertions when the call can not fail (because the CPU type
> must be registered).
> 
> Fixes: 5422d2a8fa ("machine: Print CPU model name instead of CPU type")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   cpu-target.c          | 1 +
>   hw/core/machine.c     | 5 +++++
>   target/ppc/cpu_init.c | 1 +
>   3 files changed, 7 insertions(+)
> 
> diff --git a/cpu-target.c b/cpu-target.c
> index 5eecd7ea2d..b0f6deb13b 100644
> --- a/cpu-target.c
> +++ b/cpu-target.c
> @@ -291,6 +291,7 @@ static void cpu_list_entry(gpointer data, gpointer user_data)
>       const char *typename = object_class_get_name(OBJECT_CLASS(data));
>       g_autofree char *model = cpu_model_from_type(typename);
>   
> +    assert(model);
>       if (cc->deprecation_note) {
>           qemu_printf("  %s (deprecated)\n", model);
>       } else {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fc239101f9..730ec10328 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1422,16 +1422,21 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
>           /* The user specified CPU type isn't valid */
>           if (!mc->valid_cpu_types[i]) {
>               g_autofree char *requested = cpu_model_from_type(machine->cpu_type);
> +            assert(requested);
>               error_setg(errp, "Invalid CPU model: %s", requested);
>               if (!mc->valid_cpu_types[1]) {
>                   g_autofree char *model = cpu_model_from_type(
>                                                    mc->valid_cpu_types[0]);
> +                assert(model);
>                   error_append_hint(errp, "The only valid type is: %s\n", model);
>               } else {
>                   error_append_hint(errp, "The valid models are: ");
>                   for (i = 0; mc->valid_cpu_types[i]; i++) {
>                       g_autofree char *model = cpu_model_from_type(
>                                                    mc->valid_cpu_types[i]);
> +                    if (!model) {
> +                        continue;
> +                    }

Shall we assert(model) for this case, to be consistent with other cases? :)

>                       error_append_hint(errp, "%s%s",
>                                         model,
>                                         mc->valid_cpu_types[i + 1] ? ", " : "");

Otherwise, the separator here need to be adjusted because it's uncertain that
mc->valid_cpu_types[i+1] ... mc->valid_cpu_types[END] are valid.


> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 344196a8ce..58f0c1e30e 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7037,6 +7037,7 @@ static void ppc_cpu_list_entry(gpointer data, gpointer user_data)
>       }
>   
>       name = cpu_model_from_type(typename);
> +    assert(name);
>       qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
>       for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
>           PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];

Thanks,
Gavin

Re: [PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

[Philippe Mathieu-Daudé]

Hi Gavin,

On 11/1/24 08:30, Gavin Shan wrote:
> Hi Phil,
> 
> On 1/11/24 16:47, Philippe Mathieu-Daudé wrote:
>> Per cpu_model_from_type() docstring (added in commit 445946f4dd):
>>
>>    * Returns: CPU model name or NULL if the CPU class doesn't exist
>>
>> We must check the return value in order to avoid surprises, i.e.:
>>
>>   $ qemu-system-arm -machine virt -cpu cortex-a9
>>    qemu-system-arm: Invalid CPU model: cortex-a9
>>    The valid models are: cortex-a7, cortex-a15, (null), (null), 
>> (null), (null), (null), (null), (null), (null), (null), (null), 
>> (null), max
>>
>> Add assertions when the call can not fail (because the CPU type
>> must be registered).
>>
>> Fixes: 5422d2a8fa ("machine: Print CPU model name instead of CPU type")
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   cpu-target.c          | 1 +
>>   hw/core/machine.c     | 5 +++++
>>   target/ppc/cpu_init.c | 1 +
>>   3 files changed, 7 insertions(+)
>>
>> diff --git a/cpu-target.c b/cpu-target.c
>> index 5eecd7ea2d..b0f6deb13b 100644
>> --- a/cpu-target.c
>> +++ b/cpu-target.c
>> @@ -291,6 +291,7 @@ static void cpu_list_entry(gpointer data, gpointer 
>> user_data)
>>       const char *typename = object_class_get_name(OBJECT_CLASS(data));
>>       g_autofree char *model = cpu_model_from_type(typename);
>> +    assert(model);
>>       if (cc->deprecation_note) {
>>           qemu_printf("  %s (deprecated)\n", model);
>>       } else {
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index fc239101f9..730ec10328 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -1422,16 +1422,21 @@ static bool is_cpu_type_supported(const 
>> MachineState *machine, Error **errp)
>>           /* The user specified CPU type isn't valid */
>>           if (!mc->valid_cpu_types[i]) {
>>               g_autofree char *requested = 
>> cpu_model_from_type(machine->cpu_type);
>> +            assert(requested);
>>               error_setg(errp, "Invalid CPU model: %s", requested);
>>               if (!mc->valid_cpu_types[1]) {
>>                   g_autofree char *model = cpu_model_from_type(
>>                                                    
>> mc->valid_cpu_types[0]);
>> +                assert(model);
>>                   error_append_hint(errp, "The only valid type is: 
>> %s\n", model);
>>               } else {
>>                   error_append_hint(errp, "The valid models are: ");
>>                   for (i = 0; mc->valid_cpu_types[i]; i++) {
>>                       g_autofree char *model = cpu_model_from_type(
>>                                                    
>> mc->valid_cpu_types[i]);
>> +                    if (!model) {
>> +                        continue;
>> +                    }
> 
> Shall we assert(model) for this case, to be consistent with other cases? :)

No, this is the "(null)" cases displayed in the example.

IOW, mc->valid_cpu_types[] contains a CPU type which isn't registered,
so we just skip it.

> 
>>                       error_append_hint(errp, "%s%s",
>>                                         model,
>>                                         mc->valid_cpu_types[i + 1] ? 
>> ", " : "");
> 
> Otherwise, the separator here need to be adjusted because it's uncertain 
> that
> mc->valid_cpu_types[i+1] ... mc->valid_cpu_types[END] are valid.

Here we know mc->valid_cpu_types[i] is *not* NULL, but
mc->valid_cpu_types[i + 1] might be (signaling the end
of the array).

This seems correct to me, but I might be missing something.

> 
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index 344196a8ce..58f0c1e30e 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -7037,6 +7037,7 @@ static void ppc_cpu_list_entry(gpointer data, 
>> gpointer user_data)
>>       }
>>       name = cpu_model_from_type(typename);
>> +    assert(name);
>>       qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
>>       for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
>>           PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
> 
> Thanks,
> Gavin
> 

Re: [PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

[Gavin Shan]

Hi Phil,

On 1/11/24 18:21, Philippe Mathieu-Daudé wrote:
> On 11/1/24 08:30, Gavin Shan wrote:
>> On 1/11/24 16:47, Philippe Mathieu-Daudé wrote:
>>> Per cpu_model_from_type() docstring (added in commit 445946f4dd):
>>>
>>>    * Returns: CPU model name or NULL if the CPU class doesn't exist
>>>
>>> We must check the return value in order to avoid surprises, i.e.:
>>>
>>>   $ qemu-system-arm -machine virt -cpu cortex-a9
>>>    qemu-system-arm: Invalid CPU model: cortex-a9
>>>    The valid models are: cortex-a7, cortex-a15, (null), (null), (null), (null), (null), (null), (null), (null), (null), (null), (null), max
>>>
>>> Add assertions when the call can not fail (because the CPU type
>>> must be registered).
>>>
>>> Fixes: 5422d2a8fa ("machine: Print CPU model name instead of CPU type")
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   cpu-target.c          | 1 +
>>>   hw/core/machine.c     | 5 +++++
>>>   target/ppc/cpu_init.c | 1 +
>>>   3 files changed, 7 insertions(+)
>>>
>>> diff --git a/cpu-target.c b/cpu-target.c
>>> index 5eecd7ea2d..b0f6deb13b 100644
>>> --- a/cpu-target.c
>>> +++ b/cpu-target.c
>>> @@ -291,6 +291,7 @@ static void cpu_list_entry(gpointer data, gpointer user_data)
>>>       const char *typename = object_class_get_name(OBJECT_CLASS(data));
>>>       g_autofree char *model = cpu_model_from_type(typename);
>>> +    assert(model);
>>>       if (cc->deprecation_note) {
>>>           qemu_printf("  %s (deprecated)\n", model);
>>>       } else {
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index fc239101f9..730ec10328 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -1422,16 +1422,21 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
>>>           /* The user specified CPU type isn't valid */
>>>           if (!mc->valid_cpu_types[i]) {
>>>               g_autofree char *requested = cpu_model_from_type(machine->cpu_type);
>>> +            assert(requested);
>>>               error_setg(errp, "Invalid CPU model: %s", requested);
>>>               if (!mc->valid_cpu_types[1]) {
>>>                   g_autofree char *model = cpu_model_from_type(
>>> mc->valid_cpu_types[0]);
>>> +                assert(model);
>>>                   error_append_hint(errp, "The only valid type is: %s\n", model);
>>>               } else {
>>>                   error_append_hint(errp, "The valid models are: ");
>>>                   for (i = 0; mc->valid_cpu_types[i]; i++) {
>>>                       g_autofree char *model = cpu_model_from_type(
>>> mc->valid_cpu_types[i]);
>>> +                    if (!model) {
>>> +                        continue;
>>> +                    }
>>
>> Shall we assert(model) for this case, to be consistent with other cases? :)
> 
> No, this is the "(null)" cases displayed in the example.
> 
> IOW, mc->valid_cpu_types[] contains a CPU type which isn't registered,
> so we just skip it.
> 

I thought this should be fixed by correcting mc->valid_cpu_types[] in
hw/arm/virt.c. It means the consistent mc->valid_cpu_types[] needs to
be  provided by the specific board. Otherwise, the logic is incorrect from
the code level at least. For example, "cortex-a9" isn't available to
qemu-system-arm but it has been wrongly declared as supported in hw/arm/virt.c

I've posted one patch against it:

https://lists.nongnu.org/archive/html/qemu-arm/2024-01/msg00531.html


>>
>>>                       error_append_hint(errp, "%s%s",
>>>                                         model,
>>>                                         mc->valid_cpu_types[i + 1] ? ", " : "");
>>
>> Otherwise, the separator here need to be adjusted because it's uncertain that
>> mc->valid_cpu_types[i+1] ... mc->valid_cpu_types[END] are valid.
> 
> Here we know mc->valid_cpu_types[i] is *not* NULL, but
> mc->valid_cpu_types[i + 1] might be (signaling the end
> of the array).
> 
> This seems correct to me, but I might be missing something.
> 

When the class for mc->valid_cpu_types[i + 1] isn't registered, we will
skip the entry. it's possible that the class of mc->valid_cpu_types[i + 2]
isn't registered either. mc->valid_cpu_types[i + 3] to mc->valid_cpu_types[END - 1]
have the similar situations.

In order to correct the separator, we need to invalidate the return value
from cpu_model_from_type(mc->valid_cpu_types[i + 1]) ... cpu_model_from_type(mc->valid_cpu_types[END - 1]).
Too much complex for that and it's another reason why I suggested assert(model) as above

>>
>>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>>> index 344196a8ce..58f0c1e30e 100644
>>> --- a/target/ppc/cpu_init.c
>>> +++ b/target/ppc/cpu_init.c
>>> @@ -7037,6 +7037,7 @@ static void ppc_cpu_list_entry(gpointer data, gpointer user_data)
>>>       }
>>>       name = cpu_model_from_type(typename);
>>> +    assert(name);
>>>       qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
>>>       for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
>>>           PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];
>>

Thanks,
Gavin