On Wed, Jan 13, 2016 at 09:40:54AM +0530, Bharata B Rao wrote:
> 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.
It should be accessible by name as "thread[0]" no?
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
