Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
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é
---
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
Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
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é
---
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.
Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
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é
---
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
Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
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é
---
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
Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
On Mon, 11 Sep 2023 11:43:00 +0200
Philippe Mathieu-Daudé 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é
> >> ---
> >> 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;
>
>
Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
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é
---
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;
Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
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é
---
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,
>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,
>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,
>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,
>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
Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
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~
[PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field
Add a field to return the QOM type name of a CPU class.
Signed-off-by: Philippe Mathieu-Daudé
---
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,
>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,
>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,
>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,
>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,
>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,
