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

[Xiao Guangrong]

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...

> > +    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.