The CPU core abstraction belongs to the machine code. This also gets
rid of some code duplication.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c
but this is already handled by the following cleanup patch:
https://patchwork.ozlabs.org/patch/817598/
---
hw/ppc/spapr.c | 4 ++++
hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------
include/hw/ppc/spapr_cpu_core.h | 2 +-
target/ppc/kvm.c | 12 ------------
4 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0ce3ec87ac59..e82c8532ffb0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine)
}
/* init CPUs */
+ if (kvm_enabled()) {
+ spapr_cpu_core_register_host_type();
+ }
+
if (machine->cpu_model == NULL) {
machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
}
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c08ee7571a50..6e224ba029ec 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -317,7 +317,7 @@ static Property spapr_cpu_core_properties[] = {
DEFINE_PROP_END_OF_LIST()
};
-void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
+static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc);
@@ -337,6 +337,20 @@ static const TypeInfo spapr_cpu_core_type_info = {
.class_size = sizeof(sPAPRCPUCoreClass),
};
+static void spapr_cpu_core_register_type(const char *model_name)
+{
+ TypeInfo type_info = {
+ .parent = TYPE_SPAPR_CPU_CORE,
+ .instance_size = sizeof(sPAPRCPUCore),
+ .class_init = spapr_cpu_core_class_init,
+ .class_data = (void *) model_name,
+ };
+
+ type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, model_name);
+ type_register(&type_info);
+ g_free((void *)type_info.name);
+}
+
static void spapr_cpu_core_register_types(void)
{
int i;
@@ -344,18 +358,14 @@ static void spapr_cpu_core_register_types(void)
type_register_static(&spapr_cpu_core_type_info);
for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) {
- TypeInfo type_info = {
- .parent = TYPE_SPAPR_CPU_CORE,
- .instance_size = sizeof(sPAPRCPUCore),
- .class_init = spapr_cpu_core_class_init,
- .class_data = (void *) spapr_core_models[i],
- };
-
- type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE,
- spapr_core_models[i]);
- type_register(&type_info);
- g_free((void *)type_info.name);
+ spapr_cpu_core_register_type(spapr_core_models[i]);
}
+
+}
+
+void spapr_cpu_core_register_host_type(void)
+{
+ spapr_cpu_core_register_type("host");
}
type_init(spapr_cpu_core_register_types)
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 93051e9ecf56..e3e906343048 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -36,5 +36,5 @@ typedef struct sPAPRCPUCoreClass {
} sPAPRCPUCoreClass;
char *spapr_get_cpu_core_type(const char *model);
-void spapr_cpu_core_class_init(ObjectClass *oc, void *data);
+void spapr_cpu_core_register_host_type(void);
#endif
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 5b281b2f1b6d..8dd80993ec9e 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -37,7 +37,6 @@
#include "hw/sysbus.h"
#include "hw/ppc/spapr.h"
#include "hw/ppc/spapr_vio.h"
-#include "hw/ppc/spapr_cpu_core.h"
#include "hw/ppc/ppc.h"
#include "sysemu/watchdog.h"
#include "trace.h"
@@ -2502,17 +2501,6 @@ static int kvm_ppc_register_host_cpu_type(void)
oc = object_class_by_name(type_info.name);
g_assert(oc);
-#if defined(TARGET_PPC64)
- type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host");
- type_info.parent = TYPE_SPAPR_CPU_CORE,
- type_info.instance_size = sizeof(sPAPRCPUCore);
- type_info.instance_init = NULL;
- type_info.class_init = spapr_cpu_core_class_init;
- type_info.class_data = (void *) "host";
- type_register(&type_info);
- g_free((void *)type_info.name);
-#endif
-
/*
* Update generic CPU family class alias (e.g. on a POWER8NVL host,
* we want "POWER8" to be a "family" alias that points to the current
On Mon, 25 Sep 2017 11:47:33 +0200 Greg Kurz <groug@kaod.org> wrote: > The CPU core abstraction belongs to the machine code. This also gets > rid of some code duplication. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > but this is already handled by the following cleanup patch: > > https://patchwork.ozlabs.org/patch/817598/ > --- > hw/ppc/spapr.c | 4 ++++ > hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------ > include/hw/ppc/spapr_cpu_core.h | 2 +- > target/ppc/kvm.c | 12 ------------ > 4 files changed, 27 insertions(+), 25 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 0ce3ec87ac59..e82c8532ffb0 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine) > } > > /* init CPUs */ > + if (kvm_enabled()) { > + spapr_cpu_core_register_host_type(); > + } why don't we create it statically in hw/ppc/spapr_cpu_core.c like it's done in x86, i.e. static void x86_cpu_register_types(void) { ... #ifdef CONFIG_KVM type_register_static(&host_x86_cpu_type_info); #endif } type_init(x86_cpu_register_types) and do the same for host CPU as well? my gripe with 'dynamic' types is that it creates problems if one needs to access type information before type is created. In my use-case it's before machine_init() is called but after kvm is initialized, so I was sort of 'fine' with way it's currently handled /i.e. still too much convoluted with aliases redefinition and all but sort of 'manageable' /. However I'd very much prefer static type info so it could be used regardless of place/time it's accessed. I can add a couple patches to that effect into upcomming cpu_model removal series, if that's ok for PPC folks. > + > if (machine->cpu_model == NULL) { > machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu; > } > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index c08ee7571a50..6e224ba029ec 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -317,7 +317,7 @@ static Property spapr_cpu_core_properties[] = { > DEFINE_PROP_END_OF_LIST() > }; > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc); > @@ -337,6 +337,20 @@ static const TypeInfo spapr_cpu_core_type_info = { > .class_size = sizeof(sPAPRCPUCoreClass), > }; > > +static void spapr_cpu_core_register_type(const char *model_name) > +{ > + TypeInfo type_info = { > + .parent = TYPE_SPAPR_CPU_CORE, > + .instance_size = sizeof(sPAPRCPUCore), > + .class_init = spapr_cpu_core_class_init, > + .class_data = (void *) model_name, > + }; > + > + type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, model_name); > + type_register(&type_info); > + g_free((void *)type_info.name); > +} > + > static void spapr_cpu_core_register_types(void) > { > int i; > @@ -344,18 +358,14 @@ static void spapr_cpu_core_register_types(void) > type_register_static(&spapr_cpu_core_type_info); > > for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) { > - TypeInfo type_info = { > - .parent = TYPE_SPAPR_CPU_CORE, > - .instance_size = sizeof(sPAPRCPUCore), > - .class_init = spapr_cpu_core_class_init, > - .class_data = (void *) spapr_core_models[i], > - }; > - > - type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, > - spapr_core_models[i]); > - type_register(&type_info); > - g_free((void *)type_info.name); > + spapr_cpu_core_register_type(spapr_core_models[i]); > } > + > +} > + > +void spapr_cpu_core_register_host_type(void) > +{ > + spapr_cpu_core_register_type("host"); > } > > type_init(spapr_cpu_core_register_types) > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > index 93051e9ecf56..e3e906343048 100644 > --- a/include/hw/ppc/spapr_cpu_core.h > +++ b/include/hw/ppc/spapr_cpu_core.h > @@ -36,5 +36,5 @@ typedef struct sPAPRCPUCoreClass { > } sPAPRCPUCoreClass; > > char *spapr_get_cpu_core_type(const char *model); > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data); > +void spapr_cpu_core_register_host_type(void); > #endif > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index 5b281b2f1b6d..8dd80993ec9e 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -37,7 +37,6 @@ > #include "hw/sysbus.h" > #include "hw/ppc/spapr.h" > #include "hw/ppc/spapr_vio.h" > -#include "hw/ppc/spapr_cpu_core.h" > #include "hw/ppc/ppc.h" > #include "sysemu/watchdog.h" > #include "trace.h" > @@ -2502,17 +2501,6 @@ static int kvm_ppc_register_host_cpu_type(void) > oc = object_class_by_name(type_info.name); > g_assert(oc); > > -#if defined(TARGET_PPC64) > - type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host"); > - type_info.parent = TYPE_SPAPR_CPU_CORE, > - type_info.instance_size = sizeof(sPAPRCPUCore); > - type_info.instance_init = NULL; > - type_info.class_init = spapr_cpu_core_class_init; > - type_info.class_data = (void *) "host"; > - type_register(&type_info); > - g_free((void *)type_info.name); > -#endif > - > /* > * Update generic CPU family class alias (e.g. on a POWER8NVL host, > * we want "POWER8" to be a "family" alias that points to the current > >
On Mon, 25 Sep 2017 15:41:34 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Mon, 25 Sep 2017 11:47:33 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > The CPU core abstraction belongs to the machine code. This also gets > > rid of some code duplication. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > but this is already handled by the following cleanup patch: > > > > https://patchwork.ozlabs.org/patch/817598/ > > --- > > hw/ppc/spapr.c | 4 ++++ > > hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------ > > include/hw/ppc/spapr_cpu_core.h | 2 +- > > target/ppc/kvm.c | 12 ------------ > > 4 files changed, 27 insertions(+), 25 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 0ce3ec87ac59..e82c8532ffb0 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine) > > } > > > > /* init CPUs */ > > + if (kvm_enabled()) { > > + spapr_cpu_core_register_host_type(); > > + } > why don't we create it statically in hw/ppc/spapr_cpu_core.c > like it's done in x86, i.e. > > static void x86_cpu_register_types(void) > { > ... > #ifdef CONFIG_KVM > type_register_static(&host_x86_cpu_type_info); > #endif > } > type_init(x86_cpu_register_types) > > and do the same for host CPU as well? > Hi Igor, Not sure yet why we use dynamic types, but I'd be glad to dig a bit more. Maybe I'll do this in a followup patch though, since the present patch is mostly about not referencing sPAPRCPUCore from the KVM code anymore. > my gripe with 'dynamic' types is that it creates problems > if one needs to access type information before type is created. > In my use-case it's before machine_init() is called but > after kvm is initialized, so I was sort of 'fine' with way > it's currently handled /i.e. still too much convoluted with > aliases redefinition and all but sort of 'manageable' /. > > However I'd very much prefer static type info so it > could be used regardless of place/time it's accessed. > I agree that it would be better. > I can add a couple patches to that effect into upcomming > cpu_model removal series, if that's ok for PPC folks. > Oh, and can you share an url to see how it looks like ? Cheers, -- Greg > > > + > > if (machine->cpu_model == NULL) { > > machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu; > > } > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index c08ee7571a50..6e224ba029ec 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -317,7 +317,7 @@ static Property spapr_cpu_core_properties[] = { > > DEFINE_PROP_END_OF_LIST() > > }; > > > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(oc); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc); > > @@ -337,6 +337,20 @@ static const TypeInfo spapr_cpu_core_type_info = { > > .class_size = sizeof(sPAPRCPUCoreClass), > > }; > > > > +static void spapr_cpu_core_register_type(const char *model_name) > > +{ > > + TypeInfo type_info = { > > + .parent = TYPE_SPAPR_CPU_CORE, > > + .instance_size = sizeof(sPAPRCPUCore), > > + .class_init = spapr_cpu_core_class_init, > > + .class_data = (void *) model_name, > > + }; > > + > > + type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, model_name); > > + type_register(&type_info); > > + g_free((void *)type_info.name); > > +} > > + > > static void spapr_cpu_core_register_types(void) > > { > > int i; > > @@ -344,18 +358,14 @@ static void spapr_cpu_core_register_types(void) > > type_register_static(&spapr_cpu_core_type_info); > > > > for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) { > > - TypeInfo type_info = { > > - .parent = TYPE_SPAPR_CPU_CORE, > > - .instance_size = sizeof(sPAPRCPUCore), > > - .class_init = spapr_cpu_core_class_init, > > - .class_data = (void *) spapr_core_models[i], > > - }; > > - > > - type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, > > - spapr_core_models[i]); > > - type_register(&type_info); > > - g_free((void *)type_info.name); > > + spapr_cpu_core_register_type(spapr_core_models[i]); > > } > > + > > +} > > + > > +void spapr_cpu_core_register_host_type(void) > > +{ > > + spapr_cpu_core_register_type("host"); > > } > > > > type_init(spapr_cpu_core_register_types) > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > > index 93051e9ecf56..e3e906343048 100644 > > --- a/include/hw/ppc/spapr_cpu_core.h > > +++ b/include/hw/ppc/spapr_cpu_core.h > > @@ -36,5 +36,5 @@ typedef struct sPAPRCPUCoreClass { > > } sPAPRCPUCoreClass; > > > > char *spapr_get_cpu_core_type(const char *model); > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data); > > +void spapr_cpu_core_register_host_type(void); > > #endif > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > index 5b281b2f1b6d..8dd80993ec9e 100644 > > --- a/target/ppc/kvm.c > > +++ b/target/ppc/kvm.c > > @@ -37,7 +37,6 @@ > > #include "hw/sysbus.h" > > #include "hw/ppc/spapr.h" > > #include "hw/ppc/spapr_vio.h" > > -#include "hw/ppc/spapr_cpu_core.h" > > #include "hw/ppc/ppc.h" > > #include "sysemu/watchdog.h" > > #include "trace.h" > > @@ -2502,17 +2501,6 @@ static int kvm_ppc_register_host_cpu_type(void) > > oc = object_class_by_name(type_info.name); > > g_assert(oc); > > > > -#if defined(TARGET_PPC64) > > - type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host"); > > - type_info.parent = TYPE_SPAPR_CPU_CORE, > > - type_info.instance_size = sizeof(sPAPRCPUCore); > > - type_info.instance_init = NULL; > > - type_info.class_init = spapr_cpu_core_class_init; > > - type_info.class_data = (void *) "host"; > > - type_register(&type_info); > > - g_free((void *)type_info.name); > > -#endif > > - > > /* > > * Update generic CPU family class alias (e.g. on a POWER8NVL host, > > * we want "POWER8" to be a "family" alias that points to the current > > > > > -- Gregory Kurz kurzgreg@fr.ibm.com gkurz@linux.vnet.ibm.com Software Engineer @ IBM/LTC http://www.ibm.com Tel 33-5-6218-1607 "Anarchy is about taking complete responsibility for yourself." Alan Moore.
On Mon, 25 Sep 2017 17:48:57 +0200 Greg Kurz <groug@kaod.org> wrote: > On Mon, 25 Sep 2017 15:41:34 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > > On Mon, 25 Sep 2017 11:47:33 +0200 > > Greg Kurz <groug@kaod.org> wrote: > > > > > The CPU core abstraction belongs to the machine code. This also gets > > > rid of some code duplication. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > but this is already handled by the following cleanup patch: > > > > > > https://patchwork.ozlabs.org/patch/817598/ > > > --- > > > hw/ppc/spapr.c | 4 ++++ > > > hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------ > > > include/hw/ppc/spapr_cpu_core.h | 2 +- > > > target/ppc/kvm.c | 12 ------------ > > > 4 files changed, 27 insertions(+), 25 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 0ce3ec87ac59..e82c8532ffb0 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine) > > > } > > > > > > /* init CPUs */ > > > + if (kvm_enabled()) { > > > + spapr_cpu_core_register_host_type(); > > > + } > > why don't we create it statically in hw/ppc/spapr_cpu_core.c > > like it's done in x86, i.e. > > > > static void x86_cpu_register_types(void) > > { > > ... > > #ifdef CONFIG_KVM > > type_register_static(&host_x86_cpu_type_info); > > #endif > > } > > type_init(x86_cpu_register_types) > > > > and do the same for host CPU as well? > > > > Hi Igor, > > Not sure yet why we use dynamic types, but I'd be glad to dig a bit more. So the problem is that it was decided to make the host CPU class a subclass of the host's CPU model, and this requires all the CPU model classes to be registered beforehand. commit 5ba4576b858c0d6056f59abb7e17a2b63f7905f3 Author: Andreas Färber <afaerber@suse.de> Date: Sat Feb 23 11:22:12 2013 +0000 target-ppc: Make host CPU a subclass of the host's CPU model This avoids assigning individual class fields and contributors forgetting to add field assignments in KVM-only code. ppc_cpu_class_find_by_pvr() requires the CPU model classes to be registered, so defer host CPU type registration to kvm_arch_init(). Only register the host CPU type if there is a class with matching PVR. This lets us drop error handling from instance_init. Signed-off-by: Andreas Färber <afaerber@suse.de> Signed-off-by: Alexander Graf <agraf@suse.de> I can't think of an alternate way to do this. Any suggestion ? > Maybe I'll do this in a followup patch though, since the present patch > is mostly about not referencing sPAPRCPUCore from the KVM code anymore. > > > my gripe with 'dynamic' types is that it creates problems > > if one needs to access type information before type is created. > > In my use-case it's before machine_init() is called but > > after kvm is initialized, so I was sort of 'fine' with way > > it's currently handled /i.e. still too much convoluted with > > aliases redefinition and all but sort of 'manageable' /. > > > > However I'd very much prefer static type info so it > > could be used regardless of place/time it's accessed. > > > > I agree that it would be better. > > > I can add a couple patches to that effect into upcomming > > cpu_model removal series, if that's ok for PPC folks. > > > > Oh, and can you share an url to see how it looks like ? > > Cheers, > > -- > Greg > > > > > > + > > > if (machine->cpu_model == NULL) { > > > machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu; > > > } > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > index c08ee7571a50..6e224ba029ec 100644 > > > --- a/hw/ppc/spapr_cpu_core.c > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -317,7 +317,7 @@ static Property spapr_cpu_core_properties[] = { > > > DEFINE_PROP_END_OF_LIST() > > > }; > > > > > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > > { > > > DeviceClass *dc = DEVICE_CLASS(oc); > > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc); > > > @@ -337,6 +337,20 @@ static const TypeInfo spapr_cpu_core_type_info = { > > > .class_size = sizeof(sPAPRCPUCoreClass), > > > }; > > > > > > +static void spapr_cpu_core_register_type(const char *model_name) > > > +{ > > > + TypeInfo type_info = { > > > + .parent = TYPE_SPAPR_CPU_CORE, > > > + .instance_size = sizeof(sPAPRCPUCore), > > > + .class_init = spapr_cpu_core_class_init, > > > + .class_data = (void *) model_name, > > > + }; > > > + > > > + type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, model_name); > > > + type_register(&type_info); > > > + g_free((void *)type_info.name); > > > +} > > > + > > > static void spapr_cpu_core_register_types(void) > > > { > > > int i; > > > @@ -344,18 +358,14 @@ static void spapr_cpu_core_register_types(void) > > > type_register_static(&spapr_cpu_core_type_info); > > > > > > for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) { > > > - TypeInfo type_info = { > > > - .parent = TYPE_SPAPR_CPU_CORE, > > > - .instance_size = sizeof(sPAPRCPUCore), > > > - .class_init = spapr_cpu_core_class_init, > > > - .class_data = (void *) spapr_core_models[i], > > > - }; > > > - > > > - type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, > > > - spapr_core_models[i]); > > > - type_register(&type_info); > > > - g_free((void *)type_info.name); > > > + spapr_cpu_core_register_type(spapr_core_models[i]); > > > } > > > + > > > +} > > > + > > > +void spapr_cpu_core_register_host_type(void) > > > +{ > > > + spapr_cpu_core_register_type("host"); > > > } > > > > > > type_init(spapr_cpu_core_register_types) > > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > > > index 93051e9ecf56..e3e906343048 100644 > > > --- a/include/hw/ppc/spapr_cpu_core.h > > > +++ b/include/hw/ppc/spapr_cpu_core.h > > > @@ -36,5 +36,5 @@ typedef struct sPAPRCPUCoreClass { > > > } sPAPRCPUCoreClass; > > > > > > char *spapr_get_cpu_core_type(const char *model); > > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data); > > > +void spapr_cpu_core_register_host_type(void); > > > #endif > > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > > index 5b281b2f1b6d..8dd80993ec9e 100644 > > > --- a/target/ppc/kvm.c > > > +++ b/target/ppc/kvm.c > > > @@ -37,7 +37,6 @@ > > > #include "hw/sysbus.h" > > > #include "hw/ppc/spapr.h" > > > #include "hw/ppc/spapr_vio.h" > > > -#include "hw/ppc/spapr_cpu_core.h" > > > #include "hw/ppc/ppc.h" > > > #include "sysemu/watchdog.h" > > > #include "trace.h" > > > @@ -2502,17 +2501,6 @@ static int kvm_ppc_register_host_cpu_type(void) > > > oc = object_class_by_name(type_info.name); > > > g_assert(oc); > > > > > > -#if defined(TARGET_PPC64) > > > - type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host"); > > > - type_info.parent = TYPE_SPAPR_CPU_CORE, > > > - type_info.instance_size = sizeof(sPAPRCPUCore); > > > - type_info.instance_init = NULL; > > > - type_info.class_init = spapr_cpu_core_class_init; > > > - type_info.class_data = (void *) "host"; > > > - type_register(&type_info); > > > - g_free((void *)type_info.name); > > > -#endif > > > - > > > /* > > > * Update generic CPU family class alias (e.g. on a POWER8NVL host, > > > * we want "POWER8" to be a "family" alias that points to the current > > > > > > > > > > > -- Gregory Kurz kurzgreg@fr.ibm.com gkurz@linux.vnet.ibm.com Software Engineer @ IBM/LTC http://www.ibm.com Tel 33-5-6218-1607 "Anarchy is about taking complete responsibility for yourself." Alan Moore.
On Mon, 25 Sep 2017 23:47:30 +0200 Greg Kurz <groug@kaod.org> wrote: > On Mon, 25 Sep 2017 17:48:57 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > On Mon, 25 Sep 2017 15:41:34 +0200 > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > On Mon, 25 Sep 2017 11:47:33 +0200 > > > Greg Kurz <groug@kaod.org> wrote: > > > > > > > The CPU core abstraction belongs to the machine code. This also gets > > > > rid of some code duplication. > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > > but this is already handled by the following cleanup patch: > > > > > > > > https://patchwork.ozlabs.org/patch/817598/ > > > > --- > > > > hw/ppc/spapr.c | 4 ++++ > > > > hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------ > > > > include/hw/ppc/spapr_cpu_core.h | 2 +- > > > > target/ppc/kvm.c | 12 ------------ > > > > 4 files changed, 27 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > index 0ce3ec87ac59..e82c8532ffb0 100644 > > > > --- a/hw/ppc/spapr.c > > > > +++ b/hw/ppc/spapr.c > > > > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine) > > > > } > > > > > > > > /* init CPUs */ > > > > + if (kvm_enabled()) { > > > > + spapr_cpu_core_register_host_type(); > > > > + } > > > why don't we create it statically in hw/ppc/spapr_cpu_core.c > > > like it's done in x86, i.e. > > > > > > static void x86_cpu_register_types(void) > > > { > > > ... > > > #ifdef CONFIG_KVM > > > type_register_static(&host_x86_cpu_type_info); > > > #endif > > > } > > > type_init(x86_cpu_register_types) > > > > > > and do the same for host CPU as well? > > > > > > > Hi Igor, > > > > Not sure yet why we use dynamic types, but I'd be glad to dig a bit more. > > So the problem is that it was decided to make the host CPU class a > subclass of the host's CPU model, and this requires all the CPU model > classes to be registered beforehand. > > commit 5ba4576b858c0d6056f59abb7e17a2b63f7905f3 > Author: Andreas Färber <afaerber@suse.de> > Date: Sat Feb 23 11:22:12 2013 +0000 > > target-ppc: Make host CPU a subclass of the host's CPU model > > This avoids assigning individual class fields and contributors > forgetting to add field assignments in KVM-only code. > > ppc_cpu_class_find_by_pvr() requires the CPU model classes to be > registered, so defer host CPU type registration to kvm_arch_init(). > > Only register the host CPU type if there is a class with matching PVR. > This lets us drop error handling from instance_init. > > Signed-off-by: Andreas Färber <afaerber@suse.de> > Signed-off-by: Alexander Graf <agraf@suse.de> > > I can't think of an alternate way to do this. Any suggestion ? I don't see from this commit a reason why it can't be done in cpu-models.c dependencies here are mfpvr() - which probably should work without KVM ppc_cpu_class_by_pvr() - should work fine if 'host' type is being registered as the last among the other CPU types
On Wed, 27 Sep 2017 14:19:56 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Mon, 25 Sep 2017 23:47:30 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > On Mon, 25 Sep 2017 17:48:57 +0200 > > Greg Kurz <groug@kaod.org> wrote: > > > > > On Mon, 25 Sep 2017 15:41:34 +0200 > > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > On Mon, 25 Sep 2017 11:47:33 +0200 > > > > Greg Kurz <groug@kaod.org> wrote: > > > > > > > > > The CPU core abstraction belongs to the machine code. This also gets > > > > > rid of some code duplication. > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > --- > > > > > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > > > but this is already handled by the following cleanup patch: > > > > > > > > > > https://patchwork.ozlabs.org/patch/817598/ > > > > > --- > > > > > hw/ppc/spapr.c | 4 ++++ > > > > > hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------ > > > > > include/hw/ppc/spapr_cpu_core.h | 2 +- > > > > > target/ppc/kvm.c | 12 ------------ > > > > > 4 files changed, 27 insertions(+), 25 deletions(-) > > > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > > index 0ce3ec87ac59..e82c8532ffb0 100644 > > > > > --- a/hw/ppc/spapr.c > > > > > +++ b/hw/ppc/spapr.c > > > > > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine) > > > > > } > > > > > > > > > > /* init CPUs */ > > > > > + if (kvm_enabled()) { > > > > > + spapr_cpu_core_register_host_type(); > > > > > + } > > > > why don't we create it statically in hw/ppc/spapr_cpu_core.c > > > > like it's done in x86, i.e. > > > > > > > > static void x86_cpu_register_types(void) > > > > { > > > > ... > > > > #ifdef CONFIG_KVM > > > > type_register_static(&host_x86_cpu_type_info); > > > > #endif > > > > } > > > > type_init(x86_cpu_register_types) > > > > > > > > and do the same for host CPU as well? > > > > > > > > > > Hi Igor, > > > > > > Not sure yet why we use dynamic types, but I'd be glad to dig a bit more. > > > > So the problem is that it was decided to make the host CPU class a > > subclass of the host's CPU model, and this requires all the CPU model > > classes to be registered beforehand. > > > > commit 5ba4576b858c0d6056f59abb7e17a2b63f7905f3 > > Author: Andreas Färber <afaerber@suse.de> > > Date: Sat Feb 23 11:22:12 2013 +0000 > > > > target-ppc: Make host CPU a subclass of the host's CPU model > > > > This avoids assigning individual class fields and contributors > > forgetting to add field assignments in KVM-only code. > > > > ppc_cpu_class_find_by_pvr() requires the CPU model classes to be > > registered, so defer host CPU type registration to kvm_arch_init(). > > > > Only register the host CPU type if there is a class with matching PVR. > > This lets us drop error handling from instance_init. > > > > Signed-off-by: Andreas Färber <afaerber@suse.de> > > Signed-off-by: Alexander Graf <agraf@suse.de> > > > > I can't think of an alternate way to do this. Any suggestion ? > I don't see from this commit a reason why it can't be done in cpu-models.c > dependencies here are > mfpvr() - which probably should work without KVM Correct. > ppc_cpu_class_by_pvr() - should work fine if 'host' type is being > registered as the last among the other CPU types We have: ppc_cpu_class_by_pvr() object_class_get_list() object_class_foreach() object_class_foreach_tramp() type_initialize() type_get_parent() type_initialize() recursively initializes all parent types, and type_get_parent() aborts if the parent type isn't registered yet, which may happen as long as all type_init() functions haven't been called => ppc_cpu_class_by_pvr() cannot be safely called from a type_init() function. -- Greg
On Wed, 27 Sep 2017 17:32:32 +0200 Greg Kurz <groug@kaod.org> wrote: > On Wed, 27 Sep 2017 14:19:56 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > > On Mon, 25 Sep 2017 23:47:30 +0200 > > Greg Kurz <groug@kaod.org> wrote: > > > > > On Mon, 25 Sep 2017 17:48:57 +0200 > > > Greg Kurz <groug@kaod.org> wrote: > > > > > > > On Mon, 25 Sep 2017 15:41:34 +0200 > > > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > On Mon, 25 Sep 2017 11:47:33 +0200 > > > > > Greg Kurz <groug@kaod.org> wrote: > > > > > > > > > > > The CPU core abstraction belongs to the machine code. This also gets > > > > > > rid of some code duplication. > > > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > > --- > > > > > > > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > > > > but this is already handled by the following cleanup patch: > > > > > > > > > > > > https://patchwork.ozlabs.org/patch/817598/ > > > > > > --- > > > > > > hw/ppc/spapr.c | 4 ++++ > > > > > > hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------ > > > > > > include/hw/ppc/spapr_cpu_core.h | 2 +- > > > > > > target/ppc/kvm.c | 12 ------------ > > > > > > 4 files changed, 27 insertions(+), 25 deletions(-) > > > > > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > > > index 0ce3ec87ac59..e82c8532ffb0 100644 > > > > > > --- a/hw/ppc/spapr.c > > > > > > +++ b/hw/ppc/spapr.c > > > > > > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine) > > > > > > } > > > > > > > > > > > > /* init CPUs */ > > > > > > + if (kvm_enabled()) { > > > > > > + spapr_cpu_core_register_host_type(); > > > > > > + } > > > > > why don't we create it statically in hw/ppc/spapr_cpu_core.c > > > > > like it's done in x86, i.e. > > > > > > > > > > static void x86_cpu_register_types(void) > > > > > { > > > > > ... > > > > > #ifdef CONFIG_KVM > > > > > type_register_static(&host_x86_cpu_type_info); > > > > > #endif > > > > > } > > > > > type_init(x86_cpu_register_types) > > > > > > > > > > and do the same for host CPU as well? > > > > > > > > > > > > > Hi Igor, > > > > > > > > Not sure yet why we use dynamic types, but I'd be glad to dig a bit more. > > > > > > So the problem is that it was decided to make the host CPU class a > > > subclass of the host's CPU model, and this requires all the CPU model > > > classes to be registered beforehand. > > > > > > commit 5ba4576b858c0d6056f59abb7e17a2b63f7905f3 > > > Author: Andreas Färber <afaerber@suse.de> > > > Date: Sat Feb 23 11:22:12 2013 +0000 > > > > > > target-ppc: Make host CPU a subclass of the host's CPU model > > > > > > This avoids assigning individual class fields and contributors > > > forgetting to add field assignments in KVM-only code. > > > > > > ppc_cpu_class_find_by_pvr() requires the CPU model classes to be > > > registered, so defer host CPU type registration to kvm_arch_init(). > > > > > > Only register the host CPU type if there is a class with matching PVR. > > > This lets us drop error handling from instance_init. > > > > > > Signed-off-by: Andreas Färber <afaerber@suse.de> > > > Signed-off-by: Alexander Graf <agraf@suse.de> > > > > > > I can't think of an alternate way to do this. Any suggestion ? > > I don't see from this commit a reason why it can't be done in cpu-models.c > > dependencies here are > > mfpvr() - which probably should work without KVM > > Correct. > > > ppc_cpu_class_by_pvr() - should work fine if 'host' type is being > > registered as the last among the other CPU types > > We have: > > ppc_cpu_class_by_pvr() > object_class_get_list() > object_class_foreach() > object_class_foreach_tramp() > type_initialize() > type_get_parent() > > type_initialize() recursively initializes all parent types, and > type_get_parent() aborts if the parent type isn't registered yet, > which may happen as long as all type_init() functions haven't been > called => ppc_cpu_class_by_pvr() cannot be safely called from a > type_init() function. I don't get what you are trying to say, could you be more specific about what parent type might be not registered?
On Fri, 29 Sep 2017 08:41:11 +0200 Igor Mammedov <imammedo@redhat.com> wrote: [...] > > > I don't see from this commit a reason why it can't be done in cpu-models.c > > > dependencies here are > > > mfpvr() - which probably should work without KVM > > > > Correct. > > > > > ppc_cpu_class_by_pvr() - should work fine if 'host' type is being > > > registered as the last among the other CPU types > > > > We have: > > > > ppc_cpu_class_by_pvr() > > object_class_get_list() > > object_class_foreach() > > object_class_foreach_tramp() > > type_initialize() > > type_get_parent() > > > > type_initialize() recursively initializes all parent types, and > > type_get_parent() aborts if the parent type isn't registered yet, > > which may happen as long as all type_init() functions haven't been > > called => ppc_cpu_class_by_pvr() cannot be safely called from a > > type_init() function. > I don't get what you are trying to say, > could you be more specific about what parent type might be not > registered? With the patch below applied, TYPE_CPU isn't registered at the time we call ppc_cpu_class_by_pvr(). diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c index cb5777afa0b4..4445a292d578 100644 --- a/target/ppc/kvm.c +++ b/target/ppc/kvm.c @@ -123,8 +123,6 @@ static bool kvmppc_is_pr(KVMState *ks) return kvm_vm_check_extension(ks, KVM_CAP_PPC_GET_PVINFO) != 0; } -static int kvm_ppc_register_host_cpu_type(void); - int kvm_arch_init(MachineState *ms, KVMState *s) { cap_interrupt_unset = kvm_check_extension(s, KVM_CAP_PPC_UNSET_IRQ); @@ -163,8 +161,6 @@ int kvm_arch_init(MachineState *ms, KVMState *s) "VM to stall at times!\n"); } - kvm_ppc_register_host_cpu_type(); - return 0; } @@ -2487,7 +2483,7 @@ PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void) return pvr_pcc; } -static int kvm_ppc_register_host_cpu_type(void) +int kvm_ppc_register_host_cpu_type(void) { TypeInfo type_info = { .name = TYPE_HOST_POWERPC_CPU, diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h index d6be38ecafbd..a46c5a36402a 100644 --- a/target/ppc/kvm_ppc.h +++ b/target/ppc/kvm_ppc.h @@ -71,6 +71,7 @@ bool kvmppc_pvr_workaround_required(PowerPCCPU *cpu); bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path); +int kvm_ppc_register_host_cpu_type(void); #else static inline uint32_t kvmppc_get_tbfreq(void) diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c index c6399a3a0d0d..b6e02c583836 100644 --- a/target/ppc/translate_init.c +++ b/target/ppc/translate_init.c @@ -10766,6 +10766,9 @@ static void ppc_cpu_register_types(void) { type_register_static(&ppc_cpu_type_info); type_register_static(&ppc_vhyp_type_info); +#ifdef CONFIG_KVM + kvm_ppc_register_host_cpu_type(); +#endif } type_init(ppc_cpu_register_types)
On Mon, Sep 25, 2017 at 11:47:33AM +0200, Greg Kurz wrote: > The CPU core abstraction belongs to the machine code. This also gets > rid of some code duplication. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > but this is already handled by the following cleanup patch: I don't really see what the advantage of this is. As others have pointed out it leads to the host type being registered very late, which could cause problems. > > https://patchwork.ozlabs.org/patch/817598/ > --- > hw/ppc/spapr.c | 4 ++++ > hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------ > include/hw/ppc/spapr_cpu_core.h | 2 +- > target/ppc/kvm.c | 12 ------------ > 4 files changed, 27 insertions(+), 25 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 0ce3ec87ac59..e82c8532ffb0 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine) > } > > /* init CPUs */ > + if (kvm_enabled()) { > + spapr_cpu_core_register_host_type(); > + } > + > if (machine->cpu_model == NULL) { > machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu; > } > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index c08ee7571a50..6e224ba029ec 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -317,7 +317,7 @@ static Property spapr_cpu_core_properties[] = { > DEFINE_PROP_END_OF_LIST() > }; > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc); > @@ -337,6 +337,20 @@ static const TypeInfo spapr_cpu_core_type_info = { > .class_size = sizeof(sPAPRCPUCoreClass), > }; > > +static void spapr_cpu_core_register_type(const char *model_name) > +{ > + TypeInfo type_info = { > + .parent = TYPE_SPAPR_CPU_CORE, > + .instance_size = sizeof(sPAPRCPUCore), > + .class_init = spapr_cpu_core_class_init, > + .class_data = (void *) model_name, > + }; > + > + type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, model_name); > + type_register(&type_info); > + g_free((void *)type_info.name); > +} > + > static void spapr_cpu_core_register_types(void) > { > int i; > @@ -344,18 +358,14 @@ static void spapr_cpu_core_register_types(void) > type_register_static(&spapr_cpu_core_type_info); > > for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) { > - TypeInfo type_info = { > - .parent = TYPE_SPAPR_CPU_CORE, > - .instance_size = sizeof(sPAPRCPUCore), > - .class_init = spapr_cpu_core_class_init, > - .class_data = (void *) spapr_core_models[i], > - }; > - > - type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, > - spapr_core_models[i]); > - type_register(&type_info); > - g_free((void *)type_info.name); > + spapr_cpu_core_register_type(spapr_core_models[i]); > } > + > +} > + > +void spapr_cpu_core_register_host_type(void) > +{ > + spapr_cpu_core_register_type("host"); > } > > type_init(spapr_cpu_core_register_types) > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > index 93051e9ecf56..e3e906343048 100644 > --- a/include/hw/ppc/spapr_cpu_core.h > +++ b/include/hw/ppc/spapr_cpu_core.h > @@ -36,5 +36,5 @@ typedef struct sPAPRCPUCoreClass { > } sPAPRCPUCoreClass; > > char *spapr_get_cpu_core_type(const char *model); > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data); > +void spapr_cpu_core_register_host_type(void); > #endif > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index 5b281b2f1b6d..8dd80993ec9e 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -37,7 +37,6 @@ > #include "hw/sysbus.h" > #include "hw/ppc/spapr.h" > #include "hw/ppc/spapr_vio.h" > -#include "hw/ppc/spapr_cpu_core.h" > #include "hw/ppc/ppc.h" > #include "sysemu/watchdog.h" > #include "trace.h" > @@ -2502,17 +2501,6 @@ static int kvm_ppc_register_host_cpu_type(void) > oc = object_class_by_name(type_info.name); > g_assert(oc); > > -#if defined(TARGET_PPC64) > - type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host"); > - type_info.parent = TYPE_SPAPR_CPU_CORE, > - type_info.instance_size = sizeof(sPAPRCPUCore); > - type_info.instance_init = NULL; > - type_info.class_init = spapr_cpu_core_class_init; > - type_info.class_data = (void *) "host"; > - type_register(&type_info); > - g_free((void *)type_info.name); > -#endif > - > /* > * Update generic CPU family class alias (e.g. on a POWER8NVL host, > * we want "POWER8" to be a "family" alias that points to the current > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Tue, 26 Sep 2017 12:57:39 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Sep 25, 2017 at 11:47:33AM +0200, Greg Kurz wrote: > > The CPU core abstraction belongs to the machine code. This also gets > > rid of some code duplication. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > but this is already handled by the following cleanup patch: > > I don't really see what the advantage of this is. As others have > pointed out it leads to the host type being registered very late, > which could cause problems. > Well, the goal was to consolidate the code to register sPAPRCPUCore types in the spapr code, instead of open-coding it in spapr_cpu_core.c and kvm.c... But now I realize that delaying the registration even more is a bad idea. And, the other way round, registering a static type earlier as asked by Igor would require all parent types to be already registered, which seems to be impossible to guarantee with the current code. Maybe we could at least have kvm_ppc_register_host_cpu_type() to call a function in spapr_cpu_core.c instead of duplicating the registration code ? > > > > https://patchwork.ozlabs.org/patch/817598/ > > --- > > hw/ppc/spapr.c | 4 ++++ > > hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------ > > include/hw/ppc/spapr_cpu_core.h | 2 +- > > target/ppc/kvm.c | 12 ------------ > > 4 files changed, 27 insertions(+), 25 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 0ce3ec87ac59..e82c8532ffb0 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine) > > } > > > > /* init CPUs */ > > + if (kvm_enabled()) { > > + spapr_cpu_core_register_host_type(); > > + } > > + > > if (machine->cpu_model == NULL) { > > machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu; > > } > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index c08ee7571a50..6e224ba029ec 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -317,7 +317,7 @@ static Property spapr_cpu_core_properties[] = { > > DEFINE_PROP_END_OF_LIST() > > }; > > > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(oc); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc); > > @@ -337,6 +337,20 @@ static const TypeInfo spapr_cpu_core_type_info = { > > .class_size = sizeof(sPAPRCPUCoreClass), > > }; > > > > +static void spapr_cpu_core_register_type(const char *model_name) > > +{ > > + TypeInfo type_info = { > > + .parent = TYPE_SPAPR_CPU_CORE, > > + .instance_size = sizeof(sPAPRCPUCore), > > + .class_init = spapr_cpu_core_class_init, > > + .class_data = (void *) model_name, > > + }; > > + > > + type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, model_name); > > + type_register(&type_info); > > + g_free((void *)type_info.name); > > +} > > + > > static void spapr_cpu_core_register_types(void) > > { > > int i; > > @@ -344,18 +358,14 @@ static void spapr_cpu_core_register_types(void) > > type_register_static(&spapr_cpu_core_type_info); > > > > for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) { > > - TypeInfo type_info = { > > - .parent = TYPE_SPAPR_CPU_CORE, > > - .instance_size = sizeof(sPAPRCPUCore), > > - .class_init = spapr_cpu_core_class_init, > > - .class_data = (void *) spapr_core_models[i], > > - }; > > - > > - type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, > > - spapr_core_models[i]); > > - type_register(&type_info); > > - g_free((void *)type_info.name); > > + spapr_cpu_core_register_type(spapr_core_models[i]); > > } > > + > > +} > > + > > +void spapr_cpu_core_register_host_type(void) > > +{ > > + spapr_cpu_core_register_type("host"); > > } > > > > type_init(spapr_cpu_core_register_types) > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > > index 93051e9ecf56..e3e906343048 100644 > > --- a/include/hw/ppc/spapr_cpu_core.h > > +++ b/include/hw/ppc/spapr_cpu_core.h > > @@ -36,5 +36,5 @@ typedef struct sPAPRCPUCoreClass { > > } sPAPRCPUCoreClass; > > > > char *spapr_get_cpu_core_type(const char *model); > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data); > > +void spapr_cpu_core_register_host_type(void); > > #endif > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > index 5b281b2f1b6d..8dd80993ec9e 100644 > > --- a/target/ppc/kvm.c > > +++ b/target/ppc/kvm.c > > @@ -37,7 +37,6 @@ > > #include "hw/sysbus.h" > > #include "hw/ppc/spapr.h" > > #include "hw/ppc/spapr_vio.h" > > -#include "hw/ppc/spapr_cpu_core.h" > > #include "hw/ppc/ppc.h" > > #include "sysemu/watchdog.h" > > #include "trace.h" > > @@ -2502,17 +2501,6 @@ static int kvm_ppc_register_host_cpu_type(void) > > oc = object_class_by_name(type_info.name); > > g_assert(oc); > > > > -#if defined(TARGET_PPC64) > > - type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host"); > > - type_info.parent = TYPE_SPAPR_CPU_CORE, > > - type_info.instance_size = sizeof(sPAPRCPUCore); > > - type_info.instance_init = NULL; > > - type_info.class_init = spapr_cpu_core_class_init; > > - type_info.class_data = (void *) "host"; > > - type_register(&type_info); > > - g_free((void *)type_info.name); > > -#endif > > - > > /* > > * Update generic CPU family class alias (e.g. on a POWER8NVL host, > > * we want "POWER8" to be a "family" alias that points to the current > > > -- Gregory Kurz kurzgreg@fr.ibm.com gkurz@linux.vnet.ibm.com Software Engineer @ IBM/LTC http://www.ibm.com Tel 33-5-6218-1607 "Anarchy is about taking complete responsibility for yourself." Alan Moore.
On Tue, 26 Sep 2017 09:19:28 +0200 Greg Kurz <groug@kaod.org> wrote: > On Tue, 26 Sep 2017 12:57:39 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, Sep 25, 2017 at 11:47:33AM +0200, Greg Kurz wrote: > > > The CPU core abstraction belongs to the machine code. This also gets > > > rid of some code duplication. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > but this is already handled by the following cleanup patch: > > > > I don't really see what the advantage of this is. As others have > > pointed out it leads to the host type being registered very late, > > which could cause problems. > > > > Well, the goal was to consolidate the code to register sPAPRCPUCore types in > the spapr code, instead of open-coding it in spapr_cpu_core.c and kvm.c... > > But now I realize that delaying the registration even more is a bad idea. And, > the other way round, registering a static type earlier as asked by Igor would > require all parent types to be already registered, which seems to be impossible > to guarantee with the current code. so I'll leave this code as is for now as as it's a bit of topic wrt my work on cpu_model removal (or I'll try not to touch it until I have to) > Maybe we could at least have kvm_ppc_register_host_cpu_type() to call a > function in spapr_cpu_core.c instead of duplicating the registration code ? Is it possible for other boards (non core based) to use host CPU? but for simplicity sake I'd lave it where it's now. BTW I have an additional question: what PPC boards are supposed to support KVM/host cpu? (especially taking in account that aliases get patched to it as well, which I very much dislike since user asked for one CPU but got another and when the same happens on another host migration probably would be screwed up) > > > > > > https://patchwork.ozlabs.org/patch/817598/ > > > --- > > > hw/ppc/spapr.c | 4 ++++ > > > hw/ppc/spapr_cpu_core.c | 34 ++++++++++++++++++++++------------ > > > include/hw/ppc/spapr_cpu_core.h | 2 +- > > > target/ppc/kvm.c | 12 ------------ > > > 4 files changed, 27 insertions(+), 25 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 0ce3ec87ac59..e82c8532ffb0 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine) > > > } > > > > > > /* init CPUs */ > > > + if (kvm_enabled()) { > > > + spapr_cpu_core_register_host_type(); > > > + } > > > + > > > if (machine->cpu_model == NULL) { > > > machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu; > > > } > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > index c08ee7571a50..6e224ba029ec 100644 > > > --- a/hw/ppc/spapr_cpu_core.c > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -317,7 +317,7 @@ static Property spapr_cpu_core_properties[] = { > > > DEFINE_PROP_END_OF_LIST() > > > }; > > > > > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > > { > > > DeviceClass *dc = DEVICE_CLASS(oc); > > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc); > > > @@ -337,6 +337,20 @@ static const TypeInfo spapr_cpu_core_type_info = { > > > .class_size = sizeof(sPAPRCPUCoreClass), > > > }; > > > > > > +static void spapr_cpu_core_register_type(const char *model_name) > > > +{ > > > + TypeInfo type_info = { > > > + .parent = TYPE_SPAPR_CPU_CORE, > > > + .instance_size = sizeof(sPAPRCPUCore), > > > + .class_init = spapr_cpu_core_class_init, > > > + .class_data = (void *) model_name, > > > + }; > > > + > > > + type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, model_name); > > > + type_register(&type_info); > > > + g_free((void *)type_info.name); > > > +} > > > + > > > static void spapr_cpu_core_register_types(void) > > > { > > > int i; > > > @@ -344,18 +358,14 @@ static void spapr_cpu_core_register_types(void) > > > type_register_static(&spapr_cpu_core_type_info); > > > > > > for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) { > > > - TypeInfo type_info = { > > > - .parent = TYPE_SPAPR_CPU_CORE, > > > - .instance_size = sizeof(sPAPRCPUCore), > > > - .class_init = spapr_cpu_core_class_init, > > > - .class_data = (void *) spapr_core_models[i], > > > - }; > > > - > > > - type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, > > > - spapr_core_models[i]); > > > - type_register(&type_info); > > > - g_free((void *)type_info.name); > > > + spapr_cpu_core_register_type(spapr_core_models[i]); > > > } > > > + > > > +} > > > + > > > +void spapr_cpu_core_register_host_type(void) > > > +{ > > > + spapr_cpu_core_register_type("host"); > > > } > > > > > > type_init(spapr_cpu_core_register_types) > > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > > > index 93051e9ecf56..e3e906343048 100644 > > > --- a/include/hw/ppc/spapr_cpu_core.h > > > +++ b/include/hw/ppc/spapr_cpu_core.h > > > @@ -36,5 +36,5 @@ typedef struct sPAPRCPUCoreClass { > > > } sPAPRCPUCoreClass; > > > > > > char *spapr_get_cpu_core_type(const char *model); > > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data); > > > +void spapr_cpu_core_register_host_type(void); > > > #endif > > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > > index 5b281b2f1b6d..8dd80993ec9e 100644 > > > --- a/target/ppc/kvm.c > > > +++ b/target/ppc/kvm.c > > > @@ -37,7 +37,6 @@ > > > #include "hw/sysbus.h" > > > #include "hw/ppc/spapr.h" > > > #include "hw/ppc/spapr_vio.h" > > > -#include "hw/ppc/spapr_cpu_core.h" > > > #include "hw/ppc/ppc.h" > > > #include "sysemu/watchdog.h" > > > #include "trace.h" > > > @@ -2502,17 +2501,6 @@ static int kvm_ppc_register_host_cpu_type(void) > > > oc = object_class_by_name(type_info.name); > > > g_assert(oc); > > > > > > -#if defined(TARGET_PPC64) > > > - type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host"); > > > - type_info.parent = TYPE_SPAPR_CPU_CORE, > > > - type_info.instance_size = sizeof(sPAPRCPUCore); > > > - type_info.instance_init = NULL; > > > - type_info.class_init = spapr_cpu_core_class_init; > > > - type_info.class_data = (void *) "host"; > > > - type_register(&type_info); > > > - g_free((void *)type_info.name); > > > -#endif > > > - > > > /* > > > * Update generic CPU family class alias (e.g. on a POWER8NVL host, > > > * we want "POWER8" to be a "family" alias that points to the current > > > > > > > >
On Tue, Sep 26, 2017 at 10:29:36AM +0200, Igor Mammedov wrote: > On Tue, 26 Sep 2017 09:19:28 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > On Tue, 26 Sep 2017 12:57:39 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Mon, Sep 25, 2017 at 11:47:33AM +0200, Greg Kurz wrote: > > > > The CPU core abstraction belongs to the machine code. This also gets > > > > rid of some code duplication. > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > > but this is already handled by the following cleanup patch: > > > > > > I don't really see what the advantage of this is. As others have > > > pointed out it leads to the host type being registered very late, > > > which could cause problems. > > > > > > > Well, the goal was to consolidate the code to register sPAPRCPUCore types in > > the spapr code, instead of open-coding it in spapr_cpu_core.c and kvm.c... > > > > But now I realize that delaying the registration even more is a bad idea. And, > > the other way round, registering a static type earlier as asked by Igor would > > require all parent types to be already registered, which seems to be impossible > > to guarantee with the current code. > so I'll leave this code as is for now as as it's a bit of topic > wrt my work on cpu_model removal (or I'll try not to touch it until I have to) > > > Maybe we could at least have kvm_ppc_register_host_cpu_type() to call a > > function in spapr_cpu_core.c instead of duplicating the registration code ? > Is it possible for other boards (non core based) to use host CPU? > > but for simplicity sake I'd lave it where it's now. > > BTW I have an additional question: > what PPC boards are supposed to support KVM/host cpu? In theory, any board, but your host has to have a cpu that's suitable for the board in question. So, if your host is a POWER8, you can use -cpu host with a machine type which supports a POWER8 cpu; which is basically just "pseries". You couldn't use -cpu host with one of the embedded board types, since they don't expect a POWER8 vCPU. If you were on an e500 based host, you could use -cpu host with one of the various e500 based machine types. You couldn't use it with "pseries", since that will only work with a POWER7/8/9 vCPU. The issue is muddied by the fact that KVM HV (the fast KVM implementation), a) will only work with a PAPR guest (i.e. pseries machine type) and b) can't present a vCPU to the guest that differs from the host (this is a hardware limitation). If these conditions aren't met, the host kernel should automatically fall back to KVM PR (or fail outright if it's not available). Of course then it's further muddied by the fact that KVM PR has _some_ ability to present a different vCPU to the guest than is on the host, but it has some curly limitations that we might or might not be able to detect in advance. And, it's muddied even further by the fact that some of the CPU "variants" are in fact identical cores - the differences are just in peripherals, pin-level connections, cache etc. which the guest can't see and doesn't care about. Unfortunately, there aren't separate ways for the CPU to advertise core version and other things, so even trivial variants like this will have a different PVR value as a rule. > (especially taking in account that aliases get patched to it as well, > which I very much dislike since user asked for one CPU but got > another Uh.. what? You should never get something different than what you asked for. The worst that can happen is if you request something fuzzy, like "POWER8" instead of "POWER8E_v2.0" then exactly which thing in that family you get depends on the host. > and when the same happens on another host migration probably would be > screwed up) So, we can migrate between different CPUs if they're similar enough (say, everything within the POWER8 family). The guest is able to see the change in the PVR, but it will be fine in practice. This is really ugly, but unfortunately there's no away around it in the hardware, it's simply not possible to virtualize the PVR to present a different CPU variant to the guest (well, short of putting the entire guest into user mode, which is what KVM PR does). -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Wed, 27 Sep 2017 16:39:03 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Tue, Sep 26, 2017 at 10:29:36AM +0200, Igor Mammedov wrote: > > On Tue, 26 Sep 2017 09:19:28 +0200 > > Greg Kurz <groug@kaod.org> wrote: > > > > > On Tue, 26 Sep 2017 12:57:39 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Mon, Sep 25, 2017 at 11:47:33AM +0200, Greg Kurz wrote: > > > > > The CPU core abstraction belongs to the machine code. This also gets > > > > > rid of some code duplication. > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > --- > > > > > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > > > but this is already handled by the following cleanup patch: > > > > > > > > I don't really see what the advantage of this is. As others have > > > > pointed out it leads to the host type being registered very late, > > > > which could cause problems. > > > > > > > > > > Well, the goal was to consolidate the code to register sPAPRCPUCore types in > > > the spapr code, instead of open-coding it in spapr_cpu_core.c and kvm.c... > > > > > > But now I realize that delaying the registration even more is a bad idea. And, > > > the other way round, registering a static type earlier as asked by Igor would > > > require all parent types to be already registered, which seems to be impossible > > > to guarantee with the current code. > > so I'll leave this code as is for now as as it's a bit of topic > > wrt my work on cpu_model removal (or I'll try not to touch it until I have to) > > > > > Maybe we could at least have kvm_ppc_register_host_cpu_type() to call a > > > function in spapr_cpu_core.c instead of duplicating the registration code ? > > Is it possible for other boards (non core based) to use host CPU? > > > > but for simplicity sake I'd lave it where it's now. > > > > BTW I have an additional question: > > what PPC boards are supposed to support KVM/host cpu? > > In theory, any board, but your host has to have a cpu that's suitable > for the board in question. So, if your host is a POWER8, you can use > -cpu host with a machine type which supports a POWER8 cpu; which is > basically just "pseries". You couldn't use -cpu host with one of the > embedded board types, since they don't expect a POWER8 vCPU. > > If you were on an e500 based host, you could use -cpu host with one of > the various e500 based machine types. You couldn't use it with > "pseries", since that will only work with a POWER7/8/9 vCPU. > > The issue is muddied by the fact that KVM HV (the fast KVM > implementation), a) will only work with a PAPR guest (i.e. pseries > machine type) and b) can't present a vCPU to the guest that differs > from the host (this is a hardware limitation). If these conditions > aren't met, the host kernel should automatically fall back to KVM PR > (or fail outright if it's not available). Of course then it's further > muddied by the fact that KVM PR has _some_ ability to present a > different vCPU to the guest than is on the host, but it has some curly > limitations that we might or might not be able to detect in advance. > > And, it's muddied even further by the fact that some of the CPU > "variants" are in fact identical cores - the differences are just in > peripherals, pin-level connections, cache etc. which the guest can't > see and doesn't care about. Unfortunately, there aren't separate ways > for the CPU to advertise core version and other things, so even > trivial variants like this will have a different PVR value as a rule. > > > (especially taking in account that aliases get patched to it as well, > > which I very much dislike since user asked for one CPU but got > > another > > Uh.. what? You should never get something different than what you > asked for. The worst that can happen is if you request something > fuzzy, like "POWER8" instead of "POWER8E_v2.0" then exactly which > thing in that family you get depends on the host. that's what bothers me since when alias is used /like POWER8/ user gets 'some' cpu and he/she has to be aware that it might be not the exactly the same cpu on another host. > > and when the same happens on another host migration probably would be > > screwed up) > > So, we can migrate between different CPUs if they're similar enough > (say, everything within the POWER8 family). The guest is able to see > the change in the PVR, but it will be fine in practice. sounds rather fragile, is there plan to make it more strict and less ambiguous? /like not allowing 'generic' cpu names and using only concrete ones/ > This is really ugly, but unfortunately there's no away around it in > the hardware, it's simply not possible to virtualize the PVR to > present a different CPU variant to the guest (well, short of putting > the entire guest into user mode, which is what KVM PR does). >
On Wed, Sep 27, 2017 at 02:11:44PM +0200, Igor Mammedov wrote: > On Wed, 27 Sep 2017 16:39:03 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Tue, Sep 26, 2017 at 10:29:36AM +0200, Igor Mammedov wrote: > > > On Tue, 26 Sep 2017 09:19:28 +0200 > > > Greg Kurz <groug@kaod.org> wrote: > > > > > > > On Tue, 26 Sep 2017 12:57:39 +1000 > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > On Mon, Sep 25, 2017 at 11:47:33AM +0200, Greg Kurz wrote: > > > > > > The CPU core abstraction belongs to the machine code. This also gets > > > > > > rid of some code duplication. > > > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > > --- > > > > > > > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > > > > but this is already handled by the following cleanup patch: > > > > > > > > > > I don't really see what the advantage of this is. As others have > > > > > pointed out it leads to the host type being registered very late, > > > > > which could cause problems. > > > > > > > > > > > > > Well, the goal was to consolidate the code to register sPAPRCPUCore types in > > > > the spapr code, instead of open-coding it in spapr_cpu_core.c and kvm.c... > > > > > > > > But now I realize that delaying the registration even more is a bad idea. And, > > > > the other way round, registering a static type earlier as asked by Igor would > > > > require all parent types to be already registered, which seems to be impossible > > > > to guarantee with the current code. > > > so I'll leave this code as is for now as as it's a bit of topic > > > wrt my work on cpu_model removal (or I'll try not to touch it until I have to) > > > > > > > Maybe we could at least have kvm_ppc_register_host_cpu_type() to call a > > > > function in spapr_cpu_core.c instead of duplicating the registration code ? > > > Is it possible for other boards (non core based) to use host CPU? > > > > > > but for simplicity sake I'd lave it where it's now. > > > > > > BTW I have an additional question: > > > what PPC boards are supposed to support KVM/host cpu? > > > > In theory, any board, but your host has to have a cpu that's suitable > > for the board in question. So, if your host is a POWER8, you can use > > -cpu host with a machine type which supports a POWER8 cpu; which is > > basically just "pseries". You couldn't use -cpu host with one of the > > embedded board types, since they don't expect a POWER8 vCPU. > > > > If you were on an e500 based host, you could use -cpu host with one of > > the various e500 based machine types. You couldn't use it with > > "pseries", since that will only work with a POWER7/8/9 vCPU. > > > > The issue is muddied by the fact that KVM HV (the fast KVM > > implementation), a) will only work with a PAPR guest (i.e. pseries > > machine type) and b) can't present a vCPU to the guest that differs > > from the host (this is a hardware limitation). If these conditions > > aren't met, the host kernel should automatically fall back to KVM PR > > (or fail outright if it's not available). Of course then it's further > > muddied by the fact that KVM PR has _some_ ability to present a > > different vCPU to the guest than is on the host, but it has some curly > > limitations that we might or might not be able to detect in advance. > > > > And, it's muddied even further by the fact that some of the CPU > > "variants" are in fact identical cores - the differences are just in > > peripherals, pin-level connections, cache etc. which the guest can't > > see and doesn't care about. Unfortunately, there aren't separate ways > > for the CPU to advertise core version and other things, so even > > trivial variants like this will have a different PVR value as a rule. > > > > > (especially taking in account that aliases get patched to it as well, > > > which I very much dislike since user asked for one CPU but got > > > another > > > > Uh.. what? You should never get something different than what you > > asked for. The worst that can happen is if you request something > > fuzzy, like "POWER8" instead of "POWER8E_v2.0" then exactly which > > thing in that family you get depends on the host. > that's what bothers me since when alias is used /like POWER8/ > user gets 'some' cpu and he/she has to be aware that it might be not > the exactly the same cpu on another host. Well, yeah. We did this for the benefit of libvirt, which really wants to specify "POWER8" or "POWER9" or whatever instead of "host". KVM HV only works with the host CPU, so it doesn't work for it to specify some specific cpu model. > > > and when the same happens on another host migration probably would be > > > screwed up) > > > > So, we can migrate between different CPUs if they're similar enough > > (say, everything within the POWER8 family). The guest is able to see > > the change in the PVR, but it will be fine in practice. > sounds rather fragile, is there plan to make it more strict and > less ambiguous? Not really. > /like not allowing 'generic' cpu names and using > only concrete ones/ I don't see how that would help. Again we *cannot* present a different cpu to the guest than is present in the host, it's simply not possible in the hardware. > > This is really ugly, but unfortunately there's no away around it in > > the hardware, it's simply not possible to virtualize the PVR to > > present a different CPU variant to the guest (well, short of putting > > the entire guest into user mode, which is what KVM PR does). -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Tue, Sep 26, 2017 at 09:19:28AM +0200, Greg Kurz wrote: > On Tue, 26 Sep 2017 12:57:39 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, Sep 25, 2017 at 11:47:33AM +0200, Greg Kurz wrote: > > > The CPU core abstraction belongs to the machine code. This also gets > > > rid of some code duplication. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > but this is already handled by the following cleanup patch: > > > > I don't really see what the advantage of this is. As others have > > pointed out it leads to the host type being registered very late, > > which could cause problems. > > > > Well, the goal was to consolidate the code to register sPAPRCPUCore types in > the spapr code, instead of open-coding it in spapr_cpu_core.c and kvm.c... > > But now I realize that delaying the registration even more is a bad idea. And, > the other way round, registering a static type earlier as asked by Igor would > require all parent types to be already registered, which seems to be impossible > to guarantee with the current code. > > Maybe we could at least have kvm_ppc_register_host_cpu_type() to call a > function in spapr_cpu_core.c instead of duplicating the registration > code ? I think that sounds like a better idea. It's still a little bodgy with the abstraction boundaries, but I think that's unavoidable: this fundamentally depends on both KVM's presence and use of the PAPR machine type. Whichever place we put it, you could argue it belongs better in the other one. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Wed, 27 Sep 2017 16:21:43 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Tue, Sep 26, 2017 at 09:19:28AM +0200, Greg Kurz wrote: > > On Tue, 26 Sep 2017 12:57:39 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Mon, Sep 25, 2017 at 11:47:33AM +0200, Greg Kurz wrote: > > > > The CPU core abstraction belongs to the machine code. This also gets > > > > rid of some code duplication. > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > > > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c > > > > but this is already handled by the following cleanup patch: > > > > > > I don't really see what the advantage of this is. As others have > > > pointed out it leads to the host type being registered very late, > > > which could cause problems. > > > > > > > Well, the goal was to consolidate the code to register sPAPRCPUCore types in > > the spapr code, instead of open-coding it in spapr_cpu_core.c and kvm.c... > > > > But now I realize that delaying the registration even more is a bad idea. And, > > the other way round, registering a static type earlier as asked by Igor would > > require all parent types to be already registered, which seems to be impossible > > to guarantee with the current code. > > > > Maybe we could at least have kvm_ppc_register_host_cpu_type() to call a > > function in spapr_cpu_core.c instead of duplicating the registration > > code ? > > I think that sounds like a better idea. It's still a little bodgy > with the abstraction boundaries, but I think that's unavoidable: this > fundamentally depends on both KVM's presence and use of the PAPR > machine type. Whichever place we put it, you could argue it belongs > better in the other one. it looks like kvm_ppc_register_host_cpu_type() doesn't depend on anything that requires KVM being present/initialized, so I'd suggest to: - 1: revert commit 715d4b96 and do alias hiding another way - 2: move host core type registration into spapr_cpu_core.c and make it static like x86 - 3: move host cpu type into target/ppc/translate_init.c where the rest of cpu types is initialized and make it static like x86
--
patch does 3 things at the same time and should be split but
it has 'host' consolidation is it as well to demonstrate idea
--
spapr core type definition doesn't have any fields that
require it to be defined at runtime. So replace code
that fills in TypeInfo at runtime with static TypeInfo
array that does the same at complie time.
And replace sPAPRCPUCoreClass::cpu_class with cpu
type name since were used just to get that at points
it were accessed.
While at that, consolidate move 'host' core type
registration into spapr_cpu_core.c, similar like
it's done in x86 target.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/hw/ppc/spapr_cpu_core.h | 5 ++-
hw/ppc/spapr.c | 6 +--
hw/ppc/spapr_cpu_core.c | 93 ++++++++++++++++-------------------------
target/ppc/kvm.c | 11 -----
4 files changed, 41 insertions(+), 74 deletions(-)
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 93051e9..42765de 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -21,6 +21,8 @@
#define SPAPR_CPU_CORE_GET_CLASS(obj) \
OBJECT_GET_CLASS(sPAPRCPUCoreClass, (obj), TYPE_SPAPR_CPU_CORE)
+#define SPAPR_CPU_CORE_TYPE_NAME(model) model "-" TYPE_SPAPR_CPU_CORE
+
typedef struct sPAPRCPUCore {
/*< private >*/
CPUCore parent_obj;
@@ -32,9 +34,8 @@ typedef struct sPAPRCPUCore {
typedef struct sPAPRCPUCoreClass {
DeviceClass parent_class;
- ObjectClass *cpu_class;
+ const char *cpu_type;
} sPAPRCPUCoreClass;
char *spapr_get_cpu_core_type(const char *model);
-void spapr_cpu_core_class_init(ObjectClass *oc, void *data);
#endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8c1a437..00cfe9a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3125,8 +3125,7 @@ void spapr_core_release(DeviceState *dev)
if (smc->pre_2_10_has_unused_icps) {
sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
- const char *typename = object_class_get_name(scc->cpu_class);
- size_t size = object_type_get_instance_size(typename);
+ size_t size = object_type_get_instance_size(scc->cpu_type);
int i;
for (i = 0; i < cc->nr_threads; i++) {
@@ -3222,8 +3221,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
if (smc->pre_2_10_has_unused_icps) {
sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
- const char *typename = object_class_get_name(scc->cpu_class);
- size_t size = object_type_get_instance_size(typename);
+ size_t size = object_type_get_instance_size(scc->cpu_type);
int i;
for (i = 0; i < cc->nr_threads; i++) {
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 5bea4c9..8a18eaf 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -104,8 +104,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
{
sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
- const char *typename = object_class_get_name(scc->cpu_class);
- size_t size = object_type_get_instance_size(typename);
+ size_t size = object_type_get_instance_size(scc->cpu_type);
CPUCore *cc = CPU_CORE(dev);
int i;
@@ -166,8 +165,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
CPUCore *cc = CPU_CORE(OBJECT(dev));
- const char *typename = object_class_get_name(scc->cpu_class);
- size_t size = object_type_get_instance_size(typename);
+ size_t size = object_type_get_instance_size(scc->cpu_type);
Error *local_err = NULL;
void *obj;
int i, j;
@@ -186,7 +184,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
obj = sc->threads + i * size;
- object_initialize(obj, size, typename);
+ object_initialize(obj, size, scc->cpu_type);
cs = CPU(obj);
cpu = POWERPC_CPU(cs);
cs->cpu_index = cc->core_id + i;
@@ -231,42 +229,12 @@ err:
error_propagate(errp, local_err);
}
-static const char *spapr_core_models[] = {
- /* 970 */
- "970_v2.2",
-
- /* 970MP variants */
- "970mp_v1.0",
- "970mp_v1.1",
-
- /* POWER5+ */
- "power5+_v2.1",
-
- /* POWER7 */
- "power7_v2.3",
-
- /* POWER7+ */
- "power7+_v2.1",
-
- /* POWER8 */
- "power8_v2.0",
-
- /* POWER8E */
- "power8e_v2.1",
-
- /* POWER8NVL */
- "power8nvl_v1.0",
-
- /* POWER9 */
- "power9_v1.0",
-};
-
static Property spapr_cpu_core_properties[] = {
DEFINE_PROP_INT32("node-id", sPAPRCPUCore, node_id, CPU_UNSET_NUMA_NODE_ID),
DEFINE_PROP_END_OF_LIST()
};
-void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
+static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc);
@@ -274,36 +242,47 @@ void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
dc->realize = spapr_cpu_core_realize;
dc->unrealize = spapr_cpu_core_unrealizefn;
dc->props = spapr_cpu_core_properties;
- scc->cpu_class = cpu_class_by_name(TYPE_POWERPC_CPU, data);
- g_assert(scc->cpu_class);
+ scc->cpu_type = data;
}
-static const TypeInfo spapr_cpu_core_type_info = {
- .name = TYPE_SPAPR_CPU_CORE,
- .parent = TYPE_CPU_CORE,
- .abstract = true,
- .instance_size = sizeof(sPAPRCPUCore),
- .class_size = sizeof(sPAPRCPUCoreClass),
+#define DEFINE_SPAPR_CPU_CORE_TYPE(cpu_model) \
+ { \
+ .parent = TYPE_SPAPR_CPU_CORE, \
+ .class_data = (void *) POWERPC_CPU_TYPE_NAME(cpu_model), \
+ .class_init = spapr_cpu_core_class_init, \
+ .name = SPAPR_CPU_CORE_TYPE_NAME(cpu_model), \
+ }
+
+static const TypeInfo spapr_cpu_core_type_infos[] = {
+ {
+ .name = TYPE_SPAPR_CPU_CORE,
+ .parent = TYPE_CPU_CORE,
+ .abstract = true,
+ .instance_size = sizeof(sPAPRCPUCore),
+ .class_size = sizeof(sPAPRCPUCoreClass),
+ },
+ DEFINE_SPAPR_CPU_CORE_TYPE("970_v2.2"),
+ DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.0"),
+ DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.1"),
+ DEFINE_SPAPR_CPU_CORE_TYPE("power5+_v2.1"),
+ DEFINE_SPAPR_CPU_CORE_TYPE("power7_v2.3"),
+ DEFINE_SPAPR_CPU_CORE_TYPE("power7+_v2.1"),
+ DEFINE_SPAPR_CPU_CORE_TYPE("power8_v2.0"),
+ DEFINE_SPAPR_CPU_CORE_TYPE("power8e_v2.1"),
+ DEFINE_SPAPR_CPU_CORE_TYPE("power8nvl_v1.0"),
+ DEFINE_SPAPR_CPU_CORE_TYPE("power9_v1.0"),
+#ifdef CONFIG_KVM
+ DEFINE_SPAPR_CPU_CORE_TYPE("host"),
+#endif
};
static void spapr_cpu_core_register_types(void)
{
int i;
- type_register_static(&spapr_cpu_core_type_info);
-
- for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) {
- TypeInfo type_info = {
- .parent = TYPE_SPAPR_CPU_CORE,
- .instance_size = sizeof(sPAPRCPUCore),
- .class_init = spapr_cpu_core_class_init,
- .class_data = (void *) spapr_core_models[i],
- };
- type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE,
- spapr_core_models[i]);
- type_register(&type_info);
- g_free((void *)type_info.name);
+ for (i = 0; i < ARRAY_SIZE(spapr_cpu_core_type_infos); i++) {
+ type_register_static(&spapr_cpu_core_type_infos[i]);
}
}
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 1deaf10..7e5c041 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2507,17 +2507,6 @@ static int kvm_ppc_register_host_cpu_type(void)
oc = object_class_by_name(type_info.name);
g_assert(oc);
-#if defined(TARGET_PPC64)
- type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host");
- type_info.parent = TYPE_SPAPR_CPU_CORE,
- type_info.instance_size = sizeof(sPAPRCPUCore);
- type_info.instance_init = NULL;
- type_info.class_init = spapr_cpu_core_class_init;
- type_info.class_data = (void *) "host";
- type_register(&type_info);
- g_free((void *)type_info.name);
-#endif
-
/*
* Update generic CPU family class alias (e.g. on a POWER8NVL host,
* we want "POWER8" to be a "family" alias that points to the current
--
2.7.4
On Wed, 27 Sep 2017 13:49:17 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > -- > patch does 3 things at the same time and should be split but > it has 'host' consolidation is it as well to demonstrate idea > -- > > spapr core type definition doesn't have any fields that > require it to be defined at runtime. So replace code > that fills in TypeInfo at runtime with static TypeInfo > array that does the same at complie time. > > And replace sPAPRCPUCoreClass::cpu_class with cpu > type name since were used just to get that at points > it were accessed. > > While at that, consolidate move 'host' core type > registration into spapr_cpu_core.c, similar like > it's done in x86 target. > This looks nice indeed (just one remark, see below). Will you re-post as a patch series or do you prefer I do it ? > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > include/hw/ppc/spapr_cpu_core.h | 5 ++- > hw/ppc/spapr.c | 6 +-- > hw/ppc/spapr_cpu_core.c | 93 ++++++++++++++++------------------------- > target/ppc/kvm.c | 11 ----- > 4 files changed, 41 insertions(+), 74 deletions(-) > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > index 93051e9..42765de 100644 > --- a/include/hw/ppc/spapr_cpu_core.h > +++ b/include/hw/ppc/spapr_cpu_core.h > @@ -21,6 +21,8 @@ > #define SPAPR_CPU_CORE_GET_CLASS(obj) \ > OBJECT_GET_CLASS(sPAPRCPUCoreClass, (obj), TYPE_SPAPR_CPU_CORE) > > +#define SPAPR_CPU_CORE_TYPE_NAME(model) model "-" TYPE_SPAPR_CPU_CORE > + > typedef struct sPAPRCPUCore { > /*< private >*/ > CPUCore parent_obj; > @@ -32,9 +34,8 @@ typedef struct sPAPRCPUCore { > > typedef struct sPAPRCPUCoreClass { > DeviceClass parent_class; > - ObjectClass *cpu_class; > + const char *cpu_type; > } sPAPRCPUCoreClass; > > char *spapr_get_cpu_core_type(const char *model); > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data); > #endif > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 8c1a437..00cfe9a 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3125,8 +3125,7 @@ void spapr_core_release(DeviceState *dev) > if (smc->pre_2_10_has_unused_icps) { > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc)); > - const char *typename = object_class_get_name(scc->cpu_class); > - size_t size = object_type_get_instance_size(typename); > + size_t size = object_type_get_instance_size(scc->cpu_type); > int i; > > for (i = 0; i < cc->nr_threads; i++) { > @@ -3222,8 +3221,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > if (smc->pre_2_10_has_unused_icps) { > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc)); > - const char *typename = object_class_get_name(scc->cpu_class); > - size_t size = object_type_get_instance_size(typename); > + size_t size = object_type_get_instance_size(scc->cpu_type); > int i; > > for (i = 0; i < cc->nr_threads; i++) { > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index 5bea4c9..8a18eaf 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -104,8 +104,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp) > { > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev)); > - const char *typename = object_class_get_name(scc->cpu_class); > - size_t size = object_type_get_instance_size(typename); > + size_t size = object_type_get_instance_size(scc->cpu_type); > CPUCore *cc = CPU_CORE(dev); > int i; > > @@ -166,8 +165,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev)); > CPUCore *cc = CPU_CORE(OBJECT(dev)); > - const char *typename = object_class_get_name(scc->cpu_class); > - size_t size = object_type_get_instance_size(typename); > + size_t size = object_type_get_instance_size(scc->cpu_type); > Error *local_err = NULL; > void *obj; > int i, j; > @@ -186,7 +184,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > obj = sc->threads + i * size; > > - object_initialize(obj, size, typename); > + object_initialize(obj, size, scc->cpu_type); > cs = CPU(obj); > cpu = POWERPC_CPU(cs); > cs->cpu_index = cc->core_id + i; > @@ -231,42 +229,12 @@ err: > error_propagate(errp, local_err); > } > > -static const char *spapr_core_models[] = { > - /* 970 */ > - "970_v2.2", > - > - /* 970MP variants */ > - "970mp_v1.0", > - "970mp_v1.1", > - > - /* POWER5+ */ > - "power5+_v2.1", > - > - /* POWER7 */ > - "power7_v2.3", > - > - /* POWER7+ */ > - "power7+_v2.1", > - > - /* POWER8 */ > - "power8_v2.0", > - > - /* POWER8E */ > - "power8e_v2.1", > - > - /* POWER8NVL */ > - "power8nvl_v1.0", > - > - /* POWER9 */ > - "power9_v1.0", > -}; > - > static Property spapr_cpu_core_properties[] = { > DEFINE_PROP_INT32("node-id", sPAPRCPUCore, node_id, CPU_UNSET_NUMA_NODE_ID), > DEFINE_PROP_END_OF_LIST() > }; > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc); > @@ -274,36 +242,47 @@ void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > dc->realize = spapr_cpu_core_realize; > dc->unrealize = spapr_cpu_core_unrealizefn; > dc->props = spapr_cpu_core_properties; > - scc->cpu_class = cpu_class_by_name(TYPE_POWERPC_CPU, data); > - g_assert(scc->cpu_class); > + scc->cpu_type = data; Maybe assert data isn't NULL ? > } > > -static const TypeInfo spapr_cpu_core_type_info = { > - .name = TYPE_SPAPR_CPU_CORE, > - .parent = TYPE_CPU_CORE, > - .abstract = true, > - .instance_size = sizeof(sPAPRCPUCore), > - .class_size = sizeof(sPAPRCPUCoreClass), > +#define DEFINE_SPAPR_CPU_CORE_TYPE(cpu_model) \ > + { \ > + .parent = TYPE_SPAPR_CPU_CORE, \ > + .class_data = (void *) POWERPC_CPU_TYPE_NAME(cpu_model), \ > + .class_init = spapr_cpu_core_class_init, \ > + .name = SPAPR_CPU_CORE_TYPE_NAME(cpu_model), \ > + } > + > +static const TypeInfo spapr_cpu_core_type_infos[] = { > + { > + .name = TYPE_SPAPR_CPU_CORE, > + .parent = TYPE_CPU_CORE, > + .abstract = true, > + .instance_size = sizeof(sPAPRCPUCore), > + .class_size = sizeof(sPAPRCPUCoreClass), > + }, > + DEFINE_SPAPR_CPU_CORE_TYPE("970_v2.2"), > + DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.0"), > + DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.1"), > + DEFINE_SPAPR_CPU_CORE_TYPE("power5+_v2.1"), > + DEFINE_SPAPR_CPU_CORE_TYPE("power7_v2.3"), > + DEFINE_SPAPR_CPU_CORE_TYPE("power7+_v2.1"), > + DEFINE_SPAPR_CPU_CORE_TYPE("power8_v2.0"), > + DEFINE_SPAPR_CPU_CORE_TYPE("power8e_v2.1"), > + DEFINE_SPAPR_CPU_CORE_TYPE("power8nvl_v1.0"), > + DEFINE_SPAPR_CPU_CORE_TYPE("power9_v1.0"), > +#ifdef CONFIG_KVM > + DEFINE_SPAPR_CPU_CORE_TYPE("host"), > +#endif > }; > > static void spapr_cpu_core_register_types(void) > { > int i; > > - type_register_static(&spapr_cpu_core_type_info); > - > - for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) { > - TypeInfo type_info = { > - .parent = TYPE_SPAPR_CPU_CORE, > - .instance_size = sizeof(sPAPRCPUCore), > - .class_init = spapr_cpu_core_class_init, > - .class_data = (void *) spapr_core_models[i], > - }; > > - type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, > - spapr_core_models[i]); > - type_register(&type_info); > - g_free((void *)type_info.name); > + for (i = 0; i < ARRAY_SIZE(spapr_cpu_core_type_infos); i++) { > + type_register_static(&spapr_cpu_core_type_infos[i]); > } > } > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index 1deaf10..7e5c041 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -2507,17 +2507,6 @@ static int kvm_ppc_register_host_cpu_type(void) > oc = object_class_by_name(type_info.name); > g_assert(oc); > > -#if defined(TARGET_PPC64) > - type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host"); > - type_info.parent = TYPE_SPAPR_CPU_CORE, > - type_info.instance_size = sizeof(sPAPRCPUCore); > - type_info.instance_init = NULL; > - type_info.class_init = spapr_cpu_core_class_init; > - type_info.class_data = (void *) "host"; > - type_register(&type_info); > - g_free((void *)type_info.name); > -#endif > - > /* > * Update generic CPU family class alias (e.g. on a POWER8NVL host, > * we want "POWER8" to be a "family" alias that points to the current
On Wed, Sep 27, 2017 at 06:18:34PM +0200, Greg Kurz wrote: > On Wed, 27 Sep 2017 13:49:17 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > > This looks nice indeed (just one remark, see below). > > Will you re-post as a patch series or do you prefer I do it ? Looks good to me as well. > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > include/hw/ppc/spapr_cpu_core.h | 5 ++- > > hw/ppc/spapr.c | 6 +-- > > hw/ppc/spapr_cpu_core.c | 93 ++++++++++++++++------------------------- > > target/ppc/kvm.c | 11 ----- > > 4 files changed, 41 insertions(+), 74 deletions(-) > > > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > > index 93051e9..42765de 100644 > > --- a/include/hw/ppc/spapr_cpu_core.h > > +++ b/include/hw/ppc/spapr_cpu_core.h > > @@ -21,6 +21,8 @@ > > #define SPAPR_CPU_CORE_GET_CLASS(obj) \ > > OBJECT_GET_CLASS(sPAPRCPUCoreClass, (obj), TYPE_SPAPR_CPU_CORE) > > > > +#define SPAPR_CPU_CORE_TYPE_NAME(model) model "-" TYPE_SPAPR_CPU_CORE > > + > > typedef struct sPAPRCPUCore { > > /*< private >*/ > > CPUCore parent_obj; > > @@ -32,9 +34,8 @@ typedef struct sPAPRCPUCore { > > > > typedef struct sPAPRCPUCoreClass { > > DeviceClass parent_class; > > - ObjectClass *cpu_class; > > + const char *cpu_type; > > } sPAPRCPUCoreClass; > > > > char *spapr_get_cpu_core_type(const char *model); > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data); > > #endif > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 8c1a437..00cfe9a 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -3125,8 +3125,7 @@ void spapr_core_release(DeviceState *dev) > > if (smc->pre_2_10_has_unused_icps) { > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc)); > > - const char *typename = object_class_get_name(scc->cpu_class); > > - size_t size = object_type_get_instance_size(typename); > > + size_t size = object_type_get_instance_size(scc->cpu_type); > > int i; > > > > for (i = 0; i < cc->nr_threads; i++) { > > @@ -3222,8 +3221,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > if (smc->pre_2_10_has_unused_icps) { > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc)); > > - const char *typename = object_class_get_name(scc->cpu_class); > > - size_t size = object_type_get_instance_size(typename); > > + size_t size = object_type_get_instance_size(scc->cpu_type); > > int i; > > > > for (i = 0; i < cc->nr_threads; i++) { > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index 5bea4c9..8a18eaf 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -104,8 +104,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp) > > { > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev)); > > - const char *typename = object_class_get_name(scc->cpu_class); > > - size_t size = object_type_get_instance_size(typename); > > + size_t size = object_type_get_instance_size(scc->cpu_type); > > CPUCore *cc = CPU_CORE(dev); > > int i; > > > > @@ -166,8 +165,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev)); > > CPUCore *cc = CPU_CORE(OBJECT(dev)); > > - const char *typename = object_class_get_name(scc->cpu_class); > > - size_t size = object_type_get_instance_size(typename); > > + size_t size = object_type_get_instance_size(scc->cpu_type); > > Error *local_err = NULL; > > void *obj; > > int i, j; > > @@ -186,7 +184,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > obj = sc->threads + i * size; > > > > - object_initialize(obj, size, typename); > > + object_initialize(obj, size, scc->cpu_type); > > cs = CPU(obj); > > cpu = POWERPC_CPU(cs); > > cs->cpu_index = cc->core_id + i; > > @@ -231,42 +229,12 @@ err: > > error_propagate(errp, local_err); > > } > > > > -static const char *spapr_core_models[] = { > > - /* 970 */ > > - "970_v2.2", > > - > > - /* 970MP variants */ > > - "970mp_v1.0", > > - "970mp_v1.1", > > - > > - /* POWER5+ */ > > - "power5+_v2.1", > > - > > - /* POWER7 */ > > - "power7_v2.3", > > - > > - /* POWER7+ */ > > - "power7+_v2.1", > > - > > - /* POWER8 */ > > - "power8_v2.0", > > - > > - /* POWER8E */ > > - "power8e_v2.1", > > - > > - /* POWER8NVL */ > > - "power8nvl_v1.0", > > - > > - /* POWER9 */ > > - "power9_v1.0", > > -}; > > - > > static Property spapr_cpu_core_properties[] = { > > DEFINE_PROP_INT32("node-id", sPAPRCPUCore, node_id, CPU_UNSET_NUMA_NODE_ID), > > DEFINE_PROP_END_OF_LIST() > > }; > > > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(oc); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc); > > @@ -274,36 +242,47 @@ void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > dc->realize = spapr_cpu_core_realize; > > dc->unrealize = spapr_cpu_core_unrealizefn; > > dc->props = spapr_cpu_core_properties; > > - scc->cpu_class = cpu_class_by_name(TYPE_POWERPC_CPU, data); > > - g_assert(scc->cpu_class); > > + scc->cpu_type = data; > > Maybe assert data isn't NULL ? > > > } > > > > -static const TypeInfo spapr_cpu_core_type_info = { > > - .name = TYPE_SPAPR_CPU_CORE, > > - .parent = TYPE_CPU_CORE, > > - .abstract = true, > > - .instance_size = sizeof(sPAPRCPUCore), > > - .class_size = sizeof(sPAPRCPUCoreClass), > > +#define DEFINE_SPAPR_CPU_CORE_TYPE(cpu_model) \ > > + { \ > > + .parent = TYPE_SPAPR_CPU_CORE, \ > > + .class_data = (void *) POWERPC_CPU_TYPE_NAME(cpu_model), \ > > + .class_init = spapr_cpu_core_class_init, \ > > + .name = SPAPR_CPU_CORE_TYPE_NAME(cpu_model), \ > > + } > > + > > +static const TypeInfo spapr_cpu_core_type_infos[] = { > > + { > > + .name = TYPE_SPAPR_CPU_CORE, > > + .parent = TYPE_CPU_CORE, > > + .abstract = true, > > + .instance_size = sizeof(sPAPRCPUCore), > > + .class_size = sizeof(sPAPRCPUCoreClass), > > + }, > > + DEFINE_SPAPR_CPU_CORE_TYPE("970_v2.2"), > > + DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.0"), > > + DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.1"), > > + DEFINE_SPAPR_CPU_CORE_TYPE("power5+_v2.1"), > > + DEFINE_SPAPR_CPU_CORE_TYPE("power7_v2.3"), > > + DEFINE_SPAPR_CPU_CORE_TYPE("power7+_v2.1"), > > + DEFINE_SPAPR_CPU_CORE_TYPE("power8_v2.0"), > > + DEFINE_SPAPR_CPU_CORE_TYPE("power8e_v2.1"), > > + DEFINE_SPAPR_CPU_CORE_TYPE("power8nvl_v1.0"), > > + DEFINE_SPAPR_CPU_CORE_TYPE("power9_v1.0"), > > +#ifdef CONFIG_KVM > > + DEFINE_SPAPR_CPU_CORE_TYPE("host"), > > +#endif > > }; > > > > static void spapr_cpu_core_register_types(void) > > { > > int i; > > > > - type_register_static(&spapr_cpu_core_type_info); > > - > > - for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) { > > - TypeInfo type_info = { > > - .parent = TYPE_SPAPR_CPU_CORE, > > - .instance_size = sizeof(sPAPRCPUCore), > > - .class_init = spapr_cpu_core_class_init, > > - .class_data = (void *) spapr_core_models[i], > > - }; > > > > - type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, > > - spapr_core_models[i]); > > - type_register(&type_info); > > - g_free((void *)type_info.name); > > + for (i = 0; i < ARRAY_SIZE(spapr_cpu_core_type_infos); i++) { > > + type_register_static(&spapr_cpu_core_type_infos[i]); > > } > > } > > > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > index 1deaf10..7e5c041 100644 > > --- a/target/ppc/kvm.c > > +++ b/target/ppc/kvm.c > > @@ -2507,17 +2507,6 @@ static int kvm_ppc_register_host_cpu_type(void) > > oc = object_class_by_name(type_info.name); > > g_assert(oc); > > > > -#if defined(TARGET_PPC64) > > - type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host"); > > - type_info.parent = TYPE_SPAPR_CPU_CORE, > > - type_info.instance_size = sizeof(sPAPRCPUCore); > > - type_info.instance_init = NULL; > > - type_info.class_init = spapr_cpu_core_class_init; > > - type_info.class_data = (void *) "host"; > > - type_register(&type_info); > > - g_free((void *)type_info.name); > > -#endif > > - > > /* > > * Update generic CPU family class alias (e.g. on a POWER8NVL host, > > * we want "POWER8" to be a "family" alias that points to the current > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Wed, 27 Sep 2017 18:18:34 +0200 Greg Kurz <groug@kaod.org> wrote: > On Wed, 27 Sep 2017 13:49:17 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > > -- > > patch does 3 things at the same time and should be split but > > it has 'host' consolidation is it as well to demonstrate idea > > -- > > > > spapr core type definition doesn't have any fields that > > require it to be defined at runtime. So replace code > > that fills in TypeInfo at runtime with static TypeInfo > > array that does the same at complie time. > > > > And replace sPAPRCPUCoreClass::cpu_class with cpu > > type name since were used just to get that at points > > it were accessed. > > > > While at that, consolidate move 'host' core type > > registration into spapr_cpu_core.c, similar like > > it's done in x86 target. > > > > This looks nice indeed (just one remark, see below). > > Will you re-post as a patch series or do you prefer I do it ? I'll repost it as part of cpu_model removal series (ppc part) > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > include/hw/ppc/spapr_cpu_core.h | 5 ++- > > hw/ppc/spapr.c | 6 +-- > > hw/ppc/spapr_cpu_core.c | 93 ++++++++++++++++------------------------- > > target/ppc/kvm.c | 11 ----- > > 4 files changed, 41 insertions(+), 74 deletions(-) > > > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h > > index 93051e9..42765de 100644 > > --- a/include/hw/ppc/spapr_cpu_core.h > > +++ b/include/hw/ppc/spapr_cpu_core.h > > @@ -21,6 +21,8 @@ > > #define SPAPR_CPU_CORE_GET_CLASS(obj) \ > > OBJECT_GET_CLASS(sPAPRCPUCoreClass, (obj), TYPE_SPAPR_CPU_CORE) > > > > +#define SPAPR_CPU_CORE_TYPE_NAME(model) model "-" TYPE_SPAPR_CPU_CORE > > + > > typedef struct sPAPRCPUCore { > > /*< private >*/ > > CPUCore parent_obj; > > @@ -32,9 +34,8 @@ typedef struct sPAPRCPUCore { > > > > typedef struct sPAPRCPUCoreClass { > > DeviceClass parent_class; > > - ObjectClass *cpu_class; > > + const char *cpu_type; > > } sPAPRCPUCoreClass; > > > > char *spapr_get_cpu_core_type(const char *model); > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data); > > #endif > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 8c1a437..00cfe9a 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -3125,8 +3125,7 @@ void spapr_core_release(DeviceState *dev) > > if (smc->pre_2_10_has_unused_icps) { > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc)); > > - const char *typename = object_class_get_name(scc->cpu_class); > > - size_t size = object_type_get_instance_size(typename); > > + size_t size = object_type_get_instance_size(scc->cpu_type); > > int i; > > > > for (i = 0; i < cc->nr_threads; i++) { > > @@ -3222,8 +3221,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > if (smc->pre_2_10_has_unused_icps) { > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc)); > > - const char *typename = object_class_get_name(scc->cpu_class); > > - size_t size = object_type_get_instance_size(typename); > > + size_t size = object_type_get_instance_size(scc->cpu_type); > > int i; > > > > for (i = 0; i < cc->nr_threads; i++) { > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index 5bea4c9..8a18eaf 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -104,8 +104,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp) > > { > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev)); > > - const char *typename = object_class_get_name(scc->cpu_class); > > - size_t size = object_type_get_instance_size(typename); > > + size_t size = object_type_get_instance_size(scc->cpu_type); > > CPUCore *cc = CPU_CORE(dev); > > int i; > > > > @@ -166,8 +165,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev)); > > CPUCore *cc = CPU_CORE(OBJECT(dev)); > > - const char *typename = object_class_get_name(scc->cpu_class); > > - size_t size = object_type_get_instance_size(typename); > > + size_t size = object_type_get_instance_size(scc->cpu_type); > > Error *local_err = NULL; > > void *obj; > > int i, j; > > @@ -186,7 +184,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > obj = sc->threads + i * size; > > > > - object_initialize(obj, size, typename); > > + object_initialize(obj, size, scc->cpu_type); > > cs = CPU(obj); > > cpu = POWERPC_CPU(cs); > > cs->cpu_index = cc->core_id + i; > > @@ -231,42 +229,12 @@ err: > > error_propagate(errp, local_err); > > } > > > > -static const char *spapr_core_models[] = { > > - /* 970 */ > > - "970_v2.2", > > - > > - /* 970MP variants */ > > - "970mp_v1.0", > > - "970mp_v1.1", > > - > > - /* POWER5+ */ > > - "power5+_v2.1", > > - > > - /* POWER7 */ > > - "power7_v2.3", > > - > > - /* POWER7+ */ > > - "power7+_v2.1", > > - > > - /* POWER8 */ > > - "power8_v2.0", > > - > > - /* POWER8E */ > > - "power8e_v2.1", > > - > > - /* POWER8NVL */ > > - "power8nvl_v1.0", > > - > > - /* POWER9 */ > > - "power9_v1.0", > > -}; > > - > > static Property spapr_cpu_core_properties[] = { > > DEFINE_PROP_INT32("node-id", sPAPRCPUCore, node_id, CPU_UNSET_NUMA_NODE_ID), > > DEFINE_PROP_END_OF_LIST() > > }; > > > > -void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(oc); > > sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc); > > @@ -274,36 +242,47 @@ void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > dc->realize = spapr_cpu_core_realize; > > dc->unrealize = spapr_cpu_core_unrealizefn; > > dc->props = spapr_cpu_core_properties; > > - scc->cpu_class = cpu_class_by_name(TYPE_POWERPC_CPU, data); > > - g_assert(scc->cpu_class); > > + scc->cpu_type = data; > > Maybe assert data isn't NULL ? I'll fix it up on respin
© 2016 - 2024 Red Hat, Inc.