cpu-target.c | 1 + hw/core/machine.c | 5 +++++ target/ppc/cpu_init.c | 1 + 3 files changed, 7 insertions(+)
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
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
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
>
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
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(+)
© 2016 - 2024 Red Hat, Inc.