From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49242) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adsm6-0006uG-Hg for qemu-devel@nongnu.org; Thu, 10 Mar 2016 00:04:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adsm3-0004y3-H1 for qemu-devel@nongnu.org; Thu, 10 Mar 2016 00:04:38 -0500 Date: Thu, 10 Mar 2016 16:04:29 +1100 From: David Gibson Message-ID: <20160310050429.GT22546@voom.fritz.box> References: <20160304113845.667c19ad@nial.brq.redhat.com> <20160304110253.GB5054@in.ibm.com> <20160304190720.4d64abc4@nial.brq.redhat.com> <20160307033655.GD22546@voom.fritz.box> <20160307083155.GA7164@in.ibm.com> <20160307114011.20456e8c@nial.brq.redhat.com> <20160308035710.GU22546@voom.fritz.box> <20160308101117.285bb681@nial.brq.redhat.com> <20160309025551.GI22546@voom.fritz.box> <20160309113228.1e895686@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="HL+2C3YiDdBCOf2R" Content-Disposition: inline In-Reply-To: <20160309113228.1e895686@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 --HL+2C3YiDdBCOf2R Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Mar 09, 2016 at 11:32:28AM +0100, Igor Mammedov wrote: > On Wed, 9 Mar 2016 13:55:51 +1100 > David Gibson wrote: >=20 > > On Tue, Mar 08, 2016 at 10:11:17AM +0100, Igor Mammedov wrote: > > > On Tue, 8 Mar 2016 14:57:10 +1100 > > > David Gibson wrote: > > > =20 > > > > On Mon, Mar 07, 2016 at 11:40:11AM +0100, Igor Mammedov wrote: =20 > > > > > On Mon, 7 Mar 2016 14:01:55 +0530 > > > > > Bharata B Rao wrote: > > > > > =20 > > > > > > On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote: = =20 > > > > > > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote= : =20 > > > > > > > > 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 w= rote: =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 m= achines that 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.o= bjs > > > > > > > > > > > index 0954a18..942a4bb 100644 > > > > > > > > > > > --- a/hw/cpu/Makefile.objs > > > > > > > > > > > +++ b/hw/cpu/Makefile.objs > > > > > > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) +=3D arm11m= pcore.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, version 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 ch= ar *val, Error **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_g= et_slot, core_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/c= ore.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, version 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 wou= ld fit all users =20 > > > > > > > > >=20 > > > > > > > > > Ok. Also note that this is a string property which is ass= ociated with the > > > > > > > > > link name (string) that we created from machine object to= this core. I think > > > > > > > > > it would be ideal if this becomes an interger property i= n which case it > > > > > > > > > becomes easier to feed the core location into your CPUSlo= tProperties.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 cl= ear 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 plat= forms > > > > > > > even necessarily have a core id? > > > > > > >=20 > > > > > > > I'm wondering if the addressing is something that needs to mo= ve the > > > > > > > the platform specific subtypes, while some other stuff can mo= ve to the > > > > > > > generic base type. > > > > > > > =20 > > > > > > > > > > on top of that I'd add numeric 'threads' property to ba= se class so > > > > > > > > > > all derived cores would inherit it. > > > > > > > > > >=20 > > > > > > > > > > Then as easy integration with -smp threads=3Dx, a machi= ne could push > > > > > > > > > > a global variable 'cpu-core.threads=3D[smp_threads]' wh= ich 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 e= very > > > > > > > > > > device_add spapr-core,core=3Dx > > > > > > > > > > as it will be taken from global property 'cpu-core.thre= ads' > > > > > > > > > > but if user wishes he/she still could override global b= y explicitly > > > > > > > > > > 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 thre= ads in property > > > > > > > > > > setter, delaying threads creation to core.realize() tim= e, > > > > > > > > > > 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 whic= h thread creation > > > > > > > > > is dependent upon: nr_threads and cpu_model. If thread ob= jects can be > > > > > > > > > created at core realize time, then we don't have to resor= t to the ugliness > > > > > > > > > of creating the threads from either of the property sette= rs. I always > > > > > > > > > assumed that we shouldn't be creating objects from realiz= e, but if that > > > > > > > > > is fine, it is good. =20 > > > > > > > > since realize is allowed to fail, it should be safe from ho= tplug pov > > > > > > > > to create internal objects there, as far as proper cleanups= are done > > > > > > > > for failure path. =20 > > > > > > >=20 > > > > > > > Right, moving the "nr_threads" property to the base type seem= s like a > > > > > > > good idea to me. =20 > > > > > >=20 > > > > > > And we will also move the cpu_model property (now being tracked= by > > > > > > an ObjectClass pointer) to the base type ? =20 > > > > > I'm not sure that moving cpu_model to the base class is the right= thing, > > > > > I'd keep it local to platform for now. =20 > > > >=20 > > > > I tend to agree, although I'm not sure that I could really explain = why > > > > :/ > > > > =20 > > > > > Could you have several spapr core types? One per CPU model? > > > > > That way you won't need to track cpu_model when using device_add.= =20 > > > >=20 > > > > We could in theory, but it would be pretty inconvenient. Because t= his > > > > is a paravirt platform, there really can't be any core-level > > > > difference between them, and it would mean creating a fair batch of > > > > core types for the various minor POWER7 and POWER8 variants - and > > > > needing to update this whenever IBM makes a new version. I suspect= it > > > > would also introduce more wrinkles in order to have a correct > > > > "spapr-core-host" type matching the "HOST" cpu thread type. Since = KVM > > > > (HV) only supports the HOST thread type, that's a fairly big issue.= =20 > > > Welcome to x86 world, that's roughly what we have there. =20 > >=20 > > I don't really follow you. x86 doesn't have core devices at all for > > the moment. > >=20 > > What I'm saying here is that using different core subtypes for every > > cpu subtype on power would mean 2 types for each minor variant, rather > > than just 1. > I think the same applies to cpu_model and transitioning CPUs to -device. > X86 also has a bunch of cpu_model-s which have some minor variations > but it still generates a type per cpu_model. > If some day we are to implement socket objects for x86 that would also > mean to have a socket types per each cpu model. Hmm. I guess. What concerns me is the possible combinatorial explosion of # cpu models * # of machine types leading to an enormous number of core/socket types. The other thing is that the platform specific core types belong with the machine type code, which means they can't naturally use macro magic or whatever to generate them in parallel with the thread device types in the core target-ppc code. > As I understand cpu_model is a legacy option which should translate to > a corresponding QOM type (CPU device) which could be used with > -device/device_add. Sure, but the "cpu_model" parameter could be "thread type name" just as easily. > As analogy, QEMU has legacy -net model=3Dfoo[1234] option, but when netwo= rk cards > were converted to -device interface that in the end became a set of QOM t= ypes > like -device foo[1234], it was easier in case of network cards as > they where a separate devices models to begin with, the thing to note > here is that they weren't converted to a single 'network_card' type with > 'model' property. --=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 --HL+2C3YiDdBCOf2R Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW4QBdAAoJEGw4ysog2bOS8EAP/17lXUwMap6HegMFa5LiBwn1 X+xgtxW8XQprR92oQttqc1TTG1n/7wEUbwYJ/nzjjj5wMrdoXGlyOn1ZUgQQHtqt L81vcdKrS7jsxAIvMkwjnybYuBRL21AzefMjt7sGKIWfoV1nzAEJFhA7kZ5ZZQ0Q ASN8vJQXyl8J5+EgUJ6bPCpcygpSWwUTNL2/Z2g1tYcaXDpLvdU0c5WR7tGycxCO +3HRJqVOYhrxcF5xRwriAhYIeyioaoNSO3Kih7M2d2pj+n+wekSALkH9vnl56xWC tdGeCA2BOawVFJPx3U2fRSGj7QQBoJhmWVUM5fq3g2AolzocsVOaNSwi1rOi3Z5q pXDEU1J+XYZ4JawGGSkl/TrmUPGUNOZNNKMg5lK8co3kFK5a3bt6aFcDQLhVyRIt VwtvastnnrRW0AVvIb6StG55l0/W0jqvqioI0GVi1XWwikfeThrBQ+BeATHg6VK6 NWHN9DxCKJl3/qurFAi9f2716HbOKUIxa/CiRSi0RPQR/8utoia8GhZHBmGnIf/6 /N3r/OxBi7KvE36DXfJBn14t9UkP4TP2bgPpYYSSgz0kFHAFFvYQJPgY52jVhtUa /hJOBTVTmfoYfEKtaRKLpPFFhWcc03rAYkD72xzDc/wbzhs4zZfRGkirpbwBvEVG /NniKnbcWXOd6Ksk8b4w =uJPE -----END PGP SIGNATURE----- --HL+2C3YiDdBCOf2R--