Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
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
---
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-phi...@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
Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
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
---
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-phi...@linaro.org/
Re: [PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
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
---
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
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
---
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
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
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
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
---
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
[PATCH v3 15/32] target/s390x: Use generic helper to show CPU model names
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
---
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 = _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
