From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46097) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afiTx-0006m3-F4 for qemu-devel@nongnu.org; Tue, 15 Mar 2016 02:29:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afiTv-0006OA-WA for qemu-devel@nongnu.org; Tue, 15 Mar 2016 02:29:29 -0400 Date: Tue, 15 Mar 2016 17:10:27 +1100 From: David Gibson Message-ID: <20160315061027.GL15272@voom.fritz.box> References: <1457443095-213125-1-git-send-email-imammedo@redhat.com> <1457443095-213125-5-git-send-email-imammedo@redhat.com> <20160308143412.GA29692@in.ibm.com> <20160309110740.2916f922@nial.brq.redhat.com> <20160310052243.GW22546@voom.fritz.box> <20160310060244.GB745@in.ibm.com> <20160310113946.651ecc97@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="KaGhPsiNaI6/sRd6" Content-Disposition: inline In-Reply-To: <20160310113946.651ecc97@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov 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, Bharata B Rao , pbonzini@redhat.com, mdroth@linux.vnet.ibm.com, afaerber@suse.de --KaGhPsiNaI6/sRd6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 10, 2016 at 11:39:46AM +0100, Igor Mammedov wrote: > On Thu, 10 Mar 2016 11:32:44 +0530 > Bharata B Rao wrote: >=20 > > On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote: > > > On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote: =20 > > > > On Tue, 8 Mar 2016 20:04:12 +0530 > > > > Bharata B Rao wrote: > > > > =20 > > > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote: = =20 > > > > > > Signed-off-by: Igor Mammedov > > > > > > --- > > > > > > replaced link set check removed in previous patch > > > > > > --- > > > > > > hw/ppc/spapr.c | 26 ++++++++++++++++++++++---- > > > > > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > > >=20 > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > > > index 6890a44..db33c29 100644 > > > > > > --- a/hw/ppc/spapr.c > > > > > > +++ b/hw/ppc/spapr.c > > > > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(Devi= ceState *dev, CPUState *cs, > > > > > > return fdt; > > > > > > } > > > > > >=20 > > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotp= lug_dev, > > > > > > + DeviceState *dev, Er= ror **errp) > > > > > > +{ > > > > > > + sPAPRMachineClass *smc =3D SPAPR_MACHINE_GET_CLASS(hotplug= _dev); > > > > > > + sPAPRMachineState *spapr =3D SPAPR_MACHINE(hotplug_dev); > > > > > > + > > > > > > + if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE))= { > > > > > > + int core =3D object_property_get_int(OBJECT(dev), CPU_= CORE_ID_PROP, > > > > > > + &error_abort); > > > > > > + > > > > > > + if (!smc->dr_cpu_enabled && dev->hotplugged) { > > > > > > + error_setg(errp, "CPU hotplug not supported for th= is machine"); > > > > > > + return; > > > > > > + } > > > > > > + if (spapr->cores[core]) { > > > > > > + error_setg(errp, "core %d is already present", cor= e); > > > > > > + return; > > > > > > + } =20 > > > > >=20 > > > > > Wondering why can't we do the above check from core's realizefn a= nd fail > > > > > the core hotplug from realizefn ? =20 > > > > that's rather simple, in ideal QOM world child shouldn't > > > > poke into parents internal if it could be helped. > > > > So hook provides responsibility separation where > > > > board/or something else(HotplugHandler) can do a necessary > > > > wiring of a component which is being hotplugged, without > > > > forcing hotplugged device being aware about it. =20 > > >=20 > > > Oh.. yes. Sorry, somehow I got confused and thought you were > > > suggesting a 'pre_realize()' method on the *object* rather than a > > > pre_plug hotplughandler hook. > > > =20 > > > > That's what HotplugHandler->plug callback is doing for > > > > post realize and HotplugHandler->pre_plug will do similar > > > > thing but allowing board to execute preliminary tasks > > > > (like check/set properties, amend its internal state) > > > > before object is realized. =20 > > > =20 > > > > That will make realize() cleaner as it won't have to hack > > > > into data it shouldn't and would prevent us calling unrealize() > > > > if we were to check it later at HotplugHandler->plug time. > > > > (i.e. realize() won't even have a chance to introduce side > > > > effects that should be undone with unlealize()) =20 > > >=20 > > > Hmm.. how big a deal is it to roll back from the existing plug() > > > handler? > realize shouldn't complete without error if object properties are > wrong /for ex: i.e. you create kvm vcpu thread, configure it > as already existing vcpu and have a lot fun afterwards/. It seems to me there are two sorts of checks. (1) properties that are wrong simply with reference to the CPU core itself (e.g. unsupported CPU model, impossible number of threads). (2) properties that are wrong only in the context of other CPUs or devices (e.g. core id already populated, too many cores, impossible core id). Is it really a problem for realize() to complete if (1) is checked, but not (2)? If it's so essential, I'm surprised we haven't hit this already. What happens if you try to device_add two PCI devices in the same slot? Where is that checked? > For example: now on x86 we do duplicate CPU check wrong way > by checking for duplicate of apic property from CPU code by > looping through existing CPUs. Instead it would be much cleaner > to move that check to machine which owns apic id assignment > and make it check for duplicate in pre_plug() handler. >=20 >=20 > > Since plug() handler is post-realize, rolling back involves > > deleting the threads of the core we created and finally deleting the co= re > > itself. > Even rolling back will leave some after effects, like created > KVM VCPU thread which can't be deleted and who know what else. >=20 > >We aleady do this kind of roll back when core hotplug is attemptedi > > on machine type version that don't support hotplug. > that's seems to be wrong, it shouldn't even come to cpu.realize() > if hotplug is not supported. To be clear here, I'm not saying I think pre_plug() is a bad idea. I'm just wondering if we can treat that change to the core hotplug APIs as a clean up for later, rather than a prereq for CPU hotplug. --=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 --KaGhPsiNaI6/sRd6 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW56dTAAoJEGw4ysog2bOS3XMQAJmhoqJ8V3L0UlPVpOr9VxsE BpM8AuBcPlR4Xs61oioLI1MZsKDif+IibHVfqxnlfp5IEAXhlIdRjr0TflVbCBW3 zAg11PLMVg4egsxISllCtRouc/y4m0aUGqQHNQ1EHwE3jZPaeL6+E8lpCj9R2mv3 s+BMCcXPgihUzr2HteA3ChABo4gxJgS/+M/Uk78RmNFx3vQjSek7iV+OLmrleN3t HzszTgbGHXGJ4lvsvbuK59rrlRhHpqiQTwyc2k/DO0W/UV35rH/0ArpKgw4DU5V4 fkhjVDH4qNfDJw2NGOVUq/UmCBf6oH3AhgZ3m3WIPK33QKGpKUgUBVvetSFgD2c4 UiHbJd7kZvHPNgVaMA723Tq9f3BmzZ2Pwq3bUzXJQUiSKYaMrS9S85iYR9vy4Xge gdFCfF7wzsgcs+dyYAizXMhed/Z5EoROaDIsw10pI5BWWaVr8l/vDxcXwV2SYfl9 BEImBTE39/0pLOuUh/Lhjw7L/dKSVER/TWk2C+vYTk356nnZpD/c7wMM5XVNDARl S6lhqTaSFaHCTz+GCg1Zf3UyRGUem0GR8p6TEqeM3QCo5swKC6QRrmBsjkXgEIFe ZARd5xE+KCORzKgHu7kW75U4hs7HVBlov+ReQU46x3GA2fkIrw7BO0yZK/0Nc+dU o19nmD+DbNhqozc+XYIi =CwLi -----END PGP SIGNATURE----- --KaGhPsiNaI6/sRd6--