Message ID | 20180517081527.14410-9-david@redhat.com |
---|---|
State | New |
Headers | show |
Series | MemoryDevice: use multi stage hotplug handlers | expand |
On Thu, 17 May 2018 10:15:21 +0200 David Hildenbrand <david@redhat.com> wrote: > Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ > unplug memory devices (which a pc-dimm is) later. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/ppc/spapr.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 2f315f963b..286c38c842 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3291,7 +3291,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms, > /* Callback to be called during DRC release. */ > void spapr_lmb_release(DeviceState *dev) > { > - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev)); > + HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev); > + sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl); > sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); > > /* This information will get lost if a migration occurs > @@ -3309,9 +3310,21 @@ void spapr_lmb_release(DeviceState *dev) > > /* > * Now that all the LMBs have been removed by the guest, call the > - * pc-dimm unplug handler to cleanup up the pc-dimm device. > + * unplug handler chain. This can never fail. > */ > - pc_dimm_memory_unplug(dev, MACHINE(spapr)); > + hotplug_ctrl = qdev_get_hotplug_handler(dev); > + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); > +} > + > +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > + Error **errp) > +{ > + sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev); > + sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); > + g_assert(ds); > + g_assert(!ds->nr_lmbs); Theses 2 lines seems to unrelated to patch topic, could you drop it? if these values should be checked, it would be better to audit 'ds' use across spapr.c and file separate patch separately from this series. > + pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev)); > object_unparent(OBJECT(dev)); > spapr_pending_dimm_unplugs_remove(spapr, ds); > } > @@ -3608,7 +3621,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > Error *local_err = NULL; > > /* final stage hotplug handler */ > - if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > + spapr_memory_unplug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, > &local_err); > } otherwise, ignoring dev->parent_bus parts, patch looks reasonable
On Thu, May 17, 2018 at 10:15:21AM +0200, David Hildenbrand wrote: > Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ > unplug memory devices (which a pc-dimm is) later. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/ppc/spapr.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 2f315f963b..286c38c842 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3291,7 +3291,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms, > /* Callback to be called during DRC release. */ > void spapr_lmb_release(DeviceState *dev) > { > - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev)); > + HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev); > + sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl); > sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); > > /* This information will get lost if a migration occurs > @@ -3309,9 +3310,21 @@ void spapr_lmb_release(DeviceState *dev) > > /* > * Now that all the LMBs have been removed by the guest, call the > - * pc-dimm unplug handler to cleanup up the pc-dimm device. > + * unplug handler chain. This can never fail. > */ > - pc_dimm_memory_unplug(dev, MACHINE(spapr)); > + hotplug_ctrl = qdev_get_hotplug_handler(dev); You're double initializing hotplug_ctrl to the same thing here, AFAICT. > + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); > +} > + > +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > + Error **errp) > +{ > + sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev); > + sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); > + > + g_assert(ds); > + g_assert(!ds->nr_lmbs); > + pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev)); > object_unparent(OBJECT(dev)); > spapr_pending_dimm_unplugs_remove(spapr, ds); > } > @@ -3608,7 +3621,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > Error *local_err = NULL; > > /* final stage hotplug handler */ > - if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > + spapr_memory_unplug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, > &local_err); > }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 2f315f963b..286c38c842 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3291,7 +3291,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms, /* Callback to be called during DRC release. */ void spapr_lmb_release(DeviceState *dev) { - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev)); + HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev); + sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl); sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); /* This information will get lost if a migration occurs @@ -3309,9 +3310,21 @@ void spapr_lmb_release(DeviceState *dev) /* * Now that all the LMBs have been removed by the guest, call the - * pc-dimm unplug handler to cleanup up the pc-dimm device. + * unplug handler chain. This can never fail. */ - pc_dimm_memory_unplug(dev, MACHINE(spapr)); + hotplug_ctrl = qdev_get_hotplug_handler(dev); + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); +} + +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, + Error **errp) +{ + sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev); + sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); + + g_assert(ds); + g_assert(!ds->nr_lmbs); + pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev)); object_unparent(OBJECT(dev)); spapr_pending_dimm_unplugs_remove(spapr, ds); } @@ -3608,7 +3621,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, Error *local_err = NULL; /* final stage hotplug handler */ - if (dev->parent_bus && dev->parent_bus->hotplug_handler) { + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { + spapr_memory_unplug(hotplug_dev, dev, &local_err); + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, &local_err); }
Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/ppc/spapr.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)