From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49665) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afyWq-0005dP-KM for qemu-devel@nongnu.org; Tue, 15 Mar 2016 19:37:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afyWn-0003ox-Da for qemu-devel@nongnu.org; Tue, 15 Mar 2016 19:37:32 -0400 Date: Wed, 16 Mar 2016 10:38:33 +1100 From: David Gibson Message-ID: <20160315233833.GK9032@voom> 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> <20160315061027.GL15272@voom.fritz.box> <20160315120506.12cb8b9f@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="BFVE2HhgxTpCzM8t" Content-Disposition: inline In-Reply-To: <20160315120506.12cb8b9f@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 --BFVE2HhgxTpCzM8t Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 15, 2016 at 12:05:06PM +0100, Igor Mammedov wrote: > On Tue, 15 Mar 2016 17:10:27 +1100 > David Gibson wrote: >=20 > > 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: =20 > > > > > 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(= DeviceState *dev, CPUState *cs, > > > > > > > > return fdt; > > > > > > > > } > > > > > > > >=20 > > > > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *= hotplug_dev, > > > > > > > > + DeviceState *dev= , Error **errp) > > > > > > > > +{ > > > > > > > > + sPAPRMachineClass *smc =3D SPAPR_MACHINE_GET_CLASS(hot= plug_dev); > > > > > > > > + sPAPRMachineState *spapr =3D SPAPR_MACHINE(hotplug_dev= ); > > > > > > > > + > > > > > > > > + if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CO= RE)) { > > > > > > > > + 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 fo= r this machine"); > > > > > > > > + return; > > > > > > > > + } > > > > > > > > + if (spapr->cores[core]) { > > > > > > > > + error_setg(errp, "core %d is already present",= core); > > > > > > > > + return; > > > > > > > > + } =20 > > > > > > >=20 > > > > > > > Wondering why can't we do the above check from core's realize= fn and 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? =20 > > > 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/. =20 > (*1 ^^^) >=20 > >=20 > > 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). > >=20 > > Is it really a problem for realize() to complete if (1) is checked, > > but not (2)? > skipping 2 would do *1, (it's hard to tell what complications would > be if CPU object with incorrect properties are created) Hm, ok. > > 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? >=20 > PCI device has 2 'address' properties, 'addr' and 'bus' > checking for valid address /including busy slot/ > happens as the first step in: >=20 > pci_qdev_realize()-> > do_pci_register_device() Ah...! So the trick here is that the PCI device registers with its bus during realize(). So now I'm wondering if we should be doing an equivalent thing for CPUs: e.g. calling spapr_register_core() or something from realize(). Or is there a fundamental difference between the cases which means pre_plug() is a better choice here. > > > 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 th= e core > > > > itself. =20 > > > 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. =20 > > > that's seems to be wrong, it shouldn't even come to cpu.realize() > > > if hotplug is not supported. =20 > >=20 > > 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. > I's too late for core hotplug being merged into 2.6 > (it's still RFC and QEMU is in soft-freeze). > It would be better to fix series so that hotplug would be > done in a clean way and be ready for merging by 2.7 dev cycle opens. Yes, I know. But I'm worried that even in the 2.7 timeframe that adding callbacks to the core hotplug model could cause long arguments and delays. --=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 --BFVE2HhgxTpCzM8t Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW6Jz5AAoJEGw4ysog2bOSmxQQANj5HNRwJWGxXemlIO0AZcxD Jl26eCAJaZsDmN+Xe/w7JYYNEVt7ts/OpToN4hCSbeDGegFQF7vajsprS+vLAcFR DrfUuTNboFtWgfVtw6+FNIo9zlGavPvkQMzvqQvC52GLAZIxHVaxaagO/NFF0T83 vgGV/OIhNKl0POvo5/VVTRRwst273ctBB2C2jwsbwyBbu55hV9N9LGhtVAfkoviB 5Obo+QaLr4oH7xUUDKZ+YuDWf2WF8lVaZ06KcBmI0HpvMET+V5C2VPWFelxr5Yw8 Qso1+8RM0Hg04SXp5Uy2JtpE6gPy65LSXqrj/uWIBn3UufUPmPRiKKWYknryigOZ gJZT8G7gPr6b5x8Hg/3r85Cs4PEnOR5gv8hQDfTlHS21KOo5EUkiOPxU7r3zOS0B tsFctjM0yRHIJYWrl0BuJm55hBdMg8rqo9oqQpFIo+4Q93rGK3kDAZm0hVVXvEfC UcSIkr0KXfKzcRMga6n034ONvCLxzZBE93C6SFssiwLhkuLdjIqawwy640/YTbhb EWX1wQVUMRqF8ITXLsmv2P0RIhy4F6b+1FXisFdv3HvafJ1zXGYlHhI2dJxYCzUd ckJp4QdQUokYpOkaA4A1c4EIeflbtVCOaUfneq2KDXO9Opa0uKZnTMrBvB5vapCU JNQFWUiHanc81J4VtI70 =bd5S -----END PGP SIGNATURE----- --BFVE2HhgxTpCzM8t--