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: > > <snip> > > +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.