Re: [Qemu-ppc] [RFC PATCH v0] spapr: Introduce sPAPRCPUCoreClass

[Cédric Le Goater]

On 09/02/2016 11:36 AM, Bharata B Rao wrote:
> Each spapr cpu core type defines an instance_init routine which just
> populates the CPU class name. This can be done in the class_init
> commonly for all core types which simplifies the registration.
> This is inspired by how PowerNV core types are registered.
> 
> Certain types of spapr cpu cores ('host' and generic type based on host
> CPU) are initialized in target-ppc/kvm.c. To convert these type
> registrations to use class_init, we need to expose
> spapr_cpu_core_class_init() outside of spapr_cpu_core.c.
> 
> Commit d11b268e1765 added a generic sPAPR CPU core family
> type to support cases like POWER8 CPU type on POWER8E host CPU.
> Switching to class_init would fix such scenarios to use the right
> CPU thread type instead of defaulting to host-powerpc64-cpu.
> 
> In an unrelated cleanup, fix a typo in .get_hotplug_handler routine.
> 
> Signed-off-by: Bharata B Rao <address@hidden>

yes. it is nicer without the SPAPR_CPU_CORE_INITFN() routine.

Reviewed-by: Cédric Le Goater <address@hidden>

C.

> ---
>  hw/ppc/spapr.c                  |   6 +--
>  hw/ppc/spapr_cpu_core.c         | 103 
> +++++++++++++++-------------------------
>  include/hw/ppc/spapr_cpu_core.h |  11 ++++-
>  target-ppc/kvm.c                |  22 +++------
>  4 files changed, 56 insertions(+), 86 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ca77bb0..79d36b3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2312,8 +2312,8 @@ static void 
> spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
>      }
>  }
>  
> -static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
> -                                             DeviceState *dev)
> +static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
> +                                                 DeviceState *dev)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
>          object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> @@ -2385,7 +2385,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> void *data)
>      mc->kvm_type = spapr_kvm_type;
>      mc->has_dynamic_sysbus = true;
>      mc->pci_allow_0_address = true;
> -    mc->get_hotplug_handler = spapr_get_hotpug_handler;
> +    mc->get_hotplug_handler = spapr_get_hotplug_handler;
>      hc->pre_plug = spapr_machine_device_pre_plug;
>      hc->plug = spapr_machine_device_plug;
>      hc->unplug = spapr_machine_device_unplug;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index bcb483d..1710908 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -112,7 +112,8 @@ char *spapr_get_cpu_core_type(const char *model)
>  static void spapr_core_release(DeviceState *dev, void *opaque)
>  {
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> -    const char *typename = object_class_get_name(sc->cpu_class);
> +    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);
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUCore *cc = CPU_CORE(dev);
> @@ -287,8 +288,9 @@ static void spapr_cpu_core_realize_child(Object *child, 
> Error **errp)
>  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(sc->cpu_class);
> +    const char *typename = object_class_get_name(scc->cpu_class);
>      size_t size = object_type_get_instance_size(typename);
>      Error *local_err = NULL;
>      void *obj;
> @@ -331,83 +333,43 @@ err:
>      error_propagate(errp, local_err);
>  }
>  
> -static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(oc);
> -    dc->realize = spapr_cpu_core_realize;
> -}
> -
> -/*
> - * instance_init routines from different flavours of sPAPR CPU cores.
> - */
> -#define SPAPR_CPU_CORE_INITFN(_type, _fname) \
> -static void glue(glue(spapr_cpu_core_, _fname), _initfn(Object *obj)) \
> -{ \
> -    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); \
> -    char *name = g_strdup_printf("%s-" TYPE_POWERPC_CPU, stringify(_type)); \
> -    ObjectClass *oc = object_class_by_name(name); \
> -    g_assert(oc); \
> -    g_free((void *)name); \
> -    core->cpu_class = oc; \
> -}
> -
> -SPAPR_CPU_CORE_INITFN(970mp_v1.0, 970MP_v10);
> -SPAPR_CPU_CORE_INITFN(970mp_v1.1, 970MP_v11);
> -SPAPR_CPU_CORE_INITFN(970_v2.2, 970);
> -SPAPR_CPU_CORE_INITFN(POWER5+_v2.1, POWER5plus);
> -SPAPR_CPU_CORE_INITFN(POWER7_v2.3, POWER7);
> -SPAPR_CPU_CORE_INITFN(POWER7+_v2.1, POWER7plus);
> -SPAPR_CPU_CORE_INITFN(POWER8_v2.0, POWER8);
> -SPAPR_CPU_CORE_INITFN(POWER8E_v2.1, POWER8E);
> -SPAPR_CPU_CORE_INITFN(POWER8NVL_v1.0, POWER8NVL);
> -
> -typedef struct SPAPRCoreInfo {
> -    const char *name;
> -    void (*initfn)(Object *obj);
> -} SPAPRCoreInfo;
> -
> -static const SPAPRCoreInfo spapr_cores[] = {
> +static const char *spapr_core_models[] = {
>      /* 970 */
> -    { .name = "970_v2.2", .initfn = spapr_cpu_core_970_initfn },
> +    "970_v2.2",
>  
>      /* 970MP variants */
> -    { .name = "970MP_v1.0", .initfn = spapr_cpu_core_970MP_v10_initfn },
> -    { .name = "970mp_v1.0", .initfn = spapr_cpu_core_970MP_v10_initfn },
> -    { .name = "970MP_v1.1", .initfn = spapr_cpu_core_970MP_v11_initfn },
> -    { .name = "970mp_v1.1", .initfn = spapr_cpu_core_970MP_v11_initfn },
> +    "970MP_v1.0",
> +    "970mp_v1.0",
> +    "970MP_v1.1",
> +    "970mp_v1.1",
>  
>      /* POWER5+ */
> -    { .name = "POWER5+_v2.1", .initfn = spapr_cpu_core_POWER5plus_initfn },
> +    "POWER5+_v2.1",
>  
>      /* POWER7 */
> -    { .name = "POWER7_v2.3", .initfn = spapr_cpu_core_POWER7_initfn },
> +    "POWER7_v2.3",
>  
>      /* POWER7+ */
> -    { .name = "POWER7+_v2.1", .initfn = spapr_cpu_core_POWER7plus_initfn },
> +    "POWER7+_v2.1",
>  
>      /* POWER8 */
> -    { .name = "POWER8_v2.0", .initfn = spapr_cpu_core_POWER8_initfn },
> +    "POWER8_v2.0",
>  
>      /* POWER8E */
> -    { .name = "POWER8E_v2.1", .initfn = spapr_cpu_core_POWER8E_initfn },
> +    "POWER8E_v2.1",
>  
>      /* POWER8NVL */
> -    { .name = "POWER8NVL_v1.0", .initfn = spapr_cpu_core_POWER8NVL_initfn },
> -
> -    { .name = NULL }
> +    "POWER8NVL_v1.0",
>  };
>  
> -static void spapr_cpu_core_register(const SPAPRCoreInfo *info)
> +void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
>  {
> -    TypeInfo type_info = {
> -        .parent = TYPE_SPAPR_CPU_CORE,
> -        .instance_size = sizeof(sPAPRCPUCore),
> -        .instance_init = info->initfn,
> -    };
> -
> -    type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, info->name);
> -    type_register(&type_info);
> -    g_free((void *)type_info.name);
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc);
> +
> +    dc->realize = spapr_cpu_core_realize;
> +    scc->cpu_class = cpu_class_by_name(TYPE_POWERPC_CPU, data);
> +    g_assert(scc->cpu_class);
>  }
>  
>  static const TypeInfo spapr_cpu_core_type_info = {
> @@ -415,17 +377,26 @@ static const TypeInfo spapr_cpu_core_type_info = {
>      .parent = TYPE_CPU_CORE,
>      .abstract = true,
>      .instance_size = sizeof(sPAPRCPUCore),
> -    .class_init = spapr_cpu_core_class_init,
>  };
>  
>  static void spapr_cpu_core_register_types(void)
>  {
> -    const SPAPRCoreInfo *info = spapr_cores;
> +    int i;
>  
>      type_register_static(&spapr_cpu_core_type_info);
> -    while (info->name) {
> -        spapr_cpu_core_register(info);
> -        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);
>      }
>  }
>  
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 1c9b319..283969b 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -16,6 +16,10 @@
>  #define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
>  #define SPAPR_CPU_CORE(obj) \
>      OBJECT_CHECK(sPAPRCPUCore, (obj), TYPE_SPAPR_CPU_CORE)
> +#define SPAPR_CPU_CORE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(sPAPRCPUCoreClass, (klass), TYPE_SPAPR_CPU_CORE)
> +#define SPAPR_CPU_CORE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(sPAPRCPUCoreClass, (obj), TYPE_SPAPR_CPU_CORE)
>  
>  typedef struct sPAPRCPUCore {
>      /*< private >*/
> @@ -23,9 +27,13 @@ typedef struct sPAPRCPUCore {
>  
>      /*< public >*/
>      void *threads;
> -    ObjectClass *cpu_class;
>  } sPAPRCPUCore;
>  
> +typedef struct sPAPRCPUCoreClass {
> +    DeviceClass parent_class;
> +    ObjectClass *cpu_class;
> +} sPAPRCPUCoreClass;
> +
>  void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                           Error **errp);
>  char *spapr_get_cpu_core_type(const char *model);
> @@ -33,4 +41,5 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, 
> DeviceState *dev,
>                       Error **errp);
>  void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                         Error **errp);
> +void spapr_cpu_core_class_init(ObjectClass *oc, void *data);
>  #endif
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index dcb68b9..4457337 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -36,6 +36,7 @@
>  #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"
> @@ -2364,19 +2365,6 @@ PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void)
>      return pvr_pcc;
>  }
>  
> -#if defined(TARGET_PPC64)
> -static void spapr_cpu_core_host_initfn(Object *obj)
> -{
> -    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> -    char *name = g_strdup_printf("%s-" TYPE_POWERPC_CPU, "host");
> -    ObjectClass *oc = object_class_by_name(name);
> -
> -    g_assert(oc);
> -    g_free((void *)name);
> -    core->cpu_class = oc;
> -}
> -#endif
> -
>  static int kvm_ppc_register_host_cpu_type(void)
>  {
>      TypeInfo type_info = {
> @@ -2404,14 +2392,16 @@ static int kvm_ppc_register_host_cpu_type(void)
>  #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 = spapr_cpu_core_host_initfn,
> -    type_info.class_init = NULL;
> +    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);
>  
>      /* Register generic spapr CPU family class for current host CPU type */
>      type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, dc->desc);
> +    type_info.class_data = (void *) dc->desc;
>      type_register(&type_info);
>      g_free((void *)type_info.name);
>  #endif
>