Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names

[Gavin Shan]

On 9/8/23 21:23, Philippe Mathieu-Daudé wrote:
> On 8/9/23 10:04, Philippe Mathieu-Daudé wrote:
> > On 8/9/23 01:44, Gavin Shan wrote:
> > > On 9/7/23 18:20, David Hildenbrand wrote:
> > > > On 07.09.23 02:35, Gavin Shan wrote:
> > > > > For target/s390x, the CPU type name is always the combination of the
> > > > > CPU modle name and suffix. The CPU model names have been correctly
> > > > > shown in s390_print_cpu_model_list_entry() and create_cpu_model_list().
> > > > >
> > > > > Use generic helper cpu_model_from_type() to show the CPU model names
> > > > > in the above two functions. Besides, we need validate the CPU class
> > > > > in s390_cpu_class_by_name(), as other targets do.
> > > > >
> > > > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > > > > ---
> > > > >   target/s390x/cpu_models.c        | 18 +++++++++++-------
> > > > >   target/s390x/cpu_models_sysemu.c |  9 ++++-----
> > > > >   2 files changed, 15 insertions(+), 12 deletions(-)
>
>
> > > > >   static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b)
> > > > > @@ -916,7 +915,12 @@ ObjectClass *s390_cpu_class_by_name(const char *name)
> > > > >       oc = object_class_by_name(typename);
> > > > >       g_free(typename);
> > > > > -    return oc;
> > > > > +    if (object_class_dynamic_cast(oc, TYPE_S390_CPU) &&
> > > > > +        !object_class_is_abstract(oc)) {
> > > > > +        return oc;
> > > > > +    }
> > > > > +
> > > > > +    return NULL;
> > > >
> > > > Why is that change required?
> > >
> > > Good question. It's possible that other class's name conflicts with
> > > CPU class's name. For example, class "abc-base-s390x-cpu" has been
> > > registered for a misc class other than a CPU class. We don't want
> > > s390_cpu_class_by_name() return the class for "abc-base-s390x-cpu".
> > > Almost all other target does similar check.
> >
> > I agree with David there is some code smell here.
> >
> > IMO this check belong to cpu_class_by_name(), not to each impl.
>
> Suggestion implemented here:
> 20230908112235.75914-1-philmd@linaro.org/">https://lore.kernel.org/qemu-devel/20230908112235.75914-1-philmd@linaro.org/

Right. That is better way to consolidate the checks. I've provided my
comments for your code changes. I believe I need to rebase my v4 series
on top your changes. Philippe, please let me know if I need include your
patches to my v4 series, modify the code accordingly and send them together
for review.

Thanks,
Gavin