| Message ID | 20230907003553.1636896-16-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unified CPU type check | expand |
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(-) > > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c > index 91ce896491..103e9072b8 100644 > --- a/target/s390x/cpu_models.c > +++ b/target/s390x/cpu_models.c > @@ -338,7 +338,8 @@ static void s390_print_cpu_model_list_entry(gpointer data, gpointer user_data) > { > const S390CPUClass *scc = S390_CPU_CLASS((ObjectClass *)data); > CPUClass *cc = CPU_CLASS(scc); > - char *name = g_strdup(object_class_get_name((ObjectClass *)data)); > + const char *typename = object_class_get_name((ObjectClass *)data); > + char *model = cpu_model_from_type(typename); > g_autoptr(GString) details = g_string_new(""); > > if (scc->is_static) { > @@ -355,14 +356,12 @@ static void s390_print_cpu_model_list_entry(gpointer data, gpointer user_data) > g_string_truncate(details, details->len - 2); > } > > - /* strip off the -s390x-cpu */ > - g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0; > if (details->len) { > - qemu_printf("s390 %-15s %-35s (%s)\n", name, scc->desc, details->str); > + qemu_printf("s390 %-15s %-35s (%s)\n", model, scc->desc, details->str); > } else { > - qemu_printf("s390 %-15s %-35s\n", name, scc->desc); > + qemu_printf("s390 %-15s %-35s\n", model, scc->desc); > } > - g_free(name); > + g_free(model); > } > > 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?
On 07/09/2023 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 s/modle/model/ Thomas
On 9/7/23 18:31, Thomas Huth wrote: > On 07/09/2023 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 > > s/modle/model/ > Thanks, will be fixed in next respin. Thanks, Gavin
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(-) >> >> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c >> index 91ce896491..103e9072b8 100644 >> --- a/target/s390x/cpu_models.c >> +++ b/target/s390x/cpu_models.c >> @@ -338,7 +338,8 @@ static void s390_print_cpu_model_list_entry(gpointer data, gpointer user_data) >> { >> const S390CPUClass *scc = S390_CPU_CLASS((ObjectClass *)data); >> CPUClass *cc = CPU_CLASS(scc); >> - char *name = g_strdup(object_class_get_name((ObjectClass *)data)); >> + const char *typename = object_class_get_name((ObjectClass *)data); >> + char *model = cpu_model_from_type(typename); >> g_autoptr(GString) details = g_string_new(""); >> if (scc->is_static) { >> @@ -355,14 +356,12 @@ static void s390_print_cpu_model_list_entry(gpointer data, gpointer user_data) >> g_string_truncate(details, details->len - 2); >> } >> - /* strip off the -s390x-cpu */ >> - g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0; >> if (details->len) { >> - qemu_printf("s390 %-15s %-35s (%s)\n", name, scc->desc, details->str); >> + qemu_printf("s390 %-15s %-35s (%s)\n", model, scc->desc, details->str); >> } else { >> - qemu_printf("s390 %-15s %-35s\n", name, scc->desc); >> + qemu_printf("s390 %-15s %-35s\n", model, scc->desc); >> } >> - g_free(name); >> + g_free(model); >> } >> 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. Thanks, Gavin
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(-) >>> >>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c >>> index 91ce896491..103e9072b8 100644 >>> --- a/target/s390x/cpu_models.c >>> +++ b/target/s390x/cpu_models.c >>> @@ -338,7 +338,8 @@ static void >>> s390_print_cpu_model_list_entry(gpointer data, gpointer user_data) >>> { >>> const S390CPUClass *scc = S390_CPU_CLASS((ObjectClass *)data); >>> CPUClass *cc = CPU_CLASS(scc); >>> - char *name = g_strdup(object_class_get_name((ObjectClass *)data)); >>> + const char *typename = object_class_get_name((ObjectClass *)data); >>> + char *model = cpu_model_from_type(typename); >>> g_autoptr(GString) details = g_string_new(""); >>> if (scc->is_static) { >>> @@ -355,14 +356,12 @@ static void >>> s390_print_cpu_model_list_entry(gpointer data, gpointer user_data) >>> g_string_truncate(details, details->len - 2); >>> } >>> - /* strip off the -s390x-cpu */ >>> - g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0; >>> if (details->len) { >>> - qemu_printf("s390 %-15s %-35s (%s)\n", name, scc->desc, >>> details->str); >>> + qemu_printf("s390 %-15s %-35s (%s)\n", model, scc->desc, >>> details->str); >>> } else { >>> - qemu_printf("s390 %-15s %-35s\n", name, scc->desc); >>> + qemu_printf("s390 %-15s %-35s\n", model, scc->desc); >>> } >>> - g_free(name); >>> + g_free(model); >>> } >>> 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.
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: https://lore.kernel.org/qemu-devel/20230908112235.75914-1-philmd@linaro.org/
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: > 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
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index 91ce896491..103e9072b8 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -338,7 +338,8 @@ static void s390_print_cpu_model_list_entry(gpointer data, gpointer user_data) { const S390CPUClass *scc = S390_CPU_CLASS((ObjectClass *)data); CPUClass *cc = CPU_CLASS(scc); - char *name = g_strdup(object_class_get_name((ObjectClass *)data)); + const char *typename = object_class_get_name((ObjectClass *)data); + char *model = cpu_model_from_type(typename); g_autoptr(GString) details = g_string_new(""); if (scc->is_static) { @@ -355,14 +356,12 @@ static void s390_print_cpu_model_list_entry(gpointer data, gpointer user_data) g_string_truncate(details, details->len - 2); } - /* strip off the -s390x-cpu */ - g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0; if (details->len) { - qemu_printf("s390 %-15s %-35s (%s)\n", name, scc->desc, details->str); + qemu_printf("s390 %-15s %-35s (%s)\n", model, scc->desc, details->str); } else { - qemu_printf("s390 %-15s %-35s\n", name, scc->desc); + qemu_printf("s390 %-15s %-35s\n", model, scc->desc); } - g_free(name); + g_free(model); } 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; } static const TypeInfo qemu_s390_cpu_type_info = { diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c index 63981bf36b..c41af253d3 100644 --- a/target/s390x/cpu_models_sysemu.c +++ b/target/s390x/cpu_models_sysemu.c @@ -55,17 +55,16 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque) struct CpuDefinitionInfoListData *cpu_list_data = opaque; CpuDefinitionInfoList **cpu_list = &cpu_list_data->list; CpuDefinitionInfo *info; - char *name = g_strdup(object_class_get_name(klass)); + const char *typename = object_class_get_name(klass); + char *model = cpu_model_from_type(typename); S390CPUClass *scc = S390_CPU_CLASS(klass); - /* strip off the -s390x-cpu */ - g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0; info = g_new0(CpuDefinitionInfo, 1); - info->name = name; + info->name = model; info->has_migration_safe = true; info->migration_safe = scc->is_migration_safe; info->q_static = scc->is_static; - info->q_typename = g_strdup(object_class_get_name(klass)); + info->q_typename = g_strdup(typename); /* check for unavailable features */ if (cpu_list_data->model) { Object *obj;
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(-)