1. Patchew
  2. QEMU
  3. [PATCH v3 00/32] Unified CPU type check
  4. View patch

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

Gavin Shan posted 32 patches 1 year, 2 months ago
Maintainers: Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Peter Maydell <peter.maydell@linaro.org>, Beniamino Galvani <b.galvani@gmail.com>, Strahinja Jankovic <strahinja.p.jankovic@gmail.com>, Subbaraya Sundeep <sundeep.lkml@gmail.com>, Tyrone Ting <kfting@nuvoton.com>, Hao Wu <wuhaotsh@google.com>, Niek Linnenbank <nieklinnenbank@gmail.com>, Radoslaw Biernacki <rad@semihalf.com>, Leif Lindholm <quic_llindhol@quicinc.com>, Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>, Laurent Vivier <laurent@vivier.eu>, Vijai Kumar K <vijai@behindbytes.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liweiwei@iscas.ac.cn>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Richard Henderson <richard.henderson@linaro.org>, Michael Rolnik <mrolnik@gmail.com>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Brian Cain <bcain@quicinc.com>, Song Gao <gaosong@loongson.cn>, Xiaojuan Yang <yangxiaojuan@loongson.cn>, Aurelien Jarno <aurelien@aurel32.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Huacai Chen <chenhuacai@kernel.org>, Chris Wulff <crwulff@gmail.com>, Marek Vasut <marex@denx.de>, Stafford Horne <shorne@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>, Nicholas Piggin <npiggin@gmail.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Max Filippov <jcmvbkbc@gmail.com>
There is a newer version of this series
[PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
Posted by Gavin Shan 1 year, 2 months ago 
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;
 }
 
 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;
-- 
2.41.0
Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
Posted by Thomas Huth 1 year, 2 months ago 
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
Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
Posted by Gavin Shan 1 year, 2 months ago 

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
Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
Posted by David Hildenbrand 1 year, 2 months ago 
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?

-- 
Cheers,

David / dhildenb
Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
Posted by Gavin Shan 1 year, 2 months ago 
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
Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
Posted by Philippe Mathieu-Daudé 1 year, 2 months ago 
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.
Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
Posted by Philippe Mathieu-Daudé 1 year, 2 months ago 
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/
Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
Posted by Gavin Shan 1 year, 2 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.