diff mbox

[v6,11/11] spapr: CPU hot unplug support

Message ID 1452236119-24452-12-git-send-email-bharata@linux.vnet.ibm.com
State New
Headers show

Commit Message

Bharata B Rao Jan. 8, 2016, 6:55 a.m. UTC
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 <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         | 113 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |   6 +++
 2 files changed, 119 insertions(+)

Comments

David Gibson Jan. 12, 2016, 6:06 a.m. UTC | #1
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 <bharata@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.

> +}
> +
> +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()?

Same question for the plug side, though I didn't think of it when I
was looking at that.

> +    if ((id % smt) != 0) {
> +        return 0;
> +    }
> +    g_assert(drc);
> +
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    drck->detach(drc, core->dev, spapr_core_release, NULL, &local_err);
> +    if (local_err) {
> +        error_propagate(core->errp, local_err);
> +        return 1;
> +    }
> +
> +    /*
> +     * In addition to hotplugged CPUs, send the hot-unplug notification
> +     * interrupt to the guest for coldplugged CPUs started via -device
> +     * option too.
> +     */
> +    spapr_hotplug_req_remove_by_index(drc);
> +    return 0;
> +}
> +
> +static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                             Error **errp)
> +{
> +    sPAPRCoreState core;
> +
> +    core.dev = dev;
> +    core.errp = errp;
> +    object_child_foreach(OBJECT(dev), spapr_core_detach, &core);
> +}
> +
>  static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          error_setg(errp, "Memory hot unplug not supported by sPAPR");
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
> +        if (!smc->dr_cpu_enabled) {
> +            error_setg(errp, "CPU hot unplug not supported on this machine");
> +            return;
> +        }
> +        spapr_core_unplug(hotplug_dev, dev, errp);
>      }
>  }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 68d51d6..4d89016 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -88,6 +88,12 @@ typedef struct sPAPRCoreState {
>      Error **errp;
>  } sPAPRCoreState;
>  
> +/* List to store unplugged CPU objects for cleanup during unplug */
> +typedef struct sPAPRCPUUnplug {
> +    Object *cpu;
> +    QLIST_ENTRY(sPAPRCPUUnplug) node;
> +} sPAPRCPUUnplug;
> +
>  #define H_SUCCESS         0
>  #define H_BUSY            1        /* Hardware busy -- retry later */
>  #define H_CLOSED          2        /* Resource closed */
Bharata B Rao Jan. 13, 2016, 4:10 a.m. UTC | #2
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 <bharata@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.
David Gibson Jan. 13, 2016, 4:57 a.m. UTC | #3
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 <bharata@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?
Bharata B Rao Jan. 13, 2016, 7:04 a.m. UTC | #4
On Wed, Jan 13, 2016 at 03:57:00PM +1100, David Gibson wrote:
> 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 <bharata@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?

Yes, like this AFAICS:

ObjectProperty *prop = object_property_find(OBJECT(core), "thread[0]", &local_err);
Object *thread = prop->opaque;

Storing the object pointer of the main thread in the core looks simpler to
me.

Regards,
Bharata.
diff mbox

Patch

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));
+}
+
+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.
+     */
+    if ((id % smt) != 0) {
+        return 0;
+    }
+    g_assert(drc);
+
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    drck->detach(drc, core->dev, spapr_core_release, NULL, &local_err);
+    if (local_err) {
+        error_propagate(core->errp, local_err);
+        return 1;
+    }
+
+    /*
+     * In addition to hotplugged CPUs, send the hot-unplug notification
+     * interrupt to the guest for coldplugged CPUs started via -device
+     * option too.
+     */
+    spapr_hotplug_req_remove_by_index(drc);
+    return 0;
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                             Error **errp)
+{
+    sPAPRCoreState core;
+
+    core.dev = dev;
+    core.errp = errp;
+    object_child_foreach(OBJECT(dev), spapr_core_detach, &core);
+}
+
 static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         error_setg(errp, "Memory hot unplug not supported by sPAPR");
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_POWERPC_CPU_CORE)) {
+        if (!smc->dr_cpu_enabled) {
+            error_setg(errp, "CPU hot unplug not supported on this machine");
+            return;
+        }
+        spapr_core_unplug(hotplug_dev, dev, errp);
     }
 }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 68d51d6..4d89016 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -88,6 +88,12 @@  typedef struct sPAPRCoreState {
     Error **errp;
 } sPAPRCoreState;
 
+/* List to store unplugged CPU objects for cleanup during unplug */
+typedef struct sPAPRCPUUnplug {
+    Object *cpu;
+    QLIST_ENTRY(sPAPRCPUUnplug) node;
+} sPAPRCPUUnplug;
+
 #define H_SUCCESS         0
 #define H_BUSY            1        /* Hardware busy -- retry later */
 #define H_CLOSED          2        /* Resource closed */