From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45521) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJCXb-00084Z-Pv for qemu-devel@nongnu.org; Tue, 12 Jan 2016 22:56:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJCXY-0001zA-NY for qemu-devel@nongnu.org; Tue, 12 Jan 2016 22:56:11 -0500 Received: from e28smtp03.in.ibm.com ([125.16.236.3]:38395) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJCXX-0001y7-NY for qemu-devel@nongnu.org; Tue, 12 Jan 2016 22:56:08 -0500 Received: from localhost by e28smtp03.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 13 Jan 2016 09:26:02 +0530 Date: Wed, 13 Jan 2016 09:25:24 +0530 From: Bharata B Rao Message-ID: <20160113035524.GC11785@in.ibm.com> References: <1452236119-24452-1-git-send-email-bharata@linux.vnet.ibm.com> <1452236119-24452-11-git-send-email-bharata@linux.vnet.ibm.com> <20160112055844.GV22925@voom.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160112055844.GV22925@voom.redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 10/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, pbonzini@redhat.com, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, imammedo@redhat.com, afaerber@suse.de, ehabkost@redhat.com On Tue, Jan 12, 2016 at 04:58:44PM +1100, David Gibson wrote: > On Fri, Jan 08, 2016 at 12:25:18PM +0530, Bharata B Rao wrote: > > > > +static int spapr_core_attach(Object *obj, void *opaque) > > +{ > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); > > + sPAPRMachineState *ms = SPAPR_MACHINE(qdev_get_machine()); > > + sPAPRCoreState *core = opaque; > > + DeviceState *dev = DEVICE(obj); > > + CPUState *cs = CPU(dev); > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > + int id = ppc_get_vcpu_dt_id(cpu); > > + sPAPRDRConnector *drc = > > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id); > > + sPAPRDRConnectorClass *drck; > > + int smt = kvmppc_smt_threads(); > > + Error *local_err = NULL; > > + void *fdt = NULL; > > + int fdt_offset = 0; > > + > > + /* > > + * Only main SMT thread (thread 0) will continue and signal the > > + * hotplug event to the guest. Other threads of the core will > > + * return from here. > > + */ > > + if ((id % smt) != 0) { > > + return 0; > > + } > > + > > + if (!smc->dr_cpu_enabled) { > > + /* > > + * This is a cold plugged CPU but the machine doesn't support > > + * DR. So skip the hotplug path ensuring that the CPU is brought > > + * up online with out an associated DR connector. > > + */ > > + return 0; > > + } > > + > > + g_assert(drc); > > + > > + /* > > + * Setup CPU DT entries only for hotplugged CPUs. For boot time or > > + * coldplugged CPUs DT entries are setup in spapr_finalize_fdt(). > > + */ > > + if (dev->hotplugged) { > > + fdt = spapr_populate_hotplug_cpu_dt(dev, cs, &fdt_offset, ms); > > + } > > + > > + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > + drck->attach(drc, core->dev, fdt, fdt_offset, !dev->hotplugged, &local_err); > > + if (local_err) { > > + g_free(fdt); > > + error_propagate(core->errp, local_err); > > + return 1; > > + } > > + > > + /* > > + * We send hotplug notification interrupt to the guest only in case > > + * of hotplugged CPUs. > > + */ > > + if (dev->hotplugged) { > > + spapr_hotplug_req_add_by_index(drc); > > + } else { > > + /* > > + * HACK to support removal of hotplugged CPU after VM migration: > > + * > > + * Since we want to be able to hot-remove those coldplugged CPUs > > + * started at boot time using -device option at the target VM, we set > > + * the right allocation_state and isolation_state for them, which for > > + * the hotplugged CPUs would be set via RTAS calls done from the > > + * guest during hotplug. > > + * > > + * This allows the coldplugged CPUs started using -device option to > > + * have the right isolation and allocation states as expected by the > > + * CPU hot removal code. > > + * > > + * This hack will be removed once we have DRC states migrated as part > > + * of VM migration. > > + */ > > + drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE); > > + drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED); > > I'm not fully understanding why this is a hack. Aren't those the > right allocation and isolation states for a cpu that was present at > boot? Those comments are already old, will remove them. I remember Michael Roth confirming that setting the initial DRC states like this for cold plugged CPUs should be alright. > > > + } > > + return 0; > > +} > > + > > +static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > + Error **errp) > > +{ > > + sPAPRCoreState core; > > + > > + core.dev = dev; > > + core.errp = errp; > > + object_child_foreach(OBJECT(dev), spapr_core_attach, &core); > > +} > > + > > 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; > > @@ -2262,6 +2401,34 @@ 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); > > + int i; > > + > > + /* Set NUMA node for the added CPUs */ > > + for (i = 0; i < nb_numa_nodes; i++) { > > + if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) { > > + cs->numa_node = i; > > + break; > > + } > > + } > > + > > + if (!smc->dr_cpu_enabled) { > > + if (dev->hotplugged) { > > + error_setg(errp, "CPU hotplug not supported for this machine"); > > + cpu_remove_sync(cs); > > + return; > > + } else { > > + spapr_cpu_init(ms, cpu); > > You could just continue onto the code below, yes? the cpu_reset() > would be unnecessary but harmless IIUC. Will do that. > > > + return; > > + } > > + } > > + > > + spapr_cpu_init(ms, cpu); > > + spapr_cpu_reset(cpu); > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) { > > + spapr_core_plug(hotplug_dev, dev, errp); > > So, I see that there are branches here for both individual vcpu > objects and for cpu core objects. I'm assuming it's only intended > that the user add core objects, and the vcpu path is for the vcpus > constructed by the core object. Is that right? That's correct. > > Does anything enforce that the user can't directly device_add a vcpu > object? CPU objects (like host-powerpc64-cpu or POWER8-powerpc64-cpu etc) will not be exposed to device_add command since they don't have cannot_instantiate_with_device_add_yet memer of their DeviceClass set to false. Regards, Bharata.