From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:36189)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <imammedo@redhat.com>) id 1XPWuV-0003FS-JE
	for qemu-devel@nongnu.org; Thu, 04 Sep 2014 09:17:17 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <imammedo@redhat.com>) id 1XPWuP-0004RP-Nm
	for qemu-devel@nongnu.org; Thu, 04 Sep 2014 09:17:11 -0400
Received: from mx1.redhat.com ([209.132.183.28]:2591)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <imammedo@redhat.com>) id 1XPWuP-0004R7-G5
	for qemu-devel@nongnu.org; Thu, 04 Sep 2014 09:17:05 -0400
Date: Thu, 4 Sep 2014 15:16:56 +0200
From: Igor Mammedov <imammedo@redhat.com>
Message-ID: <20140904151656.4882973d@nial.usersys.redhat.com>
In-Reply-To: <20140904144441.6a88b757@nial.usersys.redhat.com>
References: <1409126919-22233-1-git-send-email-tangchen@cn.fujitsu.com>
	<1409126919-22233-4-git-send-email-tangchen@cn.fujitsu.com>
	<20140904144441.6a88b757@nial.usersys.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RESEND PATCH v3 3/8] pc: Add memory hot unplug
 support for pc machine.
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Tang Chen <tangchen@cn.fujitsu.com>
Cc: zhugh.fnst@cn.fujitsu.com, mst@redhat.com, hutao@cn.fujitsu.com, qemu-devel@nongnu.org, isimatu.yasuaki@jp.fujitsu.com, pbonzini@redhat.com

On Thu, 4 Sep 2014 14:44:41 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed, 27 Aug 2014 16:08:34 +0800
> Tang Chen <tangchen@cn.fujitsu.com> wrote:
> 
> > From: Hu Tao <hutao@cn.fujitsu.com>
> > 
> > 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 <hutao@cn.fujitsu.com>
> > Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> > ---
> >  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.