From: Michael Roth <mdroth@linux.vnet.ibm.com>
To: David Gibson <david@gibson.dropbear.id.au>,
Bharata B Rao <bharata@linux.vnet.ibm.com>
Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, afaerber@suse.de,
imammedo@redhat.com, armbru@redhat.com, thuth@redhat.com,
aik@ozlabs.ru, agraf@suse.de, pbonzini@redhat.com,
ehabkost@redhat.com, pkrempa@redhat.com, eblake@redhat.com,
mjrosato@linux.vnet.ibm.com, borntraeger@de.ibm.com
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 [thread overview]
Message-ID: <20160409022152.30838.64968@loki> (raw)
In-Reply-To: <20160404001354.GB16485@voom.fritz.box>
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
next prev parent reply other threads:[~2016-04-09 2:22 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-31 8:39 [Qemu-devel] [RFC PATCH v2.1 00/12] Core based CPU hotplug for PowerPC sPAPR Bharata B Rao
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 01/12] exec: Remove cpu from cpus list during cpu_exec_exit() Bharata B Rao
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 02/12] exec: Do vmstate unregistration from cpu_exec_exit() Bharata B Rao
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 03/12] cpu: Reclaim vCPU objects Bharata B Rao
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 04/12] cpu: Add a sync version of cpu_remove() Bharata B Rao
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 05/12] qdev: hotplug: Introduce HotplugHandler.pre_plug() callback Bharata B Rao
2016-04-01 3:30 ` David Gibson
2016-04-01 10:38 ` Paolo Bonzini
2016-04-04 0:09 ` David Gibson
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 06/12] cpu: Abstract CPU core type Bharata B Rao
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 07/12] spapr: Abstract CPU core device Bharata B Rao
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 08/12] spapr: Add CPU type specific core devices Bharata B Rao
2016-04-01 5:08 ` David Gibson
2016-04-01 6:12 ` Bharata B Rao
2016-04-04 0:13 ` David Gibson
2016-04-09 2:21 ` Michael Roth [this message]
2016-04-04 0:16 ` David Gibson
2016-04-08 23:35 ` Michael Roth
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 09/12] spapr: convert boot CPUs into CPU " Bharata B Rao
2016-04-01 5:12 ` David Gibson
2016-04-08 23:35 ` Michael Roth
2016-05-05 9:19 ` Bharata B Rao
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 10/12] spapr: CPU hotplug support Bharata B Rao
2016-04-04 4:23 ` David Gibson
2016-04-05 23:47 ` Michael Roth
2016-05-05 9:22 ` Bharata B Rao
2016-05-06 8:57 ` Igor Mammedov
2016-05-06 10:14 ` Bharata B Rao
2016-05-06 11:01 ` Igor Mammedov
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 11/12] xics, xics_kvm: Handle CPU unplug correctly Bharata B Rao
2016-04-06 0:24 ` Michael Roth
2016-04-06 0:43 ` David Gibson
2016-04-08 23:40 ` Michael Roth
2016-03-31 8:39 ` [Qemu-devel] [RFC PATCH v2.1 12/12] spapr: CPU hot unplug support Bharata B Rao
2016-04-04 4:27 ` David Gibson
2016-05-09 4:24 ` Bharata B Rao
2016-04-04 14:44 ` [Qemu-devel] [RFC PATCH v2.1 00/12] Core based CPU hotplug for PowerPC sPAPR Igor Mammedov
2016-04-05 14:55 ` Bharata B Rao
2016-04-05 18:40 ` Igor Mammedov
2016-04-05 21:58 ` Igor Mammedov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160409022152.30838.64968@loki \
--to=mdroth@linux.vnet.ibm.com \
--cc=afaerber@suse.de \
--cc=agraf@suse.de \
--cc=aik@ozlabs.ru \
--cc=armbru@redhat.com \
--cc=bharata@linux.vnet.ibm.com \
--cc=borntraeger@de.ibm.com \
--cc=david@gibson.dropbear.id.au \
--cc=eblake@redhat.com \
--cc=ehabkost@redhat.com \
--cc=imammedo@redhat.com \
--cc=mjrosato@linux.vnet.ibm.com \
--cc=pbonzini@redhat.com \
--cc=pkrempa@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.