From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33128) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adthP-00067y-2b for qemu-devel@nongnu.org; Thu, 10 Mar 2016 01:03:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adthL-0004Fm-SV for qemu-devel@nongnu.org; Thu, 10 Mar 2016 01:03:51 -0500 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:42237) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adthL-0004FU-9k for qemu-devel@nongnu.org; Thu, 10 Mar 2016 01:03:47 -0500 Received: from localhost by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 10 Mar 2016 16:03:43 +1000 Date: Thu, 10 Mar 2016 11:32:44 +0530 From: Bharata B Rao Message-ID: <20160310060244.GB745@in.ibm.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160310052243.GW22546@voom.fritz.box> Subject: Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson 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, pbonzini@redhat.com, Igor Mammedov , mdroth@linux.vnet.ibm.com, afaerber@suse.de 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: > > On Tue, 8 Mar 2016 20:04:12 +0530 > > Bharata B Rao wrote: > > > > > 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(-) > > > > > > > > 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; > > > > } > > > > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, > > > > + DeviceState *dev, Error **errp) > > > > +{ > > > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev); > > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev); > > > > + > > > > + if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > > > > + int core = 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 this machine"); > > > > + return; > > > > + } > > > > + if (spapr->cores[core]) { > > > > + error_setg(errp, "core %d is already present", core); > > > > + return; > > > > + } > > > > > > 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? Since plug() handler is post-realize, rolling back involves deleting the threads of the core we created and finally deleting the core itself. We aleady do this kind of roll back when core hotplug is attemptedi on machine type version that don't support hotplug. For the present case of rejecting the hotplug for duplicate core_ids, are you in fact hinting that instead of failing the hotplug in pre_plug() lets realize then and then roll back from plug() ? Regards, Bharata.