From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47216) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alsK8-0006ZN-UU for qemu-devel@nongnu.org; Fri, 01 Apr 2016 02:12:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alsK5-00032e-Ll for qemu-devel@nongnu.org; Fri, 01 Apr 2016 02:12:48 -0400 Received: from e28smtp05.in.ibm.com ([125.16.236.5]:40083) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alsK4-0002wB-LA for qemu-devel@nongnu.org; Fri, 01 Apr 2016 02:12:45 -0400 Received: from localhost by e28smtp05.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 1 Apr 2016 11:42:40 +0530 Date: Fri, 1 Apr 2016 11:42:23 +0530 From: Bharata B Rao Message-ID: <20160401061223.GA26248@in.ibm.com> References: <1459413561-30745-1-git-send-email-bharata@linux.vnet.ibm.com> <1459413561-30745-9-git-send-email-bharata@linux.vnet.ibm.com> <20160401050844.GP416@voom.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160401050844.GP416@voom.redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH v2.1 08/12] spapr: Add CPU type specific core devices Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: mjrosato@linux.vnet.ibm.com, agraf@suse.de, thuth@redhat.com, pkrempa@redhat.com, ehabkost@redhat.com, aik@ozlabs.ru, qemu-devel@nongnu.org, armbru@redhat.com, borntraeger@de.ibm.com, qemu-ppc@nongnu.org, pbonzini@redhat.com, imammedo@redhat.com, afaerber@suse.de, mdroth@linux.vnet.ibm.com 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 > > --- > > 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.