From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41989) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aY19e-00054g-Ev for qemu-devel@nongnu.org; Mon, 22 Feb 2016 19:48:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aY19a-0005lc-5n for qemu-devel@nongnu.org; Mon, 22 Feb 2016 19:48:42 -0500 Received: from ozlabs.org ([103.22.144.67]:55257) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aY19Z-0005kc-Jy for qemu-devel@nongnu.org; Mon, 22 Feb 2016 19:48:38 -0500 Date: Tue, 23 Feb 2016 10:24:16 +1100 From: David Gibson Message-ID: <20160222232416.GO2808@voom.fritz.box> References: <1456117285-22273-1-git-send-email-bharata@linux.vnet.ibm.com> <1456117285-22273-5-git-send-email-bharata@linux.vnet.ibm.com> <56CAAE58.3060705@suse.de> <20160222074758.GJ2808@voom.fritz.box> <56CB3036.7050900@suse.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="yLaBmHMi4cq+C/u4" Content-Disposition: inline In-Reply-To: <56CB3036.7050900@suse.de> Subject: Re: [Qemu-devel] [RFC PATCH v0 4/8] spapr: Introduce CPU core device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: ehabkost@redhat.com, aik@ozlabs.ru, armbru@redhat.com, Bharata B Rao , agraf@suse.de, qemu-devel@nongnu.org, pbonzini@redhat.com, imammedo@redhat.com --yLaBmHMi4cq+C/u4 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Feb 22, 2016 at 04:58:46PM +0100, Andreas F=E4rber wrote: > Am 22.02.2016 um 08:47 schrieb David Gibson: > > On Mon, Feb 22, 2016 at 07:44:40AM +0100, Andreas F=E4rber wrote: > >> Am 22.02.2016 um 06:01 schrieb Bharata B Rao: > >>> sPAPR CPU core device is a container of CPU thread devices. CPU hotpl= ug is > >>> performed in the granularity of CPU core device by setting the "reali= zed" > >>> property of this device to "true". When hotplugged, CPU core creates = CPU > >>> thread devices. > >>> > >>> TODO: Right now allows for only homogeneous configurations as we depe= nd > >>> on global smp_threads and machine->cpu_model. > >>> > >>> Signed-off-by: Bharata B Rao > >>> --- > >>> hw/ppc/Makefile.objs | 1 + > >>> hw/ppc/spapr_cpu_package.c | 50 ++++++++++++++++++++++++++++= ++++++++++ > >>> include/hw/ppc/spapr_cpu_package.h | 26 ++++++++++++++++++++ > >>> 3 files changed, 77 insertions(+) > >>> create mode 100644 hw/ppc/spapr_cpu_package.c > >>> create mode 100644 include/hw/ppc/spapr_cpu_package.h > >>> > >>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs > >>> index c1ffc77..3000982 100644 > >>> --- a/hw/ppc/Makefile.objs > >>> +++ b/hw/ppc/Makefile.objs > >>> @@ -4,6 +4,7 @@ obj-y +=3D ppc.o ppc_booke.o > >>> obj-$(CONFIG_PSERIES) +=3D spapr.o spapr_vio.o spapr_events.o > >>> obj-$(CONFIG_PSERIES) +=3D spapr_hcall.o spapr_iommu.o spapr_rtas.o > >>> obj-$(CONFIG_PSERIES) +=3D spapr_pci.o spapr_rtc.o spapr_drc.o spapr= _rng.o > >>> +obj-$(CONFIG_PSERIES) +=3D spapr_cpu_package.o > >>> ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy) > >>> obj-y +=3D spapr_pci_vfio.o > >>> endif > >>> diff --git a/hw/ppc/spapr_cpu_package.c b/hw/ppc/spapr_cpu_package.c > >>> new file mode 100644 > >>> index 0000000..3120a16 > >>> --- /dev/null > >>> +++ b/hw/ppc/spapr_cpu_package.c > >>> @@ -0,0 +1,50 @@ > >>> +/* > >>> + * sPAPR CPU package 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 o= r later. > >>> + * See the COPYING file in the top-level directory. > >>> + */ > >>> +#include "hw/cpu/package.h" > >>> +#include "hw/ppc/spapr_cpu_package.h" > >>> +#include "hw/boards.h" > >>> +#include > >>> +#include "qemu/error-report.h" > >>> + > >>> +static void spapr_cpu_package_instance_init(Object *obj) > >>> +{ > >>> + int i; > >>> + CPUState *cpu; > >>> + MachineState *machine =3D MACHINE(qdev_get_machine()); > >>> + sPAPRCPUPackage *package =3D SPAPR_CPU_PACKAGE(obj); > >>> + > >>> + /* Create as many CPU threads as specified in the topology */ > >>> + for (i =3D 0; i < smp_threads; i++) { > >>> + cpu =3D cpu_generic_init(machine->cpu_type, machine->cpu_mod= el); > >> > >> No, no, no. This is horribly violating QOM design. > >=20 > > Ok.. why? There does not, to me, seem to be any remotely easily > > discoverable means of finding out what QOM design principles are. > >=20 > > It also would have been nice if you weighed in on my RFC this code is > > based on. So, first, apologies for my grumpy tone. This is just one of a number of frustrations I've had in the last few days, putting me in an uncharitable frame of mind. > It would've been nice had you joined our KVM call prompted by _your_ > suggestions Ok, a) I was not aware that this call was prompted by my suggestions. I got some vague second hand notices about it; as I do about KVM calls =66rom time to time, which I ignored, as usual, because b) the KVM call is at stupid o'clock for me. > (which again you could've discussed at KVM Forum where I had > a session specifically for discussing this topic), but you didn't. At KVM Forum, cpu hotplug wasn't yet on my radar. > I did discuss several aspects on the call and in the recorded session > and will not write it by email again. Feel free to put it on the call > agenda again. >=20 > I told Bharata that you can't have a base type that suddenly mutates its > topology, Ok, I'm not entirely sure what you mean by that. Do you mean the fact that the core object constructs its own sub-objects? > therefore we discussed the possibility of having a QOM > _interface_, not a base type as apparently done here. We deliberately do > not have multi-inheritence in QOM; there's not just ppc in the world. So, in other discussions I've realised the package needs to be an interface, rather than a type. But the patch your objecting to here implements the spapr-core subtype. If that were implementing rather than inheriting "cpu-package" I don't see that it would really change the logic here. Like you (I think) I do dislike the use of machine->cpu_type and machine->cpu_model. I just haven't figured out enough QOM to know how to avoid them. What is the QOM equivalent of, essentially, arguments to a constructor? > My time for reading list mails is limited. Make it easy for me to > understand what the difference is between your package and my core and > why instead of reusing my posted cpu-core type you need to propose your Because my time for reading the list is also limited, so I haven't seen your cpu-core posting. I did see Bharata's earlier core based stuff which didn't seem to go over well either. > own cpu-package type. In one point you are right, we keep going in > circles and sometimes backwards rather than forward here. But, also, from what I can tell of those parts of the discussion I have seen, locking the hotplug at core level doesn't seem to work. It seems to create an awful backwards compatibility tangle for x86 which has existing thread-level hotplug, and I expect it would cause problems when we encounter a platform with socket level hotplug only (this seems very plausible for something modelling physical hotplug). The idea of cpu-package was to abstract away the granularity of cpu hotplug from a fixed level of the socket/core/thread heirarchy, while still making that granularity easy to discover for management. --=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 --yLaBmHMi4cq+C/u4 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWy5ifAAoJEGw4ysog2bOSFT0P/j4YThsg/kB0a9OfRg5Xfq35 jwJoMgqRyLOueqn5Zs3tXY6wAP41eU4cM6BbgYYVmDHdimOMnoujDkMzoALgc4aa 8XSnxNlpYWBSACNyG91AwwmN5KJ/EmARZVmvchoikG4iOIS08+ZWsqXQKUTKVq9H 1KJLcBH+Wpk1ZR4XLntncoLmzdnOdCwsxb25pJEXM55yAXer01fAItdxzIz9NAAt F0Y5rnHM07vfoFqZ5bQUdtQb3mDTK+GVVU7x3jTKx5yuHYfRn+k2o+5S/1lwUYX2 HVLRZKWZ5SAALXU+afOlWmMjPV6b7WzvjrvLiwTmmyZjWHPTWXZu57jowkOg57xN IplsgxuHzFUN5UUqP2N6K5cl1TUc13HfwoqE7lbZnIhXXCvi/OpetAy78tzPNgLX D9vEtjtXK+RLam1DyT+lhEvQ05t1QB98ZYNXPU+AJKjaQ89B+Qgk5/LhId8c9/jB kQ9KXd+rouuI9UqMSlV0Bfm1+1Rt99yqnSBI7Aw6giJNXaMbTiyA4/1w0h87vKVr Lr6BhjSu7E9p8IpqfKxhTYSLOsKv7p4HxoDJ9StfvERJXYZ8hlnBOK7EiK5691A8 503uQOaABY02laufp+wcJx/mgSjctI7WGo/6GxvuTskc8qhqMnk4RLtT3QqpVkS6 lxB3A2Wdk9UKSDi9mCzB =4W9c -----END PGP SIGNATURE----- --yLaBmHMi4cq+C/u4--