From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56255) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ad9GI-0006sx-HQ for qemu-devel@nongnu.org; Mon, 07 Mar 2016 23:28:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ad9GG-0007p6-RP for qemu-devel@nongnu.org; Mon, 07 Mar 2016 23:28:46 -0500 Date: Tue, 8 Mar 2016 15:26:27 +1100 From: David Gibson Message-ID: <20160308042627.GV22546@voom.fritz.box> References: <1457074461-14285-1-git-send-email-bharata@linux.vnet.ibm.com> <1457074461-14285-6-git-send-email-bharata@linux.vnet.ibm.com> <20160304113845.667c19ad@nial.brq.redhat.com> <20160304110253.GB5054@in.ibm.com> <20160304190720.4d64abc4@nial.brq.redhat.com> <20160307033655.GD22546@voom.fritz.box> <20160307112929.7578fa9b@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="D2rVImvqWGvKULsk" Content-Disposition: inline In-Reply-To: <20160307112929.7578fa9b@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH v1 05/10] cpu: Abstract CPU core type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: mjrosato@linux.vnet.ibm.com, thuth@redhat.com, pkrempa@redhat.com, ehabkost@redhat.com, aik@ozlabs.ru, qemu-devel@nongnu.org, agraf@suse.de, armbru@redhat.com, borntraeger@de.ibm.com, qemu-ppc@nongnu.org, Bharata B Rao , g@voom.fritz.box, pbonzini@redhat.com, mdroth@linux.vnet.ibm.com, afaerber@suse.de --D2rVImvqWGvKULsk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Mar 07, 2016 at 11:29:29AM +0100, Igor Mammedov wrote: > On Mon, 7 Mar 2016 14:36:55 +1100 > David Gibson wrote: >=20 > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote: > > > On Fri, 4 Mar 2016 16:32:53 +0530 > > > Bharata B Rao wrote: > > > =20 > > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote: =20 > > > > > On Fri, 4 Mar 2016 12:24:16 +0530 > > > > > Bharata B Rao wrote: > > > > > =20 > > > > > > Add an abstract CPU core type that could be used by machines th= at want > > > > > > to define and hotplug CPUs in core granularity. > > > > > >=20 > > > > > > Signed-off-by: Bharata B Rao > > > > > > --- > > > > > > hw/cpu/Makefile.objs | 1 + > > > > > > hw/cpu/core.c | 44 +++++++++++++++++++++++++++++++++++= +++++++++ > > > > > > include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++ > > > > > > 3 files changed, 75 insertions(+) > > > > > > create mode 100644 hw/cpu/core.c > > > > > > create mode 100644 include/hw/cpu/core.h > > > > > >=20 > > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs > > > > > > index 0954a18..942a4bb 100644 > > > > > > --- a/hw/cpu/Makefile.objs > > > > > > +++ b/hw/cpu/Makefile.objs > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) +=3D arm11mpcore.o > > > > > > obj-$(CONFIG_REALVIEW) +=3D realview_mpcore.o > > > > > > obj-$(CONFIG_A9MPCORE) +=3D a9mpcore.o > > > > > > obj-$(CONFIG_A15MPCORE) +=3D a15mpcore.o > > > > > > +obj-y +=3D core.o > > > > > > =20 > > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c > > > > > > new file mode 100644 > > > > > > index 0000000..d8caf37 > > > > > > --- /dev/null > > > > > > +++ b/hw/cpu/core.c > > > > > > @@ -0,0 +1,44 @@ > > > > > > +/* > > > > > > + * CPU core abstract device > > > > > > + * > > > > > > + * Copyright (C) 2016 Bharata B Rao > > > > > > + * > > > > > > + * This work is licensed under the terms of the GNU GPL, versi= on 2 or later. > > > > > > + * See the COPYING file in the top-level directory. > > > > > > + */ > > > > > > +#include "hw/cpu/core.h" > > > > > > + > > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp) > > > > > > +{ > > > > > > + CPUCore *core =3D CPU_CORE(obj); > > > > > > + > > > > > > + return g_strdup(core->slot); > > > > > > +} > > > > > > + > > > > > > +static void core_prop_set_slot(Object *obj, const char *val, E= rror **errp) > > > > > > +{ > > > > > > + CPUCore *core =3D CPU_CORE(obj); > > > > > > + > > > > > > + core->slot =3D g_strdup(val); > > > > > > +} > > > > > > + > > > > > > +static void cpu_core_instance_init(Object *obj) > > > > > > +{ > > > > > > + object_property_add_str(obj, "slot", core_prop_get_slot, c= ore_prop_set_slot, > > > > > > + NULL); > > > > > > +} > > > > > > + > > > > > > +static const TypeInfo cpu_core_type_info =3D { > > > > > > + .name =3D TYPE_CPU_CORE, > > > > > > + .parent =3D TYPE_DEVICE, > > > > > > + .abstract =3D true, > > > > > > + .instance_size =3D sizeof(CPUCore), > > > > > > + .instance_init =3D cpu_core_instance_init, > > > > > > +}; > > > > > > + > > > > > > +static void cpu_core_register_types(void) > > > > > > +{ > > > > > > + type_register_static(&cpu_core_type_info); > > > > > > +} > > > > > > + > > > > > > +type_init(cpu_core_register_types) > > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h > > > > > > new file mode 100644 > > > > > > index 0000000..2daa724 > > > > > > --- /dev/null > > > > > > +++ b/include/hw/cpu/core.h > > > > > > @@ -0,0 +1,30 @@ > > > > > > +/* > > > > > > + * CPU core abstract device > > > > > > + * > > > > > > + * Copyright (C) 2016 Bharata B Rao > > > > > > + * > > > > > > + * This work is licensed under the terms of the GNU GPL, versi= on 2 or later. > > > > > > + * See the COPYING file in the top-level directory. > > > > > > + */ > > > > > > +#ifndef HW_CPU_CORE_H > > > > > > +#define HW_CPU_CORE_H > > > > > > + > > > > > > +#include "qemu/osdep.h" > > > > > > +#include "hw/qdev.h" > > > > > > + > > > > > > +#define TYPE_CPU_CORE "cpu-core" > > > > > > + > > > > > > +#define CPU_CORE(obj) \ > > > > > > + OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE) > > > > > > + > > > > > > +typedef struct CPUCore { > > > > > > + /*< private >*/ > > > > > > + DeviceState parent_obj; > > > > > > + > > > > > > + /*< public >*/ > > > > > > + char *slot; > > > > > > +} CPUCore; > > > > > > + > > > > > > +#define CPU_CORE_SLOT_PROP "slot" =20 > > > > > as it's generic property I'd rename to 'core' so it would fit all= users =20 > > > >=20 > > > > Ok. Also note that this is a string property which is associated wi= th the > > > > link name (string) that we created from machine object to this core= =2E I think > > > > it would be ideal if this becomes an interger property in which ca= se it > > > > becomes easier to feed the core location into your CPUSlotPropertie= s.core. =20 > > > agreed, it should be core number. =20 > >=20 > > The slot stuff is continuing to confuse me a bit. I see that we need > > some kind of "address" value, but how best to do it is not clear to > > me. > >=20 > > Changing this to an integer sounds like it's probably a good idea. > > I'm a bit wary of just calling it "core" though. Do all platforms > > even necessarily have a core id? > platform's that don't have core concept could or even should > use its own base type (i.e. not cpu-core). Hmm.. that's a good point. And actually makes me inclined to suggest including the cpu model property in the base type, contrary to my suggestion earlier. I can think of (somewhat contrived) cases of cpu packages where cpu_model doesn't make sense (e.g. a multi-chip bigLITTLE system, since there are multiple CPU types in a package), but in that case the package doesn't really resemble a "core" in any normal sense. > Numeric code id should work for x86, ARM and Power. Yes. I think a numeric id should be fine in general. Whether it's actually meaningful with regard to platform docs, or completely arbitrary might vary by platform, but it should be possible to create something. > > I'm wondering if the addressing is something that needs to move the > > the platform specific subtypes, while some other stuff can move to the > > generic base type. > core id looks to me as cpu-core property but I won't object if > it will be moved to subtype as so far only Power would have it. >=20 > What I'd prefer to keep is consistent naming of properties > if it's possible, i.e. property 'core' which makes sense for x86, ARM, an= d Power > from enduser point of view. >=20 > >=20 > > > > > on top of that I'd add numeric 'threads' property to base class so > > > > > all derived cores would inherit it. > > > > >=20 > > > > > Then as easy integration with -smp threads=3Dx, a machine could p= ush > > > > > a global variable 'cpu-core.threads=3D[smp_threads]' which would > > > > > make every created cpu-core object to have threads set > > > > > at instance_init() time (device_init). > > > > >=20 > > > > > That way user won't have to specify 'threads=3Dy' for every > > > > > device_add spapr-core,core=3Dx > > > > > as it will be taken from global property 'cpu-core.threads' > > > > > but if user wishes he/she still could override global by explicit= ly > > > > > providing thread property at device_add time: > > > > > device_add spapr-core,core=3Dx,threads=3Dy > > > > >=20 > > > > > wrt this series it would mean, instead of creating threads in pro= perty > > > > > setter, delaying threads creation to core.realize() time, > > > > > but since realize is allowed to fail it should be fine do so. = =20 > > > >=20 > > > > Ok that would suit us as there are two properties on which thread c= reation > > > > is dependent upon: nr_threads and cpu_model. If thread objects can = be > > > > created at core realize time, then we don't have to resort to the u= gliness > > > > of creating the threads from either of the property setters. I alwa= ys > > > > assumed that we shouldn't be creating objects from realize, but if = that > > > > is fine, it is good. =20 > > > since realize is allowed to fail, it should be safe from hotplug pov > > > to create internal objects there, as far as proper cleanups are done > > > for failure path. =20 > >=20 > [...] > > I'm not clear from the above if you're also intending to move at least > > the adding of the threads as child properties is supposed to go into > > the base type, > I'm not sure that I've got question, could you please rephrase? So, it seems like we're agreed that moving the nr_threads property to the base type is a good idea. My question is, do we also move the object_property_add_child() calls for each thread to the base type (possibly via a helper function or method the base type provides to derived types)? > > but that also sounds like a good idea, again for > > consistency. --=20 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 --D2rVImvqWGvKULsk Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW3lRzAAoJEGw4ysog2bOS2AcP/iWbfbP545lJ2hPBokhRDJ/e Zu79C7qes5sEc6VrvCMEuXik5cauKdVwDN63c+I+Ljlsk9LVEsIv2oM2knwboGuC KShaYU3PaY05f7T7kn5OK+A1P71lqyKDY1ejaijth+D6wcFpK2ewDYhPZHB9mjGt RgwM7YElvsLdraEbKPVwG/YqwdCFCST5qRMBOuMbMBU4q1LYGHMavkmgXWfaFE6p hK3TaHUyRF+6QApJRsRR+BGfeBO5KHlIUdi7tym6yIA+7hpte3b3Svshn40lR65n Xzk3yT4ujVpwqZrh6hOUHwQ7nHMQAAlXdTFxsHI1Sa61/fv9trW1lJHNnqokcKRg 3AGrzP18HNpaZ5RBsS5VV5YpM7Na9LCQ+k89dl0fMW82q6curi7FvVv4o85VZA2T GoUF8kb8nQFRT226yMviJcyWtpqynpRZFM36fwB0C4CO4JO6ad7TYMp6cpbqtsug LDdyRBKUJl2/zjne0AbhX3o8kz4lDYOvsELG2kLYe3qHqUl4C9BEankzKsQttL2U fLeo55nYyLB5d71hFtLrGXtvKCFw3hlzr6vofF+ZCTt+iT4mCCjkuD7Mw6w5d5Ml 5qZIrB41zz2WAbhEKSVnzQnvWc9NZYMNIOgvPIOGCxyjDYcY0GYTBT6Ci/U3swiZ FAlp185I9jdjBpwmfgCW =Xsoc -----END PGP SIGNATURE----- --D2rVImvqWGvKULsk--