From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54008) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adt3x-0003zf-Kl for qemu-devel@nongnu.org; Thu, 10 Mar 2016 00:23:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adt3u-0002TV-DV for qemu-devel@nongnu.org; Thu, 10 Mar 2016 00:23:05 -0500 Date: Thu, 10 Mar 2016 16:22:43 +1100 From: David Gibson Message-ID: <20160310052243.GW22546@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ddJi1IsAv4W/kcDl" Content-Disposition: inline In-Reply-To: <20160309110740.2916f922@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 --ddJi1IsAv4W/kcDl Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote: > 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: > > > 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(DeviceStat= e *dev, CPUState *cs, > > > return fdt; > > > } > > >=20 > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_de= v, > > > + DeviceState *dev, Error **= 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_I= D_PROP, > > > + &error_abort); > > > + > > > + if (!smc->dr_cpu_enabled && dev->hotplugged) { > > > + error_setg(errp, "CPU hotplug not supported for this mac= hine"); > > > + 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 realizefn and fail > > the core hotplug from realizefn ? > 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. 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. > 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. > 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()) Hmm.. how big a deal is it to roll back from the existing plug() handler? --=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 --ddJi1IsAv4W/kcDl Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW4QSjAAoJEGw4ysog2bOSLrMQAKtNv3QD6+hR7qo9zIbxYkgj 30yW2UIv0dTlDdaY3HQPsCQ2kuNU02g9WAZSDMG26YkJ1D+cBQvomDDp+9D4klO0 1j7b7IR5ygTk595rnWyYLBO8AfILCw8i5XymOUdAiEjSKREl4xT0pgD7sBeTdHiI mG4kSU01f7XK+zNk1Rdzu7p3AQsVhmDvitCPfjSx2TxZLRq0qMuaKgbmJKhJVsdf ODw9Gu5zTsiHP+OmMCOxi/J+J61fBFISpNZlDa14gCSZmQcX7pxgMTzaKvqpVkmc //nRDNj6Px3CNpYS8Uj+e/PZCxFpwLR6i+sb8wR5B1ev6+f16HqnBwr3//VUZq2d Vz/hqjt2Gx13qzQmeix5Q3vxaqg/iFPvmPG1Xa15ki7CCW4xg23HNbh5DzD6q9dK nL+S4gDUHIMTVJtC6oQ4c9Y7B1q+vIoqIc1WiwI814LTCKk3Azp6M9sYeyHNyJad WzVSGZHkGggnM90V3zKZgcxxSXSw1LSuP+126Qfsc44ThjAwXd+6NV4MdkxtVwyv cDRiq5DZ7BqFDM5EozJh3TkydbqNCRqZ9/ChAl4Kxh3di/5mvL7mUTfhZFOANMcL N7gu4+6Y/SJCFnJCD52Gju9rw9kV7CI+dqC/s7j/lmJvoC9NaUh7Fxco+VP+mF6H bTSA5t56yMwHGzY2EltQ =qqSV -----END PGP SIGNATURE----- --ddJi1IsAv4W/kcDl--