Re: [Qemu-devel] [PATCH 03/14] pc: memhp: move nvdimm hotplug out of memory hotplug

[Igor Mammedov]

On Mon,  7 Nov 2016 19:13:38 +0800
Xiao Guangrong <address@hidden> wrote:

> as they use completely different way to handle hotplug event
> 
> Suggested-by: Igor Mammedov <address@hidden>
> Signed-off-by: Xiao Guangrong <address@hidden>
Reviewed-by: Igor Mammedov <address@hidden>

> ---
>  default-configs/mips-softmmu-common.mak |  1 +
>  docs/specs/acpi_mem_hotplug.txt         |  3 ---
>  docs/specs/acpi_nvdimm.txt              |  5 +++++
>  hw/acpi/ich9.c                          |  8 ++++++--
>  hw/acpi/memory_hotplug.c                | 31 ++++++++-----------------------
>  hw/acpi/nvdimm.c                        |  7 +++++++
>  hw/acpi/piix4.c                         |  7 ++++++-
>  include/hw/mem/nvdimm.h                 |  1 +
>  8 files changed, 34 insertions(+), 29 deletions(-)
> 
> diff --git a/default-configs/mips-softmmu-common.mak 
> b/default-configs/mips-softmmu-common.mak
> index 0394514..f0676f5 100644
> --- a/default-configs/mips-softmmu-common.mak
> +++ b/default-configs/mips-softmmu-common.mak
> @@ -17,6 +17,7 @@ CONFIG_FDC=y
>  CONFIG_ACPI=y
>  CONFIG_ACPI_X86=y
>  CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_NVDIMM=y
>  CONFIG_ACPI_CPU_HOTPLUG=y
>  CONFIG_APM=y
>  CONFIG_I8257=y
> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> index cb26dd2..3df3620 100644
> --- a/docs/specs/acpi_mem_hotplug.txt
> +++ b/docs/specs/acpi_mem_hotplug.txt
> @@ -4,9 +4,6 @@ QEMU<->ACPI BIOS memory hotplug interface
>  ACPI BIOS GPE.3 handler is dedicated for notifying OS about memory hot-add
>  and hot-remove events.
>  
> -ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
> -hot-add and hot-remove events.
> -
>  Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>  ---------------------------------------------------------------
>  0xa00:
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 4aa5e3d..d244147 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -127,6 +127,11 @@ _DSM process diagram:
>   | result from the page     |      |              |
>   +--------------------------+      +--------------+
>  
> +NVDIMM hotplug
> +--------------
> +ACPI BIOS GPE.4 handler is dedicated for notifying OS about nvdimm device
> +hot-add event.
> +
>  Device Handle Reservation
>  -------------------------
>  As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e5a3c18..830c475 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -490,8 +490,12 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, 
> DeviceState *dev,
>  
>      if (lpc->pm.acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
> -                            dev, errp);
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
> +        } else {
> +            acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
> +                                dev, errp);
> +        }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          if (lpc->pm.cpu_hotplug_legacy) {
>              legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, 
> errp);
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 70f6451..ec4e64b 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -2,7 +2,6 @@
>  #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"
> @@ -233,8 +232,11 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, 
> MemHotplugState *mem_st,
>                           DeviceState *dev, Error **errp)
>  {
>      MemStatus *mdev;
> -    AcpiEventStatusBits event;
> -    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +
> +    if (!dc->hotpluggable) {
> +        return;
> +    }
>  
>      mdev = acpi_memory_slot_status(mem_st, dev, errp);
>      if (!mdev) {
> @@ -242,23 +244,10 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, 
> MemHotplugState *mem_st,
>      }
>  
>      mdev->dimm = dev;
> -
> -    /*
> -     * 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;
> -    }
> -
> +    mdev->is_enabled = true;
>      if (dev->hotplugged) {
> -        if (!is_nvdimm) {
> -            mdev->is_inserting = true;
> -        }
> -        acpi_send_event(DEVICE(hotplug_dev), event);
> +        mdev->is_inserting = true;
> +        acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
>      }
>  }
>  
> @@ -273,8 +262,6 @@ 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));
>      mdev->is_removing = true;
>      acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
>  }
> @@ -289,8 +276,6 @@ void acpi_memory_unplug_cb(MemHotplugState *mem_st,
>          return;
>      }
>  
> -    /* nvdimm device hot unplug is not supported yet. */
> -    assert(!object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM));
>      mdev->is_enabled = false;
>      mdev->dimm = NULL;
>  }
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 0fe3547..5156565 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -868,6 +868,13 @@ static const MemoryRegionOps nvdimm_dsm_ops = {
>      },
>  };
>  
> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
> +{
> +    if (dev->hotplugged) {
> +        acpi_send_event(DEVICE(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS);
> +    }
> +}
> +
>  void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>                              FWCfgState *fw_cfg, Object *owner)
>  {
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 2adc246..17d36bd 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -378,7 +378,12 @@ static void piix4_device_plug_cb(HotplugHandler 
> *hotplug_dev,
>  
>      if (s->acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug, dev, errp);
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
> +        } else {
> +            acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug,
> +                                dev, errp);
> +        }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>          acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, 
> errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index d3ffb25..60585c3 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -131,4 +131,5 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray 
> *table_data,
>                         BIOSLinker *linker, AcpiNVDIMMState *state,
>                         uint32_t ram_slots);
>  void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev);
>  #endif