[Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code

Greg Kurz posted 1 patch 6 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/150633285374.14880.11614678065344980502.stgit@bahia.lan
Test checkpatch passed
Test docker passed
Test s390x passed
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(-)
[Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Greg Kurz 6 years, 6 months ago
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


Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Igor Mammedov 6 years, 6 months ago
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
> 
> 


Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Greg Kurz 6 years, 6 months ago
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.

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Greg Kurz 6 years, 6 months ago
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.

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Igor Mammedov 6 years, 6 months ago
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

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Greg Kurz 6 years, 6 months ago
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

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Igor Mammedov 6 years, 6 months ago
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?

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Greg Kurz 6 years, 6 months ago
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)

Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by David Gibson 6 years, 6 months ago
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
Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Greg Kurz 6 years, 6 months ago
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.
Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Igor Mammedov 6 years, 6 months ago
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
> > >     
> >   
> 
> 
> 


Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by David Gibson 6 years, 6 months ago
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
Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Igor Mammedov 6 years, 6 months ago
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).
> 


Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by David Gibson 6 years, 6 months ago
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
Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by David Gibson 6 years, 6 months ago
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
Re: [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code
Posted by Igor Mammedov 6 years, 6 months ago
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


[Qemu-devel] [RFC] ppc: define spapr core types statically
Posted by Igor Mammedov 6 years, 6 months ago
--
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


Re: [Qemu-devel] [RFC] ppc: define spapr core types statically
Posted by Greg Kurz 6 years, 6 months ago
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


Re: [Qemu-devel] [RFC] ppc: define spapr core types statically
Posted by David Gibson 6 years, 6 months ago
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
Re: [Qemu-devel] [RFC] ppc: define spapr core types statically
Posted by Igor Mammedov 6 years, 6 months ago
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