| Message ID | 20240111064723.6920-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/core: Handle cpu_model_from_type() returning NULL value | expand |
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(+)
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
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];
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(+)