Re: [Qemu-devel] [PATCH v6 10/11] spapr: CPU hotplug support

[Bharata B Rao]

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.