Re: [Qemu-devel] [RESEND PATCH v3 3/8] pc: Add memory hot unplug support for pc machine.

[Igor Mammedov]

On Thu, 4 Sep 2014 14:44:41 +0200
Igor Mammedov <address@hidden> wrote:

> On Wed, 27 Aug 2014 16:08:34 +0800
> Tang Chen <address@hidden> wrote:
> 
> > From: Hu Tao <address@hidden>
> > 
> > Implement device unplug callback for PCMachine. And it now only support
> > pc-dimm hot-remove. The callback will call piix4 or ich9 callbacks 
> > introduced
> > in previous patches.
> > 
> > Signed-off-by: Hu Tao <address@hidden>
> > Signed-off-by: Tang Chen <address@hidden>
> > ---
> >  hw/i386/pc.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 8fa8d2f..8bafe7c 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -61,6 +61,8 @@
> >  #include "hw/mem/pc-dimm.h"
> >  #include "trace.h"
> >  #include "qapi/visitor.h"
> > +#include "hw/acpi/piix4.h"
> > +#include "hw/i386/ich9.h"
> >  
> >  /* debug PC/ISA interrupts */
> >  //#define DEBUG_IRQ
> > @@ -1607,6 +1609,27 @@ out:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > +static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> > +                           DeviceState *dev, Error **errp)
> > +{
> > +    Object *acpi_dev;
> > +    HotplugHandlerClass *hhc;
> > +    Error *local_err = NULL;
> > +
> > +    acpi_dev = (acpi_dev = piix4_pm_find()) ? acpi_dev : ich9_lpc_find();
> > +    if (!acpi_dev) {
> > +        error_setg(&local_err,
> > +                   "memory hotplug is not enabled: missing acpi device");
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> drop/replace above block, see: "if (!pcms->acpi_dev) {" for reference
> 
> > +
> > +    hhc = HOTPLUG_HANDLER_GET_CLASS(acpi_dev);
> > +    hhc->unplug(HOTPLUG_HANDLER(acpi_dev), dev, &local_err);
> 
> you should take care about dev's vmstate here, that was registered in 
> pc_dimm_plug()
I'm wrong here, vmstate_unregister should take place at pc_dimm unrealize time,

hhc->unplug handler is more like unplug_request handler,
perhaps we need to give some thought to renaming it to unplug_request to avoid 
confusion
and have an optional unplug callback that could do tear-down with 
HotplugHandler if needed
before device is unrealized.