Re: [Qemu-devel] [PATCH v4 3/3] pc: memhp: enable nvdimm device hotplug

[Xiao Guangrong]

On 11/03/2016 08:51 PM, Igor Mammedov wrote:
> On Thu,  3 Nov 2016 11:51:30 +0800
> Xiao Guangrong <address@hidden> wrote:
>
> > _GPE.E04 is dedicated for nvdimm device hotplug
> >
> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >  default-configs/mips-softmmu-common.mak |  1 +
> >  docs/specs/acpi_nvdimm.txt              |  5 +++++
> >  hw/acpi/ich9.c                          |  8 ++++++--
> >  hw/acpi/nvdimm.c                        |  7 +++++++
> >  hw/acpi/piix4.c                         |  7 ++++++-
> >  hw/i386/acpi-build.c                    |  7 +++++++
> >  hw/i386/pc.c                            | 12 ++++++++++++
> >  hw/mem/nvdimm.c                         |  4 ----
> >  include/hw/acpi/acpi_dev_interface.h    |  1 +
> >  include/hw/mem/nvdimm.h                 |  1 +
> >  10 files changed, 46 insertions(+), 7 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_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> > index 364e832..7887e57 100644
> > --- a/docs/specs/acpi_nvdimm.txt
> > +++ b/docs/specs/acpi_nvdimm.txt
> > @@ -137,6 +137,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 and hot-remove events.
> s/and hot-remove //

Okay.

> >  QEMU internal use only _DSM function
> >  ------------------------------------
> >  There is the function introduced by QEMU and only used by QEMU internal.
> > 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/nvdimm.c b/hw/acpi/nvdimm.c
> > index 593ac0d..b8548cc 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -874,6 +874,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/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index bc49958..32270c3 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2039,6 +2039,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          method = aml_method("_E03", 0, AML_NOTSERIALIZED);
> >          aml_append(method, aml_call0(MEMORY_HOTPLUG_HANDLER_PATH));
> >          aml_append(scope, method);
> > +
> > +        if (pcms->acpi_nvdimm_state.is_enabled) {
> > +            method = aml_method("_E04", 0, AML_NOTSERIALIZED);
> > +            aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
> > +                                          aml_int(0x80)));
> > +            aml_append(scope, method);
> > +        }
> >      }
> >      aml_append(dsdt, scope);
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 77ca7f4..0403452 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1723,6 +1723,12 @@ static void pc_dimm_unplug_request(HotplugHandler 
> > *hotplug_dev,
> >          goto out;
> >      }
> >
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> > +        error_setg(&local_err,
> > +                   "nvdimm device hot unplug is not supported yet.");
> > +        goto out;
> > +    }
> > +
> >      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> >      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> >
> > @@ -1740,6 +1746,12 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> >      HotplugHandlerClass *hhc;
> >      Error *local_err = NULL;
> >
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> > +        error_setg(&local_err,
> > +                   "nvdimm device hot unplug is not supported yet.");
> > +        goto out;
> > +    }
> drop this as it's not reachable without unplug support for
> NVDIMM in piix4/ich9, call chain for it would be:

Thanks for you pointing it out, i will drop the change in pc_dimm_unplug().