[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v2.1 08/12] spapr: Add CPU type specific cor
|
From: |
Michael Roth |
|
Subject: |
Re: [Qemu-devel] [RFC PATCH v2.1 08/12] spapr: Add CPU type specific core devices |
|
Date: |
Fri, 08 Apr 2016 21:21:52 -0500 |
|
User-agent: |
alot/0.3.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 <address@hidden>
> > > > ---
> > > > 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