On Tue, Jan 12, 2016 at 05:06:34PM +1100, David Gibson wrote:
> On Fri, Jan 08, 2016 at 12:25:19PM +0530, Bharata B Rao wrote:
> > Remove the CPU core device by removing the underlying CPU thread devices.
> > Support hot removal of CPU for sPAPR guests by sending the hot unplug
> > notification to the guest via EPOW interrupt. Release the vCPU object
> > after CPU hot unplug so that vCPU fd can be parked and reused.
> >
> > Signed-off-by: Bharata B Rao <bhar...@linux.vnet.ibm.com>
> > ---
> > hw/ppc/spapr.c | 113
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr.h | 6 +++
> > 2 files changed, 119 insertions(+)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c2af9ca..43cb726 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -93,6 +93,9 @@
> >
> > #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift))
> >
> > +static QLIST_HEAD(cpu_unplug_list, sPAPRCPUUnplug) cpu_unplug_list
> > + = QLIST_HEAD_INITIALIZER(cpu_unplug_list);
> > +
> > static XICSState *try_create_xics(const char *type, int nr_servers,
> > int nr_irqs, Error **errp)
> > {
> > @@ -2432,11 +2435,121 @@ static void
> > spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > }
> > }
> >
> > +static void spapr_cpu_destroy(PowerPCCPU *cpu)
> > +{
> > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +
> > + xics_cpu_destroy(spapr->icp, cpu);
> > + qemu_unregister_reset(spapr_cpu_reset, cpu);
> > +}
> > +
> > +static void spapr_cpu_core_cleanup(void)
> > +{
> > + sPAPRCPUUnplug *unplug, *next;
> > + Object *cpu;
> > +
> > + QLIST_FOREACH_SAFE(unplug, &cpu_unplug_list, node, next) {
> > + cpu = unplug->cpu;
> > + object_unparent(cpu);
> > + QLIST_REMOVE(unplug, node);
> > + g_free(unplug);
> > + }
> > +}
> > +
> > +static void spapr_add_cpu_to_unplug_list(Object *cpu)
> > +{
> > + sPAPRCPUUnplug *unplug = g_malloc(sizeof(*unplug));
> > +
> > + unplug->cpu = cpu;
> > + QLIST_INSERT_HEAD(&cpu_unplug_list, unplug, node);
> > +}
> > +
> > +static int spapr_cpu_release(Object *obj, void *opaque)
> > +{
> > + DeviceState *dev = DEVICE(obj);
> > + CPUState *cs = CPU(dev);
> > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > + spapr_cpu_destroy(cpu);
> > + cpu_remove_sync(cs);
> > +
> > + /*
> > + * We are still walking the core object's children list, and
> > + * hence can't cleanup this CPU thread object just yet. Put
> > + * it on a list for later removal.
> > + */
> > + spapr_add_cpu_to_unplug_list(obj);
> > + return 0;
> > +}
> > +
> > +static void spapr_core_release(DeviceState *dev, void *opaque)
> > +{
> > + object_child_foreach(OBJECT(dev), spapr_cpu_release, opaque);
> > + spapr_cpu_core_cleanup();
> > + object_unparent(OBJECT(dev));
>
> I'd prefer to see the unplug_list as a local to this function (passed
> to spapr_cpu_release via opaque) rather than using a new global.
Will try that in the next iteration.
>
> > +}
> > +
> > +static int spapr_core_detach(Object *obj, void *opaque)
> > +{
> > + sPAPRCoreState *core = opaque;
> > + DeviceState *dev = DEVICE(obj);
> > + CPUState *cs = CPU(dev);
> > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > + int id = ppc_get_vcpu_dt_id(cpu);
> > + int smt = kvmppc_smt_threads();
> > + sPAPRDRConnector *drc =
> > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> > + sPAPRDRConnectorClass *drck;
> > + Error *local_err = NULL;
> > +
> > + /*
> > + * SMT threads return from here, only main thread (thread 0) will
> > + * continue and signal hot unplug event to the guest.
> > + */
>
> This seems silly. spapr_core_unplug() iterates through all the
> children only to have all of them except thread 0 ignore the call..
>
> Can't you just grab the first thread and do this logic directly from
> spapr_core_unplug()?
If I purely rely on the children list of the core object like I am doing
here, I don't see how to grab the first thread object from the list w/o
doing what I am doing here. However I can store the first thread object
in the core object during the core object's instance_init and use it here.
Will give it a try in the next iteration.
Regards,
Bharata.
