Re: [Qemu-devel] [RFC PATCH v4 08/11] spapr: CPU hotplug support

[Bharata B Rao]

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 <address@hidden>
> > ---
> >  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.