From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57512) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeKMS-0003Ys-Tu for qemu-devel@nongnu.org; Fri, 11 Mar 2016 05:32:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aeKMP-00010y-If for qemu-devel@nongnu.org; Fri, 11 Mar 2016 05:32:00 -0500 Date: Fri, 11 Mar 2016 11:31:50 +0100 From: Igor Mammedov Message-ID: <20160311113150.4ddb0305@nial.brq.redhat.com> In-Reply-To: <20160310144523.GC745@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> <20160310060244.GB745@in.ibm.com> <20160310113946.651ecc97@nial.brq.redhat.com> <20160310144523.GC745@in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: Bharata B Rao Cc: mjrosato@linux.vnet.ibm.com, thuth@redhat.com, pkrempa@redhat.com, ehabkost@redhat.com, aik@ozlabs.ru, armbru@redhat.com, agraf@suse.de, qemu-devel@nongnu.org, borntraeger@de.ibm.com, qemu-ppc@nongnu.org, pbonzini@redhat.com, David Gibson , afaerber@suse.de, mdroth@linux.vnet.ibm.com On Thu, 10 Mar 2016 20:15:23 +0530 Bharata B Rao wrote: > 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: > > > > > 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? > > 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/. > > > > 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. > > > > > > > Since plug() handler is post-realize, rolling back involves > > > deleting the threads of the core we created and finally deleting the core > > > itself. > > Even rolling back will leave some after effects, like created > > KVM VCPU thread which can't be deleted and who know what else. > > > > >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. > > Hmm that's how we dis-allowed memory hotplug on machine type versions > that don't support memory hotplug, i,e., we failed hotplug from > ->plug() handler. So ->pre_plug() seems to be the ideal place for > such early failures ? It sure looks like it's. most of pc_dimm_memory_plug() should be done at pre_plug() except of following hunk which should stay a part of plug() pc_dimm_memory_plug(): ... memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr); vmstate_register_ram(mr, dev); numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node); ... Ok, I'll try to repost QMP series on top of the latest SPAPR today, taking in account libvirt feedback I've got this week. > > Regards, > Bharata. > >