Re: [Qemu-devel] [PULL 39/47] pc: memhp: enable nvdimm device hotplug

[Igor Mammedov]

On Thu, 3 Nov 2016 00:00:45 +0800
Xiao Guangrong <address@hidden> wrote:

> On 11/02/2016 07:19 PM, Igor Mammedov wrote:
> > On Sun, 30 Oct 2016 23:25:10 +0200
> > "Michael S. Tsirkin" <address@hidden> wrote:
> >  
> >> From: Xiao Guangrong <address@hidden>
> >>
> >> _GPE.E04 is dedicated for nvdimm device hotplug
> >>
> >> Signed-off-by: Xiao Guangrong <address@hidden>
> >> Reviewed-by: Michael S. Tsirkin <address@hidden>
> >> Signed-off-by: Michael S. Tsirkin <address@hidden>
> >> ---
> >>  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()