diff mbox

[RFC,v2.1,08/12] spapr: Add CPU type specific core devices

Message ID 1459413561-30745-9-git-send-email-bharata@linux.vnet.ibm.com
State New
Headers show

Commit Message

Bharata B Rao March 31, 2016, 8:39 a.m. UTC
Introduce core devices for each CPU type supported by sPAPR. These
core devices are derived from the base spapr-cpu-core device type.

TODO:
- Add core types for other remaining CPU types
- Handle CPU model alias correctly

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c                  |   3 +-
 hw/ppc/spapr_cpu_core.c         | 118 ++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |   1 +
 include/hw/ppc/spapr_cpu_core.h |  36 ++++++++++++
 4 files changed, 156 insertions(+), 2 deletions(-)

Comments

David Gibson April 1, 2016, 5:08 a.m. UTC | #1
On Thu, Mar 31, 2016 at 02:09:17PM +0530, Bharata B Rao wrote:
> Introduce core devices for each CPU type supported by sPAPR. These
> core devices are derived from the base spapr-cpu-core device type.
> 
> TODO:
> - Add core types for other remaining CPU types
> - Handle CPU model alias correctly
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c                  |   3 +-
>  hw/ppc/spapr_cpu_core.c         | 118 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |   1 +
>  include/hw/ppc/spapr_cpu_core.h |  36 ++++++++++++
>  4 files changed, 156 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 64c4acc..45ac5dc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1614,8 +1614,7 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
>  
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> -                           Error **errp)
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 8cbe2a5..3751a54 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -22,9 +22,127 @@ static const TypeInfo spapr_cpu_core_type_info = {
>      .instance_size = sizeof(sPAPRCPUCore),
>  };
>  
> +#define DEFINE_SPAPR_CPU_CORE(_name)                                           \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_create_threads)(DeviceState *dev, int threads,     \
> +    Error **errp)                                                              \
> +{                                                                              \
> +    int i;                                                                     \
> +    Error *local_err = NULL;                                                   \
> +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> +    glue(_name, sPAPRCPUCore) * core =                                         \
> +         glue(_name, _SPAPR_CPU_CORE)(OBJECT(dev));                            \
> +                                                                               \
> +    for (i = 0; i < threads; i++) {                                            \
> +        char id[32];                                                           \
> +                                                                               \
> +        object_initialize(&sc->threads[i], sizeof(sc->threads[i]),             \
> +                          object_class_get_name(core->cpu));                   \
> +        snprintf(id, sizeof(id), "thread[%d]", i);                             \
> +        object_property_add_child(OBJECT(core), id, OBJECT(&sc->threads[i]),   \
> +                                  &local_err);                                 \
> +        if (local_err) {                                                       \
> +            goto err;                                                          \
> +        }                                                                      \
> +    }                                                                          \
> +    return;                                                                    \
> +                                                                               \
> +err:                                                                           \
> +    while (--i) {                                                              \
> +        object_unparent(OBJECT(&sc->threads[i]));                              \
> +    }                                                                          \
> +    error_propagate(errp, local_err);                                          \
> +}                                                                              \
> +                                                                               \
> +static int                                                                     \
> +glue(_name, _spapr_cpu_core_realize_child)(Object *child, void *opaque)        \
> +{                                                                              \
> +    Error **errp = opaque;                                                     \
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());              \
> +    CPUState *cs = CPU(child);                                                 \
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);                                         \
> +                                                                               \
> +    object_property_set_bool(child, true, "realized", errp);                   \
> +    if (*errp) {                                                               \
> +        return 1;                                                              \
> +    }                                                                          \
> +                                                                               \
> +    spapr_cpu_init(spapr, cpu, errp);                                          \
> +    if (*errp) {                                                               \
> +        return 1;                                                              \
> +    }                                                                          \
> +    return 0;                                                                  \
> +}                                                                              \

If you put the ObjectClass * for the threads in the base abstract
class's class structure, then you can move most of this logic to the
abstract class as well and make the macro-ized stuff much smaller.

The realize_child stuff doesn't even need the ObjectClass* in the base
class to factor out.

> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_realize)(DeviceState *dev, Error **errp)           \
> +{                                                                              \
> +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> +    CPUCore *cc = CPU_CORE(OBJECT(dev));                                       \
> +    Error *local_err = NULL;                                                   \
> +                                                                               \
> +    /*                                                                         \
> +     * TODO: This is CPU model specific CPU core's realize routine.            \
> +     * However I am initializing "threads" field of the parent type            \
> +     * sPAPRCPUCore here. Is this ok ? If not I will have make "threads"       \
> +     * part of CPU model specific CPU core type and have different plug()      \
> +     * handlers for each type instead of having a common plug() handler        \
> +     * for all core types.                                                     \
> +     */                                                                        \
> +    sc->threads = g_new0(PowerPCCPU, cc->threads);                             \
> +    glue(_name, _spapr_cpu_core_create_threads)(dev, cc->threads, &local_err); \
> +    if (local_err) {                                                           \
> +        goto out;                                                              \
> +    }                                                                          \
> +                                                                               \
> +    object_child_foreach(OBJECT(dev),                                          \
> +                         glue(_name, _spapr_cpu_core_realize_child),           \
> +                         &local_err);                                          \
> +                                                                               \
> +out:                                                                           \
> +    if (local_err) {                                                           \
> +        g_free(sc->threads);                                                   \
> +        error_propagate(errp, local_err);                                      \
> +    }                                                                          \
> +}                                                                              \
> +                                                                               \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_instance_init)(Object *obj)                        \
> +{                                                                              \
> +    glue(_name, sPAPRCPUCore) * core = glue(_name, _SPAPR_CPU_CORE)(obj);      \
> +    const char *type = stringify(_name) "-" TYPE_POWERPC_CPU;                  \
> +    ObjectClass *oc = object_class_by_name(type);                              \
> +                                                                               \
> +    core->cpu = oc;                                                            \
> +}                                                                              \
> +                                                                               \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_class_init)(ObjectClass *oc, void *data)           \
> +{                                                                              \
> +                                                                               \
> +    DeviceClass *dc = DEVICE_CLASS(oc);                                        \
> +    dc->realize = glue(_name, _spapr_cpu_core_realize);                        \

I think the only callback you should need to construct in the macro is
class_init to initialize the ObjectClass* field.

> +}                                                                              \
> +                                                                               \
> +static const TypeInfo glue(_name, _spapr_cpu_core_type_info) =                 \
> +{                                                                              \
> +    .name = stringify(_name) "-" TYPE_SPAPR_CPU_CORE,                          \
> +    .parent = TYPE_SPAPR_CPU_CORE,                                             \
> +    .instance_size = sizeof(glue(_name, sPAPRCPUCore)),                        \
> +    .instance_init = glue(_name, _spapr_cpu_core_instance_init),               \
> +    .class_init = glue(_name, _spapr_cpu_core_class_init),                     \
> +};
> +
> +DEFINE_SPAPR_CPU_CORE(host);
> +DEFINE_SPAPR_CPU_CORE(POWER7);
> +DEFINE_SPAPR_CPU_CORE(POWER8);
> +
>  static void spapr_cpu_core_register_types(void)
>  {
>      type_register_static(&spapr_cpu_core_type_info);
> +    type_register_static(&host_spapr_cpu_core_type_info);
> +    type_register_static(&POWER7_spapr_cpu_core_type_info);
> +    type_register_static(&POWER8_spapr_cpu_core_type_info);
>  }
>  
>  type_init(spapr_cpu_core_register_types)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 098d85d..0fdf448 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -585,6 +585,7 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
>                                         uint32_t count);
>  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
>                                            uint32_t count);
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index e3340ea..71e69c0 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -24,4 +24,40 @@ typedef struct sPAPRCPUCore {
>      PowerPCCPU *threads;
>  } sPAPRCPUCore;
>  
> +#define TYPE_host_SPAPR_CPU_CORE "host-spapr-cpu-core"
> +#define host_SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(hostsPAPRCPUCore, (obj), TYPE_host_SPAPR_CPU_CORE)
> +
> +typedef struct hostsPAPRCPUCore {
> +    /*< private >*/
> +    sPAPRCPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *cpu;
> +} hostsPAPRCPUCore;
> +#define TYPE_POWER7_SPAPR_CPU_CORE "POWER7-spapr-cpu-core"
> +#define POWER7_SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(POWER7sPAPRCPUCore, (obj), TYPE_POWER7_SPAPR_CPU_CORE)
> +
> +typedef struct POWER7sPAPRCPUCore {
> +    /*< private >*/
> +    sPAPRCPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *cpu;
> +} POWER7sPAPRCPUCore;
> +
> +#define TYPE_POWER8_SPAPR_CPU_CORE "POWER8-spapr-cpu-core"
> +#define POWER8_SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(POWER8sPAPRCPUCore, (obj), TYPE_POWER8_SPAPR_CPU_CORE)
> +
> +typedef struct POWER8sPAPRCPUCore {
> +    /*< private >*/
> +    sPAPRCPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *cpu;
> +} POWER8sPAPRCPUCore;

These are all identical so should also be macro constructed as well.
I don't think there's actually any need for the structures to be
exposed in a header file either, so you should be able to do it in the
same macro that constructs the implementation.

Uh.. except if you move the ObjectClass* to the base class you won't
even need these.

>  #endif
Bharata B Rao April 1, 2016, 6:12 a.m. UTC | #2
On Fri, Apr 01, 2016 at 04:08:44PM +1100, David Gibson wrote:
> On Thu, Mar 31, 2016 at 02:09:17PM +0530, Bharata B Rao wrote:
> > Introduce core devices for each CPU type supported by sPAPR. These
> > core devices are derived from the base spapr-cpu-core device type.
> > 
> > TODO:
> > - Add core types for other remaining CPU types
> > - Handle CPU model alias correctly
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c                  |   3 +-
> >  hw/ppc/spapr_cpu_core.c         | 118 ++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h          |   1 +
> >  include/hw/ppc/spapr_cpu_core.h |  36 ++++++++++++
> >  4 files changed, 156 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 64c4acc..45ac5dc 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1614,8 +1614,7 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> >      machine->boot_order = g_strdup(boot_device);
> >  }
> >  
> > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > -                           Error **errp)
> > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> >  {
> >      CPUPPCState *env = &cpu->env;
> >  
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 8cbe2a5..3751a54 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -22,9 +22,127 @@ static const TypeInfo spapr_cpu_core_type_info = {
> >      .instance_size = sizeof(sPAPRCPUCore),
> >  };
> >  
> > +#define DEFINE_SPAPR_CPU_CORE(_name)                                           \
> > +static void                                                                    \
> > +glue(_name, _spapr_cpu_core_create_threads)(DeviceState *dev, int threads,     \
> > +    Error **errp)                                                              \
> > +{                                                                              \
> > +    int i;                                                                     \
> > +    Error *local_err = NULL;                                                   \
> > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> > +    glue(_name, sPAPRCPUCore) * core =                                         \
> > +         glue(_name, _SPAPR_CPU_CORE)(OBJECT(dev));                            \
> > +                                                                               \
> > +    for (i = 0; i < threads; i++) {                                            \
> > +        char id[32];                                                           \
> > +                                                                               \
> > +        object_initialize(&sc->threads[i], sizeof(sc->threads[i]),             \
> > +                          object_class_get_name(core->cpu));                   \
> > +        snprintf(id, sizeof(id), "thread[%d]", i);                             \
> > +        object_property_add_child(OBJECT(core), id, OBJECT(&sc->threads[i]),   \
> > +                                  &local_err);                                 \
> > +        if (local_err) {                                                       \
> > +            goto err;                                                          \
> > +        }                                                                      \
> > +    }                                                                          \
> > +    return;                                                                    \
> > +                                                                               \
> > +err:                                                                           \
> > +    while (--i) {                                                              \
> > +        object_unparent(OBJECT(&sc->threads[i]));                              \
> > +    }                                                                          \
> > +    error_propagate(errp, local_err);                                          \
> > +}                                                                              \
> > +                                                                               \
> > +static int                                                                     \
> > +glue(_name, _spapr_cpu_core_realize_child)(Object *child, void *opaque)        \
> > +{                                                                              \
> > +    Error **errp = opaque;                                                     \
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());              \
> > +    CPUState *cs = CPU(child);                                                 \
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);                                         \
> > +                                                                               \
> > +    object_property_set_bool(child, true, "realized", errp);                   \
> > +    if (*errp) {                                                               \
> > +        return 1;                                                              \
> > +    }                                                                          \
> > +                                                                               \
> > +    spapr_cpu_init(spapr, cpu, errp);                                          \
> > +    if (*errp) {                                                               \
> > +        return 1;                                                              \
> > +    }                                                                          \
> > +    return 0;                                                                  \
> > +}                                                                              \
> 
> If you put the ObjectClass * for the threads in the base abstract
> class's class structure, then you can move most of this logic to the
> abstract class as well and make the macro-ized stuff much smaller.
> 
> The realize_child stuff doesn't even need the ObjectClass* in the base
> class to factor out.
> 
> > +static void                                                                    \
> > +glue(_name, _spapr_cpu_core_realize)(DeviceState *dev, Error **errp)           \
> > +{                                                                              \
> > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> > +    CPUCore *cc = CPU_CORE(OBJECT(dev));                                       \
> > +    Error *local_err = NULL;                                                   \
> > +                                                                               \
> > +    /*                                                                         \
> > +     * TODO: This is CPU model specific CPU core's realize routine.            \
> > +     * However I am initializing "threads" field of the parent type            \
> > +     * sPAPRCPUCore here. Is this ok ? If not I will have make "threads"       \
> > +     * part of CPU model specific CPU core type and have different plug()      \
> > +     * handlers for each type instead of having a common plug() handler        \
> > +     * for all core types.                                                     \
> > +     */                                                                        \
> > +    sc->threads = g_new0(PowerPCCPU, cc->threads);                             \
> > +    glue(_name, _spapr_cpu_core_create_threads)(dev, cc->threads, &local_err); \
> > +    if (local_err) {                                                           \
> > +        goto out;                                                              \
> > +    }                                                                          \
> > +                                                                               \
> > +    object_child_foreach(OBJECT(dev),                                          \
> > +                         glue(_name, _spapr_cpu_core_realize_child),           \
> > +                         &local_err);                                          \
> > +                                                                               \
> > +out:                                                                           \
> > +    if (local_err) {                                                           \
> > +        g_free(sc->threads);                                                   \
> > +        error_propagate(errp, local_err);                                      \
> > +    }                                                                          \
> > +}                                                                              \
> > +                                                                               \
> > +static void                                                                    \
> > +glue(_name, _spapr_cpu_core_instance_init)(Object *obj)                        \
> > +{                                                                              \
> > +    glue(_name, sPAPRCPUCore) * core = glue(_name, _SPAPR_CPU_CORE)(obj);      \
> > +    const char *type = stringify(_name) "-" TYPE_POWERPC_CPU;                  \
> > +    ObjectClass *oc = object_class_by_name(type);                              \
> > +                                                                               \
> > +    core->cpu = oc;                                                            \
> > +}                                                                              \
> > +                                                                               \
> > +static void                                                                    \
> > +glue(_name, _spapr_cpu_core_class_init)(ObjectClass *oc, void *data)           \
> > +{                                                                              \
> > +                                                                               \
> > +    DeviceClass *dc = DEVICE_CLASS(oc);                                        \
> > +    dc->realize = glue(_name, _spapr_cpu_core_realize);                        \
> 
> I think the only callback you should need to construct in the macro is
> class_init to initialize the ObjectClass* field.
> 
> > +}                                                                              \
> > +                                                                               \
> > +static const TypeInfo glue(_name, _spapr_cpu_core_type_info) =                 \
> > +{                                                                              \
> > +    .name = stringify(_name) "-" TYPE_SPAPR_CPU_CORE,                          \
> > +    .parent = TYPE_SPAPR_CPU_CORE,                                             \
> > +    .instance_size = sizeof(glue(_name, sPAPRCPUCore)),                        \
> > +    .instance_init = glue(_name, _spapr_cpu_core_instance_init),               \
> > +    .class_init = glue(_name, _spapr_cpu_core_class_init),                     \
> > +};
> > +
> > +DEFINE_SPAPR_CPU_CORE(host);
> > +DEFINE_SPAPR_CPU_CORE(POWER7);
> > +DEFINE_SPAPR_CPU_CORE(POWER8);
> > +
> >  static void spapr_cpu_core_register_types(void)
> >  {
> >      type_register_static(&spapr_cpu_core_type_info);
> > +    type_register_static(&host_spapr_cpu_core_type_info);
> > +    type_register_static(&POWER7_spapr_cpu_core_type_info);
> > +    type_register_static(&POWER8_spapr_cpu_core_type_info);
> >  }
> >  
> >  type_init(spapr_cpu_core_register_types)
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 098d85d..0fdf448 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -585,6 +585,7 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
> >                                         uint32_t count);
> >  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
> >                                            uint32_t count);
> > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
> >  
> >  /* rtas-configure-connector state */
> >  struct sPAPRConfigureConnectorState {
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index e3340ea..71e69c0 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -24,4 +24,40 @@ typedef struct sPAPRCPUCore {
> >      PowerPCCPU *threads;
> >  } sPAPRCPUCore;
> >  
> > +#define TYPE_host_SPAPR_CPU_CORE "host-spapr-cpu-core"
> > +#define host_SPAPR_CPU_CORE(obj) \
> > +    OBJECT_CHECK(hostsPAPRCPUCore, (obj), TYPE_host_SPAPR_CPU_CORE)
> > +
> > +typedef struct hostsPAPRCPUCore {
> > +    /*< private >*/
> > +    sPAPRCPUCore parent_obj;
> > +
> > +    /*< public >*/
> > +    ObjectClass *cpu;
> > +} hostsPAPRCPUCore;
> > +#define TYPE_POWER7_SPAPR_CPU_CORE "POWER7-spapr-cpu-core"
> > +#define POWER7_SPAPR_CPU_CORE(obj) \
> > +    OBJECT_CHECK(POWER7sPAPRCPUCore, (obj), TYPE_POWER7_SPAPR_CPU_CORE)
> > +
> > +typedef struct POWER7sPAPRCPUCore {
> > +    /*< private >*/
> > +    sPAPRCPUCore parent_obj;
> > +
> > +    /*< public >*/
> > +    ObjectClass *cpu;
> > +} POWER7sPAPRCPUCore;
> > +
> > +#define TYPE_POWER8_SPAPR_CPU_CORE "POWER8-spapr-cpu-core"
> > +#define POWER8_SPAPR_CPU_CORE(obj) \
> > +    OBJECT_CHECK(POWER8sPAPRCPUCore, (obj), TYPE_POWER8_SPAPR_CPU_CORE)
> > +
> > +typedef struct POWER8sPAPRCPUCore {
> > +    /*< private >*/
> > +    sPAPRCPUCore parent_obj;
> > +
> > +    /*< public >*/
> > +    ObjectClass *cpu;
> > +} POWER8sPAPRCPUCore;
> 
> These are all identical so should also be macro constructed as well.
> I don't think there's actually any need for the structures to be
> exposed in a header file either, so you should be able to do it in the
> same macro that constructs the implementation.
> 
> Uh.. except if you move the ObjectClass* to the base class you won't
> even need these.

The only reason (currently) POWER8sPAPRCPUCore exists separately from
the base class sPAPRCPUCore is that it represents POWER8 core which
is stored as ObjectClass*.

Now if we don't track cpu type (ObjectClass *) as part of
POWER8sPAPRCPUCore but push that up to sPAPRCPUCore, I am not
sure if that would be at the right abstraction level.

Apart from the above, can you also look at the TODO I have put on the top
of _spapr_cpu_core_create_threads() routine in this patch ? I am not sure
if what I have done there is the right thing to do.

Regards,
Bharata.
David Gibson April 4, 2016, 12:13 a.m. UTC | #3
On Fri, Apr 01, 2016 at 11:42:23AM +0530, Bharata B Rao wrote:
> On Fri, Apr 01, 2016 at 04:08:44PM +1100, David Gibson wrote:
> > On Thu, Mar 31, 2016 at 02:09:17PM +0530, Bharata B Rao wrote:
> > > Introduce core devices for each CPU type supported by sPAPR. These
> > > core devices are derived from the base spapr-cpu-core device type.
> > > 
> > > TODO:
> > > - Add core types for other remaining CPU types
> > > - Handle CPU model alias correctly
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c                  |   3 +-
> > >  hw/ppc/spapr_cpu_core.c         | 118 ++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h          |   1 +
> > >  include/hw/ppc/spapr_cpu_core.h |  36 ++++++++++++
> > >  4 files changed, 156 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 64c4acc..45ac5dc 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1614,8 +1614,7 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> > >      machine->boot_order = g_strdup(boot_device);
> > >  }
> > >  
> > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > -                           Error **errp)
> > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > >  {
> > >      CPUPPCState *env = &cpu->env;
> > >  
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 8cbe2a5..3751a54 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -22,9 +22,127 @@ static const TypeInfo spapr_cpu_core_type_info = {
> > >      .instance_size = sizeof(sPAPRCPUCore),
> > >  };
> > >  
> > > +#define DEFINE_SPAPR_CPU_CORE(_name)                                           \
> > > +static void                                                                    \
> > > +glue(_name, _spapr_cpu_core_create_threads)(DeviceState *dev, int threads,     \
> > > +    Error **errp)                                                              \
> > > +{                                                                              \
> > > +    int i;                                                                     \
> > > +    Error *local_err = NULL;                                                   \
> > > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> > > +    glue(_name, sPAPRCPUCore) * core =                                         \
> > > +         glue(_name, _SPAPR_CPU_CORE)(OBJECT(dev));                            \
> > > +                                                                               \
> > > +    for (i = 0; i < threads; i++) {                                            \
> > > +        char id[32];                                                           \
> > > +                                                                               \
> > > +        object_initialize(&sc->threads[i], sizeof(sc->threads[i]),             \
> > > +                          object_class_get_name(core->cpu));                   \
> > > +        snprintf(id, sizeof(id), "thread[%d]", i);                             \
> > > +        object_property_add_child(OBJECT(core), id, OBJECT(&sc->threads[i]),   \
> > > +                                  &local_err);                                 \
> > > +        if (local_err) {                                                       \
> > > +            goto err;                                                          \
> > > +        }                                                                      \
> > > +    }                                                                          \
> > > +    return;                                                                    \
> > > +                                                                               \
> > > +err:                                                                           \
> > > +    while (--i) {                                                              \
> > > +        object_unparent(OBJECT(&sc->threads[i]));                              \
> > > +    }                                                                          \
> > > +    error_propagate(errp, local_err);                                          \
> > > +}                                                                              \
> > > +                                                                               \
> > > +static int                                                                     \
> > > +glue(_name, _spapr_cpu_core_realize_child)(Object *child, void *opaque)        \
> > > +{                                                                              \
> > > +    Error **errp = opaque;                                                     \
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());              \
> > > +    CPUState *cs = CPU(child);                                                 \
> > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);                                         \
> > > +                                                                               \
> > > +    object_property_set_bool(child, true, "realized", errp);                   \
> > > +    if (*errp) {                                                               \
> > > +        return 1;                                                              \
> > > +    }                                                                          \
> > > +                                                                               \
> > > +    spapr_cpu_init(spapr, cpu, errp);                                          \
> > > +    if (*errp) {                                                               \
> > > +        return 1;                                                              \
> > > +    }                                                                          \
> > > +    return 0;                                                                  \
> > > +}                                                                              \
> > 
> > If you put the ObjectClass * for the threads in the base abstract
> > class's class structure, then you can move most of this logic to the
> > abstract class as well and make the macro-ized stuff much smaller.
> > 
> > The realize_child stuff doesn't even need the ObjectClass* in the base
> > class to factor out.
> > 
> > > +static void                                                                    \
> > > +glue(_name, _spapr_cpu_core_realize)(DeviceState *dev, Error **errp)           \
> > > +{                                                                              \
> > > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> > > +    CPUCore *cc = CPU_CORE(OBJECT(dev));                                       \
> > > +    Error *local_err = NULL;                                                   \
> > > +                                                                               \
> > > +    /*                                                                         \
> > > +     * TODO: This is CPU model specific CPU core's realize routine.            \
> > > +     * However I am initializing "threads" field of the parent type            \
> > > +     * sPAPRCPUCore here. Is this ok ? If not I will have make "threads"       \
> > > +     * part of CPU model specific CPU core type and have different plug()      \
> > > +     * handlers for each type instead of having a common plug() handler        \
> > > +     * for all core types.                                                     \
> > > +     */                                                                        \
> > > +    sc->threads = g_new0(PowerPCCPU, cc->threads);                             \
> > > +    glue(_name, _spapr_cpu_core_create_threads)(dev, cc->threads, &local_err); \
> > > +    if (local_err) {                                                           \
> > > +        goto out;                                                              \
> > > +    }                                                                          \
> > > +                                                                               \
> > > +    object_child_foreach(OBJECT(dev),                                          \
> > > +                         glue(_name, _spapr_cpu_core_realize_child),           \
> > > +                         &local_err);                                          \
> > > +                                                                               \
> > > +out:                                                                           \
> > > +    if (local_err) {                                                           \
> > > +        g_free(sc->threads);                                                   \
> > > +        error_propagate(errp, local_err);                                      \
> > > +    }                                                                          \
> > > +}                                                                              \
> > > +                                                                               \
> > > +static void                                                                    \
> > > +glue(_name, _spapr_cpu_core_instance_init)(Object *obj)                        \
> > > +{                                                                              \
> > > +    glue(_name, sPAPRCPUCore) * core = glue(_name, _SPAPR_CPU_CORE)(obj);      \
> > > +    const char *type = stringify(_name) "-" TYPE_POWERPC_CPU;                  \
> > > +    ObjectClass *oc = object_class_by_name(type);                              \
> > > +                                                                               \
> > > +    core->cpu = oc;                                                            \
> > > +}                                                                              \
> > > +                                                                               \
> > > +static void                                                                    \
> > > +glue(_name, _spapr_cpu_core_class_init)(ObjectClass *oc, void *data)           \
> > > +{                                                                              \
> > > +                                                                               \
> > > +    DeviceClass *dc = DEVICE_CLASS(oc);                                        \
> > > +    dc->realize = glue(_name, _spapr_cpu_core_realize);                        \
> > 
> > I think the only callback you should need to construct in the macro is
> > class_init to initialize the ObjectClass* field.
> > 
> > > +}                                                                              \
> > > +                                                                               \
> > > +static const TypeInfo glue(_name, _spapr_cpu_core_type_info) =                 \
> > > +{                                                                              \
> > > +    .name = stringify(_name) "-" TYPE_SPAPR_CPU_CORE,                          \
> > > +    .parent = TYPE_SPAPR_CPU_CORE,                                             \
> > > +    .instance_size = sizeof(glue(_name, sPAPRCPUCore)),                        \
> > > +    .instance_init = glue(_name, _spapr_cpu_core_instance_init),               \
> > > +    .class_init = glue(_name, _spapr_cpu_core_class_init),                     \
> > > +};
> > > +
> > > +DEFINE_SPAPR_CPU_CORE(host);
> > > +DEFINE_SPAPR_CPU_CORE(POWER7);
> > > +DEFINE_SPAPR_CPU_CORE(POWER8);
> > > +
> > >  static void spapr_cpu_core_register_types(void)
> > >  {
> > >      type_register_static(&spapr_cpu_core_type_info);
> > > +    type_register_static(&host_spapr_cpu_core_type_info);
> > > +    type_register_static(&POWER7_spapr_cpu_core_type_info);
> > > +    type_register_static(&POWER8_spapr_cpu_core_type_info);
> > >  }
> > >  
> > >  type_init(spapr_cpu_core_register_types)
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index 098d85d..0fdf448 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -585,6 +585,7 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
> > >                                         uint32_t count);
> > >  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
> > >                                            uint32_t count);
> > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
> > >  
> > >  /* rtas-configure-connector state */
> > >  struct sPAPRConfigureConnectorState {
> > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > > index e3340ea..71e69c0 100644
> > > --- a/include/hw/ppc/spapr_cpu_core.h
> > > +++ b/include/hw/ppc/spapr_cpu_core.h
> > > @@ -24,4 +24,40 @@ typedef struct sPAPRCPUCore {
> > >      PowerPCCPU *threads;
> > >  } sPAPRCPUCore;
> > >  
> > > +#define TYPE_host_SPAPR_CPU_CORE "host-spapr-cpu-core"
> > > +#define host_SPAPR_CPU_CORE(obj) \
> > > +    OBJECT_CHECK(hostsPAPRCPUCore, (obj), TYPE_host_SPAPR_CPU_CORE)
> > > +
> > > +typedef struct hostsPAPRCPUCore {
> > > +    /*< private >*/
> > > +    sPAPRCPUCore parent_obj;
> > > +
> > > +    /*< public >*/
> > > +    ObjectClass *cpu;
> > > +} hostsPAPRCPUCore;
> > > +#define TYPE_POWER7_SPAPR_CPU_CORE "POWER7-spapr-cpu-core"
> > > +#define POWER7_SPAPR_CPU_CORE(obj) \
> > > +    OBJECT_CHECK(POWER7sPAPRCPUCore, (obj), TYPE_POWER7_SPAPR_CPU_CORE)
> > > +
> > > +typedef struct POWER7sPAPRCPUCore {
> > > +    /*< private >*/
> > > +    sPAPRCPUCore parent_obj;
> > > +
> > > +    /*< public >*/
> > > +    ObjectClass *cpu;
> > > +} POWER7sPAPRCPUCore;
> > > +
> > > +#define TYPE_POWER8_SPAPR_CPU_CORE "POWER8-spapr-cpu-core"
> > > +#define POWER8_SPAPR_CPU_CORE(obj) \
> > > +    OBJECT_CHECK(POWER8sPAPRCPUCore, (obj), TYPE_POWER8_SPAPR_CPU_CORE)
> > > +
> > > +typedef struct POWER8sPAPRCPUCore {
> > > +    /*< private >*/
> > > +    sPAPRCPUCore parent_obj;
> > > +
> > > +    /*< public >*/
> > > +    ObjectClass *cpu;
> > > +} POWER8sPAPRCPUCore;
> > 
> > These are all identical so should also be macro constructed as well.
> > I don't think there's actually any need for the structures to be
> > exposed in a header file either, so you should be able to do it in the
> > same macro that constructs the implementation.
> > 
> > Uh.. except if you move the ObjectClass* to the base class you won't
> > even need these.
> 
> The only reason (currently) POWER8sPAPRCPUCore exists separately from
> the base class sPAPRCPUCore is that it represents POWER8 core which
> is stored as ObjectClass*.
> 
> Now if we don't track cpu type (ObjectClass *) as part of
> POWER8sPAPRCPUCore but push that up to sPAPRCPUCore, I am not
> sure if that would be at the right abstraction level.

Not as part of sPAPRCPUCore, but as part of sPAPRCPUCoreClass.  We
don't have a structure for the class at present, but we can add one.
You can think of it as a method that sPAPRCPUCore and subclasses have
which returns the correct ObjectClass *, except that we don't actually
need a method (function pointer) - a simple data pointer in the class
will suffice.
David Gibson April 4, 2016, 12:16 a.m. UTC | #4
On Thu, Mar 31, 2016 at 02:09:17PM +0530, Bharata B Rao wrote:
> Introduce core devices for each CPU type supported by sPAPR. These
> core devices are derived from the base spapr-cpu-core device type.
> 
> TODO:
> - Add core types for other remaining CPU types
> - Handle CPU model alias correctly
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c                  |   3 +-
>  hw/ppc/spapr_cpu_core.c         | 118 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |   1 +
>  include/hw/ppc/spapr_cpu_core.h |  36 ++++++++++++
>  4 files changed, 156 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 64c4acc..45ac5dc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1614,8 +1614,7 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
>  
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> -                           Error **errp)
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 8cbe2a5..3751a54 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -22,9 +22,127 @@ static const TypeInfo spapr_cpu_core_type_info = {
>      .instance_size = sizeof(sPAPRCPUCore),
>  };
>  
> +#define DEFINE_SPAPR_CPU_CORE(_name)                                           \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_create_threads)(DeviceState *dev, int threads,     \
> +    Error **errp)                                                              \
> +{                                                                              \
> +    int i;                                                                     \
> +    Error *local_err = NULL;                                                   \
> +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> +    glue(_name, sPAPRCPUCore) * core =                                         \
> +         glue(_name, _SPAPR_CPU_CORE)(OBJECT(dev));                            \
> +                                                                               \
> +    for (i = 0; i < threads; i++) {                                            \
> +        char id[32];                                                           \
> +                                                                               \
> +        object_initialize(&sc->threads[i], sizeof(sc->threads[i]),             \
> +                          object_class_get_name(core->cpu));                   \
> +        snprintf(id, sizeof(id), "thread[%d]", i);                             \
> +        object_property_add_child(OBJECT(core), id, OBJECT(&sc->threads[i]),   \
> +                                  &local_err);                                 \
> +        if (local_err) {                                                       \
> +            goto err;                                                          \
> +        }                                                                      \
> +    }                                                                          \
> +    return;                                                                    \
> +                                                                               \
> +err:                                                                           \
> +    while (--i) {                                                              \
> +        object_unparent(OBJECT(&sc->threads[i]));                              \
> +    }                                                                          \
> +    error_propagate(errp, local_err);                                          \
> +}                                                                              \
> +                                                                               \
> +static int                                                                     \
> +glue(_name, _spapr_cpu_core_realize_child)(Object *child, void *opaque)        \
> +{                                                                              \
> +    Error **errp = opaque;                                                     \
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());              \
> +    CPUState *cs = CPU(child);                                                 \
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);                                         \
> +                                                                               \
> +    object_property_set_bool(child, true, "realized", errp);                   \
> +    if (*errp) {                                                               \
> +        return 1;                                                              \
> +    }                                                                          \
> +                                                                               \
> +    spapr_cpu_init(spapr, cpu, errp);                                          \
> +    if (*errp) {                                                               \
> +        return 1;                                                              \
> +    }                                                                          \
> +    return 0;                                                                  \
> +}                                                                              \
> +                                                                               \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_realize)(DeviceState *dev, Error **errp)           \
> +{                                                                              \
> +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> +    CPUCore *cc = CPU_CORE(OBJECT(dev));                                       \
> +    Error *local_err = NULL;                                                   \
> +                                                                               \
> +    /*                                                                         \
> +     * TODO: This is CPU model specific CPU core's realize routine.            \
> +     * However I am initializing "threads" field of the parent type            \
> +     * sPAPRCPUCore here. Is this ok ? If not I will have make "threads"       \
> +     * part of CPU model specific CPU core type and have different plug()      \
> +     * handlers for each type instead of having a common plug() handler        \
> +     * for all core types.                                                     \
> +     */
> \


I think that should be fine - it's generally ok for methods to
manipulate superclass data fields (technically this is what C++ calls
a "protected" as opposed to "private" field).

> +    sc->threads = g_new0(PowerPCCPU, cc->threads);                             \
> +    glue(_name, _spapr_cpu_core_create_threads)(dev, cc->threads, &local_err); \
> +    if (local_err) {                                                           \
> +        goto out;                                                              \
> +    }                                                                          \
> +                                                                               \
> +    object_child_foreach(OBJECT(dev),                                          \
> +                         glue(_name, _spapr_cpu_core_realize_child),           \
> +                         &local_err);                                          \
> +                                                                               \
> +out:                                                                           \
> +    if (local_err) {                                                           \
> +        g_free(sc->threads);                                                   \
> +        error_propagate(errp, local_err);                                      \
> +    }                                                                          \
> +}                                                                              \
> +                                                                               \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_instance_init)(Object *obj)                        \
> +{                                                                              \
> +    glue(_name, sPAPRCPUCore) * core = glue(_name, _SPAPR_CPU_CORE)(obj);      \
> +    const char *type = stringify(_name) "-" TYPE_POWERPC_CPU;                  \
> +    ObjectClass *oc = object_class_by_name(type);                              \
> +                                                                               \
> +    core->cpu = oc;                                                            \
> +}                                                                              \
> +                                                                               \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_class_init)(ObjectClass *oc, void *data)           \
> +{                                                                              \
> +                                                                               \
> +    DeviceClass *dc = DEVICE_CLASS(oc);                                        \
> +    dc->realize = glue(_name, _spapr_cpu_core_realize);                        \
> +}                                                                              \
> +                                                                               \
> +static const TypeInfo glue(_name, _spapr_cpu_core_type_info) =                 \
> +{                                                                              \
> +    .name = stringify(_name) "-" TYPE_SPAPR_CPU_CORE,                          \
> +    .parent = TYPE_SPAPR_CPU_CORE,                                             \
> +    .instance_size = sizeof(glue(_name, sPAPRCPUCore)),                        \
> +    .instance_init = glue(_name, _spapr_cpu_core_instance_init),               \
> +    .class_init = glue(_name, _spapr_cpu_core_class_init),                     \
> +};
> +
> +DEFINE_SPAPR_CPU_CORE(host);
> +DEFINE_SPAPR_CPU_CORE(POWER7);
> +DEFINE_SPAPR_CPU_CORE(POWER8);
> +
>  static void spapr_cpu_core_register_types(void)
>  {
>      type_register_static(&spapr_cpu_core_type_info);
> +    type_register_static(&host_spapr_cpu_core_type_info);
> +    type_register_static(&POWER7_spapr_cpu_core_type_info);
> +    type_register_static(&POWER8_spapr_cpu_core_type_info);
>  }
>  
>  type_init(spapr_cpu_core_register_types)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 098d85d..0fdf448 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -585,6 +585,7 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
>                                         uint32_t count);
>  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
>                                            uint32_t count);
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index e3340ea..71e69c0 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -24,4 +24,40 @@ typedef struct sPAPRCPUCore {
>      PowerPCCPU *threads;
>  } sPAPRCPUCore;
>  
> +#define TYPE_host_SPAPR_CPU_CORE "host-spapr-cpu-core"
> +#define host_SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(hostsPAPRCPUCore, (obj), TYPE_host_SPAPR_CPU_CORE)
> +
> +typedef struct hostsPAPRCPUCore {
> +    /*< private >*/
> +    sPAPRCPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *cpu;
> +} hostsPAPRCPUCore;
> +
> +#define TYPE_POWER7_SPAPR_CPU_CORE "POWER7-spapr-cpu-core"
> +#define POWER7_SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(POWER7sPAPRCPUCore, (obj), TYPE_POWER7_SPAPR_CPU_CORE)
> +
> +typedef struct POWER7sPAPRCPUCore {
> +    /*< private >*/
> +    sPAPRCPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *cpu;
> +} POWER7sPAPRCPUCore;
> +
> +#define TYPE_POWER8_SPAPR_CPU_CORE "POWER8-spapr-cpu-core"
> +#define POWER8_SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(POWER8sPAPRCPUCore, (obj), TYPE_POWER8_SPAPR_CPU_CORE)
> +
> +typedef struct POWER8sPAPRCPUCore {
> +    /*< private >*/
> +    sPAPRCPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *cpu;
> +} POWER8sPAPRCPUCore;
> +
>  #endif
Michael Roth April 8, 2016, 11:35 p.m. UTC | #5
Quoting Bharata B Rao (2016-03-31 03:39:17)
> Introduce core devices for each CPU type supported by sPAPR. These
> core devices are derived from the base spapr-cpu-core device type.
> 
> TODO:
> - Add core types for other remaining CPU types
> - Handle CPU model alias correctly
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c                  |   3 +-
>  hw/ppc/spapr_cpu_core.c         | 118 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |   1 +
>  include/hw/ppc/spapr_cpu_core.h |  36 ++++++++++++
>  4 files changed, 156 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 64c4acc..45ac5dc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1614,8 +1614,7 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
> 
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> -                           Error **errp)
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 8cbe2a5..3751a54 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -22,9 +22,127 @@ static const TypeInfo spapr_cpu_core_type_info = {
>      .instance_size = sizeof(sPAPRCPUCore),
>  };
> 
> +#define DEFINE_SPAPR_CPU_CORE(_name)                                           \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_create_threads)(DeviceState *dev, int threads,     \
> +    Error **errp)                                                              \
> +{                                                                              \
> +    int i;                                                                     \
> +    Error *local_err = NULL;                                                   \
> +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> +    glue(_name, sPAPRCPUCore) * core =                                         \
> +         glue(_name, _SPAPR_CPU_CORE)(OBJECT(dev));                            \
> +                                                                               \
> +    for (i = 0; i < threads; i++) {                                            \
> +        char id[32];                                                           \
> +                                                                               \
> +        object_initialize(&sc->threads[i], sizeof(sc->threads[i]),             \
> +                          object_class_get_name(core->cpu));                   \
> +        snprintf(id, sizeof(id), "thread[%d]", i);                             \
> +        object_property_add_child(OBJECT(core), id, OBJECT(&sc->threads[i]),   \
> +                                  &local_err);                                 \
> +        if (local_err) {                                                       \
> +            goto err;                                                          \
> +        }                                                                      \
> +    }                                                                          \
> +    return;                                                                    \
> +                                                                               \
> +err:                                                                           \
> +    while (--i) {                                                              \
> +        object_unparent(OBJECT(&sc->threads[i]));                              \
> +    }                                                                          \
> +    error_propagate(errp, local_err);                                          \
> +}                                                                              \
> +                                                                               \
> +static int                                                                     \
> +glue(_name, _spapr_cpu_core_realize_child)(Object *child, void *opaque)        \
> +{                                                                              \
> +    Error **errp = opaque;                                                     \
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());              \
> +    CPUState *cs = CPU(child);                                                 \
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);                                         \
> +                                                                               \
> +    object_property_set_bool(child, true, "realized", errp);                   \
> +    if (*errp) {                                                               \
> +        return 1;                                                              \
> +    }                                                                          \
> +                                                                               \
> +    spapr_cpu_init(spapr, cpu, errp);                                          \
> +    if (*errp) {                                                               \
> +        return 1;                                                              \
> +    }                                                                          \
> +    return 0;                                                                  \
> +}                                                                              \
> +                                                                               \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_realize)(DeviceState *dev, Error **errp)           \
> +{                                                                              \
> +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> +    CPUCore *cc = CPU_CORE(OBJECT(dev));                                       \
> +    Error *local_err = NULL;                                                   \
> +                                                                               \
> +    /*                                                                         \
> +     * TODO: This is CPU model specific CPU core's realize routine.            \
> +     * However I am initializing "threads" field of the parent type            \
> +     * sPAPRCPUCore here. Is this ok ? If not I will have make "threads"       \
> +     * part of CPU model specific CPU core type and have different plug()      \
> +     * handlers for each type instead of having a common plug() handler        \
> +     * for all core types.                                                     \
> +     */                                                                        \

Not sure, but there seems to be precedence for descendants of CPUClass
at least. e.g. arm_cpu_realizefn() modifies it's parent CPUState.

But do we even need a model-specific realize? It seems like once
core->cpu is set via the model's instance_init, the same realize /
create_threads functions can be shared by everyone thanks to
object_class_get_name(). So maybe we could move ObjectClass *cpu up into
sPAPRCPUCore, and use a common realize set via spapr_cpu_core_class_init?

If that's doable, we could drop all the macros, or at least limit them
to basically just a model-specific instance_init that sets
sPAPRCPUCore->cpu. You might not even need typedefs for the different
models at that point, since instance_size == sizeof(sPAPRCPUCore) for
everyone at that point. Currently, at least.

In fact, arm_cpu_register_types() seems to do something very similar
for registering TypeInfos for tuples of (model name, init fn).

> +    sc->threads = g_new0(PowerPCCPU, cc->threads);                             \
> +    glue(_name, _spapr_cpu_core_create_threads)(dev, cc->threads, &local_err); \
> +    if (local_err) {                                                           \
> +        goto out;                                                              \
> +    }                                                                          \
> +                                                                               \
> +    object_child_foreach(OBJECT(dev),                                          \
> +                         glue(_name, _spapr_cpu_core_realize_child),           \
> +                         &local_err);                                          \
> +                                                                               \
> +out:                                                                           \
> +    if (local_err) {                                                           \
> +        g_free(sc->threads);                                                   \
> +        error_propagate(errp, local_err);                                      \
> +    }                                                                          \
> +}                                                                              \
> +                                                                               \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_instance_init)(Object *obj)                        \
> +{                                                                              \
> +    glue(_name, sPAPRCPUCore) * core = glue(_name, _SPAPR_CPU_CORE)(obj);      \
> +    const char *type = stringify(_name) "-" TYPE_POWERPC_CPU;                  \
> +    ObjectClass *oc = object_class_by_name(type);                              \
> +                                                                               \
> +    core->cpu = oc;                                                            \
> +}                                                                              \
> +                                                                               \
> +static void                                                                    \
> +glue(_name, _spapr_cpu_core_class_init)(ObjectClass *oc, void *data)           \
> +{                                                                              \
> +                                                                               \
> +    DeviceClass *dc = DEVICE_CLASS(oc);                                        \
> +    dc->realize = glue(_name, _spapr_cpu_core_realize);                        \
> +}                                                                              \
> +                                                                               \
> +static const TypeInfo glue(_name, _spapr_cpu_core_type_info) =                 \
> +{                                                                              \
> +    .name = stringify(_name) "-" TYPE_SPAPR_CPU_CORE,                          \
> +    .parent = TYPE_SPAPR_CPU_CORE,                                             \
> +    .instance_size = sizeof(glue(_name, sPAPRCPUCore)),                        \
> +    .instance_init = glue(_name, _spapr_cpu_core_instance_init),               \
> +    .class_init = glue(_name, _spapr_cpu_core_class_init),                     \
> +};
> +
> +DEFINE_SPAPR_CPU_CORE(host);
> +DEFINE_SPAPR_CPU_CORE(POWER7);
> +DEFINE_SPAPR_CPU_CORE(POWER8);
> +
>  static void spapr_cpu_core_register_types(void)
>  {
>      type_register_static(&spapr_cpu_core_type_info);
> +    type_register_static(&host_spapr_cpu_core_type_info);
> +    type_register_static(&POWER7_spapr_cpu_core_type_info);
> +    type_register_static(&POWER8_spapr_cpu_core_type_info);
>  }
> 
>  type_init(spapr_cpu_core_register_types)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 098d85d..0fdf448 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -585,6 +585,7 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
>                                         uint32_t count);
>  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
>                                            uint32_t count);
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
> 
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index e3340ea..71e69c0 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -24,4 +24,40 @@ typedef struct sPAPRCPUCore {
>      PowerPCCPU *threads;
>  } sPAPRCPUCore;
> 
> +#define TYPE_host_SPAPR_CPU_CORE "host-spapr-cpu-core"
> +#define host_SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(hostsPAPRCPUCore, (obj), TYPE_host_SPAPR_CPU_CORE)
> +
> +typedef struct hostsPAPRCPUCore {
> +    /*< private >*/
> +    sPAPRCPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *cpu;
> +} hostsPAPRCPUCore;
> +
> +#define TYPE_POWER7_SPAPR_CPU_CORE "POWER7-spapr-cpu-core"
> +#define POWER7_SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(POWER7sPAPRCPUCore, (obj), TYPE_POWER7_SPAPR_CPU_CORE)
> +
> +typedef struct POWER7sPAPRCPUCore {
> +    /*< private >*/
> +    sPAPRCPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *cpu;
> +} POWER7sPAPRCPUCore;
> +
> +#define TYPE_POWER8_SPAPR_CPU_CORE "POWER8-spapr-cpu-core"
> +#define POWER8_SPAPR_CPU_CORE(obj) \
> +    OBJECT_CHECK(POWER8sPAPRCPUCore, (obj), TYPE_POWER8_SPAPR_CPU_CORE)
> +
> +typedef struct POWER8sPAPRCPUCore {
> +    /*< private >*/
> +    sPAPRCPUCore parent_obj;
> +
> +    /*< public >*/
> +    ObjectClass *cpu;
> +} POWER8sPAPRCPUCore;
> +
>  #endif
> -- 
> 2.1.0
>
Michael Roth April 9, 2016, 2:21 a.m. UTC | #6
Quoting David Gibson (2016-04-03 19:13:54)
> On Fri, Apr 01, 2016 at 11:42:23AM +0530, Bharata B Rao wrote:
> > On Fri, Apr 01, 2016 at 04:08:44PM +1100, David Gibson wrote:
> > > On Thu, Mar 31, 2016 at 02:09:17PM +0530, Bharata B Rao wrote:
> > > > Introduce core devices for each CPU type supported by sPAPR. These
> > > > core devices are derived from the base spapr-cpu-core device type.
> > > > 
> > > > TODO:
> > > > - Add core types for other remaining CPU types
> > > > - Handle CPU model alias correctly
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/ppc/spapr.c                  |   3 +-
> > > >  hw/ppc/spapr_cpu_core.c         | 118 ++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/ppc/spapr.h          |   1 +
> > > >  include/hw/ppc/spapr_cpu_core.h |  36 ++++++++++++
> > > >  4 files changed, 156 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 64c4acc..45ac5dc 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1614,8 +1614,7 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> > > >      machine->boot_order = g_strdup(boot_device);
> > > >  }
> > > >  
> > > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > > -                           Error **errp)
> > > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> > > >  {
> > > >      CPUPPCState *env = &cpu->env;
> > > >  
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > index 8cbe2a5..3751a54 100644
> > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -22,9 +22,127 @@ static const TypeInfo spapr_cpu_core_type_info = {
> > > >      .instance_size = sizeof(sPAPRCPUCore),
> > > >  };
> > > >  
> > > > +#define DEFINE_SPAPR_CPU_CORE(_name)                                           \
> > > > +static void                                                                    \
> > > > +glue(_name, _spapr_cpu_core_create_threads)(DeviceState *dev, int threads,     \
> > > > +    Error **errp)                                                              \
> > > > +{                                                                              \
> > > > +    int i;                                                                     \
> > > > +    Error *local_err = NULL;                                                   \
> > > > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> > > > +    glue(_name, sPAPRCPUCore) * core =                                         \
> > > > +         glue(_name, _SPAPR_CPU_CORE)(OBJECT(dev));                            \
> > > > +                                                                               \
> > > > +    for (i = 0; i < threads; i++) {                                            \
> > > > +        char id[32];                                                           \
> > > > +                                                                               \
> > > > +        object_initialize(&sc->threads[i], sizeof(sc->threads[i]),             \
> > > > +                          object_class_get_name(core->cpu));                   \
> > > > +        snprintf(id, sizeof(id), "thread[%d]", i);                             \
> > > > +        object_property_add_child(OBJECT(core), id, OBJECT(&sc->threads[i]),   \
> > > > +                                  &local_err);                                 \
> > > > +        if (local_err) {                                                       \
> > > > +            goto err;                                                          \
> > > > +        }                                                                      \
> > > > +    }                                                                          \
> > > > +    return;                                                                    \
> > > > +                                                                               \
> > > > +err:                                                                           \
> > > > +    while (--i) {                                                              \
> > > > +        object_unparent(OBJECT(&sc->threads[i]));                              \
> > > > +    }                                                                          \
> > > > +    error_propagate(errp, local_err);                                          \
> > > > +}                                                                              \
> > > > +                                                                               \
> > > > +static int                                                                     \
> > > > +glue(_name, _spapr_cpu_core_realize_child)(Object *child, void *opaque)        \
> > > > +{                                                                              \
> > > > +    Error **errp = opaque;                                                     \
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());              \
> > > > +    CPUState *cs = CPU(child);                                                 \
> > > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);                                         \
> > > > +                                                                               \
> > > > +    object_property_set_bool(child, true, "realized", errp);                   \
> > > > +    if (*errp) {                                                               \
> > > > +        return 1;                                                              \
> > > > +    }                                                                          \
> > > > +                                                                               \
> > > > +    spapr_cpu_init(spapr, cpu, errp);                                          \
> > > > +    if (*errp) {                                                               \
> > > > +        return 1;                                                              \
> > > > +    }                                                                          \
> > > > +    return 0;                                                                  \
> > > > +}                                                                              \
> > > 
> > > If you put the ObjectClass * for the threads in the base abstract
> > > class's class structure, then you can move most of this logic to the
> > > abstract class as well and make the macro-ized stuff much smaller.
> > > 
> > > The realize_child stuff doesn't even need the ObjectClass* in the base
> > > class to factor out.
> > > 
> > > > +static void                                                                    \
> > > > +glue(_name, _spapr_cpu_core_realize)(DeviceState *dev, Error **errp)           \
> > > > +{                                                                              \
> > > > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
> > > > +    CPUCore *cc = CPU_CORE(OBJECT(dev));                                       \
> > > > +    Error *local_err = NULL;                                                   \
> > > > +                                                                               \
> > > > +    /*                                                                         \
> > > > +     * TODO: This is CPU model specific CPU core's realize routine.            \
> > > > +     * However I am initializing "threads" field of the parent type            \
> > > > +     * sPAPRCPUCore here. Is this ok ? If not I will have make "threads"       \
> > > > +     * part of CPU model specific CPU core type and have different plug()      \
> > > > +     * handlers for each type instead of having a common plug() handler        \
> > > > +     * for all core types.                                                     \
> > > > +     */                                                                        \
> > > > +    sc->threads = g_new0(PowerPCCPU, cc->threads);                             \
> > > > +    glue(_name, _spapr_cpu_core_create_threads)(dev, cc->threads, &local_err); \
> > > > +    if (local_err) {                                                           \
> > > > +        goto out;                                                              \
> > > > +    }                                                                          \
> > > > +                                                                               \
> > > > +    object_child_foreach(OBJECT(dev),                                          \
> > > > +                         glue(_name, _spapr_cpu_core_realize_child),           \
> > > > +                         &local_err);                                          \
> > > > +                                                                               \
> > > > +out:                                                                           \
> > > > +    if (local_err) {                                                           \
> > > > +        g_free(sc->threads);                                                   \
> > > > +        error_propagate(errp, local_err);                                      \
> > > > +    }                                                                          \
> > > > +}                                                                              \
> > > > +                                                                               \
> > > > +static void                                                                    \
> > > > +glue(_name, _spapr_cpu_core_instance_init)(Object *obj)                        \
> > > > +{                                                                              \
> > > > +    glue(_name, sPAPRCPUCore) * core = glue(_name, _SPAPR_CPU_CORE)(obj);      \
> > > > +    const char *type = stringify(_name) "-" TYPE_POWERPC_CPU;                  \
> > > > +    ObjectClass *oc = object_class_by_name(type);                              \
> > > > +                                                                               \
> > > > +    core->cpu = oc;                                                            \
> > > > +}                                                                              \
> > > > +                                                                               \
> > > > +static void                                                                    \
> > > > +glue(_name, _spapr_cpu_core_class_init)(ObjectClass *oc, void *data)           \
> > > > +{                                                                              \
> > > > +                                                                               \
> > > > +    DeviceClass *dc = DEVICE_CLASS(oc);                                        \
> > > > +    dc->realize = glue(_name, _spapr_cpu_core_realize);                        \
> > > 
> > > I think the only callback you should need to construct in the macro is
> > > class_init to initialize the ObjectClass* field.
> > > 
> > > > +}                                                                              \
> > > > +                                                                               \
> > > > +static const TypeInfo glue(_name, _spapr_cpu_core_type_info) =                 \
> > > > +{                                                                              \
> > > > +    .name = stringify(_name) "-" TYPE_SPAPR_CPU_CORE,                          \
> > > > +    .parent = TYPE_SPAPR_CPU_CORE,                                             \
> > > > +    .instance_size = sizeof(glue(_name, sPAPRCPUCore)),                        \
> > > > +    .instance_init = glue(_name, _spapr_cpu_core_instance_init),               \
> > > > +    .class_init = glue(_name, _spapr_cpu_core_class_init),                     \
> > > > +};
> > > > +
> > > > +DEFINE_SPAPR_CPU_CORE(host);
> > > > +DEFINE_SPAPR_CPU_CORE(POWER7);
> > > > +DEFINE_SPAPR_CPU_CORE(POWER8);
> > > > +
> > > >  static void spapr_cpu_core_register_types(void)
> > > >  {
> > > >      type_register_static(&spapr_cpu_core_type_info);
> > > > +    type_register_static(&host_spapr_cpu_core_type_info);
> > > > +    type_register_static(&POWER7_spapr_cpu_core_type_info);
> > > > +    type_register_static(&POWER8_spapr_cpu_core_type_info);
> > > >  }
> > > >  
> > > >  type_init(spapr_cpu_core_register_types)
> > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > index 098d85d..0fdf448 100644
> > > > --- a/include/hw/ppc/spapr.h
> > > > +++ b/include/hw/ppc/spapr.h
> > > > @@ -585,6 +585,7 @@ void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
> > > >                                         uint32_t count);
> > > >  void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
> > > >                                            uint32_t count);
> > > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
> > > >  
> > > >  /* rtas-configure-connector state */
> > > >  struct sPAPRConfigureConnectorState {
> > > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > > > index e3340ea..71e69c0 100644
> > > > --- a/include/hw/ppc/spapr_cpu_core.h
> > > > +++ b/include/hw/ppc/spapr_cpu_core.h
> > > > @@ -24,4 +24,40 @@ typedef struct sPAPRCPUCore {
> > > >      PowerPCCPU *threads;
> > > >  } sPAPRCPUCore;
> > > >  
> > > > +#define TYPE_host_SPAPR_CPU_CORE "host-spapr-cpu-core"
> > > > +#define host_SPAPR_CPU_CORE(obj) \
> > > > +    OBJECT_CHECK(hostsPAPRCPUCore, (obj), TYPE_host_SPAPR_CPU_CORE)
> > > > +
> > > > +typedef struct hostsPAPRCPUCore {
> > > > +    /*< private >*/
> > > > +    sPAPRCPUCore parent_obj;
> > > > +
> > > > +    /*< public >*/
> > > > +    ObjectClass *cpu;
> > > > +} hostsPAPRCPUCore;
> > > > +#define TYPE_POWER7_SPAPR_CPU_CORE "POWER7-spapr-cpu-core"
> > > > +#define POWER7_SPAPR_CPU_CORE(obj) \
> > > > +    OBJECT_CHECK(POWER7sPAPRCPUCore, (obj), TYPE_POWER7_SPAPR_CPU_CORE)
> > > > +
> > > > +typedef struct POWER7sPAPRCPUCore {
> > > > +    /*< private >*/
> > > > +    sPAPRCPUCore parent_obj;
> > > > +
> > > > +    /*< public >*/
> > > > +    ObjectClass *cpu;
> > > > +} POWER7sPAPRCPUCore;
> > > > +
> > > > +#define TYPE_POWER8_SPAPR_CPU_CORE "POWER8-spapr-cpu-core"
> > > > +#define POWER8_SPAPR_CPU_CORE(obj) \
> > > > +    OBJECT_CHECK(POWER8sPAPRCPUCore, (obj), TYPE_POWER8_SPAPR_CPU_CORE)
> > > > +
> > > > +typedef struct POWER8sPAPRCPUCore {
> > > > +    /*< private >*/
> > > > +    sPAPRCPUCore parent_obj;
> > > > +
> > > > +    /*< public >*/
> > > > +    ObjectClass *cpu;
> > > > +} POWER8sPAPRCPUCore;
> > > 
> > > These are all identical so should also be macro constructed as well.
> > > I don't think there's actually any need for the structures to be
> > > exposed in a header file either, so you should be able to do it in the
> > > same macro that constructs the implementation.
> > > 
> > > Uh.. except if you move the ObjectClass* to the base class you won't
> > > even need these.
> > 
> > The only reason (currently) POWER8sPAPRCPUCore exists separately from
> > the base class sPAPRCPUCore is that it represents POWER8 core which
> > is stored as ObjectClass*.
> > 
> > Now if we don't track cpu type (ObjectClass *) as part of
> > POWER8sPAPRCPUCore but push that up to sPAPRCPUCore, I am not
> > sure if that would be at the right abstraction level.
> 
> Not as part of sPAPRCPUCore, but as part of sPAPRCPUCoreClass.  We
> don't have a structure for the class at present, but we can add one.
> You can think of it as a method that sPAPRCPUCore and subclasses have
> which returns the correct ObjectClass *, except that we don't actually
> need a method (function pointer) - a simple data pointer in the class
> will suffice.

Sorry, I'd missed this thread when I posted my other reply.

I agree it should be pushed up a level so we can drop most of the
macro stuff. arm_cpu_register_types() seems to provide a fairly
straightforward example of defining derivative types by modifying
parent Object's state via their instance_init functions.

But it does seem unecessary in this case to do it for every
instance, since that state (ObjectClass *cpu) will be
common/immutable for all instances.

So I think both approaches are workable and acceptable, but doing
it your way vs. what I suggested in my other reply does seem like
the nicer way to handle it.

> 
> -- 
> 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
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 64c4acc..45ac5dc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1614,8 +1614,7 @@  static void spapr_boot_set(void *opaque, const char *boot_device,
     machine->boot_order = g_strdup(boot_device);
 }
 
-static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
-                           Error **errp)
+void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
 {
     CPUPPCState *env = &cpu->env;
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 8cbe2a5..3751a54 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -22,9 +22,127 @@  static const TypeInfo spapr_cpu_core_type_info = {
     .instance_size = sizeof(sPAPRCPUCore),
 };
 
+#define DEFINE_SPAPR_CPU_CORE(_name)                                           \
+static void                                                                    \
+glue(_name, _spapr_cpu_core_create_threads)(DeviceState *dev, int threads,     \
+    Error **errp)                                                              \
+{                                                                              \
+    int i;                                                                     \
+    Error *local_err = NULL;                                                   \
+    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
+    glue(_name, sPAPRCPUCore) * core =                                         \
+         glue(_name, _SPAPR_CPU_CORE)(OBJECT(dev));                            \
+                                                                               \
+    for (i = 0; i < threads; i++) {                                            \
+        char id[32];                                                           \
+                                                                               \
+        object_initialize(&sc->threads[i], sizeof(sc->threads[i]),             \
+                          object_class_get_name(core->cpu));                   \
+        snprintf(id, sizeof(id), "thread[%d]", i);                             \
+        object_property_add_child(OBJECT(core), id, OBJECT(&sc->threads[i]),   \
+                                  &local_err);                                 \
+        if (local_err) {                                                       \
+            goto err;                                                          \
+        }                                                                      \
+    }                                                                          \
+    return;                                                                    \
+                                                                               \
+err:                                                                           \
+    while (--i) {                                                              \
+        object_unparent(OBJECT(&sc->threads[i]));                              \
+    }                                                                          \
+    error_propagate(errp, local_err);                                          \
+}                                                                              \
+                                                                               \
+static int                                                                     \
+glue(_name, _spapr_cpu_core_realize_child)(Object *child, void *opaque)        \
+{                                                                              \
+    Error **errp = opaque;                                                     \
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());              \
+    CPUState *cs = CPU(child);                                                 \
+    PowerPCCPU *cpu = POWERPC_CPU(cs);                                         \
+                                                                               \
+    object_property_set_bool(child, true, "realized", errp);                   \
+    if (*errp) {                                                               \
+        return 1;                                                              \
+    }                                                                          \
+                                                                               \
+    spapr_cpu_init(spapr, cpu, errp);                                          \
+    if (*errp) {                                                               \
+        return 1;                                                              \
+    }                                                                          \
+    return 0;                                                                  \
+}                                                                              \
+                                                                               \
+static void                                                                    \
+glue(_name, _spapr_cpu_core_realize)(DeviceState *dev, Error **errp)           \
+{                                                                              \
+    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));                            \
+    CPUCore *cc = CPU_CORE(OBJECT(dev));                                       \
+    Error *local_err = NULL;                                                   \
+                                                                               \
+    /*                                                                         \
+     * TODO: This is CPU model specific CPU core's realize routine.            \
+     * However I am initializing "threads" field of the parent type            \
+     * sPAPRCPUCore here. Is this ok ? If not I will have make "threads"       \
+     * part of CPU model specific CPU core type and have different plug()      \
+     * handlers for each type instead of having a common plug() handler        \
+     * for all core types.                                                     \
+     */                                                                        \
+    sc->threads = g_new0(PowerPCCPU, cc->threads);                             \
+    glue(_name, _spapr_cpu_core_create_threads)(dev, cc->threads, &local_err); \
+    if (local_err) {                                                           \
+        goto out;                                                              \
+    }                                                                          \
+                                                                               \
+    object_child_foreach(OBJECT(dev),                                          \
+                         glue(_name, _spapr_cpu_core_realize_child),           \
+                         &local_err);                                          \
+                                                                               \
+out:                                                                           \
+    if (local_err) {                                                           \
+        g_free(sc->threads);                                                   \
+        error_propagate(errp, local_err);                                      \
+    }                                                                          \
+}                                                                              \
+                                                                               \
+static void                                                                    \
+glue(_name, _spapr_cpu_core_instance_init)(Object *obj)                        \
+{                                                                              \
+    glue(_name, sPAPRCPUCore) * core = glue(_name, _SPAPR_CPU_CORE)(obj);      \
+    const char *type = stringify(_name) "-" TYPE_POWERPC_CPU;                  \
+    ObjectClass *oc = object_class_by_name(type);                              \
+                                                                               \
+    core->cpu = oc;                                                            \
+}                                                                              \
+                                                                               \
+static void                                                                    \
+glue(_name, _spapr_cpu_core_class_init)(ObjectClass *oc, void *data)           \
+{                                                                              \
+                                                                               \
+    DeviceClass *dc = DEVICE_CLASS(oc);                                        \
+    dc->realize = glue(_name, _spapr_cpu_core_realize);                        \
+}                                                                              \
+                                                                               \
+static const TypeInfo glue(_name, _spapr_cpu_core_type_info) =                 \
+{                                                                              \
+    .name = stringify(_name) "-" TYPE_SPAPR_CPU_CORE,                          \
+    .parent = TYPE_SPAPR_CPU_CORE,                                             \
+    .instance_size = sizeof(glue(_name, sPAPRCPUCore)),                        \
+    .instance_init = glue(_name, _spapr_cpu_core_instance_init),               \
+    .class_init = glue(_name, _spapr_cpu_core_class_init),                     \
+};
+
+DEFINE_SPAPR_CPU_CORE(host);
+DEFINE_SPAPR_CPU_CORE(POWER7);
+DEFINE_SPAPR_CPU_CORE(POWER8);
+
 static void spapr_cpu_core_register_types(void)
 {
     type_register_static(&spapr_cpu_core_type_info);
+    type_register_static(&host_spapr_cpu_core_type_info);
+    type_register_static(&POWER7_spapr_cpu_core_type_info);
+    type_register_static(&POWER8_spapr_cpu_core_type_info);
 }
 
 type_init(spapr_cpu_core_register_types)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 098d85d..0fdf448 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -585,6 +585,7 @@  void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
                                        uint32_t count);
 void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
                                           uint32_t count);
+void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index e3340ea..71e69c0 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -24,4 +24,40 @@  typedef struct sPAPRCPUCore {
     PowerPCCPU *threads;
 } sPAPRCPUCore;
 
+#define TYPE_host_SPAPR_CPU_CORE "host-spapr-cpu-core"
+#define host_SPAPR_CPU_CORE(obj) \
+    OBJECT_CHECK(hostsPAPRCPUCore, (obj), TYPE_host_SPAPR_CPU_CORE)
+
+typedef struct hostsPAPRCPUCore {
+    /*< private >*/
+    sPAPRCPUCore parent_obj;
+
+    /*< public >*/
+    ObjectClass *cpu;
+} hostsPAPRCPUCore;
+
+#define TYPE_POWER7_SPAPR_CPU_CORE "POWER7-spapr-cpu-core"
+#define POWER7_SPAPR_CPU_CORE(obj) \
+    OBJECT_CHECK(POWER7sPAPRCPUCore, (obj), TYPE_POWER7_SPAPR_CPU_CORE)
+
+typedef struct POWER7sPAPRCPUCore {
+    /*< private >*/
+    sPAPRCPUCore parent_obj;
+
+    /*< public >*/
+    ObjectClass *cpu;
+} POWER7sPAPRCPUCore;
+
+#define TYPE_POWER8_SPAPR_CPU_CORE "POWER8-spapr-cpu-core"
+#define POWER8_SPAPR_CPU_CORE(obj) \
+    OBJECT_CHECK(POWER8sPAPRCPUCore, (obj), TYPE_POWER8_SPAPR_CPU_CORE)
+
+typedef struct POWER8sPAPRCPUCore {
+    /*< private >*/
+    sPAPRCPUCore parent_obj;
+
+    /*< public >*/
+    ObjectClass *cpu;
+} POWER8sPAPRCPUCore;
+
 #endif