From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54450) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZZGF-00013L-Fo for qemu-devel@nongnu.org; Wed, 09 Sep 2015 02:53:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZZGB-00044g-AN for qemu-devel@nongnu.org; Wed, 09 Sep 2015 02:53:39 -0400 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:49624) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZZGA-00044K-Od for qemu-devel@nongnu.org; Wed, 09 Sep 2015 02:53:35 -0400 Received: from /spool/local by e23smtp09.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 9 Sep 2015 16:53:31 +1000 Date: Wed, 9 Sep 2015 12:22:28 +0530 From: Bharata B Rao Message-ID: <20150909065228.GG17433@in.ibm.com> References: <1438838837-28504-1-git-send-email-bharata@linux.vnet.ibm.com> <1438838837-28504-9-git-send-email-bharata@linux.vnet.ibm.com> <20150904065838.GA6537@voom.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150904065838.GA6537@voom.redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH v4 08/11] spapr: CPU hotplug support Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: mdroth@linux.vnet.ibm.com, aik@ozlabs.ru, agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, imammedo@redhat.com, afaerber@suse.de On Fri, Sep 04, 2015 at 04:58:38PM +1000, David Gibson wrote: > On Thu, Aug 06, 2015 at 10:57:14AM +0530, Bharata B Rao wrote: > > Support CPU hotplug via device-add command. Set up device tree > > entries for the hotplugged CPU core and use the exising EPOW event > > infrastructure to send CPU hotplug notification to the guest. > > > > Create only cores explicitly from boot path as well as hotplug path > > and let the ->plug() handler of the core create the threads of the core. > > > > Also support cold plugged CPUs that are specified by -device option > > on cmdline. > > > > Signed-off-by: Bharata B Rao > > --- > > hw/ppc/spapr.c | 166 +++++++++++++++++++++++++++++++++++++++++++- > > hw/ppc/spapr_events.c | 3 + > > hw/ppc/spapr_rtas.c | 11 +++ > > target-ppc/translate_init.c | 8 +++ > > 4 files changed, 186 insertions(+), 2 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index a106980..74637b3 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -599,6 +599,18 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset, > > unsigned sockets = opts ? qemu_opt_get_number(opts, "sockets", 0) : 0; > > uint32_t cpus_per_socket = sockets ? (smp_cpus / sockets) : 1; > > uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)}; > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); > > + sPAPRDRConnector *drc; > > + sPAPRDRConnectorClass *drck; > > + int drc_index; > > + > > + if (smc->dr_cpu_enabled) { > > + drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index); > > + g_assert(drc); > > + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > + drc_index = drck->get_index(drc); > > + _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index))); > > + } > > > > _FDT((fdt_setprop_cell(fdt, offset, "reg", index))); > > _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu"))); > > @@ -1686,6 +1698,7 @@ static void ppc_spapr_init(MachineState *machine) > > char *filename; > > int smt = kvmppc_smt_threads(); > > int smp_max_cores = DIV_ROUND_UP(max_cpus, smp_threads); > > + int smp_cores = DIV_ROUND_UP(smp_cpus, smp_threads); > > This shadows the global variable 'smp_cores' which has a different > meaning, so this is a very bad idea. Oh ok, will fix this. > > static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > DeviceState *dev, Error **errp) > > { > > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); > > + sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); > > > > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > > int node; > > @@ -2192,6 +2330,29 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > } > > > > spapr_memory_plug(hotplug_dev, dev, node, errp); > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > > + CPUState *cs = CPU(dev); > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > + > > + if (!smc->dr_cpu_enabled && dev->hotplugged) { > > + error_setg(errp, "CPU hotplug not supported for this machine"); > > + cpu_remove_sync(cs); > > + return; > > + } > > + > > + if (((smp_cpus % smp_threads) || (max_cpus % smp_threads)) && > > + dev->hotplugged) { > > + error_setg(errp, "CPU hotplug not supported for the topology " > > + "with %d threads %d cpus and %d maxcpus since " > > + "CPUs can't be fit fully into cores", > > + smp_threads, smp_cpus, max_cpus); > > + cpu_remove_sync(cs); > > I'd kind of prefer to reject partial cores at initial startup, rather > than only when we actually attempt to hotplug. I am enforcing correct topologies only while hotplugging to ensure that existing guests with such topologies continue to work. If that is not required then this explicit check for only hotplugged CPUs won't be needed. If Thomas' patch ensures that we never end up in topologies with partially filled cores, then this check isn't required. Regards, Bharata.