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