From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34728) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTjFh-00066w-OK for qemu-devel@nongnu.org; Mon, 15 Sep 2014 23:16:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTjFb-0004Mc-Ow for qemu-devel@nongnu.org; Mon, 15 Sep 2014 23:16:25 -0400 Received: from [59.151.112.132] (port=63183 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTjFb-0004Lf-25 for qemu-devel@nongnu.org; Mon, 15 Sep 2014 23:16:19 -0400 Message-ID: <5417ABD2.8070809@cn.fujitsu.com> Date: Tue, 16 Sep 2014 11:17:38 +0800 From: Tang Chen MIME-Version: 1.0 References: <1409126919-22233-1-git-send-email-tangchen@cn.fujitsu.com> <1409126919-22233-2-git-send-email-tangchen@cn.fujitsu.com> <20140904141551.740e43fc@nial.usersys.redhat.com> In-Reply-To: <20140904141551.740e43fc@nial.usersys.redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RESEND PATCH v3 1/8] acpi, piix4: Add memory hot unplug support for piix4. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov 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 Hi Igor, On 09/04/2014 08:15 PM, Igor Mammedov wrote: > On Wed, 27 Aug 2014 16:08:32 +0800 > Tang Chen wrote: > >> From: Hu Tao >> >> Implement acpi_memory_unplug_cb(), sending an sci to guest to trigger >> memory hot-remove, and call it in piix4_device_unplug_cb(). >> >> Signed-off-by: Hu Tao >> Signed-off-by: Tang Chen >> --- >> hw/acpi/memory_hotplug.c | 25 +++++++++++++++++++++++++ >> hw/acpi/piix4.c | 3 +++ >> include/hw/acpi/memory_hotplug.h | 2 ++ >> 3 files changed, 30 insertions(+) >> >> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c >> index ed39241..c310b44 100644 >> --- a/hw/acpi/memory_hotplug.c >> +++ b/hw/acpi/memory_hotplug.c >> @@ -195,6 +195,31 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, >> return; >> } >> >> +void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, >> + DeviceState *dev, Error **errp) >> +{ >> + Error *local_err = NULL; >> + int slot = object_property_get_int(OBJECT(dev), "slot", &local_err); > might be worth to use PC_DIMM_SLOT_PROP here, and while at it > do the same to acpi_memory_plug_cb() as well in separate patch. > Followed. >> + >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + >> + if (slot >= mem_st->dev_count) { >> + char *dev_path = object_get_canonical_path(OBJECT(dev)); >> + error_setg(errp, "acpi_memory_plug_cb: " >> + "device [%s] returned invalid memory slot[%d]", >> + dev_path, slot); >> + g_free(dev_path); >> + return; >> + } > move 3 above blocks into separate function to reduce code duplication > with acpi_memory_plug_cb(), perhaps something like following: > > MemStatus * > acpi_memory_get_slot_status_descriptor(MemHotplugState *mem_st, > DeviceState *dev, Error **errp); Followed. >> + > place here also missing removal signaling for selected mdev, > perhaps it's due to wrong patch ordering It is not a wrong ordering of patches. I meant to define mdev->is_removing in patch 6/8. But it is OK to define it here. So followed. >> + /* do ACPI magic */ >> + ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS; >> + acpi_update_sci(ar, irq); > worth to make this block a separate function and reuse in > acpi_memory_plug_cb() Followed. Thanks. >> +} >> + >> static const VMStateDescription vmstate_memhp_sts = { >> .name = "memory hotplug device state", >> .version_id = 1, >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c >> index b72b34e..c1d5187 100644 >> --- a/hw/acpi/piix4.c >> +++ b/hw/acpi/piix4.c >> @@ -362,6 +362,9 @@ static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev, >> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { >> acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev, >> errp); >> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { >> + acpi_memory_unplug_cb(&s->ar, s->irq, &s->acpi_memory_hotplug, dev, >> + errp); >> } else { >> error_setg(errp, "acpi: device unplug request for not supported device" >> " type: %s", object_get_typename(OBJECT(dev))); >> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h >> index 7bbf8a0..fc6b868 100644 >> --- a/include/hw/acpi/memory_hotplug.h >> +++ b/include/hw/acpi/memory_hotplug.h >> @@ -28,6 +28,8 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner, >> >> void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, >> DeviceState *dev, Error **errp); >> +void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, >> + DeviceState *dev, Error **errp); >> >> extern const VMStateDescription vmstate_memory_hotplug; >> #define VMSTATE_MEMORY_HOTPLUG(memhp, state) \ > . >