[Qemu-devel] [RESEND PATCH v4 0/6] QEMU memory hot unplug support

[Qemu-devel] [RESEND PATCH v4 0/6] QEMU memory hot unplug support

[Zhu Guihua]

Memory hot unplug are both asynchronous procedures.
When the unplug operation happens, unplug request cb is called first.
And when guest OS finished handling unplug, unplug cb will be called
to do the real removal of device.

v4:
 -reorganize the patchset
 -drop the new API acpi_send_gpe_event()
 -update ssdt-mem

v3:
 -commit message changes
 -reorganize the patchset, squash and separate some patches
 -update specs about acpi_mem_hotplug
 -first cleanup external state, then un-map and un-register memory device

v2:
 -do a generic for acpi to send gpe event
 -unparent object by PC_MACHINE
 -update description in acpi_mem_hotplug.txt
 -combine the last two patches in the last version
 -cleanup external state in acpi_memory_unplug_cb

Tang Chen (5):
  acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus
  acpi, mem-hotplug: Add unplug request cb for memory device
  pc-dimm: Add memory hot unplug request support for pc-dimm
  acpi, mem-hotplug: Add unplug cb for memory device
  pc-dimm: Add memory hot unplug support for pc-dimm

Zhu Guihua (1):
  acpi: Add hardware implementation for memory hot unplug

 docs/specs/acpi_mem_hotplug.txt   | 11 +++++-
 hw/acpi/ich9.c                    | 19 ++++++++--
 hw/acpi/memory_hotplug.c          | 78 +++++++++++++++++++++++++++++++++++----
 hw/acpi/piix4.c                   | 16 ++++++--
 hw/core/qdev.c                    |  2 +-
 hw/i386/acpi-build.c              |  9 +++++
 hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 +++++
 hw/i386/pc.c                      | 54 +++++++++++++++++++++++++--
 include/hw/acpi/memory_hotplug.h  |  6 +++
 include/hw/acpi/pc-hotplug.h      |  2 +
 include/hw/qdev-core.h            |  1 +
 trace-events                      |  1 +
 12 files changed, 187 insertions(+), 22 deletions(-)

-- 
1.9.3

[Qemu-devel] [RESEND PATCH v4 1/6] acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus

[Zhu Guihua]

From: Tang Chen <tangchen@cn.fujitsu.com>

Add a new API named acpi_memory_slot_status() to obtain a single memory
slot status. Doing this is because this procedure will be used by other
functions in the next coming patches.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/acpi/memory_hotplug.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index c6580da..0efc357 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -163,29 +163,41 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
     memory_region_add_subregion(as, ACPI_MEMORY_HOTPLUG_BASE, &state->io);
 }
 
-void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
-                         DeviceState *dev, Error **errp)
+static MemStatus *
+acpi_memory_slot_status(MemHotplugState *mem_st,
+                        DeviceState *dev, Error **errp)
 {
-    MemStatus *mdev;
     Error *local_err = NULL;
     int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
                                        &local_err);
 
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        return NULL;
     }
 
     if (slot >= mem_st->dev_count) {
         char *dev_path = object_get_canonical_path(OBJECT(dev));
-        error_setg(errp, "acpi_memory_plug_cb: "
+        error_setg(errp, "acpi_memory_slot_status: "
                    "device [%s] returned invalid memory slot[%d]",
                     dev_path, slot);
         g_free(dev_path);
+        return NULL;
+    }
+
+    return &mem_st->devs[slot];
+}
+
+void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
+                         DeviceState *dev, Error **errp)
+{
+    MemStatus *mdev;
+
+    mdev = acpi_memory_slot_status(mem_st, dev, errp);
+    if (!mdev) {
         return;
     }
 
-    mdev = &mem_st->devs[slot];
     mdev->dimm = dev;
     mdev->is_enabled = true;
     mdev->is_inserting = true;
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v4 2/6] acpi, mem-hotplug: Add unplug request cb for memory device

[Zhu Guihua]

From: Tang Chen <tangchen@cn.fujitsu.com>

Memory hot unplug are both asynchronous procedures.
When the unplug operation happens, unplug request cb is called first.
And when guest OS finished handling unplug, unplug cb will be called
to do the real removal of device.

This patch adds unplug request cb for memory device, and adds the
is_removing boolean field to MemStatus. This field is used to indicate
whether the memory slot is being removed. This field is set to true in
acpi_memory_unplug_request_cb().

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/acpi/ich9.c                   | 10 ++++++++--
 hw/acpi/memory_hotplug.c         | 19 +++++++++++++++++++
 hw/acpi/piix4.c                  |  6 +++++-
 include/hw/acpi/memory_hotplug.h |  4 ++++
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 5352e19..b85eed4 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -400,8 +400,14 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
 void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
                                       Error **errp)
 {
-    error_setg(errp, "acpi: device unplug request for not supported device"
-               " type: %s", object_get_typename(OBJECT(dev)));
+    if (pm->acpi_memory_hotplug.is_enabled &&
+        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
+                                      &pm->acpi_memory_hotplug, dev, errp);
+    } else {
+        error_setg(errp, "acpi: device unplug request for not supported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
 }
 
 void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 0efc357..2ef6a94 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -75,6 +75,7 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
     case 0x14: /* pack and return is_* fields */
         val |= mdev->is_enabled   ? 1 : 0;
         val |= mdev->is_inserting ? 2 : 0;
+        val |= mdev->is_removing  ? 4 : 0;
         trace_mhp_acpi_read_flags(mem_st->selector, val);
         break;
     default:
@@ -208,6 +209,24 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
     return;
 }
 
+void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
+                                   MemHotplugState *mem_st,
+                                   DeviceState *dev, Error **errp)
+{
+    MemStatus *mdev;
+
+    mdev = acpi_memory_slot_status(mem_st, dev, errp);
+    if (!mdev) {
+        return;
+    }
+
+    mdev->is_removing = true;
+
+    /* Do ACPI magic */
+    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
+    acpi_update_sci(ar, irq);
+}
+
 static const VMStateDescription vmstate_memhp_sts = {
     .name = "memory hotplug device state",
     .version_id = 1,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index d1f1179..f716e91 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -361,7 +361,11 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 {
     PIIX4PMState *s = PIIX4_PM(hotplug_dev);
 
-    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+    if (s->acpi_memory_hotplug.is_enabled &&
+        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        acpi_memory_unplug_request_cb(&s->ar, s->irq, &s->acpi_memory_hotplug,
+                                      dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
         acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
                                     errp);
     } else {
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index 7bbf8a0..c437a85 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -11,6 +11,7 @@ typedef struct MemStatus {
     DeviceState *dimm;
     bool is_enabled;
     bool is_inserting;
+    bool is_removing;
     uint32_t ost_event;
     uint32_t ost_status;
 } MemStatus;
@@ -28,6 +29,9 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
 
 void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
                          DeviceState *dev, Error **errp);
+void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
+                                   MemHotplugState *mem_st,
+                                   DeviceState *dev, Error **errp);
 
 extern const VMStateDescription vmstate_memory_hotplug;
 #define VMSTATE_MEMORY_HOTPLUG(memhp, state) \
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v4 3/6] pc-dimm: Add memory hot unplug request support for pc-dimm

[Zhu Guihua]

From: Tang Chen <tangchen@cn.fujitsu.com>

Implement memory unplug request cb for pc-dimm, and call it in
pc_machine_device_unplug_request_cb().

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b5b2aad..9c7c318 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1669,6 +1669,26 @@ out:
     error_propagate(errp, local_err);
 }
 
+static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
+                                   DeviceState *dev, Error **errp)
+{
+    HotplugHandlerClass *hhc;
+    Error *local_err = NULL;
+    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+
+    if (!pcms->acpi_dev) {
+        error_setg(&local_err,
+                   "memory hotplug is not enabled: missing acpi device");
+        goto out;
+    }
+
+    hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
+    hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
+
+out:
+    error_propagate(errp, local_err);
+}
+
 static void pc_cpu_plug(HotplugHandler *hotplug_dev,
                         DeviceState *dev, Error **errp)
 {
@@ -1711,8 +1731,12 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {
-    error_setg(errp, "acpi: device unplug request for not supported device"
-               " type: %s", object_get_typename(OBJECT(dev)));
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        pc_dimm_unplug_request(hotplug_dev, dev, errp);
+    } else {
+        error_setg(errp, "acpi: device unplug request for not supported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
 }
 
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v4 4/6] acpi, mem-hotplug: Add unplug cb for memory device

[Zhu Guihua]

From: Tang Chen <tangchen@cn.fujitsu.com>

This patch adds unplug cb for memory device. It resets some memory status
in acpi_memory_unplug_cb().

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/acpi/ich9.c                   |  9 +++++++--
 hw/acpi/memory_hotplug.c         | 14 ++++++++++++++
 hw/acpi/piix4.c                  | 10 ++++++++--
 include/hw/acpi/memory_hotplug.h |  2 ++
 4 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index b85eed4..84e5bb8 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -413,8 +413,13 @@ void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
 void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
                               Error **errp)
 {
-    error_setg(errp, "acpi: device unplug for not supported device"
-               " type: %s", object_get_typename(OBJECT(dev)));
+    if (pm->acpi_memory_hotplug.is_enabled &&
+        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        acpi_memory_unplug_cb(&pm->acpi_memory_hotplug, dev, errp);
+    } else {
+        error_setg(errp, "acpi: device unplug for not supported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
 }
 
 void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 2ef6a94..687b2f1 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -227,6 +227,20 @@ void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
     acpi_update_sci(ar, irq);
 }
 
+void acpi_memory_unplug_cb(MemHotplugState *mem_st,
+                           DeviceState *dev, Error **errp)
+{
+    MemStatus *mdev;
+
+    mdev = acpi_memory_slot_status(mem_st, dev, errp);
+    if (!mdev) {
+        return;
+    }
+
+    mdev->is_enabled = false;
+    mdev->dimm = NULL;
+}
+
 static const VMStateDescription vmstate_memhp_sts = {
     .name = "memory hotplug device state",
     .version_id = 1,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f716e91..781ad33 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -377,8 +377,14 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
                                    DeviceState *dev, Error **errp)
 {
-    error_setg(errp, "acpi: device unplug for not supported device"
-               " type: %s", object_get_typename(OBJECT(dev)));
+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        acpi_memory_unplug_cb(&s->acpi_memory_hotplug, dev, errp);
+    } else {
+        error_setg(errp, "acpi: device unplug for not supported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
 }
 
 static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index c437a85..15deae0 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -32,6 +32,8 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
 void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
                                    MemHotplugState *mem_st,
                                    DeviceState *dev, Error **errp);
+void acpi_memory_unplug_cb(MemHotplugState *mem_st,
+                           DeviceState *dev, Error **errp);
 
 extern const VMStateDescription vmstate_memory_hotplug;
 #define VMSTATE_MEMORY_HOTPLUG(memhp, state) \
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v4 5/6] pc-dimm: Add memory hot unplug support for pc-dimm

[Zhu Guihua]

From: Tang Chen <tangchen@cn.fujitsu.com>

Implement unplug cb for pc-dimm. It calls memory unplug cb to reset
some memory status, removes the corresponding memory region, and
unregisters vmstate.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9c7c318..141fa6a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1689,6 +1689,23 @@ out:
     error_propagate(errp, local_err);
 }
 
+static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
+                           DeviceState *dev, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
+    HotplugHandlerClass *hhc;
+    Error *local_err = NULL;
+
+    hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
+    hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
+
+    memory_region_del_subregion(&pcms->hotplug_memory, mr);
+    vmstate_unregister_ram(mr, dev);
+}
+
 static void pc_cpu_plug(HotplugHandler *hotplug_dev,
                         DeviceState *dev, Error **errp)
 {
@@ -1742,8 +1759,13 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
-    error_setg(errp, "acpi: device unplug for not supported device"
-               " type: %s", object_get_typename(OBJECT(dev)));
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        pc_dimm_unplug(hotplug_dev, dev, errp);
+        object_unparent(OBJECT(dev));
+    } else {
+        error_setg(errp, "acpi: device unplug for not supported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Zhu Guihua]

This patch adds a new bit to memory hotplug IO port indicating that
EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do
the real removal.

Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
 hw/acpi/memory_hotplug.c          | 21 +++++++++++++++++++--
 hw/core/qdev.c                    |  2 +-
 hw/i386/acpi-build.c              |  9 +++++++++
 hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 ++++++++++
 include/hw/acpi/pc-hotplug.h      |  2 ++
 include/hw/qdev-core.h            |  1 +
 trace-events                      |  1 +
 8 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 1290994..85cd4b8 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
               1: Device insert event, used to distinguish device for which
                  no device check event to OSPM was issued.
                  It's valid only when bit 1 is set.
-              2-7: reserved and should be ignored by OSPM
+              2: Device remove event, used to distinguish device for which
+                 no device check event to OSPM was issued.
+              3-7: reserved and should be ignored by OSPM
       [0x15-0x17] reserved
 
   write access:
@@ -35,7 +37,12 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
               1: if set to 1 clears device insert event, set by OSPM
                  after it has emitted device check event for the
                  selected memory device
-              2-7: reserved, OSPM must clear them before writing to register
+              2: if set to 1 clears device remove event, set by OSPM
+                 after it has emitted device check event for the
+                 selected memory device. if guest fails to eject device, it
+                 should send OST event about it and forget about device
+                 removal.
+              3-7: reserved, OSPM must clear them before writing to register
 
 Selecting memory device slot beyond present range has no effect on platform:
    - write accesses to memory hot-plug registers not documented above are
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 687b2f1..d6b8c89 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -2,6 +2,7 @@
 #include "hw/acpi/pc-hotplug.h"
 #include "hw/mem/pc-dimm.h"
 #include "hw/boards.h"
+#include "hw/qdev-core.h"
 #include "trace.h"
 #include "qapi-event.h"
 
@@ -91,6 +92,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
     MemHotplugState *mem_st = opaque;
     MemStatus *mdev;
     ACPIOSTInfo *info;
+    DeviceState *dev = NULL;
+    HotplugHandler *hotplug_ctrl = NULL;
 
     if (!mem_st->dev_count) {
         return;
@@ -122,19 +125,33 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
         mdev = &mem_st->devs[mem_st->selector];
         mdev->ost_status = data;
         trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
-        /* TODO: implement memory removal on guest signal */
 
         info = acpi_memory_device_status(mem_st->selector, mdev);
         qapi_event_send_acpi_device_ost(info, &error_abort);
         qapi_free_ACPIOSTInfo(info);
         break;
-    case 0x14:
+    case 0x14: /* set is_* fields */
         mdev = &mem_st->devs[mem_st->selector];
         if (data & 2) { /* clear insert event */
             mdev->is_inserting  = false;
             trace_mhp_acpi_clear_insert_evt(mem_st->selector);
+        } else if (data & 4) { /* request removal of device */
+            mdev->is_removing = false;
+            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
+            /*
+             * QEMU memory hot unplug is an asynchronous procedure. QEMU first
+             * calls pc-dimm unplug request cb to send a SCI to guest. When the
+             * guest OS finished handling the SCI, it evaluates ACPI EJ0, and
+             * QEMU calls pc-dimm unplug cb to remove memory device.
+             */
+            dev = DEVICE(mdev->dimm);
+            hotplug_ctrl = qdev_get_hotplug_handler(dev);
+            /* Call pc-dimm unplug cb. */
+            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
         }
         break;
+    default:
+        break;
     }
 
 }
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6be5866..4676ffb 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -273,7 +273,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
     dev->alias_required_for_version = required_for_version;
 }
 
-static HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
+HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
 {
     HotplugHandler *hotplug_ctrl = NULL;
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d0a5c85..1ba6102 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -929,6 +929,9 @@ build_ssdt(GArray *table_data, GArray *linker,
         aml_append(field,
             /*(read) 1 if has a insert event. (write) 1 to clear event */
             aml_named_field(stringify(MEMORY_SLOT_INSERT_EVENT), 1));
+        aml_append(field,
+            /*(read) 1 if has a remove event. (write) 1 to clear event */
+            aml_named_field(stringify(MEMORY_SLOT_REMOVE_EVENT), 1));
         aml_append(scope, field);
 
         field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc);
@@ -972,6 +975,12 @@ build_ssdt(GArray *table_data, GArray *linker,
             )));
             aml_append(dev, method);
 
+            method = aml_method("_EJ0", 1);
+            s = BASEPATH stringify(MEMORY_SLOT_EJECT_METHOD);
+            aml_append(method, aml_return(aml_call2(s, aml_name("_UID"),
+                       aml_arg(0))));
+            aml_append(dev, method);
+
             aml_append(sb_scope, dev);
         }
 
diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
index 1e9ec39..ef847e2 100644
--- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
+++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
@@ -29,6 +29,7 @@
             External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only
             External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, read only
             External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if has a insert event. (write) 1 to clear event
+            External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if has a remove event. (write) 1 to clear event
             External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, write only
             External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event code, write only
             External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status code, write only
@@ -55,6 +56,8 @@
                     If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
                         MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
                         Store(1, MEMORY_SLOT_INSERT_EVENT)
+                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
+                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
                     }
                     // TODO: handle memory eject request
                     Add(Local0, One, Local0) // goto next DIMM
@@ -156,5 +159,12 @@
                 Store(Arg2, MEMORY_SLOT_OST_STATUS)
                 Release(MEMORY_SLOT_LOCK)
             }
+
+            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
+                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
+                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
+                Store(One, MEMORY_SLOT_REMOVE_EVENT)
+                Release(MEMORY_SLOT_LOCK)
+            }
         } // Device()
     } // Scope()
diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
index efa6ed7..680810b 100644
--- a/include/hw/acpi/pc-hotplug.h
+++ b/include/hw/acpi/pc-hotplug.h
@@ -43,6 +43,7 @@
 #define MEMORY_SLOT_PROXIMITY        MPX
 #define MEMORY_SLOT_ENABLED          MES
 #define MEMORY_SLOT_INSERT_EVENT     MINS
+#define MEMORY_SLOT_REMOVE_EVENT     MRMV
 #define MEMORY_SLOT_SLECTOR          MSEL
 #define MEMORY_SLOT_OST_EVENT        MOEV
 #define MEMORY_SLOT_OST_STATUS       MOSC
@@ -51,6 +52,7 @@
 #define MEMORY_SLOT_CRS_METHOD       MCRS
 #define MEMORY_SLOT_OST_METHOD       MOST
 #define MEMORY_SLOT_PROXIMITY_METHOD MPXM
+#define MEMORY_SLOT_EJECT_METHOD     MEJ0
 #define MEMORY_SLOT_NOTIFY_METHOD    MTFY
 #define MEMORY_SLOT_SCAN_METHOD      MSCN
 
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4e673f9..5b7acf1 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -266,6 +266,7 @@ int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
+HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
 void qdev_unplug(DeviceState *dev, Error **errp);
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
                                   DeviceState *dev, Error **errp);
diff --git a/trace-events b/trace-events
index 30eba92..e552355 100644
--- a/trace-events
+++ b/trace-events
@@ -1572,6 +1572,7 @@ mhp_acpi_write_slot(uint32_t slot) "set active slot: 0x%"PRIx32
 mhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "slot[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
 mhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "slot[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
 mhp_acpi_clear_insert_evt(uint32_t slot) "slot[0x%"PRIx32"] clear insert event"
+mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"] clear remove event"
 
 # hw/i386/pc.c
 mhp_pc_dimm_assigned_slot(int slot) "0x%d"
-- 
1.9.3

Re: [Qemu-devel] [RESEND PATCH v4 2/6] acpi, mem-hotplug: Add unplug request cb for memory device

[Igor Mammedov]

On Mon, 16 Mar 2015 16:58:14 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> Memory hot unplug are both asynchronous procedures.
> When the unplug operation happens, unplug request cb is called first.
> And when guest OS finished handling unplug, unplug cb will be called
> to do the real removal of device.
Since unplug is rather complicated multi-stage process,
it's worth to describe in in acpi_mem_hotplug.txt rather then put it
in commit message here. Preferably with process flow diagram.


> 
> This patch adds unplug request cb for memory device, and adds the
> is_removing boolean field to MemStatus. This field is used to indicate
> whether the memory slot is being removed. This field is set to true in
> acpi_memory_unplug_request_cb().
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
'is_removing' field is introduced here without associated
documentation about how it's used and then in 6/6 it's usage is extended
and some docs are added.

Perhaps it would be better to put documentation patch here or as
a separate one before this patch so that reviewers would know
intended use of new fields that are added here in advance.

Also documentation in 6/6 is not complete. It adds 'removing' event
description but it doesn't amend following text with unplug part:

"ACPI BIOS GPE.3 handler is dedicated for notifying OS about memory hot-add
events."


> ---
>  hw/acpi/ich9.c                   | 10 ++++++++--
>  hw/acpi/memory_hotplug.c         | 19 +++++++++++++++++++
>  hw/acpi/piix4.c                  |  6 +++++-
>  include/hw/acpi/memory_hotplug.h |  4 ++++
>  4 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 5352e19..b85eed4 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -400,8 +400,14 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
>  void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
>                                        Error **errp)
>  {
> -    error_setg(errp, "acpi: device unplug request for not supported device"
> -               " type: %s", object_get_typename(OBJECT(dev)));
> +    if (pm->acpi_memory_hotplug.is_enabled &&
> +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
> +                                      &pm->acpi_memory_hotplug, dev, errp);
> +    } else {
> +        error_setg(errp, "acpi: device unplug request for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));
> +    }
>  }
>  
>  void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 0efc357..2ef6a94 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -75,6 +75,7 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
>      case 0x14: /* pack and return is_* fields */
>          val |= mdev->is_enabled   ? 1 : 0;
>          val |= mdev->is_inserting ? 2 : 0;
> +        val |= mdev->is_removing  ? 4 : 0;
>          trace_mhp_acpi_read_flags(mem_st->selector, val);
>          break;
>      default:
> @@ -208,6 +209,24 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>      return;
>  }
>  
> +void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
> +                                   MemHotplugState *mem_st,
> +                                   DeviceState *dev, Error **errp)
> +{
> +    MemStatus *mdev;
> +
> +    mdev = acpi_memory_slot_status(mem_st, dev, errp);
> +    if (!mdev) {
> +        return;
> +    }
> +
> +    mdev->is_removing = true;
> +
> +    /* Do ACPI magic */
> +    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> +    acpi_update_sci(ar, irq);
> +}
> +
>  static const VMStateDescription vmstate_memhp_sts = {
>      .name = "memory hotplug device state",
>      .version_id = 1,
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index d1f1179..f716e91 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -361,7 +361,11 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>  {
>      PIIX4PMState *s = PIIX4_PM(hotplug_dev);
>  
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +    if (s->acpi_memory_hotplug.is_enabled &&
> +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        acpi_memory_unplug_request_cb(&s->ar, s->irq, &s->acpi_memory_hotplug,
> +                                      dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>          acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
>                                      errp);
>      } else {
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index 7bbf8a0..c437a85 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -11,6 +11,7 @@ typedef struct MemStatus {
>      DeviceState *dimm;
>      bool is_enabled;
>      bool is_inserting;
> +    bool is_removing;
>      uint32_t ost_event;
>      uint32_t ost_status;
>  } MemStatus;
> @@ -28,6 +29,9 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
>  
>  void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>                           DeviceState *dev, Error **errp);
> +void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
> +                                   MemHotplugState *mem_st,
> +                                   DeviceState *dev, Error **errp);
>  
>  extern const VMStateDescription vmstate_memory_hotplug;
>  #define VMSTATE_MEMORY_HOTPLUG(memhp, state) \

Re: [Qemu-devel] [RESEND PATCH v4 3/6] pc-dimm: Add memory hot unplug request support for pc-dimm

[Igor Mammedov]

On Mon, 16 Mar 2015 16:58:15 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> Implement memory unplug request cb for pc-dimm, and call it in
> pc_machine_device_unplug_request_cb().
Please, merge this patch with 2/6.

> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b5b2aad..9c7c318 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1669,6 +1669,26 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> +static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
> +                                   DeviceState *dev, Error **errp)
> +{
> +    HotplugHandlerClass *hhc;
> +    Error *local_err = NULL;
> +    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +
> +    if (!pcms->acpi_dev) {
> +        error_setg(&local_err,
> +                   "memory hotplug is not enabled: missing acpi device");
> +        goto out;
> +    }
> +
> +    hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> +    hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> +
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
>  static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>                          DeviceState *dev, Error **errp)
>  {
> @@ -1711,8 +1731,12 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                                  DeviceState *dev, Error **errp)
>  {
> -    error_setg(errp, "acpi: device unplug request for not supported device"
> -               " type: %s", object_get_typename(OBJECT(dev)));
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        pc_dimm_unplug_request(hotplug_dev, dev, errp);
> +    } else {
> +        error_setg(errp, "acpi: device unplug request for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));
> +    }
>  }
>  
>  static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,

Re: [Qemu-devel] [RESEND PATCH v4 1/6] acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus

[Michael S. Tsirkin]

On Mon, Mar 16, 2015 at 04:58:13PM +0800, Zhu Guihua wrote:
> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> Add a new API named acpi_memory_slot_status() to obtain a single memory
> slot status. Doing this is because this procedure will be used by other
> functions in the next coming patches.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

It's generally only OK to add API in a separate
patch from its use if the API is well documented.
There's 0 documentation here, so please just squash this
with the user so one can review without jumping
back and forth.


> ---
>  hw/acpi/memory_hotplug.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index c6580da..0efc357 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -163,29 +163,41 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
>      memory_region_add_subregion(as, ACPI_MEMORY_HOTPLUG_BASE, &state->io);
>  }
>  
> -void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> -                         DeviceState *dev, Error **errp)
> +static MemStatus *
> +acpi_memory_slot_status(MemHotplugState *mem_st,
> +                        DeviceState *dev, Error **errp)
>  {
> -    MemStatus *mdev;
>      Error *local_err = NULL;
>      int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
>                                         &local_err);
>  
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        return;
> +        return NULL;
>      }
>  
>      if (slot >= mem_st->dev_count) {
>          char *dev_path = object_get_canonical_path(OBJECT(dev));
> -        error_setg(errp, "acpi_memory_plug_cb: "
> +        error_setg(errp, "acpi_memory_slot_status: "
>                     "device [%s] returned invalid memory slot[%d]",
>                      dev_path, slot);
>          g_free(dev_path);
> +        return NULL;
> +    }
> +
> +    return &mem_st->devs[slot];
> +}
> +
> +void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> +                         DeviceState *dev, Error **errp)
> +{
> +    MemStatus *mdev;
> +
> +    mdev = acpi_memory_slot_status(mem_st, dev, errp);
> +    if (!mdev) {
>          return;
>      }
>  
> -    mdev = &mem_st->devs[slot];
>      mdev->dimm = dev;
>      mdev->is_enabled = true;
>      mdev->is_inserting = true;
> -- 
> 1.9.3

Re: [Qemu-devel] [RESEND PATCH v4 4/6] acpi, mem-hotplug: Add unplug cb for memory device

[Igor Mammedov]

On Mon, 16 Mar 2015 16:58:16 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> This patch adds unplug cb for memory device. It resets some memory status
> in acpi_memory_unplug_cb().
'some' is quite vague. Pls, explain what it does in commit message.

[...]
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f716e91..781ad33 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -377,8 +377,14 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>  static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                     DeviceState *dev, Error **errp)
>  {
> -    error_setg(errp, "acpi: device unplug for not supported device"
> -               " type: %s", object_get_typename(OBJECT(dev)));
> +    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
missing condition "pm->acpi_memory_hotplug.is_enabled &&"?

> +        acpi_memory_unplug_cb(&s->acpi_memory_hotplug, dev, errp);
> +    } else {
> +        error_setg(errp, "acpi: device unplug for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));
> +    }
>  }
>  
>  static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index c437a85..15deae0 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -32,6 +32,8 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>  void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
>                                     MemHotplugState *mem_st,
>                                     DeviceState *dev, Error **errp);
> +void acpi_memory_unplug_cb(MemHotplugState *mem_st,
> +                           DeviceState *dev, Error **errp);
>  
>  extern const VMStateDescription vmstate_memory_hotplug;
>  #define VMSTATE_MEMORY_HOTPLUG(memhp, state) \

Re: [Qemu-devel] [RESEND PATCH v4 5/6] pc-dimm: Add memory hot unplug support for pc-dimm

[Igor Mammedov]

On Mon, 16 Mar 2015 16:58:17 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> Implement unplug cb for pc-dimm. It calls memory unplug cb to reset
> some memory status, removes the corresponding memory region, and
> unregisters vmstate.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Please, squash this patch into previous.

> ---
>  hw/i386/pc.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 9c7c318..141fa6a 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1689,6 +1689,23 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> +static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> +                           DeviceState *dev, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
> +    HotplugHandlerClass *hhc;
> +    Error *local_err = NULL;
> +
> +    hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> +    hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
potentially leaking local_err, should be propagated to errp
and maybe in case of error here we shouldn't proceed with removal.


> +
> +    memory_region_del_subregion(&pcms->hotplug_memory, mr);
> +    vmstate_unregister_ram(mr, dev);
> +}
> +
>  static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>                          DeviceState *dev, Error **errp)
>  {
> @@ -1742,8 +1759,13 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>  static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                          DeviceState *dev, Error **errp)
>  {
> -    error_setg(errp, "acpi: device unplug for not supported device"
> -               " type: %s", object_get_typename(OBJECT(dev)));
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        pc_dimm_unplug(hotplug_dev, dev, errp);
> +        object_unparent(OBJECT(dev));
Pls, do unparenting in pc_dimm_unplug()

> +    } else {
> +        error_setg(errp, "acpi: device unplug for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));
> +    }
>  }
>  
>  static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Igor Mammedov]

On Mon, 16 Mar 2015 16:58:18 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> This patch adds a new bit to memory hotplug IO port indicating that
actually bit was added in 2/6 where is_removing had been added.

> EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do
> the real removal.
> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
>  hw/acpi/memory_hotplug.c          | 21 +++++++++++++++++++--
>  hw/core/qdev.c                    |  2 +-
>  hw/i386/acpi-build.c              |  9 +++++++++
>  hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 ++++++++++
>  include/hw/acpi/pc-hotplug.h      |  2 ++
>  include/hw/qdev-core.h            |  1 +
>  trace-events                      |  1 +
>  8 files changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> index 1290994..85cd4b8 100644
> --- a/docs/specs/acpi_mem_hotplug.txt
> +++ b/docs/specs/acpi_mem_hotplug.txt
> @@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>                1: Device insert event, used to distinguish device for which
>                   no device check event to OSPM was issued.
>                   It's valid only when bit 1 is set.
> -              2-7: reserved and should be ignored by OSPM
> +              2: Device remove event, used to distinguish device for which
> +                 no device check event to OSPM was issued.
> +              3-7: reserved and should be ignored by OSPM
>        [0x15-0x17] reserved
>  
>    write access:
> @@ -35,7 +37,12 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>                1: if set to 1 clears device insert event, set by OSPM
>                   after it has emitted device check event for the
>                   selected memory device
> -              2-7: reserved, OSPM must clear them before writing to register
> +              2: if set to 1 clears device remove event, set by OSPM
> +                 after it has emitted device check event for the
> +                 selected memory device. if guest fails to eject device, it
> +                 should send OST event about it and forget about device
> +                 removal.
> +              3-7: reserved, OSPM must clear them before writing to register
>  
>  Selecting memory device slot beyond present range has no effect on platform:
>     - write accesses to memory hot-plug registers not documented above are
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 687b2f1..d6b8c89 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -2,6 +2,7 @@
>  #include "hw/acpi/pc-hotplug.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/boards.h"
> +#include "hw/qdev-core.h"
>  #include "trace.h"
>  #include "qapi-event.h"
>  
> @@ -91,6 +92,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>      MemHotplugState *mem_st = opaque;
>      MemStatus *mdev;
>      ACPIOSTInfo *info;
> +    DeviceState *dev = NULL;
> +    HotplugHandler *hotplug_ctrl = NULL;
>  
>      if (!mem_st->dev_count) {
>          return;
> @@ -122,19 +125,33 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>          mdev = &mem_st->devs[mem_st->selector];
>          mdev->ost_status = data;
>          trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
> -        /* TODO: implement memory removal on guest signal */
>  
>          info = acpi_memory_device_status(mem_st->selector, mdev);
>          qapi_event_send_acpi_device_ost(info, &error_abort);
>          qapi_free_ACPIOSTInfo(info);
>          break;
> -    case 0x14:
> +    case 0x14: /* set is_* fields */
>          mdev = &mem_st->devs[mem_st->selector];
>          if (data & 2) { /* clear insert event */
>              mdev->is_inserting  = false;
>              trace_mhp_acpi_clear_insert_evt(mem_st->selector);
> +        } else if (data & 4) { /* request removal of device */
fix comment to match docs above.

> +            mdev->is_removing = false;
> +            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
just clear event here and don't do removal part as it doesn't match
documentation you've written above regarding this field.

It would be better to move is_removing handling from here to 2/6
+ related ASL code from DSDT which should clear it after sending device check.

> +            /*
> +             * QEMU memory hot unplug is an asynchronous procedure. QEMU first
> +             * calls pc-dimm unplug request cb to send a SCI to guest. When the
> +             * guest OS finished handling the SCI, it evaluates ACPI EJ0, and
> +             * QEMU calls pc-dimm unplug cb to remove memory device.
> +             */
something like this comment, should be in acpi_mem_hotplug.txt not here.


There is 'is_enabled' field, which is 1 if device is present, we can use it
for triggering actual ejecting in QEMU from EJ0(), something like:

} else if (data & 1) { /* eject device */

> +            dev = DEVICE(mdev->dimm);
potential NULL dereference, dimm could be NULL if guest does eject twice
or does eject of empty slot.
Perhaps add check before accessing dimm.

 if(!mdev->is_enabled) {
    trace_..._ejecting_invalid_slot(...)
    break;
 }

> +            hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +            /* Call pc-dimm unplug cb. */
> +            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
It's not that we can do anything about error at this point
but instead of forgetting it silently at least log error in trace,
the best would be in addition to that send QMP event to notify mgmt
about it. (sending QMP event could be a separate patch)


>          }
>          break;
> +    default:
> +        break;
>      }
>  
>  }
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6be5866..4676ffb 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -273,7 +273,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>      dev->alias_required_for_version = required_for_version;
>  }
>  
> -static HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> +HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>  {
>      HotplugHandler *hotplug_ctrl = NULL;
>  
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d0a5c85..1ba6102 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -929,6 +929,9 @@ build_ssdt(GArray *table_data, GArray *linker,
>          aml_append(field,
>              /*(read) 1 if has a insert event. (write) 1 to clear event */
>              aml_named_field(stringify(MEMORY_SLOT_INSERT_EVENT), 1));
> +        aml_append(field,
> +            /*(read) 1 if has a remove event. (write) 1 to clear event */
> +            aml_named_field(stringify(MEMORY_SLOT_REMOVE_EVENT), 1));
>          aml_append(scope, field);
>  
>          field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc);
> @@ -972,6 +975,12 @@ build_ssdt(GArray *table_data, GArray *linker,
>              )));
>              aml_append(dev, method);
>  
> +            method = aml_method("_EJ0", 1);
> +            s = BASEPATH stringify(MEMORY_SLOT_EJECT_METHOD);
> +            aml_append(method, aml_return(aml_call2(s, aml_name("_UID"),
> +                       aml_arg(0))));
> +            aml_append(dev, method);
> +
>              aml_append(sb_scope, dev);
>          }
>  
> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> index 1e9ec39..ef847e2 100644
> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> @@ -29,6 +29,7 @@
>              External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only
>              External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, read only
>              External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if has a insert event. (write) 1 to clear event
> +            External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if has a remove event. (write) 1 to clear event
>              External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, write only
>              External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event code, write only
>              External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status code, write only
> @@ -55,6 +56,8 @@
>                      If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
>                          MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
>                          Store(1, MEMORY_SLOT_INSERT_EVENT)
> +                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
> +                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
clear removing field here.

>                      }
>                      // TODO: handle memory eject request
>                      Add(Local0, One, Local0) // goto next DIMM
> @@ -156,5 +159,12 @@
>                  Store(Arg2, MEMORY_SLOT_OST_STATUS)
>                  Release(MEMORY_SLOT_LOCK)
>              }
> +
> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
> +                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
> +                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
> +                Store(One, MEMORY_SLOT_REMOVE_EVENT)
redo it using enabled field. Otherwise it could cause guest/QEMU crash:

- if 1st memory was asked to be removed
- then OSPM processes it but has not called _EJ0 yet leaving is_removed set
- then QEMU adds/removes another DIMM triggering slots scan
   which would issue second Notify(remove) since is_removed is still set
- as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0()


> +                Release(MEMORY_SLOT_LOCK)
> +            }
>          } // Device()
>      } // Scope()
> diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
> index efa6ed7..680810b 100644
> --- a/include/hw/acpi/pc-hotplug.h
> +++ b/include/hw/acpi/pc-hotplug.h
> @@ -43,6 +43,7 @@
>  #define MEMORY_SLOT_PROXIMITY        MPX
>  #define MEMORY_SLOT_ENABLED          MES
>  #define MEMORY_SLOT_INSERT_EVENT     MINS
> +#define MEMORY_SLOT_REMOVE_EVENT     MRMV
>  #define MEMORY_SLOT_SLECTOR          MSEL
>  #define MEMORY_SLOT_OST_EVENT        MOEV
>  #define MEMORY_SLOT_OST_STATUS       MOSC
> @@ -51,6 +52,7 @@
>  #define MEMORY_SLOT_CRS_METHOD       MCRS
>  #define MEMORY_SLOT_OST_METHOD       MOST
>  #define MEMORY_SLOT_PROXIMITY_METHOD MPXM
> +#define MEMORY_SLOT_EJECT_METHOD     MEJ0
>  #define MEMORY_SLOT_NOTIFY_METHOD    MTFY
>  #define MEMORY_SLOT_SCAN_METHOD      MSCN
>  
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 4e673f9..5b7acf1 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -266,6 +266,7 @@ int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
>  void qdev_init_nofail(DeviceState *dev);
>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version);
> +HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>  void qdev_unplug(DeviceState *dev, Error **errp);
>  void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                    DeviceState *dev, Error **errp);
> diff --git a/trace-events b/trace-events
> index 30eba92..e552355 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1572,6 +1572,7 @@ mhp_acpi_write_slot(uint32_t slot) "set active slot: 0x%"PRIx32
>  mhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "slot[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
>  mhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "slot[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
>  mhp_acpi_clear_insert_evt(uint32_t slot) "slot[0x%"PRIx32"] clear insert event"
> +mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"] clear remove event"
>  
>  # hw/i386/pc.c
>  mhp_pc_dimm_assigned_slot(int slot) "0x%d"

Re: [Qemu-devel] [RESEND PATCH v4 1/6] acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus

[Zhu Guihua]

On 03/16/2015 09:58 PM, Michael S. Tsirkin wrote:
> On Mon, Mar 16, 2015 at 04:58:13PM +0800, Zhu Guihua wrote:
>> From: Tang Chen <tangchen@cn.fujitsu.com>
>>
>> Add a new API named acpi_memory_slot_status() to obtain a single memory
>> slot status. Doing this is because this procedure will be used by other
>> functions in the next coming patches.
>>
>> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> It's generally only OK to add API in a separate
> patch from its use if the API is well documented.
> There's 0 documentation here, so please just squash this
> with the user so one can review without jumping
> back and forth.

The new API will be used in several places. If I squash this with the
user, I think the patch will be too large.

So I think it is better to add documentation here. what's your opinion?

Thanks,
Zhu

>
>
>> ---
>>   hw/acpi/memory_hotplug.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> index c6580da..0efc357 100644
>> --- a/hw/acpi/memory_hotplug.c
>> +++ b/hw/acpi/memory_hotplug.c
>> @@ -163,29 +163,41 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
>>       memory_region_add_subregion(as, ACPI_MEMORY_HOTPLUG_BASE, &state->io);
>>   }
>>   
>> -void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>> -                         DeviceState *dev, Error **errp)
>> +static MemStatus *
>> +acpi_memory_slot_status(MemHotplugState *mem_st,
>> +                        DeviceState *dev, Error **errp)
>>   {
>> -    MemStatus *mdev;
>>       Error *local_err = NULL;
>>       int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
>>                                          &local_err);
>>   
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>> -        return;
>> +        return NULL;
>>       }
>>   
>>       if (slot >= mem_st->dev_count) {
>>           char *dev_path = object_get_canonical_path(OBJECT(dev));
>> -        error_setg(errp, "acpi_memory_plug_cb: "
>> +        error_setg(errp, "acpi_memory_slot_status: "
>>                      "device [%s] returned invalid memory slot[%d]",
>>                       dev_path, slot);
>>           g_free(dev_path);
>> +        return NULL;
>> +    }
>> +
>> +    return &mem_st->devs[slot];
>> +}
>> +
>> +void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>> +                         DeviceState *dev, Error **errp)
>> +{
>> +    MemStatus *mdev;
>> +
>> +    mdev = acpi_memory_slot_status(mem_st, dev, errp);
>> +    if (!mdev) {
>>           return;
>>       }
>>   
>> -    mdev = &mem_st->devs[slot];
>>       mdev->dimm = dev;
>>       mdev->is_enabled = true;
>>       mdev->is_inserting = true;
>> -- 
>> 1.9.3
> .
>

Re: [Qemu-devel] [RESEND PATCH v4 1/6] acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 09:52:59AM +0800, Zhu Guihua wrote:
> 
> On 03/16/2015 09:58 PM, Michael S. Tsirkin wrote:
> >On Mon, Mar 16, 2015 at 04:58:13PM +0800, Zhu Guihua wrote:
> >>From: Tang Chen <tangchen@cn.fujitsu.com>
> >>
> >>Add a new API named acpi_memory_slot_status() to obtain a single memory
> >>slot status. Doing this is because this procedure will be used by other
> >>functions in the next coming patches.
> >>
> >>Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> >>Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> >It's generally only OK to add API in a separate
> >patch from its use if the API is well documented.
> >There's 0 documentation here, so please just squash this
> >with the user so one can review without jumping
> >back and forth.
> 
> The new API will be used in several places. If I squash this with the
> user, I think the patch will be too large.
> 
> So I think it is better to add documentation here. what's your opinion?
> 
> Thanks,
> Zhu

That's fine too.

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Zhu Guihua]

On 03/16/2015 10:59 PM, Igor Mammedov wrote:
[...]
>   
> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> index 1e9ec39..ef847e2 100644
> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> @@ -29,6 +29,7 @@
>               External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only
>               External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, read only
>               External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if has a insert event. (write) 1 to clear event
> +            External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if has a remove event. (write) 1 to clear event
>               External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, write only
>               External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event code, write only
>               External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status code, write only
> @@ -55,6 +56,8 @@
>                       If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
>                           MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
>                           Store(1, MEMORY_SLOT_INSERT_EVENT)
> +                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
> +                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
> clear removing field here.

You mean clear remove event here?

>
>>                       }
>>                       // TODO: handle memory eject request
>>                       Add(Local0, One, Local0) // goto next DIMM
>> @@ -156,5 +159,12 @@
>>                   Store(Arg2, MEMORY_SLOT_OST_STATUS)
>>                   Release(MEMORY_SLOT_LOCK)
>>               }
>> +
>> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
>> +                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
>> +                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
>> +                Store(One, MEMORY_SLOT_REMOVE_EVENT)
> redo it using enabled field. Otherwise it could cause guest/QEMU crash:
>
> - if 1st memory was asked to be removed
> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set
> - then QEMU adds/removes another DIMM triggering slots scan
>     which would issue second Notify(remove) since is_removed is still set
> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0()
>

If OSPM processed the ejection request but not called _EJ0, the device 
will still be present in qemu,
we must handle this. So I think OSPM issues double EJ0 maybe reasonable 
in this situation.
What's your opinion?

Thanks,
Zhu

>> +                Release(MEMORY_SLOT_LOCK)
>> +            }
>>           } // Device()
>>       } // Scope()
>> diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
>> index efa6ed7..680810b 100644
>> --- a/include/hw/acpi/pc-hotplug.h
>> +++ b/include/hw/acpi/pc-hotplug.h
>> @@ -43,6 +43,7 @@
>>   #define MEMORY_SLOT_PROXIMITY        MPX
>>   #define MEMORY_SLOT_ENABLED          MES
>>   #define MEMORY_SLOT_INSERT_EVENT     MINS
>> +#define MEMORY_SLOT_REMOVE_EVENT     MRMV
>>   #define MEMORY_SLOT_SLECTOR          MSEL
>>   #define MEMORY_SLOT_OST_EVENT        MOEV
>>   #define MEMORY_SLOT_OST_STATUS       MOSC
>> @@ -51,6 +52,7 @@
>>   #define MEMORY_SLOT_CRS_METHOD       MCRS
>>   #define MEMORY_SLOT_OST_METHOD       MOST
>>   #define MEMORY_SLOT_PROXIMITY_METHOD MPXM
>> +#define MEMORY_SLOT_EJECT_METHOD     MEJ0
>>   #define MEMORY_SLOT_NOTIFY_METHOD    MTFY
>>   #define MEMORY_SLOT_SCAN_METHOD      MSCN
>>   
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 4e673f9..5b7acf1 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -266,6 +266,7 @@ int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
>>   void qdev_init_nofail(DeviceState *dev);
>>   void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>>                                    int required_for_version);
>> +HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>>   void qdev_unplug(DeviceState *dev, Error **errp);
>>   void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>>                                     DeviceState *dev, Error **errp);
>> diff --git a/trace-events b/trace-events
>> index 30eba92..e552355 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1572,6 +1572,7 @@ mhp_acpi_write_slot(uint32_t slot) "set active slot: 0x%"PRIx32
>>   mhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "slot[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
>>   mhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "slot[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
>>   mhp_acpi_clear_insert_evt(uint32_t slot) "slot[0x%"PRIx32"] clear insert event"
>> +mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"] clear remove event"
>>   
>>   # hw/i386/pc.c
>>   mhp_pc_dimm_assigned_slot(int slot) "0x%d"
> .
>

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Igor Mammedov]

On Mon, 23 Mar 2015 18:59:28 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> 
> On 03/16/2015 10:59 PM, Igor Mammedov wrote:
> [...]
> >   
> > diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> > index 1e9ec39..ef847e2 100644
> > --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> > +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> > @@ -29,6 +29,7 @@
> >               External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only
> >               External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, read only
> >               External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if has a insert event. (write) 1 to clear event
> > +            External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if has a remove event. (write) 1 to clear event
> >               External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, write only
> >               External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event code, write only
> >               External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status code, write only
> > @@ -55,6 +56,8 @@
> >                       If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
> >                           MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
> >                           Store(1, MEMORY_SLOT_INSERT_EVENT)
> > +                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
> > +                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
> > clear removing field here.
> 
> You mean clear remove event here?
yes

> 
> >
> >>                       }
> >>                       // TODO: handle memory eject request
> >>                       Add(Local0, One, Local0) // goto next DIMM
> >> @@ -156,5 +159,12 @@
> >>                   Store(Arg2, MEMORY_SLOT_OST_STATUS)
> >>                   Release(MEMORY_SLOT_LOCK)
> >>               }
> >> +
> >> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
> >> +                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
> >> +                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
> >> +                Store(One, MEMORY_SLOT_REMOVE_EVENT)
> > redo it using enabled field. Otherwise it could cause guest/QEMU crash:
> >
> > - if 1st memory was asked to be removed
> > - then OSPM processes it but has not called _EJ0 yet leaving is_removed set
> > - then QEMU adds/removes another DIMM triggering slots scan
> >     which would issue second Notify(remove) since is_removed is still set
> > - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0()
> >
> 
> If OSPM processed the ejection request but not called _EJ0, the device 
> will still be present in qemu,
> we must handle this. 
There is nothing to handle in this case, if OSPM hasn't called _EJ0 then
nothing happens and device stays in QEMU.

>So I think OSPM issues double EJ0 maybe reasonable 
> in this situation.
> What's your opinion?
the first _EJ0 must do ejection, as for the second I think it should be NOP.

> 
> Thanks,
> Zhu
> 
[...]

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Zhu Guihua]

On 03/23/2015 08:47 PM, Igor Mammedov wrote:
> On Mon, 23 Mar 2015 18:59:28 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> On 03/16/2015 10:59 PM, Igor Mammedov wrote:
>> [...]
>>>    
>>> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
>>> index 1e9ec39..ef847e2 100644
>>> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
>>> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
>>> @@ -29,6 +29,7 @@
>>>                External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only
>>>                External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, read only
>>>                External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if has a insert event. (write) 1 to clear event
>>> +            External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if has a remove event. (write) 1 to clear event
>>>                External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, write only
>>>                External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event code, write only
>>>                External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status code, write only
>>> @@ -55,6 +56,8 @@
>>>                        If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
>>>                            MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
>>>                            Store(1, MEMORY_SLOT_INSERT_EVENT)
>>> +                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
>>> +                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
>>> clear removing field here.
>> You mean clear remove event here?
> yes

I tested this method, clear remove event here will lead to guest
kernel panic.

>>>>                        }
>>>>                        // TODO: handle memory eject request
>>>>                        Add(Local0, One, Local0) // goto next DIMM
>>>> @@ -156,5 +159,12 @@
>>>>                    Store(Arg2, MEMORY_SLOT_OST_STATUS)
>>>>                    Release(MEMORY_SLOT_LOCK)
>>>>                }
>>>> +
>>>> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
>>>> +                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
>>>> +                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
>>>> +                Store(One, MEMORY_SLOT_REMOVE_EVENT)
>>> redo it using enabled field. Otherwise it could cause guest/QEMU crash:
>>>
>>> - if 1st memory was asked to be removed
>>> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set
>>> - then QEMU adds/removes another DIMM triggering slots scan
>>>      which would issue second Notify(remove) since is_removed is still set
>>> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0()
>>>
>> If OSPM processed the ejection request but not called _EJ0, the device
>> will still be present in qemu,
>> we must handle this.
> There is nothing to handle in this case, if OSPM hasn't called _EJ0 then
> nothing happens and device stays in QEMU.
>
>> So I think OSPM issues double EJ0 maybe reasonable
>> in this situation.
>> What's your opinion?
> the first _EJ0 must do ejection, as for the second I think it should be NOP.

So we should judge the enabled field to check whether the device is 
present before
issuing Notify(remove)?

Thanks,
Zhu

>> Thanks,
>> Zhu
>>
> [...]
> .
>

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Zhu Guihua]

On 03/16/2015 10:59 PM, Igor Mammedov wrote:
> On Mon, 16 Mar 2015 16:58:18 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> This patch adds a new bit to memory hotplug IO port indicating that
> actually bit was added in 2/6 where is_removing had been added.
>
>> EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do
>> the real removal.
>>
>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
>> ---
>>   docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
>>   hw/acpi/memory_hotplug.c          | 21 +++++++++++++++++++--
>>   hw/core/qdev.c                    |  2 +-
>>   hw/i386/acpi-build.c              |  9 +++++++++
>>   hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 ++++++++++
>>   include/hw/acpi/pc-hotplug.h      |  2 ++
>>   include/hw/qdev-core.h            |  1 +
>>   trace-events                      |  1 +
>>   8 files changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
>> index 1290994..85cd4b8 100644
>> --- a/docs/specs/acpi_mem_hotplug.txt
>> +++ b/docs/specs/acpi_mem_hotplug.txt
>> @@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>>                 1: Device insert event, used to distinguish device for which
>>                    no device check event to OSPM was issued.
>>                    It's valid only when bit 1 is set.
>> -              2-7: reserved and should be ignored by OSPM
>> +              2: Device remove event, used to distinguish device for which
>> +                 no device check event to OSPM was issued.
>> +              3-7: reserved and should be ignored by OSPM
>>         [0x15-0x17] reserved
>>   
>>     write access:
>> @@ -35,7 +37,12 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>>                 1: if set to 1 clears device insert event, set by OSPM
>>                    after it has emitted device check event for the
>>                    selected memory device
>> -              2-7: reserved, OSPM must clear them before writing to register
>> +              2: if set to 1 clears device remove event, set by OSPM
>> +                 after it has emitted device check event for the
>> +                 selected memory device. if guest fails to eject device, it
>> +                 should send OST event about it and forget about device
>> +                 removal.
>> +              3-7: reserved, OSPM must clear them before writing to register
>>   
>>   Selecting memory device slot beyond present range has no effect on platform:
>>      - write accesses to memory hot-plug registers not documented above are
>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> index 687b2f1..d6b8c89 100644
>> --- a/hw/acpi/memory_hotplug.c
>> +++ b/hw/acpi/memory_hotplug.c
>> @@ -2,6 +2,7 @@
>>   #include "hw/acpi/pc-hotplug.h"
>>   #include "hw/mem/pc-dimm.h"
>>   #include "hw/boards.h"
>> +#include "hw/qdev-core.h"
>>   #include "trace.h"
>>   #include "qapi-event.h"
>>   
>> @@ -91,6 +92,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>>       MemHotplugState *mem_st = opaque;
>>       MemStatus *mdev;
>>       ACPIOSTInfo *info;
>> +    DeviceState *dev = NULL;
>> +    HotplugHandler *hotplug_ctrl = NULL;
>>   
>>       if (!mem_st->dev_count) {
>>           return;
>> @@ -122,19 +125,33 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>>           mdev = &mem_st->devs[mem_st->selector];
>>           mdev->ost_status = data;
>>           trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
>> -        /* TODO: implement memory removal on guest signal */
>>   
>>           info = acpi_memory_device_status(mem_st->selector, mdev);
>>           qapi_event_send_acpi_device_ost(info, &error_abort);
>>           qapi_free_ACPIOSTInfo(info);
>>           break;
>> -    case 0x14:
>> +    case 0x14: /* set is_* fields */
>>           mdev = &mem_st->devs[mem_st->selector];
>>           if (data & 2) { /* clear insert event */
>>               mdev->is_inserting  = false;
>>               trace_mhp_acpi_clear_insert_evt(mem_st->selector);
>> +        } else if (data & 4) { /* request removal of device */
> fix comment to match docs above.
>
>> +            mdev->is_removing = false;
>> +            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
> just clear event here and don't do removal part as it doesn't match
> documentation you've written above regarding this field.
>
> It would be better to move is_removing handling from here to 2/6
> + related ASL code from DSDT which should clear it after sending device check.
>
>> +            /*
>> +             * QEMU memory hot unplug is an asynchronous procedure. QEMU first
>> +             * calls pc-dimm unplug request cb to send a SCI to guest. When the
>> +             * guest OS finished handling the SCI, it evaluates ACPI EJ0, and
>> +             * QEMU calls pc-dimm unplug cb to remove memory device.
>> +             */
> something like this comment, should be in acpi_mem_hotplug.txt not here.
>
>
> There is 'is_enabled' field, which is 1 if device is present, we can use it
> for triggering actual ejecting in QEMU from EJ0(), something like:
>
> } else if (data & 1) { /* eject device */

I think this is not correct. When you clear insert event, the 
'is_enabled' filed was also 1.
And when we hot remove memory, the addr 0x14 will be written only once.

Thanks,
Zhu

>> +            dev = DEVICE(mdev->dimm);
> potential NULL dereference, dimm could be NULL if guest does eject twice
> or does eject of empty slot.
> Perhaps add check before accessing dimm.
>
>   if(!mdev->is_enabled) {
>      trace_..._ejecting_invalid_slot(...)
>      break;
>   }
>
>> +            hotplug_ctrl = qdev_get_hotplug_handler(dev);
>> +            /* Call pc-dimm unplug cb. */
>> +            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> It's not that we can do anything about error at this point
> but instead of forgetting it silently at least log error in trace,
> the best would be in addition to that send QMP event to notify mgmt
> about it. (sending QMP event could be a separate patch)
>
>
>>           }
>>           break;
>> +    default:
>> +        break;
>>       }
>>   
>>   }
>>
[...]

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Igor Mammedov]

On Tue, 24 Mar 2015 17:34:29 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> 
> On 03/23/2015 08:47 PM, Igor Mammedov wrote:
> > On Mon, 23 Mar 2015 18:59:28 +0800
> > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> >
> >> On 03/16/2015 10:59 PM, Igor Mammedov wrote:
> >> [...]
> >>>    
> >>> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> >>> index 1e9ec39..ef847e2 100644
> >>> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> >>> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> >>> @@ -29,6 +29,7 @@
> >>>                External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only
> >>>                External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, read only
> >>>                External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if has a insert event. (write) 1 to clear event
> >>> +            External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if has a remove event. (write) 1 to clear event
> >>>                External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, write only
> >>>                External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event code, write only
> >>>                External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status code, write only
> >>> @@ -55,6 +56,8 @@
> >>>                        If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
> >>>                            MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
> >>>                            Store(1, MEMORY_SLOT_INSERT_EVENT)
> >>> +                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
> >>> +                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
> >>> clear removing field here.
> >> You mean clear remove event here?
> > yes
> 
> I tested this method, clear remove event here will lead to guest
> kernel panic.
it shouldn't cause panic if it only clears flag in QEMU
(that's what it should do).


> 
> >>>>                        }
> >>>>                        // TODO: handle memory eject request
> >>>>                        Add(Local0, One, Local0) // goto next DIMM
> >>>> @@ -156,5 +159,12 @@
> >>>>                    Store(Arg2, MEMORY_SLOT_OST_STATUS)
> >>>>                    Release(MEMORY_SLOT_LOCK)
> >>>>                }
> >>>> +
> >>>> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
> >>>> +                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
> >>>> +                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
> >>>> +                Store(One, MEMORY_SLOT_REMOVE_EVENT)
> >>> redo it using enabled field. Otherwise it could cause guest/QEMU crash:
> >>>
> >>> - if 1st memory was asked to be removed
> >>> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set
> >>> - then QEMU adds/removes another DIMM triggering slots scan
> >>>      which would issue second Notify(remove) since is_removed is still set
> >>> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0()
> >>>
> >> If OSPM processed the ejection request but not called _EJ0, the device
> >> will still be present in qemu,
> >> we must handle this.
> > There is nothing to handle in this case, if OSPM hasn't called _EJ0 then
> > nothing happens and device stays in QEMU.
> >
> >> So I think OSPM issues double EJ0 maybe reasonable
> >> in this situation.
> >> What's your opinion?
> > the first _EJ0 must do ejection, as for the second I think it should be NOP.
> 
> So we should judge the enabled field to check whether the device is 
> present before
> issuing Notify(remove)?
I wouldn't check if device is present.
I'd unconditionally clear it and make sure on QEMU side that
operation is NOP if device is not present.

> 
> Thanks,
> Zhu
> 
> >> Thanks,
> >> Zhu
> >>
> > [...]
> > .
> >
> 

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Igor Mammedov]

On Tue, 24 Mar 2015 17:38:53 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> 
> On 03/16/2015 10:59 PM, Igor Mammedov wrote:
> > On Mon, 16 Mar 2015 16:58:18 +0800
> > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> >
> >> This patch adds a new bit to memory hotplug IO port indicating that
> > actually bit was added in 2/6 where is_removing had been added.
> >
> >> EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do
> >> the real removal.
> >>
> >> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> >> ---
> >>   docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
> >>   hw/acpi/memory_hotplug.c          | 21 +++++++++++++++++++--
> >>   hw/core/qdev.c                    |  2 +-
> >>   hw/i386/acpi-build.c              |  9 +++++++++
> >>   hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 ++++++++++
> >>   include/hw/acpi/pc-hotplug.h      |  2 ++
> >>   include/hw/qdev-core.h            |  1 +
> >>   trace-events                      |  1 +
> >>   8 files changed, 52 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> >> index 1290994..85cd4b8 100644
> >> --- a/docs/specs/acpi_mem_hotplug.txt
> >> +++ b/docs/specs/acpi_mem_hotplug.txt
> >> @@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
> >>                 1: Device insert event, used to distinguish device for which
> >>                    no device check event to OSPM was issued.
> >>                    It's valid only when bit 1 is set.
> >> -              2-7: reserved and should be ignored by OSPM
> >> +              2: Device remove event, used to distinguish device for which
> >> +                 no device check event to OSPM was issued.
> >> +              3-7: reserved and should be ignored by OSPM
> >>         [0x15-0x17] reserved
> >>   
> >>     write access:
> >> @@ -35,7 +37,12 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
> >>                 1: if set to 1 clears device insert event, set by OSPM
> >>                    after it has emitted device check event for the
> >>                    selected memory device
> >> -              2-7: reserved, OSPM must clear them before writing to register
> >> +              2: if set to 1 clears device remove event, set by OSPM
> >> +                 after it has emitted device check event for the
> >> +                 selected memory device. if guest fails to eject device, it
> >> +                 should send OST event about it and forget about device
> >> +                 removal.
> >> +              3-7: reserved, OSPM must clear them before writing to register
> >>   
> >>   Selecting memory device slot beyond present range has no effect on platform:
> >>      - write accesses to memory hot-plug registers not documented above are
> >> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> >> index 687b2f1..d6b8c89 100644
> >> --- a/hw/acpi/memory_hotplug.c
> >> +++ b/hw/acpi/memory_hotplug.c
> >> @@ -2,6 +2,7 @@
> >>   #include "hw/acpi/pc-hotplug.h"
> >>   #include "hw/mem/pc-dimm.h"
> >>   #include "hw/boards.h"
> >> +#include "hw/qdev-core.h"
> >>   #include "trace.h"
> >>   #include "qapi-event.h"
> >>   
> >> @@ -91,6 +92,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> >>       MemHotplugState *mem_st = opaque;
> >>       MemStatus *mdev;
> >>       ACPIOSTInfo *info;
> >> +    DeviceState *dev = NULL;
> >> +    HotplugHandler *hotplug_ctrl = NULL;
> >>   
> >>       if (!mem_st->dev_count) {
> >>           return;
> >> @@ -122,19 +125,33 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> >>           mdev = &mem_st->devs[mem_st->selector];
> >>           mdev->ost_status = data;
> >>           trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
> >> -        /* TODO: implement memory removal on guest signal */
> >>   
> >>           info = acpi_memory_device_status(mem_st->selector, mdev);
> >>           qapi_event_send_acpi_device_ost(info, &error_abort);
> >>           qapi_free_ACPIOSTInfo(info);
> >>           break;
> >> -    case 0x14:
> >> +    case 0x14: /* set is_* fields */
> >>           mdev = &mem_st->devs[mem_st->selector];
> >>           if (data & 2) { /* clear insert event */
> >>               mdev->is_inserting  = false;
> >>               trace_mhp_acpi_clear_insert_evt(mem_st->selector);
> >> +        } else if (data & 4) { /* request removal of device */
> > fix comment to match docs above.
> >
> >> +            mdev->is_removing = false;
> >> +            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
> > just clear event here and don't do removal part as it doesn't match
> > documentation you've written above regarding this field.
> >
> > It would be better to move is_removing handling from here to 2/6
> > + related ASL code from DSDT which should clear it after sending device check.
> >
> >> +            /*
> >> +             * QEMU memory hot unplug is an asynchronous procedure. QEMU first
> >> +             * calls pc-dimm unplug request cb to send a SCI to guest. When the
> >> +             * guest OS finished handling the SCI, it evaluates ACPI EJ0, and
> >> +             * QEMU calls pc-dimm unplug cb to remove memory device.
> >> +             */
> > something like this comment, should be in acpi_mem_hotplug.txt not here.
> >
> >
> > There is 'is_enabled' field, which is 1 if device is present, we can use it
> > for triggering actual ejecting in QEMU from EJ0(), something like:
> >
> > } else if (data & 1) { /* eject device */
> 
> I think this is not correct. When you clear insert event, the 
> 'is_enabled' filed was also 1.
> And when we hot remove memory, the addr 0x14 will be written only once.
It's not clear to me what a problem you see here.
Could you give more extended explanation, pls?

> 
> Thanks,
> Zhu
> 
> >> +            dev = DEVICE(mdev->dimm);
> > potential NULL dereference, dimm could be NULL if guest does eject twice
> > or does eject of empty slot.
> > Perhaps add check before accessing dimm.
> >
> >   if(!mdev->is_enabled) {
> >      trace_..._ejecting_invalid_slot(...)
> >      break;
> >   }
> >
> >> +            hotplug_ctrl = qdev_get_hotplug_handler(dev);
> >> +            /* Call pc-dimm unplug cb. */
> >> +            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> > It's not that we can do anything about error at this point
> > but instead of forgetting it silently at least log error in trace,
> > the best would be in addition to that send QMP event to notify mgmt
> > about it. (sending QMP event could be a separate patch)
> >
> >
> >>           }
> >>           break;
> >> +    default:
> >> +        break;
> >>       }
> >>   
> >>   }
> >>
> [...]

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Zhu Guihua]

On 03/24/2015 06:31 PM, Igor Mammedov wrote:
> On Tue, 24 Mar 2015 17:38:53 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> On 03/16/2015 10:59 PM, Igor Mammedov wrote:
>>> On Mon, 16 Mar 2015 16:58:18 +0800
>>> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>>>
>>>> This patch adds a new bit to memory hotplug IO port indicating that
>>> actually bit was added in 2/6 where is_removing had been added.
>>>
>>>> EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do
>>>> the real removal.
>>>>
>>>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
>>>> ---
>>>>    docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
>>>>    hw/acpi/memory_hotplug.c          | 21 +++++++++++++++++++--
>>>>    hw/core/qdev.c                    |  2 +-
>>>>    hw/i386/acpi-build.c              |  9 +++++++++
>>>>    hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 ++++++++++
>>>>    include/hw/acpi/pc-hotplug.h      |  2 ++
>>>>    include/hw/qdev-core.h            |  1 +
>>>>    trace-events                      |  1 +
>>>>    8 files changed, 52 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
>>>> index 1290994..85cd4b8 100644
>>>> --- a/docs/specs/acpi_mem_hotplug.txt
>>>> +++ b/docs/specs/acpi_mem_hotplug.txt
>>>> @@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>>>>                  1: Device insert event, used to distinguish device for which
>>>>                     no device check event to OSPM was issued.
>>>>                     It's valid only when bit 1 is set.
>>>> -              2-7: reserved and should be ignored by OSPM
>>>> +              2: Device remove event, used to distinguish device for which
>>>> +                 no device check event to OSPM was issued.
>>>> +              3-7: reserved and should be ignored by OSPM
>>>>          [0x15-0x17] reserved
>>>>    
>>>>      write access:
>>>> @@ -35,7 +37,12 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
>>>>                  1: if set to 1 clears device insert event, set by OSPM
>>>>                     after it has emitted device check event for the
>>>>                     selected memory device
>>>> -              2-7: reserved, OSPM must clear them before writing to register
>>>> +              2: if set to 1 clears device remove event, set by OSPM
>>>> +                 after it has emitted device check event for the
>>>> +                 selected memory device. if guest fails to eject device, it
>>>> +                 should send OST event about it and forget about device
>>>> +                 removal.
>>>> +              3-7: reserved, OSPM must clear them before writing to register
>>>>    
>>>>    Selecting memory device slot beyond present range has no effect on platform:
>>>>       - write accesses to memory hot-plug registers not documented above are
>>>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>>>> index 687b2f1..d6b8c89 100644
>>>> --- a/hw/acpi/memory_hotplug.c
>>>> +++ b/hw/acpi/memory_hotplug.c
>>>> @@ -2,6 +2,7 @@
>>>>    #include "hw/acpi/pc-hotplug.h"
>>>>    #include "hw/mem/pc-dimm.h"
>>>>    #include "hw/boards.h"
>>>> +#include "hw/qdev-core.h"
>>>>    #include "trace.h"
>>>>    #include "qapi-event.h"
>>>>    
>>>> @@ -91,6 +92,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>>>>        MemHotplugState *mem_st = opaque;
>>>>        MemStatus *mdev;
>>>>        ACPIOSTInfo *info;
>>>> +    DeviceState *dev = NULL;
>>>> +    HotplugHandler *hotplug_ctrl = NULL;
>>>>    
>>>>        if (!mem_st->dev_count) {
>>>>            return;
>>>> @@ -122,19 +125,33 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>>>>            mdev = &mem_st->devs[mem_st->selector];
>>>>            mdev->ost_status = data;
>>>>            trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
>>>> -        /* TODO: implement memory removal on guest signal */
>>>>    
>>>>            info = acpi_memory_device_status(mem_st->selector, mdev);
>>>>            qapi_event_send_acpi_device_ost(info, &error_abort);
>>>>            qapi_free_ACPIOSTInfo(info);
>>>>            break;
>>>> -    case 0x14:
>>>> +    case 0x14: /* set is_* fields */
>>>>            mdev = &mem_st->devs[mem_st->selector];
>>>>            if (data & 2) { /* clear insert event */
>>>>                mdev->is_inserting  = false;
>>>>                trace_mhp_acpi_clear_insert_evt(mem_st->selector);
>>>> +        } else if (data & 4) { /* request removal of device */
>>> fix comment to match docs above.
>>>
>>>> +            mdev->is_removing = false;
>>>> +            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
>>> just clear event here and don't do removal part as it doesn't match
>>> documentation you've written above regarding this field.
>>>
>>> It would be better to move is_removing handling from here to 2/6
>>> + related ASL code from DSDT which should clear it after sending device check.
>>>
>>>> +            /*
>>>> +             * QEMU memory hot unplug is an asynchronous procedure. QEMU first
>>>> +             * calls pc-dimm unplug request cb to send a SCI to guest. When the
>>>> +             * guest OS finished handling the SCI, it evaluates ACPI EJ0, and
>>>> +             * QEMU calls pc-dimm unplug cb to remove memory device.
>>>> +             */
>>> something like this comment, should be in acpi_mem_hotplug.txt not here.
>>>
>>>
>>> There is 'is_enabled' field, which is 1 if device is present, we can use it
>>> for triggering actual ejecting in QEMU from EJ0(), something like:
>>>
>>> } else if (data & 1) { /* eject device */
>> I think this is not correct. When you clear insert event, the
>> 'is_enabled' filed was also 1.
>> And when we hot remove memory, the addr 0x14 will be written only once.
> It's not clear to me what a problem you see here.
> Could you give more extended explanation, pls?

When we clear insert event, 'data & 1' was also true, so unplug 
operation maybe called.
This is only asumed situation, because when the addr is 0x14, 
acpi_memory_hotplug_write()
will be called only once, and the value of data is 5, we could not 
execute the condition of 'data & 1',
the unplug operation could not be called.

>
>> Thanks,
>> Zhu
>>
>>>> +            dev = DEVICE(mdev->dimm);
>>> potential NULL dereference, dimm could be NULL if guest does eject twice
>>> or does eject of empty slot.
>>> Perhaps add check before accessing dimm.
>>>
>>>    if(!mdev->is_enabled) {
>>>       trace_..._ejecting_invalid_slot(...)
>>>       break;
>>>    }
>>>
>>>> +            hotplug_ctrl = qdev_get_hotplug_handler(dev);
>>>> +            /* Call pc-dimm unplug cb. */
>>>> +            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
>>> It's not that we can do anything about error at this point
>>> but instead of forgetting it silently at least log error in trace,
>>> the best would be in addition to that send QMP event to notify mgmt
>>> about it. (sending QMP event could be a separate patch)
>>>
>>>
>>>>            }
>>>>            break;
>>>> +    default:
>>>> +        break;
>>>>        }
>>>>    
>>>>    }
>>>>
>> [...]
> .
>

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Zhu Guihua]

On 03/24/2015 06:26 PM, Igor Mammedov wrote:
> On Tue, 24 Mar 2015 17:34:29 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> On 03/23/2015 08:47 PM, Igor Mammedov wrote:
>>> On Mon, 23 Mar 2015 18:59:28 +0800
>>> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>>>
>>>> On 03/16/2015 10:59 PM, Igor Mammedov wrote:
>>>> [...]
>>>>>     
>>>>> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
>>>>> index 1e9ec39..ef847e2 100644
>>>>> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
>>>>> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
>>>>> @@ -29,6 +29,7 @@
>>>>>                 External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only
>>>>>                 External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, read only
>>>>>                 External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if has a insert event. (write) 1 to clear event
>>>>> +            External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if has a remove event. (write) 1 to clear event
>>>>>                 External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, write only
>>>>>                 External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event code, write only
>>>>>                 External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status code, write only
>>>>> @@ -55,6 +56,8 @@
>>>>>                         If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
>>>>>                             MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
>>>>>                             Store(1, MEMORY_SLOT_INSERT_EVENT)
>>>>> +                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
>>>>> +                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
>>>>> clear removing field here.
>>>> You mean clear remove event here?
>>> yes
>> I tested this method, clear remove event here will lead to guest
>> kernel panic.
> it shouldn't cause panic if it only clears flag in QEMU
> (that's what it should do).
>
>
>>>>>>                         }
>>>>>>                         // TODO: handle memory eject request
>>>>>>                         Add(Local0, One, Local0) // goto next DIMM
>>>>>> @@ -156,5 +159,12 @@
>>>>>>                     Store(Arg2, MEMORY_SLOT_OST_STATUS)
>>>>>>                     Release(MEMORY_SLOT_LOCK)
>>>>>>                 }
>>>>>> +
>>>>>> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
>>>>>> +                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
>>>>>> +                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
>>>>>> +                Store(One, MEMORY_SLOT_REMOVE_EVENT)
>>>>> redo it using enabled field. Otherwise it could cause guest/QEMU crash:
>>>>>
>>>>> - if 1st memory was asked to be removed
>>>>> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set
>>>>> - then QEMU adds/removes another DIMM triggering slots scan
>>>>>       which would issue second Notify(remove) since is_removed is still set
>>>>> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0()
>>>>>
>>>> If OSPM processed the ejection request but not called _EJ0, the device
>>>> will still be present in qemu,
>>>> we must handle this.
>>> There is nothing to handle in this case, if OSPM hasn't called _EJ0 then
>>> nothing happens and device stays in QEMU.
>>>
>>>> So I think OSPM issues double EJ0 maybe reasonable
>>>> in this situation.
>>>> What's your opinion?
>>> the first _EJ0 must do ejection, as for the second I think it should be NOP.
>> So we should judge the enabled field to check whether the device is
>> present before
>> issuing Notify(remove)?
> I wouldn't check if device is present.
> I'd unconditionally clear it and make sure on QEMU side that
> operation is NOP if device is not present.

I'm sorry that I have not fully understood your meaning about
'redo it using enabled field'.  How to do it?

MEMORY_SLOT_ENABLED is read only, how can I use it to handle EJ0?

Thanks,
Zhu

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Igor Mammedov]

On Tue, 24 Mar 2015 18:48:02 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> 
> On 03/24/2015 06:31 PM, Igor Mammedov wrote:
> > On Tue, 24 Mar 2015 17:38:53 +0800
> > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> >
> >> On 03/16/2015 10:59 PM, Igor Mammedov wrote:
> >>> On Mon, 16 Mar 2015 16:58:18 +0800
> >>> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> >>>
> >>>> This patch adds a new bit to memory hotplug IO port indicating that
> >>> actually bit was added in 2/6 where is_removing had been added.
> >>>
> >>>> EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do
> >>>> the real removal.
> >>>>
> >>>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> >>>> ---
> >>>>    docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
> >>>>    hw/acpi/memory_hotplug.c          | 21 +++++++++++++++++++--
> >>>>    hw/core/qdev.c                    |  2 +-
> >>>>    hw/i386/acpi-build.c              |  9 +++++++++
> >>>>    hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 ++++++++++
> >>>>    include/hw/acpi/pc-hotplug.h      |  2 ++
> >>>>    include/hw/qdev-core.h            |  1 +
> >>>>    trace-events                      |  1 +
> >>>>    8 files changed, 52 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> >>>> index 1290994..85cd4b8 100644
> >>>> --- a/docs/specs/acpi_mem_hotplug.txt
> >>>> +++ b/docs/specs/acpi_mem_hotplug.txt
> >>>> @@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
> >>>>                  1: Device insert event, used to distinguish device for which
> >>>>                     no device check event to OSPM was issued.
> >>>>                     It's valid only when bit 1 is set.
> >>>> -              2-7: reserved and should be ignored by OSPM
> >>>> +              2: Device remove event, used to distinguish device for which
> >>>> +                 no device check event to OSPM was issued.
> >>>> +              3-7: reserved and should be ignored by OSPM
> >>>>          [0x15-0x17] reserved
> >>>>    
> >>>>      write access:
> >>>> @@ -35,7 +37,12 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
> >>>>                  1: if set to 1 clears device insert event, set by OSPM
> >>>>                     after it has emitted device check event for the
> >>>>                     selected memory device
> >>>> -              2-7: reserved, OSPM must clear them before writing to register
> >>>> +              2: if set to 1 clears device remove event, set by OSPM
> >>>> +                 after it has emitted device check event for the
> >>>> +                 selected memory device. if guest fails to eject device, it
> >>>> +                 should send OST event about it and forget about device
> >>>> +                 removal.
> >>>> +              3-7: reserved, OSPM must clear them before writing to register
> >>>>    
> >>>>    Selecting memory device slot beyond present range has no effect on platform:
> >>>>       - write accesses to memory hot-plug registers not documented above are
> >>>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> >>>> index 687b2f1..d6b8c89 100644
> >>>> --- a/hw/acpi/memory_hotplug.c
> >>>> +++ b/hw/acpi/memory_hotplug.c
> >>>> @@ -2,6 +2,7 @@
> >>>>    #include "hw/acpi/pc-hotplug.h"
> >>>>    #include "hw/mem/pc-dimm.h"
> >>>>    #include "hw/boards.h"
> >>>> +#include "hw/qdev-core.h"
> >>>>    #include "trace.h"
> >>>>    #include "qapi-event.h"
> >>>>    
> >>>> @@ -91,6 +92,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> >>>>        MemHotplugState *mem_st = opaque;
> >>>>        MemStatus *mdev;
> >>>>        ACPIOSTInfo *info;
> >>>> +    DeviceState *dev = NULL;
> >>>> +    HotplugHandler *hotplug_ctrl = NULL;
> >>>>    
> >>>>        if (!mem_st->dev_count) {
> >>>>            return;
> >>>> @@ -122,19 +125,33 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> >>>>            mdev = &mem_st->devs[mem_st->selector];
> >>>>            mdev->ost_status = data;
> >>>>            trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
> >>>> -        /* TODO: implement memory removal on guest signal */
> >>>>    
> >>>>            info = acpi_memory_device_status(mem_st->selector, mdev);
> >>>>            qapi_event_send_acpi_device_ost(info, &error_abort);
> >>>>            qapi_free_ACPIOSTInfo(info);
> >>>>            break;
> >>>> -    case 0x14:
> >>>> +    case 0x14: /* set is_* fields */
> >>>>            mdev = &mem_st->devs[mem_st->selector];
> >>>>            if (data & 2) { /* clear insert event */
> >>>>                mdev->is_inserting  = false;
> >>>>                trace_mhp_acpi_clear_insert_evt(mem_st->selector);
> >>>> +        } else if (data & 4) { /* request removal of device */
> >>> fix comment to match docs above.
> >>>
> >>>> +            mdev->is_removing = false;
> >>>> +            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
> >>> just clear event here and don't do removal part as it doesn't match
> >>> documentation you've written above regarding this field.
> >>>
> >>> It would be better to move is_removing handling from here to 2/6
> >>> + related ASL code from DSDT which should clear it after sending device check.
> >>>
> >>>> +            /*
> >>>> +             * QEMU memory hot unplug is an asynchronous procedure. QEMU first
> >>>> +             * calls pc-dimm unplug request cb to send a SCI to guest. When the
> >>>> +             * guest OS finished handling the SCI, it evaluates ACPI EJ0, and
> >>>> +             * QEMU calls pc-dimm unplug cb to remove memory device.
> >>>> +             */
> >>> something like this comment, should be in acpi_mem_hotplug.txt not here.
> >>>
> >>>
> >>> There is 'is_enabled' field, which is 1 if device is present, we can use it
> >>> for triggering actual ejecting in QEMU from EJ0(), something like:
> >>>
> >>> } else if (data & 1) { /* eject device */
> >> I think this is not correct. When you clear insert event, the
> >> 'is_enabled' filed was also 1.
> >> And when we hot remove memory, the addr 0x14 will be written only once.
> > It's not clear to me what a problem you see here.
> > Could you give more extended explanation, pls?
> 
> When we clear insert event, 'data & 1' was also true, so unplug 
> operation maybe called.
> This is only asumed situation, because when the addr is 0x14, 
> acpi_memory_hotplug_write()
> will be called only once, and the value of data is 5, we could not 
> execute the condition of 'data & 1',
> the unplug operation could not be called.
Looks like there is a bug that wouldn't allow us to reuse
MEMORY_SLOT_ENABLED bit for removal, acpi_mem_hotplug.txt clearly states
that 0 bit must be cleared and marks it as reserved.
It's caused by

            Field(MEMORY_HOTPLUG_IO_REGION, ByteAcc, NoLock, Preserve) {
                                                             ^^^^^^^^
                Offset(20),
                MEMORY_SLOT_ENABLED,  1, // 1 if enabled, read only
                MEMORY_SLOT_INSERT_EVENT, 1, // (read) 1 if has a insert event. (write) 1 to clear event
            }

the fact that flags field is declared with "Preserve" update rule[1].
We can't fix it since that old guest (with Preserve UpdateRule) migrated
to new QEMU will always write to 1st bit whatever value it has had
the rest of reserved bits would be always 0 since old guest doesn't use
them on read side.

However since read/write semantics of fields are different, I'd fix
UpdateRule to WriteAsZeros and update protocol documentation as follow:


diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 1290994..e2cfbfc 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
               1: Device insert event, used to distinguish device for which
                  no device check event to OSPM was issued.
                  It's valid only when bit 1 is set.
-              2-7: reserved and should be ignored by OSPM
+              2: Device remove event, used to distinguish device for which
+                 no device check event to OSPM was issued.
+              3-7: reserved and should be ignored by OSPM
       [0x15-0x17] reserved
 
   write access:
@@ -31,11 +33,19 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
       [0xc-0x13] reserved, writes into it are ignored
       [0x14] Memory device control fields
           bits:
-              0: reserved, OSPM must clear it before writing to register
+              0: reserved, OSPM must clear it before writing to register.
+                 Due to BUG in versions prior 2.4 that field isn't
+                 cleared when other fields are written.Keep it reserved
+                 and don't try to reuse it.
               1: if set to 1 clears device insert event, set by OSPM
                  after it has emitted device check event for the
                  selected memory device
-              2-7: reserved, OSPM must clear them before writing to register
+              2: if set to 1 clears device remove event, set by OSPM
+                 after it has emitted device check event for the
+                 selected memory device
+              3: if set to 1 initiates device eject, set by OSPM when it
+                 triggers memory device removal and calls _EJ0 method
+              4-7: reserved, OSPM must clear them before writing to register

and extend aml_field() to support UpdateRule + modify acpi-build.c as follow:

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d0a5c85..4e81dda 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -922,7 +922,8 @@ build_ssdt(GArray *table_data, GArray *linker,
             aml_named_field(stringify(MEMORY_SLOT_PROXIMITY), 32));
         aml_append(scope, field);
 
-        field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_byte_acc);
+        field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_byte_acc,
+                          aml_write_as_zeros);
         aml_append(field, aml_reserved_field(160 /* bits, Offset(20) */));
         aml_append(field, /* 1 if enabled, read only */
             aml_named_field(stringify(MEMORY_SLOT_ENABLED), 1));


1. ACPI 5.1: 19.5.46 Field
> 
> >
> >> Thanks,
> >> Zhu
> >>
> >>>> +            dev = DEVICE(mdev->dimm);
> >>> potential NULL dereference, dimm could be NULL if guest does eject twice
> >>> or does eject of empty slot.
> >>> Perhaps add check before accessing dimm.
> >>>
> >>>    if(!mdev->is_enabled) {
> >>>       trace_..._ejecting_invalid_slot(...)
> >>>       break;
> >>>    }
> >>>
> >>>> +            hotplug_ctrl = qdev_get_hotplug_handler(dev);
> >>>> +            /* Call pc-dimm unplug cb. */
> >>>> +            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> >>> It's not that we can do anything about error at this point
> >>> but instead of forgetting it silently at least log error in trace,
> >>> the best would be in addition to that send QMP event to notify mgmt
> >>> about it. (sending QMP event could be a separate patch)
> >>>
> >>>
> >>>>            }
> >>>>            break;
> >>>> +    default:
> >>>> +        break;
> >>>>        }
> >>>>    
> >>>>    }
> >>>>
> >> [...]
> > .
> >
> 

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Igor Mammedov]

On Wed, 25 Mar 2015 14:13:23 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> 
> On 03/24/2015 06:26 PM, Igor Mammedov wrote:
> > On Tue, 24 Mar 2015 17:34:29 +0800
> > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> >
> >> On 03/23/2015 08:47 PM, Igor Mammedov wrote:
> >>> On Mon, 23 Mar 2015 18:59:28 +0800
> >>> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> >>>
> >>>> On 03/16/2015 10:59 PM, Igor Mammedov wrote:
> >>>> [...]
> >>>>>     
> >>>>> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> >>>>> index 1e9ec39..ef847e2 100644
> >>>>> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> >>>>> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> >>>>> @@ -29,6 +29,7 @@
> >>>>>                 External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only
> >>>>>                 External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, read only
> >>>>>                 External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if has a insert event. (write) 1 to clear event
> >>>>> +            External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if has a remove event. (write) 1 to clear event
> >>>>>                 External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, write only
> >>>>>                 External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event code, write only
> >>>>>                 External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status code, write only
> >>>>> @@ -55,6 +56,8 @@
> >>>>>                         If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
> >>>>>                             MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
> >>>>>                             Store(1, MEMORY_SLOT_INSERT_EVENT)
> >>>>> +                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
> >>>>> +                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
> >>>>> clear removing field here.
> >>>> You mean clear remove event here?
> >>> yes
> >> I tested this method, clear remove event here will lead to guest
> >> kernel panic.
> > it shouldn't cause panic if it only clears flag in QEMU
> > (that's what it should do).
> >
> >
> >>>>>>                         }
> >>>>>>                         // TODO: handle memory eject request
> >>>>>>                         Add(Local0, One, Local0) // goto next DIMM
> >>>>>> @@ -156,5 +159,12 @@
> >>>>>>                     Store(Arg2, MEMORY_SLOT_OST_STATUS)
> >>>>>>                     Release(MEMORY_SLOT_LOCK)
> >>>>>>                 }
> >>>>>> +
> >>>>>> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
> >>>>>> +                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
> >>>>>> +                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
> >>>>>> +                Store(One, MEMORY_SLOT_REMOVE_EVENT)
> >>>>> redo it using enabled field. Otherwise it could cause guest/QEMU crash:
> >>>>>
> >>>>> - if 1st memory was asked to be removed
> >>>>> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set
> >>>>> - then QEMU adds/removes another DIMM triggering slots scan
> >>>>>       which would issue second Notify(remove) since is_removed is still set
> >>>>> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0()
> >>>>>
> >>>> If OSPM processed the ejection request but not called _EJ0, the device
> >>>> will still be present in qemu,
> >>>> we must handle this.
> >>> There is nothing to handle in this case, if OSPM hasn't called _EJ0 then
> >>> nothing happens and device stays in QEMU.
> >>>
> >>>> So I think OSPM issues double EJ0 maybe reasonable
> >>>> in this situation.
> >>>> What's your opinion?
> >>> the first _EJ0 must do ejection, as for the second I think it should be NOP.
> >> So we should judge the enabled field to check whether the device is
> >> present before
> >> issuing Notify(remove)?
> > I wouldn't check if device is present.
> > I'd unconditionally clear it and make sure on QEMU side that
> > operation is NOP if device is not present.
> 
> I'm sorry that I have not fully understood your meaning about
> 'redo it using enabled field'.  How to do it?
> 
> MEMORY_SLOT_ENABLED is read only, how can I use it to handle EJ0?
it is reserved in write side if you look at spec,
but appears that we can't reuse MEMORY_SLOT_ENABLED due to bug in
already released QEMU versions.
See another reply where I explained it in more details and suggested
how QSPM<->QEMU protocol should be amended.

> 
> Thanks,
> Zhu
> 
> 
>