From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33232) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2EWq-0006hL-VV for qemu-devel@nongnu.org; Thu, 03 Nov 2016 05:41:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2EWm-0001VI-2X for qemu-devel@nongnu.org; Thu, 03 Nov 2016 05:41:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53136) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2EWl-0001Ux-RN for qemu-devel@nongnu.org; Thu, 03 Nov 2016 05:41:44 -0400 Date: Thu, 3 Nov 2016 10:41:39 +0100 From: Igor Mammedov Message-ID: <20161103104139.01bb17e2@nial.brq.redhat.com> In-Reply-To: References: <1477850917-1214-1-git-send-email-mst@redhat.com> <1477850917-1214-40-git-send-email-mst@redhat.com> <20161102121900.675a4e07@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 39/47] pc: memhp: enable nvdimm device hotplug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Peter Maydell , Paolo Bonzini , Richard Henderson , Eduardo Habkost On Thu, 3 Nov 2016 00:00:45 +0800 Xiao Guangrong wrote: > On 11/02/2016 07:19 PM, Igor Mammedov wrote: > > On Sun, 30 Oct 2016 23:25:10 +0200 > > "Michael S. Tsirkin" wrote: > > > >> From: Xiao Guangrong > >> > >> _GPE.E04 is dedicated for nvdimm device hotplug > >> > >> Signed-off-by: Xiao Guangrong > >> Reviewed-by: Michael S. Tsirkin > >> Signed-off-by: Michael S. Tsirkin > >> --- > >> include/hw/acpi/acpi_dev_interface.h | 1 + > >> hw/acpi/memory_hotplug.c | 31 +++++++++++++++++++++++-------- > >> hw/i386/acpi-build.c | 7 +++++++ > >> hw/i386/pc.c | 12 ++++++++++++ > >> hw/mem/nvdimm.c | 4 ---- > >> docs/specs/acpi_mem_hotplug.txt | 3 +++ > >> 6 files changed, 46 insertions(+), 12 deletions(-) > >> > >> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h > >> index da4ef7f..901a4ae 100644 > >> --- a/include/hw/acpi/acpi_dev_interface.h > >> +++ b/include/hw/acpi/acpi_dev_interface.h > >> @@ -10,6 +10,7 @@ typedef enum { > >> ACPI_PCI_HOTPLUG_STATUS = 2, > >> ACPI_CPU_HOTPLUG_STATUS = 4, > >> ACPI_MEMORY_HOTPLUG_STATUS = 8, > >> + ACPI_NVDIMM_HOTPLUG_STATUS = 16, > >> } AcpiEventStatusBits; > >> > >> #define TYPE_ACPI_DEVICE_IF "acpi-device-interface" > >> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > >> index ec4e64b..70f6451 100644 > >> --- a/hw/acpi/memory_hotplug.c > >> +++ b/hw/acpi/memory_hotplug.c > >> @@ -2,6 +2,7 @@ > >> #include "hw/acpi/memory_hotplug.h" > >> #include "hw/acpi/pc-hotplug.h" > >> #include "hw/mem/pc-dimm.h" > >> +#include "hw/mem/nvdimm.h" > >> #include "hw/boards.h" > >> #include "hw/qdev-core.h" > >> #include "trace.h" > >> @@ -232,11 +233,8 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, > >> DeviceState *dev, Error **errp) > >> { > >> MemStatus *mdev; > >> - DeviceClass *dc = DEVICE_GET_CLASS(dev); > >> - > >> - if (!dc->hotpluggable) { > >> - return; > >> - } > > shouldn't be removed. > > Well, all memory devices support hotplug now, i thought it > is reasonable to drop it, actually it was introduced by nvdimm... this hunk could still be useful if we would have non hotpluggable pc-dimm but yes for now you can remove it, a separate patch would be even better. > > > > >> + AcpiEventStatusBits event; > >> + bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM); > >> > >> mdev = acpi_memory_slot_status(mem_st, dev, errp); > >> if (!mdev) { > >> @@ -244,10 +242,23 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, > >> } > >> > >> mdev->dimm = dev; > >> - mdev->is_enabled = true; > >> + > >> + /* > >> + * do not set is_enabled and is_inserting if the slot is plugged with > >> + * a nvdimm device to stop OSPM inquires memory region from the slot. > >> + */ > >> + if (is_nvdimm) { > >> + event = ACPI_NVDIMM_HOTPLUG_STATUS; > >> + } else { > >> + mdev->is_enabled = true; > >> + event = ACPI_MEMORY_HOTPLUG_STATUS; > >> + } > >> + > >> if (dev->hotplugged) { > >> - mdev->is_inserting = true; > >> - acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); > >> + if (!is_nvdimm) { > >> + mdev->is_inserting = true; > >> + } > >> + acpi_send_event(DEVICE(hotplug_dev), event); > >> } > >> } > >> > >> @@ -262,6 +273,8 @@ void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, > >> return; > >> } > >> > >> + /* nvdimm device hot unplug is not supported yet. */ > >> + assert(!object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)); > > that would crash guest instead of failing gracefully as it's supposed to. > > This is really a bug if it is triggered as nvdimm hot unplug is > stopped in the upper caller. point here is not crash VM on hotplug/unplug path if possible, it's fragile to rely on checks somewhere else as that code might change and cause crashes later. just use normal error handling infrastructure for this. i.e. error_setg()