From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44800) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YH5YD-00046R-E3 for qemu-devel@nongnu.org; Fri, 30 Jan 2015 01:59:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YH5Y9-00033j-7f for qemu-devel@nongnu.org; Fri, 30 Jan 2015 01:59:33 -0500 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:39154) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YH5Y8-0002x2-Jk for qemu-devel@nongnu.org; Fri, 30 Jan 2015 01:59:29 -0500 Received: from /spool/local by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 30 Jan 2015 16:59:22 +1000 Received: from d23relay09.au.ibm.com (d23relay09.au.ibm.com [9.185.63.181]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 9207F2BB0040 for ; Fri, 30 Jan 2015 17:59:20 +1100 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay09.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t0U6xKEo4063462 for ; Fri, 30 Jan 2015 17:59:20 +1100 Received: from d23av04.au.ibm.com (localhost [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t0U6xIUw005242 for ; Fri, 30 Jan 2015 17:59:18 +1100 Date: Fri, 30 Jan 2015 12:29:03 +0530 From: Bharata B Rao Message-ID: <20150130065902.GA24041@in.ibm.com> References: <1420697420-16053-1-git-send-email-bharata@linux.vnet.ibm.com> <1420697420-16053-7-git-send-email-bharata@linux.vnet.ibm.com> <20150123134138.34334599@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150123134138.34334599@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH v1 06/13] spapr: CPU hotplug support Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: agraf@suse.de, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com On Fri, Jan 23, 2015 at 01:41:38PM +0100, Igor Mammedov wrote: > On Thu, 8 Jan 2015 11:40:13 +0530 > Bharata B Rao wrote: > > > Support CPU hotplug via device-add command. Use the exising EPOW event > > infrastructure to send CPU hotplug notification to the guest. > > > > Signed-off-by: Bharata B Rao > > --- > > hw/ppc/spapr.c | 205 +++++++++++++++++++++++++++++++++++++++++++- > > hw/ppc/spapr_events.c | 8 +- > > target-ppc/translate_init.c | 6 ++ > > 3 files changed, 215 insertions(+), 4 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 515d770..a293a59 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -330,6 +330,8 @@ static void add_str(GString *s, const gchar *s1) > > g_string_append_len(s, s1, strlen(s1) + 1); > > } > > > > +uint32_t cpus_per_socket; > static ??? Sure. > > + > > +static void spapr_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > + Error **errp) > > +{ > > + Error *local_err = NULL; > > + CPUState *cs = CPU(dev); > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > + sPAPRDRConnector *drc = > > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cpu->cpu_dt_id); > just rant: does this have any relation to hotplug_dev, the point here is to get > these data from hotplug_dev object/some child of it rather then via direct adhoc call. I see how hotplug_dev is being used to pass on the plug request to ACPI, but have to check how hotplug_dev can be used more meaningfully here. > > > + > > + /* TODO: Check if DR is enabled ? */ > > + g_assert(drc); > > + > > + spapr_cpu_reset(POWERPC_CPU(CPU(dev))); > reset probably should be don at realize time, see x86_cpu_realizefn() for example. Yes, can be done. > > > + spapr_cpu_hotplug_add(dev, cs); > > + spapr_hotplug_req_add_event(drc); > > + error_propagate(errp, local_err); > > + return; > > +} > > + > > +static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > + DeviceState *dev, Error **errp) > > +{ > > + if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > > + if (dev->hotplugged) { > > + spapr_cpu_plug(hotplug_dev, dev, errp); > Would be nicer if this could do cold-plugged CPUs wiring too. Yes, will check and see how intrusive change that would be. > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > index 9c642a5..cf9d8d3 100644 > > --- a/target-ppc/translate_init.c > > +++ b/target-ppc/translate_init.c > > @@ -32,6 +32,7 @@ > > #include "hw/qdev-properties.h" > > #include "hw/ppc/spapr.h" > > #include "hw/ppc/ppc.h" > > +#include "sysemu/sysemu.h" > > > > //#define PPC_DUMP_CPU > > //#define PPC_DEBUG_SPR > > @@ -8909,6 +8910,11 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp) > > return; > > } > > > > + if (cs->cpu_index >= max_cpus) { > pls note that cpu_index is monotonically increases, so when you do unplug > and then plug it will go above max_cpus or the same will happen if > one device_add fails in the middle, the next CPU add will fail because of > cs->cpu_index goes overboard. > > I'd suggest not to rely/use cpu_index for any purposes and use other means > to identify where cpu is plugged in. On x68 we slowly getting rid of this > dependency in favor of apic_id (topology information), eventually it could > become: > -device cpu_foo,socket=X,core=Y[,thread=Z][,node=N] > > you probably could do the same. > It doesn't have to be in this series, just be aware of potential issues. I see your point and this needs to be fixed as I see this causing problems with CPU removal (from the middle) and subsequent addition (which makes use of "vcpu fd parking and reuse" mechanism). Thanks for your review. Regards, Bharata.