From patchwork Wed Jun 8 09:42:42 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bharata B Rao X-Patchwork-Id: 632135 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3rPl0t2HYTz9sDG for ; Wed, 8 Jun 2016 20:24:30 +1000 (AEST) Received: from localhost ([::1]:55832 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAaey-0002XF-CC for incoming@patchwork.ozlabs.org; Wed, 08 Jun 2016 06:24:28 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36336) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAa1t-000345-Ch for qemu-devel@nongnu.org; Wed, 08 Jun 2016 05:44:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAa1p-0007S1-6S for qemu-devel@nongnu.org; Wed, 08 Jun 2016 05:44:04 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:36608) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAa1o-0007Rj-Ts for qemu-devel@nongnu.org; Wed, 08 Jun 2016 05:44:01 -0400 Received: from pps.filterd (m0048827.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u589cZkj032980 for ; Wed, 8 Jun 2016 05:43:58 -0400 Received: from e23smtp04.au.ibm.com (e23smtp04.au.ibm.com [202.81.31.146]) by mx0a-001b2d01.pphosted.com with ESMTP id 23e9mdnr52-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 08 Jun 2016 05:43:58 -0400 Received: from localhost by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 8 Jun 2016 19:43:55 +1000 Received: from d23dlp03.au.ibm.com (202.81.31.214) by e23smtp04.au.ibm.com (202.81.31.210) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 8 Jun 2016 19:43:53 +1000 X-IBM-Helo: d23dlp03.au.ibm.com X-IBM-MailFrom: bharata@linux.vnet.ibm.com X-IBM-RcptTo: qemu-devel@nongnu.org;qemu-ppc@nongnu.org Received: from d23relay06.au.ibm.com (d23relay06.au.ibm.com [9.185.63.219]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 9B903357805C; Wed, 8 Jun 2016 19:43:35 +1000 (EST) Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay06.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u589hBcA2883922; Wed, 8 Jun 2016 19:43:11 +1000 Received: from d23av02.au.ibm.com (localhost [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u589gxgi002118; Wed, 8 Jun 2016 19:42:59 +1000 Received: from in.ibm.com ([9.79.179.234]) by d23av02.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id u589glvw001689 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA256 bits=256 verify=NO); Wed, 8 Jun 2016 19:42:54 +1000 Date: Wed, 8 Jun 2016 15:12:42 +0530 From: Bharata B Rao To: David Gibson References: <1463024905-28401-1-git-send-email-bharata@linux.vnet.ibm.com> <1463024905-28401-8-git-send-email-bharata@linux.vnet.ibm.com> <20160603052508.GJ1087@voom.fritz.box> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20160603052508.GJ1087@voom.fritz.box> User-Agent: Mutt/1.5.23 (2014-03-12) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16060809-0012-0000-0000-0000019CCF3D X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16060809-0013-0000-0000-0000055BFA78 Message-Id: <20160608094241.GB8861@in.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-06-08_03:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1604210000 definitions=main-1606080114 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] X-Received-From: 148.163.156.1 Subject: Re: [Qemu-devel] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: bharata@linux.vnet.ibm.com 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 Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" On Fri, Jun 03, 2016 at 03:25:08PM +1000, David Gibson wrote: > On Thu, May 12, 2016 at 09:18:17AM +0530, Bharata B Rao wrote: > > Add sPAPR specific abastract CPU core device that is based on generic > > CPU core device. Use this as base type to create sPAPR CPU specific core > > devices. > > > > TODO: > > - Add core types for other remaining CPU types > > - Handle CPU model alias correctly > > > > Signed-off-by: Bharata B Rao > > This is looking pretty cood, but there's some minor changes I'd like > to see. > > > --- > > hw/ppc/Makefile.objs | 1 + > > hw/ppc/spapr.c | 3 +- > > hw/ppc/spapr_cpu_core.c | 168 ++++++++++++++++++++++++++++++++++++++++ > > include/hw/ppc/spapr.h | 1 + > > include/hw/ppc/spapr_cpu_core.h | 28 +++++++ > > 5 files changed, 199 insertions(+), 2 deletions(-) > > create mode 100644 hw/ppc/spapr_cpu_core.c > > create mode 100644 include/hw/ppc/spapr_cpu_core.h > > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs > > index c1ffc77..5cc6608 100644 > > --- a/hw/ppc/Makefile.objs > > +++ b/hw/ppc/Makefile.objs > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o > > obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o > > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o > > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o > > ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy) > > obj-y += spapr_pci_vfio.o > > endif > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index b69995e..95db047 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1605,8 +1605,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) > > I think this function should actually move into spapr_cpu_core.c Actually, this is a CPU thread specific routine and will be called from spapr.c too. But I moved this to spapr_cpu_core.c as you suggest. > > > { > > CPUPPCState *env = &cpu->env; > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > new file mode 100644 > > index 0000000..af63ed9 > > --- /dev/null > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -0,0 +1,168 @@ > > +/* > > + * sPAPR CPU core device, acts as container of CPU thread devices. > > + * > > + * Copyright (C) 2016 Bharata B Rao > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > > + * See the COPYING file in the top-level directory. > > + */ > > +#include "hw/cpu/core.h" > > +#include "hw/ppc/spapr_cpu_core.h" > > +#include "hw/ppc/spapr.h" > > +#include "hw/boards.h" > > +#include "qapi/error.h" > > +#include > > +#include "target-ppc/kvm_ppc.h" > > + > > +static void spapr_cpu_core_create_threads(DeviceState *dev, int threads, > > + Error **errp) > > This function could be folded into spapr_cpu_core_realize(), that's > the only caller and they're both fairly short. Ok done. > > > +{ > > + int i; > > + Error *local_err = NULL; > > + sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev)); > > + > > + for (i = 0; i < threads; i++) { > > + char id[32]; > > + > > + object_initialize(&core->threads[i], sizeof(core->threads[i]), > > + object_class_get_name(core->cpu)); > > Given we have to go from the class pointer to the class name to > actually use it here, maybe we would be better off storing the name > rather than a class pointer. Up to you, I'm happy either way. I was doing typename initially and there were suggestions to move to ObjectClass. May be I will leave it like this while I fix all the other things and finally revisit this ? > > > + snprintf(id, sizeof(id), "thread[%d]", i); > > + object_property_add_child(OBJECT(core), id, OBJECT(&core->threads[i]), > > + &local_err); > > + if (local_err) { > > + goto err; > > + } > > + } > > + return; > > + > > +err: > > + while (--i) { > > + object_unparent(OBJECT(&core->threads[i])); > > Is this safe if some of the threads haven't been initialized? This is in the error path and only those threads which have been initialized will be unparented. > > > + } > > + error_propagate(errp, local_err); > > +} > > + > > +static int 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 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; > > + > > + sc->threads = g_new0(PowerPCCPU, cc->threads); > > This isn't quite safe, because it assume the structure size for all > the threads is that of PowerPCCPU. That's true now, but cpu thread > subtypes could in theory extend that structure with subtype specific > fields. I think we need to actually look up the class instance size > here. I couldn't find a way to obtain the instance_size of a type. I had to introduce a QOM API like this: sc->threads as PowerPCCPU pointer. > > > + spapr_cpu_core_create_threads(dev, cc->threads, &local_err); > > + if (local_err) { > > + goto out; > > + } > > + object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err); > > + > > +out: > > + if (local_err) { > > + g_free(sc->threads); > > + error_propagate(errp, local_err); > > + } > > +} > > + > > +static void spapr_cpu_core_class_init(ObjectClass *oc, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(oc); > > + dc->realize = spapr_cpu_core_realize; > > +} > > + > > +/* > > + * instance_init routines from different flavours of sPAPR CPU cores. > > + * TODO: Add support for 'host' core type. > > + */ > > +#define SPAPR_CPU_CORE_INITFN(_type, _fname) \ > > +static void glue(glue(spapr_cpu_core_, _fname), _initfn(Object *obj)) \ > > +{ \ > > + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); \ > > + char *name = g_strdup_printf("%s-" TYPE_POWERPC_CPU, stringify(_type)); \ > > + ObjectClass *oc = object_class_by_name(name); \ > > + g_assert(oc); \ > > + g_free((void *)name); \ > > + core->cpu = oc; \ > > This all looks like stuff which could be done at class_init time, but > this is an instance_init function. Essentially what we do here is to store the CPU thread ObjectClass within the abstract sPAPRCPUCore object so that we can initialize the CPU threads of right type from abstract sPAPRCPUCore object's realize routine. I am not sure if core object is available in class_init routine to achieve this. > > I'm also wondering if it might just be simpler to store _type in the > subclass and do this in the base class. Not sure if I really understand this suggestion correctly. However, if you see, I am not yet defining a separate structures for the subtypes of sPAPRCPUCore where _type can be stored, but instead using sPAPRCPUCore as the instance_size for sub core types too. > > > +} > > + > > +SPAPR_CPU_CORE_INITFN(POWER7_v2.3, POWER7); > > +SPAPR_CPU_CORE_INITFN(POWER7+_v2.1, POWER7plus); > > +SPAPR_CPU_CORE_INITFN(POWER8_v2.0, POWER8); > > +SPAPR_CPU_CORE_INITFN(POWER8E_v2.1, POWER8E); > > + > > +typedef struct SPAPRCoreInfo { > > + const char *name; > > + void (*initfn)(Object *obj); > > +} SPAPRCoreInfo; > > + > > +static const SPAPRCoreInfo spapr_cores[] = { > > + /* POWER7 and aliases */ > > + { .name = "POWER7_v2.3", .initfn = spapr_cpu_core_POWER7_initfn }, > > + { .name = "POWER7", .initfn = spapr_cpu_core_POWER7_initfn }, > > + > > + /* POWER7+ and aliases */ > > + { .name = "POWER7+_v2.1", .initfn = spapr_cpu_core_POWER7plus_initfn }, > > + { .name = "POWER7+", .initfn = spapr_cpu_core_POWER7plus_initfn }, > > + > > + /* POWER8 and aliases */ > > + { .name = "POWER8_v2.0", .initfn = spapr_cpu_core_POWER8_initfn }, > > + { .name = "POWER8", .initfn = spapr_cpu_core_POWER8_initfn }, > > + { .name = "power8", .initfn = spapr_cpu_core_POWER8_initfn }, > > + > > + /* POWER8E and aliases */ > > + { .name = "POWER8E_v2.1", .initfn = spapr_cpu_core_POWER8E_initfn }, > > + { .name = "POWER8E", .initfn = spapr_cpu_core_POWER8E_initfn }, > > + > > + { .name = NULL } > > +}; > > > + > > +static void spapr_cpu_core_register(const SPAPRCoreInfo *info) > > +{ > > + TypeInfo type_info = { > > + .parent = TYPE_SPAPR_CPU_CORE, > > + .instance_size = sizeof(sPAPRCPUCore), > > + .instance_init = info->initfn, > > + }; > > + > > + type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE, info->name); > > + type_register(&type_info); > > + g_free((void *)type_info.name); > > +} > > The way the above registration works is pretty clunky. I'm not > immediately sure how to make it nicer though, so I'm happy enough to > merge it this way and hope we can clean up later. There is a precedence for this in target-arm/cpu.c :) > > > + > > +static const TypeInfo spapr_cpu_core_type_info = { > > + .name = TYPE_SPAPR_CPU_CORE, > > + .parent = TYPE_CPU_CORE, > > + .abstract = true, > > + .instance_size = sizeof(sPAPRCPUCore), > > + .class_init = spapr_cpu_core_class_init, > > +}; > > + > > +static void spapr_cpu_core_register_types(void) > > +{ > > + const SPAPRCoreInfo *info = spapr_cores; > > + > > + type_register_static(&spapr_cpu_core_type_info); > > + while (info->name) { > > + spapr_cpu_core_register(info); > > + info++; > > + } > > +} > > + > > +type_init(spapr_cpu_core_register_types) > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index 815d5ee..bcd9de6 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -580,6 +580,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 > > new file mode 100644 > > index 0000000..b6aa39e > > --- /dev/null > > +++ b/include/hw/ppc/spapr_cpu_core.h > > @@ -0,0 +1,28 @@ > > +/* > > + * sPAPR CPU core device. > > + * > > + * Copyright (C) 2016 Bharata B Rao > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > > + * See the COPYING file in the top-level directory. > > + */ > > +#ifndef HW_SPAPR_CPU_CORE_H > > +#define HW_SPAPR_CPU_CORE_H > > + > > +#include "hw/qdev.h" > > +#include "hw/cpu/core.h" > > + > > +#define TYPE_SPAPR_CPU_CORE "spapr-cpu-core" > > +#define SPAPR_CPU_CORE(obj) \ > > + OBJECT_CHECK(sPAPRCPUCore, (obj), TYPE_SPAPR_CPU_CORE) > > + > > +typedef struct sPAPRCPUCore { > > + /*< private >*/ > > + CPUCore parent_obj; > > + > > + /*< public >*/ > > + PowerPCCPU *threads; > > + ObjectClass *cpu; > > I'd prefer to see this called 'cpu_class' or something - seeing 'cpu' > makes me think an instance rather than a class. Changed to cpu_class. Regards, Bharata. diff --git a/qom/object.c b/qom/object.c index 3bc8a00..0e75877 100644 --- a/qom/object.c +++ b/qom/object.c @@ -202,6 +202,14 @@ static size_t type_object_get_size(TypeImpl *ti) return 0; } +size_t object_type_get_size(const char *typename) +{ + TypeImpl *type = type_get_by_name(typename); + + g_assert(type != NULL); + return type_object_get_size(type); +} + With the above I can obtain the size and use the same with object_initialize(). With this we don't have to make