All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] hw/core/cpu-common: Consolidate cpu_class_by_name()
@ 2023-09-08 11:22 Philippe Mathieu-Daudé
  2023-09-08 11:22 ` [PATCH 1/4] target/alpha: Tidy up alpha_cpu_class_by_name() Philippe Mathieu-Daudé
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-09-08 11:22 UTC (permalink / raw
  To: qemu-devel, David Hildenbrand, Gavin Shan
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	Philippe Mathieu-Daudé, qemu-riscv, Aurelien Jarno, Bin Meng,
	Xiaojuan Yang, Daniel Henrique Barboza, Aleksandar Rikalo,
	Artyom Tarasenko, Song Gao, Stafford Horne, Yanan Wang,
	Alistair Francis, Brian Cain, Cédric Le Goater, Thomas Huth,
	Liu Zhiwei

Gavin noticed the same pattern is duplicated in many
CPUClass::class_by_name() handlers [*].
This series consolidate the calls to
 - object_class_is_abstract()
 - object_class_dynamic_cast()
in the common cpu_class_by_name(), by introducing
the CPUClass::cpu_resolving_type field.

[*] https://lore.kernel.org/qemu-devel/ab07d81c-da98-a270-c3f6-0625912c6d0b@redhat.com/

Philippe Mathieu-Daudé (4):
  target/alpha: Tidy up alpha_cpu_class_by_name()
  hw/cpu: Call object_class_is_abstract() once in cpu_class_by_name()
  hw/cpu: Introduce CPUClass::cpu_resolving_type field
  hw/cpu: Call object_class_dynamic_cast() once in cpu_class_by_name()

 include/hw/core/cpu.h   |  9 ++++++---
 hw/core/cpu-common.c    | 15 ++++++++++++---
 target/alpha/cpu.c      | 11 ++++-------
 target/arm/cpu.c        |  6 ++----
 target/avr/cpu.c        |  6 ++----
 target/cris/cpu.c       |  6 ++----
 target/hexagon/cpu.c    |  6 ++----
 target/hppa/cpu.c       |  1 +
 target/i386/cpu.c       |  1 +
 target/loongarch/cpu.c  |  7 ++-----
 target/m68k/cpu.c       |  6 ++----
 target/microblaze/cpu.c |  1 +
 target/mips/cpu.c       |  1 +
 target/nios2/cpu.c      |  1 +
 target/openrisc/cpu.c   |  6 ++----
 target/ppc/cpu_init.c   |  1 +
 target/riscv/cpu.c      |  6 ++----
 target/rx/cpu.c         |  7 ++-----
 target/s390x/cpu.c      |  1 +
 target/sh4/cpu.c        |  4 +---
 target/sparc/cpu.c      |  1 +
 target/tricore/cpu.c    |  6 ++----
 target/xtensa/cpu.c     |  6 ++----
 23 files changed, 53 insertions(+), 62 deletions(-)

-- 
2.41.0



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/4] target/alpha: Tidy up alpha_cpu_class_by_name()
  2023-09-08 11:22 [PATCH 0/4] hw/core/cpu-common: Consolidate cpu_class_by_name() Philippe Mathieu-Daudé
@ 2023-09-08 11:22 ` Philippe Mathieu-Daudé
  2023-09-09 22:17   ` Richard Henderson
  2023-09-10 23:29   ` Gavin Shan
  2023-09-08 11:22 ` [PATCH 2/4] hw/cpu: Call object_class_is_abstract() once in cpu_class_by_name() Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-09-08 11:22 UTC (permalink / raw
  To: qemu-devel, David Hildenbrand, Gavin Shan
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	Philippe Mathieu-Daudé, qemu-riscv, Aurelien Jarno, Bin Meng,
	Xiaojuan Yang, Daniel Henrique Barboza, Aleksandar Rikalo,
	Artyom Tarasenko, Song Gao, Stafford Horne, Yanan Wang,
	Alistair Francis, Brian Cain, Cédric Le Goater, Thomas Huth,
	Liu Zhiwei

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/alpha/cpu.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 270ae787b1..351ee2e9f2 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -142,13 +142,10 @@ static ObjectClass *alpha_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(ALPHA_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && object_class_is_abstract(oc)) {
-        oc = NULL;
-    }
 
     /* TODO: remove match everything nonsense */
-    /* Default to ev67; no reason not to emulate insns by default. */
-    if (!oc) {
+    if (!oc || object_class_is_abstract(oc)) {
+        /* Default to ev67; no reason not to emulate insns by default. */
         oc = object_class_by_name(ALPHA_CPU_TYPE_NAME("ev67"));
     }
 
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/4] hw/cpu: Call object_class_is_abstract() once in cpu_class_by_name()
  2023-09-08 11:22 [PATCH 0/4] hw/core/cpu-common: Consolidate cpu_class_by_name() Philippe Mathieu-Daudé
  2023-09-08 11:22 ` [PATCH 1/4] target/alpha: Tidy up alpha_cpu_class_by_name() Philippe Mathieu-Daudé
@ 2023-09-08 11:22 ` Philippe Mathieu-Daudé
  2023-09-09 22:18   ` Richard Henderson
  2023-09-08 11:22 ` [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field Philippe Mathieu-Daudé
  2023-09-08 11:22 ` [PATCH 4/4] hw/cpu: Call object_class_dynamic_cast() once in cpu_class_by_name() Philippe Mathieu-Daudé
  3 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-09-08 11:22 UTC (permalink / raw
  To: qemu-devel, David Hildenbrand, Gavin Shan
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	Philippe Mathieu-Daudé, qemu-riscv, Aurelien Jarno, Bin Meng,
	Xiaojuan Yang, Daniel Henrique Barboza, Aleksandar Rikalo,
	Artyom Tarasenko, Song Gao, Stafford Horne, Yanan Wang,
	Alistair Francis, Brian Cain, Cédric Le Goater, Thomas Huth,
	Liu Zhiwei

Let CPUClass::class_by_name() handlers to return abstract classes,
and filter them once in the public cpu_class_by_name() method.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/core/cpu.h  |  7 ++++---
 hw/core/cpu-common.c   | 14 +++++++++++---
 target/arm/cpu.c       |  3 +--
 target/avr/cpu.c       |  3 +--
 target/cris/cpu.c      |  3 +--
 target/hexagon/cpu.c   |  3 +--
 target/loongarch/cpu.c |  3 +--
 target/m68k/cpu.c      |  3 +--
 target/openrisc/cpu.c  |  3 +--
 target/riscv/cpu.c     |  3 +--
 target/rx/cpu.c        |  6 +-----
 target/sh4/cpu.c       |  3 ---
 target/tricore/cpu.c   |  3 +--
 target/xtensa/cpu.c    |  3 +--
 14 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 92a4234439..129d179937 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -101,7 +101,7 @@ struct SysemuCPUOps;
 /**
  * CPUClass:
  * @class_by_name: Callback to map -cpu command line model name to an
- * instantiatable CPU type.
+ *                 instantiatable CPU type.
  * @parse_features: Callback to parse command line arguments.
  * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
  * @has_work: Callback for checking if there is work to do.
@@ -630,9 +630,10 @@ void cpu_reset(CPUState *cpu);
  * @typename: The CPU base type.
  * @cpu_model: The model string without any parameters.
  *
- * Looks up a CPU #ObjectClass matching name @cpu_model.
+ * Looks up a concrete CPU #ObjectClass matching name @cpu_model.
  *
- * Returns: A #CPUClass or %NULL if not matching class is found.
+ * Returns: A concrete #CPUClass or %NULL if no matching class is found
+ *          or if the matching class is abstract.
  */
 ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model);
 
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index ced66c2b34..c6a0c9390c 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -149,10 +149,18 @@ static bool cpu_common_has_work(CPUState *cs)
 
 ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
 {
-    CPUClass *cc = CPU_CLASS(object_class_by_name(typename));
+    ObjectClass *oc;
+    CPUClass *cc;
 
-    assert(cpu_model && cc->class_by_name);
-    return cc->class_by_name(cpu_model);
+    assert(cpu_model);
+    oc = object_class_by_name(typename);
+    cc = CPU_CLASS(oc);
+    assert(cc->class_by_name);
+    oc = cc->class_by_name(cpu_model);
+    if (oc == NULL || object_class_is_abstract(oc)) {
+        return NULL;
+    }
+    return oc;
 }
 
 static void cpu_common_parse_features(const char *typename, char *features,
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 0bb0585441..42e29816cc 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2300,8 +2300,7 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
     oc = object_class_by_name(typename);
     g_strfreev(cpuname);
     g_free(typename);
-    if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
-        object_class_is_abstract(oc)) {
+    if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU)) {
         return NULL;
     }
     return oc;
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 8f741f258c..4b255eade1 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -159,8 +159,7 @@ static ObjectClass *avr_cpu_class_by_name(const char *cpu_model)
     ObjectClass *oc;
 
     oc = object_class_by_name(cpu_model);
-    if (object_class_dynamic_cast(oc, TYPE_AVR_CPU) == NULL ||
-        object_class_is_abstract(oc)) {
+    if (object_class_dynamic_cast(oc, TYPE_AVR_CPU) == NULL) {
         oc = NULL;
     }
     return oc;
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index a6a93c2359..115f6e2ea2 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -95,8 +95,7 @@ static ObjectClass *cris_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(CRIS_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_CRIS_CPU) ||
-                       object_class_is_abstract(oc))) {
+    if (oc != NULL && !object_class_dynamic_cast(oc, TYPE_CRIS_CPU)) {
         oc = NULL;
     }
     return oc;
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index f155936289..5e301327d3 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -63,8 +63,7 @@ static ObjectClass *hexagon_cpu_class_by_name(const char *cpu_model)
     oc = object_class_by_name(typename);
     g_strfreev(cpuname);
     g_free(typename);
-    if (!oc || !object_class_dynamic_cast(oc, TYPE_HEXAGON_CPU) ||
-        object_class_is_abstract(oc)) {
+    if (!oc || !object_class_dynamic_cast(oc, TYPE_HEXAGON_CPU)) {
         return NULL;
     }
     return oc;
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 65f9320e34..fe2e5ecc46 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -646,8 +646,7 @@ static ObjectClass *loongarch_cpu_class_by_name(const char *cpu_model)
         }
     }
 
-    if (object_class_dynamic_cast(oc, TYPE_LOONGARCH_CPU)
-        && !object_class_is_abstract(oc)) {
+    if (object_class_dynamic_cast(oc, TYPE_LOONGARCH_CPU)) {
         return oc;
     }
     return NULL;
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 70d58471dc..004f3d6265 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -111,8 +111,7 @@ static ObjectClass *m68k_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(M68K_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && (object_class_dynamic_cast(oc, TYPE_M68K_CPU) == NULL ||
-                       object_class_is_abstract(oc))) {
+    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_M68K_CPU) == NULL) {
         return NULL;
     }
     return oc;
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 61d748cfdc..3bbbcc4e63 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -168,8 +168,7 @@ static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(OPENRISC_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) ||
-                       object_class_is_abstract(oc))) {
+    if (oc != NULL && !object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU)) {
         return NULL;
     }
     return oc;
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 6b93b04453..17b00eb7c0 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -619,8 +619,7 @@ static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
     oc = object_class_by_name(typename);
     g_strfreev(cpuname);
     g_free(typename);
-    if (!oc || !object_class_dynamic_cast(oc, TYPE_RISCV_CPU) ||
-        object_class_is_abstract(oc)) {
+    if (!oc || !object_class_dynamic_cast(oc, TYPE_RISCV_CPU)) {
         return NULL;
     }
     return oc;
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 157e57da0f..c98034540d 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -111,16 +111,12 @@ static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
     char *typename;
 
     oc = object_class_by_name(cpu_model);
-    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_RX_CPU) != NULL &&
-        !object_class_is_abstract(oc)) {
+    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_RX_CPU) != NULL) {
         return oc;
     }
     typename = g_strdup_printf(RX_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && object_class_is_abstract(oc)) {
-        oc = NULL;
-    }
 
     return oc;
 }
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index 61769ffdfa..bc112776fc 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -152,9 +152,6 @@ static ObjectClass *superh_cpu_class_by_name(const char *cpu_model)
 
     typename = g_strdup_printf(SUPERH_CPU_TYPE_NAME("%s"), s);
     oc = object_class_by_name(typename);
-    if (oc != NULL && object_class_is_abstract(oc)) {
-        oc = NULL;
-    }
 
 out:
     g_free(s);
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 133a9ac70e..a2381b0dc1 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -140,8 +140,7 @@ static ObjectClass *tricore_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(TRICORE_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (!oc || !object_class_dynamic_cast(oc, TYPE_TRICORE_CPU) ||
-        object_class_is_abstract(oc)) {
+    if (!oc || !object_class_dynamic_cast(oc, TYPE_TRICORE_CPU)) {
         return NULL;
     }
     return oc;
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index acaf8c905f..a31825a2b5 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -141,8 +141,7 @@ static ObjectClass *xtensa_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(XTENSA_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc == NULL || !object_class_dynamic_cast(oc, TYPE_XTENSA_CPU) ||
-        object_class_is_abstract(oc)) {
+    if (oc == NULL || !object_class_dynamic_cast(oc, TYPE_XTENSA_CPU)) {
         return NULL;
     }
     return oc;
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
  2023-09-08 11:22 [PATCH 0/4] hw/core/cpu-common: Consolidate cpu_class_by_name() Philippe Mathieu-Daudé
  2023-09-08 11:22 ` [PATCH 1/4] target/alpha: Tidy up alpha_cpu_class_by_name() Philippe Mathieu-Daudé
  2023-09-08 11:22 ` [PATCH 2/4] hw/cpu: Call object_class_is_abstract() once in cpu_class_by_name() Philippe Mathieu-Daudé
@ 2023-09-08 11:22 ` Philippe Mathieu-Daudé
  2023-09-09 22:21   ` Richard Henderson
  2023-09-10 23:28   ` Gavin Shan
  2023-09-08 11:22 ` [PATCH 4/4] hw/cpu: Call object_class_dynamic_cast() once in cpu_class_by_name() Philippe Mathieu-Daudé
  3 siblings, 2 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-09-08 11:22 UTC (permalink / raw
  To: qemu-devel, David Hildenbrand, Gavin Shan
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	Philippe Mathieu-Daudé, qemu-riscv, Aurelien Jarno, Bin Meng,
	Xiaojuan Yang, Daniel Henrique Barboza, Aleksandar Rikalo,
	Artyom Tarasenko, Song Gao, Stafford Horne, Yanan Wang,
	Alistair Francis, Brian Cain, Cédric Le Goater, Thomas Huth,
	Liu Zhiwei

Add a field to return the QOM type name of a CPU class.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/core/cpu.h   | 2 ++
 hw/core/cpu-common.c    | 2 +-
 target/alpha/cpu.c      | 1 +
 target/arm/cpu.c        | 1 +
 target/avr/cpu.c        | 1 +
 target/cris/cpu.c       | 1 +
 target/hexagon/cpu.c    | 1 +
 target/hppa/cpu.c       | 1 +
 target/i386/cpu.c       | 1 +
 target/loongarch/cpu.c  | 1 +
 target/m68k/cpu.c       | 1 +
 target/microblaze/cpu.c | 1 +
 target/mips/cpu.c       | 1 +
 target/nios2/cpu.c      | 1 +
 target/openrisc/cpu.c   | 1 +
 target/ppc/cpu_init.c   | 1 +
 target/riscv/cpu.c      | 1 +
 target/rx/cpu.c         | 1 +
 target/s390x/cpu.c      | 1 +
 target/sh4/cpu.c        | 1 +
 target/sparc/cpu.c      | 1 +
 target/tricore/cpu.c    | 1 +
 target/xtensa/cpu.c     | 1 +
 23 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 129d179937..e469efd409 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -100,6 +100,7 @@ struct SysemuCPUOps;
 
 /**
  * CPUClass:
+ * @cpu_resolving_type: CPU QOM type name
  * @class_by_name: Callback to map -cpu command line model name to an
  *                 instantiatable CPU type.
  * @parse_features: Callback to parse command line arguments.
@@ -148,6 +149,7 @@ struct CPUClass {
     DeviceClass parent_class;
     /*< public >*/
 
+    const char *cpu_resolving_type;
     ObjectClass *(*class_by_name)(const char *cpu_model);
     void (*parse_features)(const char *typename, char *str, Error **errp);
 
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index c6a0c9390c..2d24261a6a 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -155,7 +155,7 @@ ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
     assert(cpu_model);
     oc = object_class_by_name(typename);
     cc = CPU_CLASS(oc);
-    assert(cc->class_by_name);
+    assert(cc->cpu_resolving_type && cc->class_by_name);
     oc = cc->class_by_name(cpu_model);
     if (oc == NULL || object_class_is_abstract(oc)) {
         return NULL;
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 351ee2e9f2..0ddea8004c 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -254,6 +254,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
     device_class_set_parent_realize(dc, alpha_cpu_realizefn,
                                     &acc->parent_realize);
 
+    cc->cpu_resolving_type = TYPE_ALPHA_CPU;
     cc->class_by_name = alpha_cpu_class_by_name;
     cc->has_work = alpha_cpu_has_work;
     cc->dump_state = alpha_cpu_dump_state;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 42e29816cc..9e51bde170 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2377,6 +2377,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, arm_cpu_reset_hold, NULL,
                                        &acc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_ARM_CPU;
     cc->class_by_name = arm_cpu_class_by_name;
     cc->has_work = arm_cpu_has_work;
     cc->dump_state = arm_cpu_dump_state;
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 4b255eade1..f6004169ac 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -233,6 +233,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, avr_cpu_reset_hold, NULL,
                                        &mcc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_AVR_CPU;
     cc->class_by_name = avr_cpu_class_by_name;
 
     cc->has_work = avr_cpu_has_work;
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index 115f6e2ea2..adde4f599d 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -314,6 +314,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, cris_cpu_reset_hold, NULL,
                                        &ccc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_CRIS_CPU;
     cc->class_by_name = cris_cpu_class_by_name;
     cc->has_work = cris_cpu_has_work;
     cc->dump_state = cris_cpu_dump_state;
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 5e301327d3..2d4fed838d 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -381,6 +381,7 @@ static void hexagon_cpu_class_init(ObjectClass *c, void *data)
     resettable_class_set_parent_phases(rc, NULL, hexagon_cpu_reset_hold, NULL,
                                        &mcc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_HEXAGON_CPU;
     cc->class_by_name = hexagon_cpu_class_by_name;
     cc->has_work = hexagon_cpu_has_work;
     cc->dump_state = hexagon_dump_state;
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 11022f9c99..47950a15ae 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -192,6 +192,7 @@ static void hppa_cpu_class_init(ObjectClass *oc, void *data)
     device_class_set_parent_realize(dc, hppa_cpu_realizefn,
                                     &acc->parent_realize);
 
+    cc->cpu_resolving_type = TYPE_HPPA_CPU;
     cc->class_by_name = hppa_cpu_class_by_name;
     cc->has_work = hppa_cpu_has_work;
     cc->dump_state = hppa_cpu_dump_state;
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 00f913b638..9979464420 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7951,6 +7951,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
                                        &xcc->parent_phases);
     cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;
 
+    cc->cpu_resolving_type = TYPE_X86_CPU;
     cc->class_by_name = x86_cpu_class_by_name;
     cc->parse_features = x86_cpu_parse_featurestr;
     cc->has_work = x86_cpu_has_work;
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index fe2e5ecc46..189dfd32d1 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -743,6 +743,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
     resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
                                        &lacc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_LOONGARCH_CPU;
     cc->class_by_name = loongarch_cpu_class_by_name;
     cc->has_work = loongarch_cpu_has_work;
     cc->dump_state = loongarch_cpu_dump_state;
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 004f3d6265..bd7bb103d7 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -558,6 +558,7 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
     resettable_class_set_parent_phases(rc, NULL, m68k_cpu_reset_hold, NULL,
                                        &mcc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_M68K_CPU;
     cc->class_by_name = m68k_cpu_class_by_name;
     cc->has_work = m68k_cpu_has_work;
     cc->dump_state = m68k_cpu_dump_state;
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 03c2c4db1f..bb5f2c1f00 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -414,6 +414,7 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, mb_cpu_reset_hold, NULL,
                                        &mcc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_MICROBLAZE_CPU;
     cc->class_by_name = mb_cpu_class_by_name;
     cc->has_work = mb_cpu_has_work;
 
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 63da1948fd..649147df2e 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -578,6 +578,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
     resettable_class_set_parent_phases(rc, NULL, mips_cpu_reset_hold, NULL,
                                        &mcc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_MIPS_CPU;
     cc->class_by_name = mips_cpu_class_by_name;
     cc->has_work = mips_cpu_has_work;
     cc->dump_state = mips_cpu_dump_state;
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index bc5cbf81c2..fc7c6a83ee 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -381,6 +381,7 @@ static void nios2_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, nios2_cpu_reset_hold, NULL,
                                        &ncc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_NIOS2_CPU;
     cc->class_by_name = nios2_cpu_class_by_name;
     cc->has_work = nios2_cpu_has_work;
     cc->dump_state = nios2_cpu_dump_state;
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 3bbbcc4e63..5e1e0576e0 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -243,6 +243,7 @@ static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, openrisc_cpu_reset_hold, NULL,
                                        &occ->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_OPENRISC_CPU;
     cc->class_by_name = openrisc_cpu_class_by_name;
     cc->has_work = openrisc_cpu_has_work;
     cc->dump_state = openrisc_cpu_dump_state;
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 02b7aad9b0..bc106d01a2 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7357,6 +7357,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
                                        &pcc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_POWERPC_CPU;
     cc->class_by_name = ppc_cpu_class_by_name;
     cc->has_work = ppc_cpu_has_work;
     cc->dump_state = ppc_cpu_dump_state;
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 17b00eb7c0..e8f04ef82b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -2130,6 +2130,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     resettable_class_set_parent_phases(rc, NULL, riscv_cpu_reset_hold, NULL,
                                        &mcc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_RISCV_CPU;
     cc->class_by_name = riscv_cpu_class_by_name;
     cc->has_work = riscv_cpu_has_work;
     cc->dump_state = riscv_cpu_dump_state;
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index c98034540d..2a6df418a8 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -222,6 +222,7 @@ static void rx_cpu_class_init(ObjectClass *klass, void *data)
     resettable_class_set_parent_phases(rc, NULL, rx_cpu_reset_hold, NULL,
                                        &rcc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_RX_CPU;
     cc->class_by_name = rx_cpu_class_by_name;
     cc->has_work = rx_cpu_has_work;
     cc->dump_state = rx_cpu_dump_state;
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index df167493c3..bcba466bb4 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -336,6 +336,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     device_class_set_parent_reset(dc, s390_cpu_reset_full, &scc->parent_reset);
 
     scc->reset = s390_cpu_reset;
+    cc->cpu_resolving_type = TYPE_S390_CPU;
     cc->class_by_name = s390_cpu_class_by_name,
     cc->has_work = s390_cpu_has_work;
     cc->dump_state = s390_cpu_dump_state;
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index bc112776fc..17c87f15f2 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -283,6 +283,7 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, superh_cpu_reset_hold, NULL,
                                        &scc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_SUPERH_CPU;
     cc->class_by_name = superh_cpu_class_by_name;
     cc->has_work = superh_cpu_has_work;
     cc->dump_state = superh_cpu_dump_state;
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 130ab8f578..e41a9f4ee2 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -902,6 +902,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, sparc_cpu_reset_hold, NULL,
                                        &scc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_SPARC_CPU;
     cc->class_by_name = sparc_cpu_class_by_name;
     cc->parse_features = sparc_cpu_parse_features;
     cc->has_work = sparc_cpu_has_work;
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index a2381b0dc1..ffe5158786 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -202,6 +202,7 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data)
 
     resettable_class_set_parent_phases(rc, NULL, tricore_cpu_reset_hold, NULL,
                                        &mcc->parent_phases);
+    cc->cpu_resolving_type = TYPE_TRICORE_CPU;
     cc->class_by_name = tricore_cpu_class_by_name;
     cc->has_work = tricore_cpu_has_work;
 
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index a31825a2b5..13bed05d0c 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -252,6 +252,7 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
     resettable_class_set_parent_phases(rc, NULL, xtensa_cpu_reset_hold, NULL,
                                        &xcc->parent_phases);
 
+    cc->cpu_resolving_type = TYPE_XTENSA_CPU;
     cc->class_by_name = xtensa_cpu_class_by_name;
     cc->has_work = xtensa_cpu_has_work;
     cc->dump_state = xtensa_cpu_dump_state;
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 4/4] hw/cpu: Call object_class_dynamic_cast() once in cpu_class_by_name()
  2023-09-08 11:22 [PATCH 0/4] hw/core/cpu-common: Consolidate cpu_class_by_name() Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2023-09-08 11:22 ` [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field Philippe Mathieu-Daudé
@ 2023-09-08 11:22 ` Philippe Mathieu-Daudé
  2023-09-09 23:26   ` Richard Henderson
  2023-09-10 23:40   ` Gavin Shan
  3 siblings, 2 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-09-08 11:22 UTC (permalink / raw
  To: qemu-devel, David Hildenbrand, Gavin Shan
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	Philippe Mathieu-Daudé, qemu-riscv, Aurelien Jarno, Bin Meng,
	Xiaojuan Yang, Daniel Henrique Barboza, Aleksandar Rikalo,
	Artyom Tarasenko, Song Gao, Stafford Horne, Yanan Wang,
	Alistair Francis, Brian Cain, Cédric Le Goater, Thomas Huth,
	Liu Zhiwei

Leverage the public CPUClass::cpu_resolving_type field and
call object_class_dynamic_cast() once in cpu_class_by_name().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/cpu-common.c   | 3 ++-
 target/alpha/cpu.c     | 3 +--
 target/arm/cpu.c       | 4 +---
 target/avr/cpu.c       | 4 +---
 target/cris/cpu.c      | 4 +---
 target/hexagon/cpu.c   | 4 +---
 target/loongarch/cpu.c | 5 +----
 target/m68k/cpu.c      | 4 +---
 target/openrisc/cpu.c  | 4 +---
 target/riscv/cpu.c     | 4 +---
 target/rx/cpu.c        | 2 +-
 target/tricore/cpu.c   | 4 +---
 target/xtensa/cpu.c    | 4 +---
 13 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 2d24261a6a..f4a2ccebea 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -157,7 +157,8 @@ ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
     cc = CPU_CLASS(oc);
     assert(cc->cpu_resolving_type && cc->class_by_name);
     oc = cc->class_by_name(cpu_model);
-    if (oc == NULL || object_class_is_abstract(oc)) {
+    if (oc == NULL || object_class_is_abstract(oc)
+                   || !object_class_dynamic_cast(oc, cc->cpu_resolving_type)) {
         return NULL;
     }
     return oc;
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 0ddea8004c..b184fcc123 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -126,8 +126,7 @@ static ObjectClass *alpha_cpu_class_by_name(const char *cpu_model)
     int i;
 
     oc = object_class_by_name(cpu_model);
-    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_ALPHA_CPU) != NULL &&
-        !object_class_is_abstract(oc)) {
+    if (oc != NULL && !object_class_is_abstract(oc)) {
         return oc;
     }
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9e51bde170..d29040cd8c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2300,9 +2300,7 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
     oc = object_class_by_name(typename);
     g_strfreev(cpuname);
     g_free(typename);
-    if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU)) {
-        return NULL;
-    }
+
     return oc;
 }
 
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index f6004169ac..53735ff1dd 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -159,9 +159,7 @@ static ObjectClass *avr_cpu_class_by_name(const char *cpu_model)
     ObjectClass *oc;
 
     oc = object_class_by_name(cpu_model);
-    if (object_class_dynamic_cast(oc, TYPE_AVR_CPU) == NULL) {
-        oc = NULL;
-    }
+
     return oc;
 }
 
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index adde4f599d..b307d0b9db 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -95,9 +95,7 @@ static ObjectClass *cris_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(CRIS_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && !object_class_dynamic_cast(oc, TYPE_CRIS_CPU)) {
-        oc = NULL;
-    }
+
     return oc;
 }
 
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 2d4fed838d..4b8d63c4a7 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -63,9 +63,7 @@ static ObjectClass *hexagon_cpu_class_by_name(const char *cpu_model)
     oc = object_class_by_name(typename);
     g_strfreev(cpuname);
     g_free(typename);
-    if (!oc || !object_class_dynamic_cast(oc, TYPE_HEXAGON_CPU)) {
-        return NULL;
-    }
+
     return oc;
 }
 
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 189dfd32d1..1eb2c579eb 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -646,10 +646,7 @@ static ObjectClass *loongarch_cpu_class_by_name(const char *cpu_model)
         }
     }
 
-    if (object_class_dynamic_cast(oc, TYPE_LOONGARCH_CPU)) {
-        return oc;
-    }
-    return NULL;
+    return oc;
 }
 
 void loongarch_cpu_dump_state(CPUState *cs, FILE *f, int flags)
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index bd7bb103d7..e8b86c80f1 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -111,9 +111,7 @@ static ObjectClass *m68k_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(M68K_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_M68K_CPU) == NULL) {
-        return NULL;
-    }
+
     return oc;
 }
 
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 5e1e0576e0..7aac9105bd 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -168,9 +168,7 @@ static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(OPENRISC_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && !object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU)) {
-        return NULL;
-    }
+
     return oc;
 }
 
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index e8f04ef82b..0170e288e7 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -619,9 +619,7 @@ static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
     oc = object_class_by_name(typename);
     g_strfreev(cpuname);
     g_free(typename);
-    if (!oc || !object_class_dynamic_cast(oc, TYPE_RISCV_CPU)) {
-        return NULL;
-    }
+
     return oc;
 }
 
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 2a6df418a8..879d4fcdef 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -111,7 +111,7 @@ static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
     char *typename;
 
     oc = object_class_by_name(cpu_model);
-    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_RX_CPU) != NULL) {
+    if (oc != NULL) {
         return oc;
     }
     typename = g_strdup_printf(RX_CPU_TYPE_NAME("%s"), cpu_model);
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index ffe5158786..f65b8761b0 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -140,9 +140,7 @@ static ObjectClass *tricore_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(TRICORE_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (!oc || !object_class_dynamic_cast(oc, TYPE_TRICORE_CPU)) {
-        return NULL;
-    }
+
     return oc;
 }
 
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index 13bed05d0c..6d96e5ab27 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -141,9 +141,7 @@ static ObjectClass *xtensa_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf(XTENSA_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc == NULL || !object_class_dynamic_cast(oc, TYPE_XTENSA_CPU)) {
-        return NULL;
-    }
+
     return oc;
 }
 
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] target/alpha: Tidy up alpha_cpu_class_by_name()
  2023-09-08 11:22 ` [PATCH 1/4] target/alpha: Tidy up alpha_cpu_class_by_name() Philippe Mathieu-Daudé
@ 2023-09-09 22:17   ` Richard Henderson
  2023-09-10 23:29   ` Gavin Shan
  1 sibling, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2023-09-09 22:17 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel

On 9/8/23 04:22, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/alpha/cpu.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] hw/cpu: Call object_class_is_abstract() once in cpu_class_by_name()
  2023-09-08 11:22 ` [PATCH 2/4] hw/cpu: Call object_class_is_abstract() once in cpu_class_by_name() Philippe Mathieu-Daudé
@ 2023-09-09 22:18   ` Richard Henderson
  0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2023-09-09 22:18 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel

On 9/8/23 04:22, Philippe Mathieu-Daudé wrote:
> Let CPUClass::class_by_name() handlers to return abstract classes,
> and filter them once in the public cpu_class_by_name() method.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/core/cpu.h  |  7 ++++---
>   hw/core/cpu-common.c   | 14 +++++++++++---
>   target/arm/cpu.c       |  3 +--
>   target/avr/cpu.c       |  3 +--
>   target/cris/cpu.c      |  3 +--
>   target/hexagon/cpu.c   |  3 +--
>   target/loongarch/cpu.c |  3 +--
>   target/m68k/cpu.c      |  3 +--
>   target/openrisc/cpu.c  |  3 +--
>   target/riscv/cpu.c     |  3 +--
>   target/rx/cpu.c        |  6 +-----
>   target/sh4/cpu.c       |  3 ---
>   target/tricore/cpu.c   |  3 +--
>   target/xtensa/cpu.c    |  3 +--
>   14 files changed, 26 insertions(+), 34 deletions(-)

Missed the cleanup for alpha, which you introduced in patch 1.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
  2023-09-08 11:22 ` [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field Philippe Mathieu-Daudé
@ 2023-09-09 22:21   ` Richard Henderson
  2023-09-10 23:28   ` Gavin Shan
  1 sibling, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2023-09-09 22:21 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel

On 9/8/23 04:22, Philippe Mathieu-Daudé wrote:
> Add a field to return the QOM type name of a CPU class.

It isn't the type name of the cpu class, it's the name of the base class that one 
particular use case expects.

I don't think this is a good idea.


r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] hw/cpu: Call object_class_dynamic_cast() once in cpu_class_by_name()
  2023-09-08 11:22 ` [PATCH 4/4] hw/cpu: Call object_class_dynamic_cast() once in cpu_class_by_name() Philippe Mathieu-Daudé
@ 2023-09-09 23:26   ` Richard Henderson
  2023-09-10 23:40   ` Gavin Shan
  1 sibling, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2023-09-09 23:26 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel

On 9/8/23 04:22, Philippe Mathieu-Daudé wrote:
> +++ b/hw/core/cpu-common.c
> @@ -157,7 +157,8 @@ ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
>       cc = CPU_CLASS(oc);
>       assert(cc->cpu_resolving_type && cc->class_by_name);
>       oc = cc->class_by_name(cpu_model);
> -    if (oc == NULL || object_class_is_abstract(oc)) {
> +    if (oc == NULL || object_class_is_abstract(oc)
> +                   || !object_class_dynamic_cast(oc, cc->cpu_resolving_type)) {
>           return NULL;
>       }

cc->cpu_resolving_type, as constructed in patch 3, is always @typename.
In the only (!) caller in toplevel/cpu.c, this is CPU_RESOLVING_TYPE.

Indentation:

     if (oc == NULL
         || object_class_is_abstract(oc)
         || !object_class_dynamic_cast(oc, typename)


r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
  2023-09-08 11:22 ` [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field Philippe Mathieu-Daudé
  2023-09-09 22:21   ` Richard Henderson
@ 2023-09-10 23:28   ` Gavin Shan
  2023-09-11  9:43     ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 18+ messages in thread
From: Gavin Shan @ 2023-09-10 23:28 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel, David Hildenbrand
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	qemu-riscv, Aurelien Jarno, Bin Meng, Xiaojuan Yang,
	Daniel Henrique Barboza, Aleksandar Rikalo, Artyom Tarasenko,
	Song Gao, Stafford Horne, Yanan Wang, Alistair Francis,
	Brian Cain, Cédric Le Goater, Thomas Huth, Liu Zhiwei

Hi Philippe,

On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
> Add a field to return the QOM type name of a CPU class.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/core/cpu.h   | 2 ++
>   hw/core/cpu-common.c    | 2 +-
>   target/alpha/cpu.c      | 1 +
>   target/arm/cpu.c        | 1 +
>   target/avr/cpu.c        | 1 +
>   target/cris/cpu.c       | 1 +
>   target/hexagon/cpu.c    | 1 +
>   target/hppa/cpu.c       | 1 +
>   target/i386/cpu.c       | 1 +
>   target/loongarch/cpu.c  | 1 +
>   target/m68k/cpu.c       | 1 +
>   target/microblaze/cpu.c | 1 +
>   target/mips/cpu.c       | 1 +
>   target/nios2/cpu.c      | 1 +
>   target/openrisc/cpu.c   | 1 +
>   target/ppc/cpu_init.c   | 1 +
>   target/riscv/cpu.c      | 1 +
>   target/rx/cpu.c         | 1 +
>   target/s390x/cpu.c      | 1 +
>   target/sh4/cpu.c        | 1 +
>   target/sparc/cpu.c      | 1 +
>   target/tricore/cpu.c    | 1 +
>   target/xtensa/cpu.c     | 1 +
>   23 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 129d179937..e469efd409 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -100,6 +100,7 @@ struct SysemuCPUOps;
>   
>   /**
>    * CPUClass:
> + * @cpu_resolving_type: CPU QOM type name
>    * @class_by_name: Callback to map -cpu command line model name to an
>    *                 instantiatable CPU type.
>    * @parse_features: Callback to parse command line arguments.
> @@ -148,6 +149,7 @@ struct CPUClass {
>       DeviceClass parent_class;
>       /*< public >*/
>   
> +    const char *cpu_resolving_type;
>       ObjectClass *(*class_by_name)(const char *cpu_model);
>       void (*parse_features)(const char *typename, char *str, Error **errp);
>   

The question is why not use CPU_RESOLVING_TYPE directly? It seems CPU_RESOLVING_TYPE
is exactly what you want here. Besides, I guess the changes can be squeezed into two
patches (commits) as below:

PATCH[1] target/alpha: Tidy up alpha_cpu_class_by_name()
PATCH[2] Move the checks (oc == NULL || object_class_is_abstract() || !object_class_dynamic_cast())
          from individual targets to hw/core/cpu-common.c::cpu_class_by_name()

I rebase my series of '[PATCH v3 00/32] Unified CPU type check' on top of yours. Please
let me know if I need to include your patch into my v4 series for review. In that case,
I can include your patches with above changes applied.

Thanks,
Gavin

> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index c6a0c9390c..2d24261a6a 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -155,7 +155,7 @@ ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
>       assert(cpu_model);
>       oc = object_class_by_name(typename);
>       cc = CPU_CLASS(oc);
> -    assert(cc->class_by_name);
> +    assert(cc->cpu_resolving_type && cc->class_by_name);
>       oc = cc->class_by_name(cpu_model);
>       if (oc == NULL || object_class_is_abstract(oc)) {
>           return NULL;
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 351ee2e9f2..0ddea8004c 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -254,6 +254,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
>       device_class_set_parent_realize(dc, alpha_cpu_realizefn,
>                                       &acc->parent_realize);
>   
> +    cc->cpu_resolving_type = TYPE_ALPHA_CPU;
>       cc->class_by_name = alpha_cpu_class_by_name;
>       cc->has_work = alpha_cpu_has_work;
>       cc->dump_state = alpha_cpu_dump_state;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 42e29816cc..9e51bde170 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2377,6 +2377,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, arm_cpu_reset_hold, NULL,
>                                          &acc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_ARM_CPU;
>       cc->class_by_name = arm_cpu_class_by_name;
>       cc->has_work = arm_cpu_has_work;
>       cc->dump_state = arm_cpu_dump_state;
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 4b255eade1..f6004169ac 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -233,6 +233,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, avr_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_AVR_CPU;
>       cc->class_by_name = avr_cpu_class_by_name;
>   
>       cc->has_work = avr_cpu_has_work;
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index 115f6e2ea2..adde4f599d 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -314,6 +314,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, cris_cpu_reset_hold, NULL,
>                                          &ccc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_CRIS_CPU;
>       cc->class_by_name = cris_cpu_class_by_name;
>       cc->has_work = cris_cpu_has_work;
>       cc->dump_state = cris_cpu_dump_state;
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index 5e301327d3..2d4fed838d 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -381,6 +381,7 @@ static void hexagon_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, hexagon_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_HEXAGON_CPU;
>       cc->class_by_name = hexagon_cpu_class_by_name;
>       cc->has_work = hexagon_cpu_has_work;
>       cc->dump_state = hexagon_dump_state;
> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
> index 11022f9c99..47950a15ae 100644
> --- a/target/hppa/cpu.c
> +++ b/target/hppa/cpu.c
> @@ -192,6 +192,7 @@ static void hppa_cpu_class_init(ObjectClass *oc, void *data)
>       device_class_set_parent_realize(dc, hppa_cpu_realizefn,
>                                       &acc->parent_realize);
>   
> +    cc->cpu_resolving_type = TYPE_HPPA_CPU;
>       cc->class_by_name = hppa_cpu_class_by_name;
>       cc->has_work = hppa_cpu_has_work;
>       cc->dump_state = hppa_cpu_dump_state;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 00f913b638..9979464420 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7951,6 +7951,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>                                          &xcc->parent_phases);
>       cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;
>   
> +    cc->cpu_resolving_type = TYPE_X86_CPU;
>       cc->class_by_name = x86_cpu_class_by_name;
>       cc->parse_features = x86_cpu_parse_featurestr;
>       cc->has_work = x86_cpu_has_work;
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index fe2e5ecc46..189dfd32d1 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -743,6 +743,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
>                                          &lacc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_LOONGARCH_CPU;
>       cc->class_by_name = loongarch_cpu_class_by_name;
>       cc->has_work = loongarch_cpu_has_work;
>       cc->dump_state = loongarch_cpu_dump_state;
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 004f3d6265..bd7bb103d7 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -558,6 +558,7 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, m68k_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_M68K_CPU;
>       cc->class_by_name = m68k_cpu_class_by_name;
>       cc->has_work = m68k_cpu_has_work;
>       cc->dump_state = m68k_cpu_dump_state;
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 03c2c4db1f..bb5f2c1f00 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -414,6 +414,7 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, mb_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_MICROBLAZE_CPU;
>       cc->class_by_name = mb_cpu_class_by_name;
>       cc->has_work = mb_cpu_has_work;
>   
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 63da1948fd..649147df2e 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -578,6 +578,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, mips_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_MIPS_CPU;
>       cc->class_by_name = mips_cpu_class_by_name;
>       cc->has_work = mips_cpu_has_work;
>       cc->dump_state = mips_cpu_dump_state;
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index bc5cbf81c2..fc7c6a83ee 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -381,6 +381,7 @@ static void nios2_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, nios2_cpu_reset_hold, NULL,
>                                          &ncc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_NIOS2_CPU;
>       cc->class_by_name = nios2_cpu_class_by_name;
>       cc->has_work = nios2_cpu_has_work;
>       cc->dump_state = nios2_cpu_dump_state;
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index 3bbbcc4e63..5e1e0576e0 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -243,6 +243,7 @@ static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, openrisc_cpu_reset_hold, NULL,
>                                          &occ->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_OPENRISC_CPU;
>       cc->class_by_name = openrisc_cpu_class_by_name;
>       cc->has_work = openrisc_cpu_has_work;
>       cc->dump_state = openrisc_cpu_dump_state;
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 02b7aad9b0..bc106d01a2 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7357,6 +7357,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
>                                          &pcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_POWERPC_CPU;
>       cc->class_by_name = ppc_cpu_class_by_name;
>       cc->has_work = ppc_cpu_has_work;
>       cc->dump_state = ppc_cpu_dump_state;
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 17b00eb7c0..e8f04ef82b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2130,6 +2130,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, riscv_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_RISCV_CPU;
>       cc->class_by_name = riscv_cpu_class_by_name;
>       cc->has_work = riscv_cpu_has_work;
>       cc->dump_state = riscv_cpu_dump_state;
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index c98034540d..2a6df418a8 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -222,6 +222,7 @@ static void rx_cpu_class_init(ObjectClass *klass, void *data)
>       resettable_class_set_parent_phases(rc, NULL, rx_cpu_reset_hold, NULL,
>                                          &rcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_RX_CPU;
>       cc->class_by_name = rx_cpu_class_by_name;
>       cc->has_work = rx_cpu_has_work;
>       cc->dump_state = rx_cpu_dump_state;
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index df167493c3..bcba466bb4 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -336,6 +336,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>       device_class_set_parent_reset(dc, s390_cpu_reset_full, &scc->parent_reset);
>   
>       scc->reset = s390_cpu_reset;
> +    cc->cpu_resolving_type = TYPE_S390_CPU;
>       cc->class_by_name = s390_cpu_class_by_name,
>       cc->has_work = s390_cpu_has_work;
>       cc->dump_state = s390_cpu_dump_state;
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index bc112776fc..17c87f15f2 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -283,6 +283,7 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, superh_cpu_reset_hold, NULL,
>                                          &scc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_SUPERH_CPU;
>       cc->class_by_name = superh_cpu_class_by_name;
>       cc->has_work = superh_cpu_has_work;
>       cc->dump_state = superh_cpu_dump_state;
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index 130ab8f578..e41a9f4ee2 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -902,6 +902,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, sparc_cpu_reset_hold, NULL,
>                                          &scc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_SPARC_CPU;
>       cc->class_by_name = sparc_cpu_class_by_name;
>       cc->parse_features = sparc_cpu_parse_features;
>       cc->has_work = sparc_cpu_has_work;
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index a2381b0dc1..ffe5158786 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -202,6 +202,7 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data)
>   
>       resettable_class_set_parent_phases(rc, NULL, tricore_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
> +    cc->cpu_resolving_type = TYPE_TRICORE_CPU;
>       cc->class_by_name = tricore_cpu_class_by_name;
>       cc->has_work = tricore_cpu_has_work;
>   
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index a31825a2b5..13bed05d0c 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -252,6 +252,7 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, xtensa_cpu_reset_hold, NULL,
>                                          &xcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_XTENSA_CPU;
>       cc->class_by_name = xtensa_cpu_class_by_name;
>       cc->has_work = xtensa_cpu_has_work;
>       cc->dump_state = xtensa_cpu_dump_state;



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] target/alpha: Tidy up alpha_cpu_class_by_name()
  2023-09-08 11:22 ` [PATCH 1/4] target/alpha: Tidy up alpha_cpu_class_by_name() Philippe Mathieu-Daudé
  2023-09-09 22:17   ` Richard Henderson
@ 2023-09-10 23:29   ` Gavin Shan
  1 sibling, 0 replies; 18+ messages in thread
From: Gavin Shan @ 2023-09-10 23:29 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel, David Hildenbrand
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	qemu-riscv, Aurelien Jarno, Bin Meng, Xiaojuan Yang,
	Daniel Henrique Barboza, Aleksandar Rikalo, Artyom Tarasenko,
	Song Gao, Stafford Horne, Yanan Wang, Alistair Francis,
	Brian Cain, Cédric Le Goater, Thomas Huth, Liu Zhiwei



On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/alpha/cpu.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 270ae787b1..351ee2e9f2 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -142,13 +142,10 @@ static ObjectClass *alpha_cpu_class_by_name(const char *cpu_model)
>       typename = g_strdup_printf(ALPHA_CPU_TYPE_NAME("%s"), cpu_model);
>       oc = object_class_by_name(typename);
>       g_free(typename);
> -    if (oc != NULL && object_class_is_abstract(oc)) {
> -        oc = NULL;
> -    }
>   
>       /* TODO: remove match everything nonsense */
> -    /* Default to ev67; no reason not to emulate insns by default. */
> -    if (!oc) {
> +    if (!oc || object_class_is_abstract(oc)) {
> +        /* Default to ev67; no reason not to emulate insns by default. */
>           oc = object_class_by_name(ALPHA_CPU_TYPE_NAME("ev67"));
>       }
>   



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] hw/cpu: Call object_class_dynamic_cast() once in cpu_class_by_name()
  2023-09-08 11:22 ` [PATCH 4/4] hw/cpu: Call object_class_dynamic_cast() once in cpu_class_by_name() Philippe Mathieu-Daudé
  2023-09-09 23:26   ` Richard Henderson
@ 2023-09-10 23:40   ` Gavin Shan
  1 sibling, 0 replies; 18+ messages in thread
From: Gavin Shan @ 2023-09-10 23:40 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel, David Hildenbrand
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	qemu-riscv, Aurelien Jarno, Bin Meng, Xiaojuan Yang,
	Daniel Henrique Barboza, Aleksandar Rikalo, Artyom Tarasenko,
	Song Gao, Stafford Horne, Yanan Wang, Alistair Francis,
	Brian Cain, Cédric Le Goater, Thomas Huth, Liu Zhiwei


On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
> Leverage the public CPUClass::cpu_resolving_type field and
> call object_class_dynamic_cast() once in cpu_class_by_name().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/core/cpu-common.c   | 3 ++-
>   target/alpha/cpu.c     | 3 +--
>   target/arm/cpu.c       | 4 +---
>   target/avr/cpu.c       | 4 +---
>   target/cris/cpu.c      | 4 +---
>   target/hexagon/cpu.c   | 4 +---
>   target/loongarch/cpu.c | 5 +----
>   target/m68k/cpu.c      | 4 +---
>   target/openrisc/cpu.c  | 4 +---
>   target/riscv/cpu.c     | 4 +---
>   target/rx/cpu.c        | 2 +-
>   target/tricore/cpu.c   | 4 +---
>   target/xtensa/cpu.c    | 4 +---
>   13 files changed, 14 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 2d24261a6a..f4a2ccebea 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -157,7 +157,8 @@ ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
>       cc = CPU_CLASS(oc);
>       assert(cc->cpu_resolving_type && cc->class_by_name);
>       oc = cc->class_by_name(cpu_model);
> -    if (oc == NULL || object_class_is_abstract(oc)) {
> +    if (oc == NULL || object_class_is_abstract(oc)
> +                   || !object_class_dynamic_cast(oc, cc->cpu_resolving_type)) {
>           return NULL;
>       }
>       return oc;

Alignment.

     if (!oc || object_class_is_abstract(oc) ||
         !object_class_dynamic_cast(oc, cc->cpu_resolving_type)) {

> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 0ddea8004c..b184fcc123 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -126,8 +126,7 @@ static ObjectClass *alpha_cpu_class_by_name(const char *cpu_model)
>       int i;
>   
>       oc = object_class_by_name(cpu_model);
> -    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_ALPHA_CPU) != NULL &&
> -        !object_class_is_abstract(oc)) {
> +    if (oc != NULL && !object_class_is_abstract(oc)) {
>           return oc;
>       }
>   

No, the check "!object_class_is_abstract(oc)" for this level can't be dropped.
The same check for the last level can be dropped because it's going to be
covered by hw/core/cpu-common.c::cpu_class_by_name().

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 9e51bde170..d29040cd8c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2300,9 +2300,7 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
>       oc = object_class_by_name(typename);
>       g_strfreev(cpuname);
>       g_free(typename);
> -    if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU)) {
> -        return NULL;
> -    }
> +
>       return oc;
>   }
>   
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index f6004169ac..53735ff1dd 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -159,9 +159,7 @@ static ObjectClass *avr_cpu_class_by_name(const char *cpu_model)
>       ObjectClass *oc;
>   
>       oc = object_class_by_name(cpu_model);
> -    if (object_class_dynamic_cast(oc, TYPE_AVR_CPU) == NULL) {
> -        oc = NULL;
> -    }
> +
>       return oc;
>   }
>   
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index adde4f599d..b307d0b9db 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -95,9 +95,7 @@ static ObjectClass *cris_cpu_class_by_name(const char *cpu_model)
>       typename = g_strdup_printf(CRIS_CPU_TYPE_NAME("%s"), cpu_model);
>       oc = object_class_by_name(typename);
>       g_free(typename);
> -    if (oc != NULL && !object_class_dynamic_cast(oc, TYPE_CRIS_CPU)) {
> -        oc = NULL;
> -    }
> +
>       return oc;
>   }
>   
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index 2d4fed838d..4b8d63c4a7 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -63,9 +63,7 @@ static ObjectClass *hexagon_cpu_class_by_name(const char *cpu_model)
>       oc = object_class_by_name(typename);
>       g_strfreev(cpuname);
>       g_free(typename);
> -    if (!oc || !object_class_dynamic_cast(oc, TYPE_HEXAGON_CPU)) {
> -        return NULL;
> -    }
> +
>       return oc;
>   }
>   
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 189dfd32d1..1eb2c579eb 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -646,10 +646,7 @@ static ObjectClass *loongarch_cpu_class_by_name(const char *cpu_model)
>           }
>       }
>   
> -    if (object_class_dynamic_cast(oc, TYPE_LOONGARCH_CPU)) {
> -        return oc;
> -    }
> -    return NULL;
> +    return oc;
>   }
>   
>   void loongarch_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index bd7bb103d7..e8b86c80f1 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -111,9 +111,7 @@ static ObjectClass *m68k_cpu_class_by_name(const char *cpu_model)
>       typename = g_strdup_printf(M68K_CPU_TYPE_NAME("%s"), cpu_model);
>       oc = object_class_by_name(typename);
>       g_free(typename);
> -    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_M68K_CPU) == NULL) {
> -        return NULL;
> -    }
> +
>       return oc;
>   }
>   
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index 5e1e0576e0..7aac9105bd 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -168,9 +168,7 @@ static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
>       typename = g_strdup_printf(OPENRISC_CPU_TYPE_NAME("%s"), cpu_model);
>       oc = object_class_by_name(typename);
>       g_free(typename);
> -    if (oc != NULL && !object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU)) {
> -        return NULL;
> -    }
> +
>       return oc;
>   }
>   
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e8f04ef82b..0170e288e7 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -619,9 +619,7 @@ static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
>       oc = object_class_by_name(typename);
>       g_strfreev(cpuname);
>       g_free(typename);
> -    if (!oc || !object_class_dynamic_cast(oc, TYPE_RISCV_CPU)) {
> -        return NULL;
> -    }
> +
>       return oc;
>   }
>   
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index 2a6df418a8..879d4fcdef 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -111,7 +111,7 @@ static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
>       char *typename;
>   
>       oc = object_class_by_name(cpu_model);
> -    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_RX_CPU) != NULL) {
> +    if (oc != NULL) {
>           return oc;
>       }
>       typename = g_strdup_printf(RX_CPU_TYPE_NAME("%s"), cpu_model);

Same as above. The check (object_class_dynamic_cast()) can't be dropped in
this level. Besides, the check object_class_is_abstract() is missed for this
level.

> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index ffe5158786..f65b8761b0 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -140,9 +140,7 @@ static ObjectClass *tricore_cpu_class_by_name(const char *cpu_model)
>       typename = g_strdup_printf(TRICORE_CPU_TYPE_NAME("%s"), cpu_model);
>       oc = object_class_by_name(typename);
>       g_free(typename);
> -    if (!oc || !object_class_dynamic_cast(oc, TYPE_TRICORE_CPU)) {
> -        return NULL;
> -    }
> +
>       return oc;
>   }
>   
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index 13bed05d0c..6d96e5ab27 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -141,9 +141,7 @@ static ObjectClass *xtensa_cpu_class_by_name(const char *cpu_model)
>       typename = g_strdup_printf(XTENSA_CPU_TYPE_NAME("%s"), cpu_model);
>       oc = object_class_by_name(typename);
>       g_free(typename);
> -    if (oc == NULL || !object_class_dynamic_cast(oc, TYPE_XTENSA_CPU)) {
> -        return NULL;
> -    }
> +
>       return oc;
>   }
>   

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
  2023-09-10 23:28   ` Gavin Shan
@ 2023-09-11  9:43     ` Philippe Mathieu-Daudé
  2023-09-11 10:55       ` Igor Mammedov
  2023-09-11 22:40       ` Gavin Shan
  0 siblings, 2 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-09-11  9:43 UTC (permalink / raw
  To: Gavin Shan, qemu-devel, David Hildenbrand
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	qemu-riscv, Aurelien Jarno, Bin Meng, Xiaojuan Yang,
	Daniel Henrique Barboza, Aleksandar Rikalo, Artyom Tarasenko,
	Song Gao, Stafford Horne, Yanan Wang, Alistair Francis,
	Brian Cain, Cédric Le Goater, Thomas Huth, Liu Zhiwei

On 11/9/23 01:28, Gavin Shan wrote:
> Hi Philippe,
> 
> On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
>> Add a field to return the QOM type name of a CPU class.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/core/cpu.h   | 2 ++
>>   hw/core/cpu-common.c    | 2 +-
>>   target/alpha/cpu.c      | 1 +
>>   target/arm/cpu.c        | 1 +
>>   target/avr/cpu.c        | 1 +
>>   target/cris/cpu.c       | 1 +
>>   target/hexagon/cpu.c    | 1 +
>>   target/hppa/cpu.c       | 1 +
>>   target/i386/cpu.c       | 1 +
>>   target/loongarch/cpu.c  | 1 +
>>   target/m68k/cpu.c       | 1 +
>>   target/microblaze/cpu.c | 1 +
>>   target/mips/cpu.c       | 1 +
>>   target/nios2/cpu.c      | 1 +
>>   target/openrisc/cpu.c   | 1 +
>>   target/ppc/cpu_init.c   | 1 +
>>   target/riscv/cpu.c      | 1 +
>>   target/rx/cpu.c         | 1 +
>>   target/s390x/cpu.c      | 1 +
>>   target/sh4/cpu.c        | 1 +
>>   target/sparc/cpu.c      | 1 +
>>   target/tricore/cpu.c    | 1 +
>>   target/xtensa/cpu.c     | 1 +
>>   23 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index 129d179937..e469efd409 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -100,6 +100,7 @@ struct SysemuCPUOps;
>>   /**
>>    * CPUClass:
>> + * @cpu_resolving_type: CPU QOM type name
>>    * @class_by_name: Callback to map -cpu command line model name to an
>>    *                 instantiatable CPU type.
>>    * @parse_features: Callback to parse command line arguments.
>> @@ -148,6 +149,7 @@ struct CPUClass {
>>       DeviceClass parent_class;
>>       /*< public >*/
>> +    const char *cpu_resolving_type;
>>       ObjectClass *(*class_by_name)(const char *cpu_model);
>>       void (*parse_features)(const char *typename, char *str, Error 
>> **errp);
> 
> The question is why not use CPU_RESOLVING_TYPE directly? It seems 
> CPU_RESOLVING_TYPE
> is exactly what you want here.

Unfortunately CPU_RESOLVING_TYPE is target-specific, we want
hw/core/cpu-common.c to be target-agnostic (build once for all
targets). This is particularly important in the context of
heterogeneous QEMU, where a single binary will be able to create
CPUs from different targets.

> Besides, I guess the changes can be 
> squeezed into two
> patches (commits) as below:
> 
> PATCH[1] target/alpha: Tidy up alpha_cpu_class_by_name()
> PATCH[2] Move the checks (oc == NULL || object_class_is_abstract() || 
> !object_class_dynamic_cast())
>           from individual targets to 
> hw/core/cpu-common.c::cpu_class_by_name()
> 
> I rebase my series of '[PATCH v3 00/32] Unified CPU type check' on top 
> of yours. Please
> let me know if I need to include your patch into my v4 series for 
> review. In that case,
> I can include your patches with above changes applied.
> 
> Thanks,
> Gavin
> 
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index c6a0c9390c..2d24261a6a 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -155,7 +155,7 @@ ObjectClass *cpu_class_by_name(const char 
>> *typename, const char *cpu_model)
>>       assert(cpu_model);
>>       oc = object_class_by_name(typename);
>>       cc = CPU_CLASS(oc);
>> -    assert(cc->class_by_name);
>> +    assert(cc->cpu_resolving_type && cc->class_by_name);
>>       oc = cc->class_by_name(cpu_model);
>>       if (oc == NULL || object_class_is_abstract(oc)) {
>>           return NULL;



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
  2023-09-11  9:43     ` Philippe Mathieu-Daudé
@ 2023-09-11 10:55       ` Igor Mammedov
  2023-09-11 22:40       ` Gavin Shan
  1 sibling, 0 replies; 18+ messages in thread
From: Igor Mammedov @ 2023-09-11 10:55 UTC (permalink / raw
  To: Philippe Mathieu-Daudé
  Cc: Gavin Shan, qemu-devel, David Hildenbrand, Chris Wulff,
	David Gibson, qemu-s390x, Weiwei Li, qemu-arm, Mark Cave-Ayland,
	Jiaxun Yang, Yoshinori Sato, Richard Henderson, Marcel Apfelbaum,
	Max Filippov, Nicholas Piggin, Eduardo Habkost, Ilya Leoshkevich,
	Bastian Koppelmann, Greg Kurz, Edgar E. Iglesias, qemu-ppc,
	Daniel Henrique Barboza, Marek Vasut, Palmer Dabbelt,
	Michael Rolnik, Laurent Vivier, Peter Maydell, qemu-riscv,
	Aurelien Jarno, Bin Meng, Xiaojuan Yang, Daniel Henrique Barboza,
	Aleksandar Rikalo, Artyom Tarasenko, Song Gao, Stafford Horne,
	Yanan Wang, Alistair Francis, Brian Cain, Cédric Le Goater,
	Thomas Huth, Liu Zhiwei

On Mon, 11 Sep 2023 11:43:00 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 11/9/23 01:28, Gavin Shan wrote:
> > Hi Philippe,
> > 
> > On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:  
> >> Add a field to return the QOM type name of a CPU class.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>   include/hw/core/cpu.h   | 2 ++
> >>   hw/core/cpu-common.c    | 2 +-
> >>   target/alpha/cpu.c      | 1 +
> >>   target/arm/cpu.c        | 1 +
> >>   target/avr/cpu.c        | 1 +
> >>   target/cris/cpu.c       | 1 +
> >>   target/hexagon/cpu.c    | 1 +
> >>   target/hppa/cpu.c       | 1 +
> >>   target/i386/cpu.c       | 1 +
> >>   target/loongarch/cpu.c  | 1 +
> >>   target/m68k/cpu.c       | 1 +
> >>   target/microblaze/cpu.c | 1 +
> >>   target/mips/cpu.c       | 1 +
> >>   target/nios2/cpu.c      | 1 +
> >>   target/openrisc/cpu.c   | 1 +
> >>   target/ppc/cpu_init.c   | 1 +
> >>   target/riscv/cpu.c      | 1 +
> >>   target/rx/cpu.c         | 1 +
> >>   target/s390x/cpu.c      | 1 +
> >>   target/sh4/cpu.c        | 1 +
> >>   target/sparc/cpu.c      | 1 +
> >>   target/tricore/cpu.c    | 1 +
> >>   target/xtensa/cpu.c     | 1 +
> >>   23 files changed, 24 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> >> index 129d179937..e469efd409 100644
> >> --- a/include/hw/core/cpu.h
> >> +++ b/include/hw/core/cpu.h
> >> @@ -100,6 +100,7 @@ struct SysemuCPUOps;
> >>   /**
> >>    * CPUClass:
> >> + * @cpu_resolving_type: CPU QOM type name
> >>    * @class_by_name: Callback to map -cpu command line model name to an
> >>    *                 instantiatable CPU type.
> >>    * @parse_features: Callback to parse command line arguments.
> >> @@ -148,6 +149,7 @@ struct CPUClass {
> >>       DeviceClass parent_class;
> >>       /*< public >*/
> >> +    const char *cpu_resolving_type;
> >>       ObjectClass *(*class_by_name)(const char *cpu_model);
> >>       void (*parse_features)(const char *typename, char *str, Error 
> >> **errp);  
> > 
> > The question is why not use CPU_RESOLVING_TYPE directly? It seems 
> > CPU_RESOLVING_TYPE
> > is exactly what you want here.  
> 
> Unfortunately CPU_RESOLVING_TYPE is target-specific, we want
> hw/core/cpu-common.c to be target-agnostic (build once for all
> targets). This is particularly important in the context of
> heterogeneous QEMU, where a single binary will be able to create
> CPUs from different targets.

Well cpu model resolving ain't going to work with heterogeneous env.
But otherwise it's argument towards dropping CPU model and use
cpu types directly (then we can get rid all this resolving nonsense).

Though for Gavin's purposes, given existing cpu type naming convention
it's sufficient to just cut of the last 2 '-' from typename to get
cpumodel (target independent and no need for resolving type). 


> 
> > Besides, I guess the changes can be 
> > squeezed into two
> > patches (commits) as below:
> > 
> > PATCH[1] target/alpha: Tidy up alpha_cpu_class_by_name()
> > PATCH[2] Move the checks (oc == NULL || object_class_is_abstract() || 
> > !object_class_dynamic_cast())
> >           from individual targets to 
> > hw/core/cpu-common.c::cpu_class_by_name()
> > 
> > I rebase my series of '[PATCH v3 00/32] Unified CPU type check' on top 
> > of yours. Please
> > let me know if I need to include your patch into my v4 series for 
> > review. In that case,
> > I can include your patches with above changes applied.
> > 
> > Thanks,
> > Gavin
> >   
> >> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> >> index c6a0c9390c..2d24261a6a 100644
> >> --- a/hw/core/cpu-common.c
> >> +++ b/hw/core/cpu-common.c
> >> @@ -155,7 +155,7 @@ ObjectClass *cpu_class_by_name(const char 
> >> *typename, const char *cpu_model)
> >>       assert(cpu_model);
> >>       oc = object_class_by_name(typename);
> >>       cc = CPU_CLASS(oc);
> >> -    assert(cc->class_by_name);
> >> +    assert(cc->cpu_resolving_type && cc->class_by_name);
> >>       oc = cc->class_by_name(cpu_model);
> >>       if (oc == NULL || object_class_is_abstract(oc)) {
> >>           return NULL;  
> 
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
  2023-09-11  9:43     ` Philippe Mathieu-Daudé
  2023-09-11 10:55       ` Igor Mammedov
@ 2023-09-11 22:40       ` Gavin Shan
  2023-09-25  0:24         ` Gavin Shan
  1 sibling, 1 reply; 18+ messages in thread
From: Gavin Shan @ 2023-09-11 22:40 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel, David Hildenbrand
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	qemu-riscv, Aurelien Jarno, Bin Meng, Xiaojuan Yang,
	Daniel Henrique Barboza, Aleksandar Rikalo, Artyom Tarasenko,
	Song Gao, Stafford Horne, Yanan Wang, Alistair Francis,
	Brian Cain, Cédric Le Goater, Thomas Huth, Liu Zhiwei


On 9/11/23 19:43, Philippe Mathieu-Daudé wrote:
> On 11/9/23 01:28, Gavin Shan wrote:
>> On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
>>> Add a field to return the QOM type name of a CPU class.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   include/hw/core/cpu.h   | 2 ++
>>>   hw/core/cpu-common.c    | 2 +-
>>>   target/alpha/cpu.c      | 1 +
>>>   target/arm/cpu.c        | 1 +
>>>   target/avr/cpu.c        | 1 +
>>>   target/cris/cpu.c       | 1 +
>>>   target/hexagon/cpu.c    | 1 +
>>>   target/hppa/cpu.c       | 1 +
>>>   target/i386/cpu.c       | 1 +
>>>   target/loongarch/cpu.c  | 1 +
>>>   target/m68k/cpu.c       | 1 +
>>>   target/microblaze/cpu.c | 1 +
>>>   target/mips/cpu.c       | 1 +
>>>   target/nios2/cpu.c      | 1 +
>>>   target/openrisc/cpu.c   | 1 +
>>>   target/ppc/cpu_init.c   | 1 +
>>>   target/riscv/cpu.c      | 1 +
>>>   target/rx/cpu.c         | 1 +
>>>   target/s390x/cpu.c      | 1 +
>>>   target/sh4/cpu.c        | 1 +
>>>   target/sparc/cpu.c      | 1 +
>>>   target/tricore/cpu.c    | 1 +
>>>   target/xtensa/cpu.c     | 1 +
>>>   23 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>> index 129d179937..e469efd409 100644
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -100,6 +100,7 @@ struct SysemuCPUOps;
>>>   /**
>>>    * CPUClass:
>>> + * @cpu_resolving_type: CPU QOM type name
>>>    * @class_by_name: Callback to map -cpu command line model name to an
>>>    *                 instantiatable CPU type.
>>>    * @parse_features: Callback to parse command line arguments.
>>> @@ -148,6 +149,7 @@ struct CPUClass {
>>>       DeviceClass parent_class;
>>>       /*< public >*/
>>> +    const char *cpu_resolving_type;
>>>       ObjectClass *(*class_by_name)(const char *cpu_model);
>>>       void (*parse_features)(const char *typename, char *str, Error **errp);
>>
>> The question is why not use CPU_RESOLVING_TYPE directly? It seems CPU_RESOLVING_TYPE
>> is exactly what you want here.
> 
> Unfortunately CPU_RESOLVING_TYPE is target-specific, we want
> hw/core/cpu-common.c to be target-agnostic (build once for all
> targets). This is particularly important in the context of
> heterogeneous QEMU, where a single binary will be able to create
> CPUs from different targets.
> 

CPU_RESOLVING_TYPE and CPUClass::cpu_resolving_type is duplicate to
each other. There are two options I can figure out to avoid the
duplication.

(a) move cpu_class_by_name() from hw/core/cpu-common.c to cpu.c, so that
     CPU_RESOLVING_TYPE can be seen. cpu.c::list_cpus() is the example.

(b) remove hw/core/cpu-common.c::cpu_calss_by_name() and squeeze its
     logic to cpu.c::parse_cpu_option() since there are not too much
     users for it. target/arm and target/s390 needs some tweaks so that
     hw/core/cpu-common.c::cpu_calss_by_name() can be removed.

     [gshan@gshan q]$ git grep \ cpu_class_by_name\(
     cpu.c:    oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]);
     target/arm/arm-qmp-cmds.c:    oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
     target/s390x/cpu_models_sysemu.c:    oc = cpu_class_by_name(TYPE_S390_CPU, info->name);

When option (b) is taken, this series to have the checks against @oc
in hw/core/cpu-common.c::cpu_calss_by_name() becomes non-sense. Instead,
we need the same (and complete) checks in CPUClass::class_by_name() for
individual targets. Further more, an inline helper can be provided to do
the check in CPUClass::class_by_name() for individual targets.

    include/hw/core/cpu.h

    static inline bool cpu_class_is_valid(ObjectClass *oc, const char *parent)
    {
        if (!object_class_dynamic_cast(oc, parent) ||
            object_class_is_abstract(oc)) {
            return false;
        }

        return true;
    }

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
  2023-09-11 22:40       ` Gavin Shan
@ 2023-09-25  0:24         ` Gavin Shan
  2023-10-11  3:28           ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 18+ messages in thread
From: Gavin Shan @ 2023-09-25  0:24 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel, David Hildenbrand
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	qemu-riscv, Aurelien Jarno, Bin Meng, Xiaojuan Yang,
	Daniel Henrique Barboza, Aleksandar Rikalo, Artyom Tarasenko,
	Song Gao, Stafford Horne, Yanan Wang, Alistair Francis,
	Brian Cain, Cédric Le Goater, Thomas Huth, Liu Zhiwei

Hi Philippe,

On 9/12/23 08:40, Gavin Shan wrote:
> On 9/11/23 19:43, Philippe Mathieu-Daudé wrote:
>> On 11/9/23 01:28, Gavin Shan wrote:
>>> On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
>>>> Add a field to return the QOM type name of a CPU class.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>   include/hw/core/cpu.h   | 2 ++
>>>>   hw/core/cpu-common.c    | 2 +-
>>>>   target/alpha/cpu.c      | 1 +
>>>>   target/arm/cpu.c        | 1 +
>>>>   target/avr/cpu.c        | 1 +
>>>>   target/cris/cpu.c       | 1 +
>>>>   target/hexagon/cpu.c    | 1 +
>>>>   target/hppa/cpu.c       | 1 +
>>>>   target/i386/cpu.c       | 1 +
>>>>   target/loongarch/cpu.c  | 1 +
>>>>   target/m68k/cpu.c       | 1 +
>>>>   target/microblaze/cpu.c | 1 +
>>>>   target/mips/cpu.c       | 1 +
>>>>   target/nios2/cpu.c      | 1 +
>>>>   target/openrisc/cpu.c   | 1 +
>>>>   target/ppc/cpu_init.c   | 1 +
>>>>   target/riscv/cpu.c      | 1 +
>>>>   target/rx/cpu.c         | 1 +
>>>>   target/s390x/cpu.c      | 1 +
>>>>   target/sh4/cpu.c        | 1 +
>>>>   target/sparc/cpu.c      | 1 +
>>>>   target/tricore/cpu.c    | 1 +
>>>>   target/xtensa/cpu.c     | 1 +
>>>>   23 files changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>>> index 129d179937..e469efd409 100644
>>>> --- a/include/hw/core/cpu.h
>>>> +++ b/include/hw/core/cpu.h
>>>> @@ -100,6 +100,7 @@ struct SysemuCPUOps;
>>>>   /**
>>>>    * CPUClass:
>>>> + * @cpu_resolving_type: CPU QOM type name
>>>>    * @class_by_name: Callback to map -cpu command line model name to an
>>>>    *                 instantiatable CPU type.
>>>>    * @parse_features: Callback to parse command line arguments.
>>>> @@ -148,6 +149,7 @@ struct CPUClass {
>>>>       DeviceClass parent_class;
>>>>       /*< public >*/
>>>> +    const char *cpu_resolving_type;
>>>>       ObjectClass *(*class_by_name)(const char *cpu_model);
>>>>       void (*parse_features)(const char *typename, char *str, Error **errp);
>>>
>>> The question is why not use CPU_RESOLVING_TYPE directly? It seems CPU_RESOLVING_TYPE
>>> is exactly what you want here.
>>
>> Unfortunately CPU_RESOLVING_TYPE is target-specific, we want
>> hw/core/cpu-common.c to be target-agnostic (build once for all
>> targets). This is particularly important in the context of
>> heterogeneous QEMU, where a single binary will be able to create
>> CPUs from different targets.
>>
> 
> CPU_RESOLVING_TYPE and CPUClass::cpu_resolving_type is duplicate to
> each other. There are two options I can figure out to avoid the
> duplication.
> 
> (a) move cpu_class_by_name() from hw/core/cpu-common.c to cpu.c, so that
>      CPU_RESOLVING_TYPE can be seen. cpu.c::list_cpus() is the example.
> 
> (b) remove hw/core/cpu-common.c::cpu_calss_by_name() and squeeze its
>      logic to cpu.c::parse_cpu_option() since there are not too much
>      users for it. target/arm and target/s390 needs some tweaks so that
>      hw/core/cpu-common.c::cpu_calss_by_name() can be removed.
> 
>      [gshan@gshan q]$ git grep \ cpu_class_by_name\(
>      cpu.c:    oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]);
>      target/arm/arm-qmp-cmds.c:    oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
>      target/s390x/cpu_models_sysemu.c:    oc = cpu_class_by_name(TYPE_S390_CPU, info->name);
> 
> When option (b) is taken, this series to have the checks against @oc
> in hw/core/cpu-common.c::cpu_calss_by_name() becomes non-sense. Instead,
> we need the same (and complete) checks in CPUClass::class_by_name() for
> individual targets. Further more, an inline helper can be provided to do
> the check in CPUClass::class_by_name() for individual targets.
> 
>     include/hw/core/cpu.h
> 
>     static inline bool cpu_class_is_valid(ObjectClass *oc, const char *parent)
>     {
>         if (!object_class_dynamic_cast(oc, parent) ||
>             object_class_is_abstract(oc)) {
>             return false;
>         }
> 
>         return true;
>     }
> 

Since my series to make CPU type check unified depends on this series, could
you please share your thoughts? If you don't have bandwidth for this, I can
improve the code based on your thoughts, and include your patches to my series
so that they can be reviewed at once. Please just let me know.

Thanks,
Gavin




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
  2023-09-25  0:24         ` Gavin Shan
@ 2023-10-11  3:28           ` Philippe Mathieu-Daudé
  2023-10-11  6:45             ` Gavin Shan
  0 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-11  3:28 UTC (permalink / raw
  To: Gavin Shan, qemu-devel, David Hildenbrand
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	qemu-riscv, Aurelien Jarno, Bin Meng, Xiaojuan Yang,
	Daniel Henrique Barboza, Aleksandar Rikalo, Artyom Tarasenko,
	Song Gao, Stafford Horne, Yanan Wang, Alistair Francis,
	Brian Cain, Cédric Le Goater, Thomas Huth, Liu Zhiwei

Hi Gavin,

On 25/9/23 02:24, Gavin Shan wrote:
> On 9/12/23 08:40, Gavin Shan wrote:
>> On 9/11/23 19:43, Philippe Mathieu-Daudé wrote:
>>> On 11/9/23 01:28, Gavin Shan wrote:
>>>> On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
>>>>> Add a field to return the QOM type name of a CPU class.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---


>>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>>>> index 129d179937..e469efd409 100644
>>>>> --- a/include/hw/core/cpu.h
>>>>> +++ b/include/hw/core/cpu.h
>>>>> @@ -100,6 +100,7 @@ struct SysemuCPUOps;
>>>>>   /**
>>>>>    * CPUClass:
>>>>> + * @cpu_resolving_type: CPU QOM type name
>>>>>    * @class_by_name: Callback to map -cpu command line model name 
>>>>> to an
>>>>>    *                 instantiatable CPU type.
>>>>>    * @parse_features: Callback to parse command line arguments.
>>>>> @@ -148,6 +149,7 @@ struct CPUClass {
>>>>>       DeviceClass parent_class;
>>>>>       /*< public >*/
>>>>> +    const char *cpu_resolving_type;
>>>>>       ObjectClass *(*class_by_name)(const char *cpu_model);
>>>>>       void (*parse_features)(const char *typename, char *str, Error 
>>>>> **errp);
>>>>
>>>> The question is why not use CPU_RESOLVING_TYPE directly? It seems 
>>>> CPU_RESOLVING_TYPE
>>>> is exactly what you want here.
>>>
>>> Unfortunately CPU_RESOLVING_TYPE is target-specific, we want
>>> hw/core/cpu-common.c to be target-agnostic (build once for all
>>> targets). This is particularly important in the context of
>>> heterogeneous QEMU, where a single binary will be able to create
>>> CPUs from different targets.
>>>
>>
>> CPU_RESOLVING_TYPE and CPUClass::cpu_resolving_type is duplicate to
>> each other. There are two options I can figure out to avoid the
>> duplication.
>>
>> (a) move cpu_class_by_name() from hw/core/cpu-common.c to cpu.c, so that
>>      CPU_RESOLVING_TYPE can be seen. cpu.c::list_cpus() is the example.
>>
>> (b) remove hw/core/cpu-common.c::cpu_calss_by_name() and squeeze its
>>      logic to cpu.c::parse_cpu_option() since there are not too much
>>      users for it. target/arm and target/s390 needs some tweaks so that
>>      hw/core/cpu-common.c::cpu_calss_by_name() can be removed.
>>
>>      [gshan@gshan q]$ git grep \ cpu_class_by_name\(
>>      cpu.c:    oc = cpu_class_by_name(CPU_RESOLVING_TYPE, 
>> model_pieces[0]);
>>      target/arm/arm-qmp-cmds.c:    oc = 
>> cpu_class_by_name(TYPE_ARM_CPU, model->name);
>>      target/s390x/cpu_models_sysemu.c:    oc = 
>> cpu_class_by_name(TYPE_S390_CPU, info->name);
>>
>> When option (b) is taken, this series to have the checks against @oc
>> in hw/core/cpu-common.c::cpu_calss_by_name() becomes non-sense. Instead,
>> we need the same (and complete) checks in CPUClass::class_by_name() for
>> individual targets. Further more, an inline helper can be provided to do
>> the check in CPUClass::class_by_name() for individual targets.
>>
>>     include/hw/core/cpu.h
>>
>>     static inline bool cpu_class_is_valid(ObjectClass *oc, const char 
>> *parent)
>>     {
>>         if (!object_class_dynamic_cast(oc, parent) ||
>>             object_class_is_abstract(oc)) {
>>             return false;
>>         }
>>
>>         return true;
>>     }
>>
> 
> Since my series to make CPU type check unified depends on this series, 
> could
> you please share your thoughts? If you don't have bandwidth for this, I can
> improve the code based on your thoughts, and include your patches to my 
> series
> so that they can be reviewed at once. Please just let me know.

You seem to prove (b) is not useful, so we have to do (a).

Unfortunately at this moment I feel hopeless with this topic.

I don't want to delay your work further. If you find a way to integrate
both series, please go ahead. Otherwise let's drop my approach and
continue with your previous work.

I apologize I kept you waiting that long.

Regards,

Phil.



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
  2023-10-11  3:28           ` Philippe Mathieu-Daudé
@ 2023-10-11  6:45             ` Gavin Shan
  0 siblings, 0 replies; 18+ messages in thread
From: Gavin Shan @ 2023-10-11  6:45 UTC (permalink / raw
  To: Philippe Mathieu-Daudé, qemu-devel, David Hildenbrand
  Cc: Chris Wulff, David Gibson, qemu-s390x, Weiwei Li, qemu-arm,
	Mark Cave-Ayland, Jiaxun Yang, Yoshinori Sato, Richard Henderson,
	Marcel Apfelbaum, Max Filippov, Nicholas Piggin, Eduardo Habkost,
	Ilya Leoshkevich, Bastian Koppelmann, Greg Kurz,
	Edgar E. Iglesias, qemu-ppc, Daniel Henrique Barboza, Marek Vasut,
	Palmer Dabbelt, Michael Rolnik, Laurent Vivier, Peter Maydell,
	qemu-riscv, Aurelien Jarno, Bin Meng, Xiaojuan Yang,
	Daniel Henrique Barboza, Aleksandar Rikalo, Artyom Tarasenko,
	Song Gao, Stafford Horne, Yanan Wang, Alistair Francis,
	Brian Cain, Cédric Le Goater, Thomas Huth, Liu Zhiwei

Hi Philippe,

On 10/11/23 13:28, Philippe Mathieu-Daudé wrote:
> On 25/9/23 02:24, Gavin Shan wrote:
>> On 9/12/23 08:40, Gavin Shan wrote:
>>> On 9/11/23 19:43, Philippe Mathieu-Daudé wrote:
>>>> On 11/9/23 01:28, Gavin Shan wrote:
>>>>> On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
>>>>>> Add a field to return the QOM type name of a CPU class.
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
> 
> 
>>>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>>>>> index 129d179937..e469efd409 100644
>>>>>> --- a/include/hw/core/cpu.h
>>>>>> +++ b/include/hw/core/cpu.h
>>>>>> @@ -100,6 +100,7 @@ struct SysemuCPUOps;
>>>>>>   /**
>>>>>>    * CPUClass:
>>>>>> + * @cpu_resolving_type: CPU QOM type name
>>>>>>    * @class_by_name: Callback to map -cpu command line model name to an
>>>>>>    *                 instantiatable CPU type.
>>>>>>    * @parse_features: Callback to parse command line arguments.
>>>>>> @@ -148,6 +149,7 @@ struct CPUClass {
>>>>>>       DeviceClass parent_class;
>>>>>>       /*< public >*/
>>>>>> +    const char *cpu_resolving_type;
>>>>>>       ObjectClass *(*class_by_name)(const char *cpu_model);
>>>>>>       void (*parse_features)(const char *typename, char *str, Error **errp);
>>>>>
>>>>> The question is why not use CPU_RESOLVING_TYPE directly? It seems CPU_RESOLVING_TYPE
>>>>> is exactly what you want here.
>>>>
>>>> Unfortunately CPU_RESOLVING_TYPE is target-specific, we want
>>>> hw/core/cpu-common.c to be target-agnostic (build once for all
>>>> targets). This is particularly important in the context of
>>>> heterogeneous QEMU, where a single binary will be able to create
>>>> CPUs from different targets.
>>>>
>>>
>>> CPU_RESOLVING_TYPE and CPUClass::cpu_resolving_type is duplicate to
>>> each other. There are two options I can figure out to avoid the
>>> duplication.
>>>
>>> (a) move cpu_class_by_name() from hw/core/cpu-common.c to cpu.c, so that
>>>      CPU_RESOLVING_TYPE can be seen. cpu.c::list_cpus() is the example.
>>>
>>> (b) remove hw/core/cpu-common.c::cpu_calss_by_name() and squeeze its
>>>      logic to cpu.c::parse_cpu_option() since there are not too much
>>>      users for it. target/arm and target/s390 needs some tweaks so that
>>>      hw/core/cpu-common.c::cpu_calss_by_name() can be removed.
>>>
>>>      [gshan@gshan q]$ git grep \ cpu_class_by_name\(
>>>      cpu.c:    oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]);
>>>      target/arm/arm-qmp-cmds.c:    oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
>>>      target/s390x/cpu_models_sysemu.c:    oc = cpu_class_by_name(TYPE_S390_CPU, info->name);
>>>
>>> When option (b) is taken, this series to have the checks against @oc
>>> in hw/core/cpu-common.c::cpu_calss_by_name() becomes non-sense. Instead,
>>> we need the same (and complete) checks in CPUClass::class_by_name() for
>>> individual targets. Further more, an inline helper can be provided to do
>>> the check in CPUClass::class_by_name() for individual targets.
>>>
>>>     include/hw/core/cpu.h
>>>
>>>     static inline bool cpu_class_is_valid(ObjectClass *oc, const char *parent)
>>>     {
>>>         if (!object_class_dynamic_cast(oc, parent) ||
>>>             object_class_is_abstract(oc)) {
>>>             return false;
>>>         }
>>>
>>>         return true;
>>>     }
>>>
>>
>> Since my series to make CPU type check unified depends on this series, could
>> you please share your thoughts? If you don't have bandwidth for this, I can
>> improve the code based on your thoughts, and include your patches to my series
>> so that they can be reviewed at once. Please just let me know.
> 
> You seem to prove (b) is not useful, so we have to do (a).
> 
> Unfortunately at this moment I feel hopeless with this topic.
> 
> I don't want to delay your work further. If you find a way to integrate
> both series, please go ahead. Otherwise let's drop my approach and
> continue with your previous work.
> 
> I apologize I kept you waiting that long.
> 

Ah, nope, nothing went to wrong here. Thanks for your reply, I will try
to follow (a) and integrate your patches to my series. Please help to
review my series when it's posted :)

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-10-11  6:46 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-08 11:22 [PATCH 0/4] hw/core/cpu-common: Consolidate cpu_class_by_name() Philippe Mathieu-Daudé
2023-09-08 11:22 ` [PATCH 1/4] target/alpha: Tidy up alpha_cpu_class_by_name() Philippe Mathieu-Daudé
2023-09-09 22:17   ` Richard Henderson
2023-09-10 23:29   ` Gavin Shan
2023-09-08 11:22 ` [PATCH 2/4] hw/cpu: Call object_class_is_abstract() once in cpu_class_by_name() Philippe Mathieu-Daudé
2023-09-09 22:18   ` Richard Henderson
2023-09-08 11:22 ` [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field Philippe Mathieu-Daudé
2023-09-09 22:21   ` Richard Henderson
2023-09-10 23:28   ` Gavin Shan
2023-09-11  9:43     ` Philippe Mathieu-Daudé
2023-09-11 10:55       ` Igor Mammedov
2023-09-11 22:40       ` Gavin Shan
2023-09-25  0:24         ` Gavin Shan
2023-10-11  3:28           ` Philippe Mathieu-Daudé
2023-10-11  6:45             ` Gavin Shan
2023-09-08 11:22 ` [PATCH 4/4] hw/cpu: Call object_class_dynamic_cast() once in cpu_class_by_name() Philippe Mathieu-Daudé
2023-09-09 23:26   ` Richard Henderson
2023-09-10 23:40   ` Gavin Shan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.