[Qemu-devel] [PATCH v3 00/10] QEMU memory hot unplug support

[Qemu-devel] [PATCH v3 00/10] 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.

This series depends on the following patchset.
[PATCH v2 0/5] Common unplug and unplug request cb for memory and CPU hot-unplug
https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg03929.html

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

Hu Tao (1):
  ich9, piix4, pc-dimm: Add memory hot unplug request support

Tang Chen (7):
  acpi, mem-hotplug: Use PC_DIMM_SLOT_PROP in acpi_memory_plug_cb().
  acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus.
  acpi, mem-hotplug: Add acpi_send_gpe_event() to rise sci for memory
    hotplug.
  acpi, mem-hotplug: Add unplug request cb for memory device.
  acpi, mem-hotplug: Add unplug cb for memory device.
  ich9, piix4, pc-dimm: Add memory hot unplug support
  acpi: Add hardware implementation for memory hot unplug.

Zhu Guihua (2):
  qdev: make qdev_get_hotplug_handler() non-static
  ssdt-mem: add MEMORY_SLOT_EJECT_METHOD

 docs/specs/acpi_mem_hotplug.txt   | 11 ++++-
 hw/acpi/core.c                    |  7 +++
 hw/acpi/ich9.c                    | 19 +++++++--
 hw/acpi/memory_hotplug.c          | 90 +++++++++++++++++++++++++++++++++------
 hw/acpi/piix4.c                   | 16 +++++--
 hw/core/qdev.c                    |  2 +-
 hw/i386/acpi-dsdt-mem-hotplug.dsl | 11 ++++-
 hw/i386/pc.c                      | 54 +++++++++++++++++++++--
 hw/i386/ssdt-mem.dsl              |  5 +++
 include/hw/acpi/acpi.h            |  3 ++
 include/hw/acpi/memory_hotplug.h  |  6 +++
 include/hw/acpi/pc-hotplug.h      |  2 +
 include/hw/qdev-core.h            |  1 +
 trace-events                      |  1 +
 14 files changed, 199 insertions(+), 29 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 01/10] acpi, mem-hotplug: Use PC_DIMM_SLOT_PROP in acpi_memory_plug_cb().

[Zhu Guihua]

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

Replace string "slot" in acpi_memory_plug_cb() with MACRO PC_DIMM_SLOT_PROP.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/acpi/memory_hotplug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index ed39241..c6580da 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -168,7 +168,8 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
 {
     MemStatus *mdev;
     Error *local_err = NULL;
-    int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
+    int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
+                                       &local_err);
 
     if (local_err) {
         error_propagate(errp, local_err);
-- 
1.9.3

[Qemu-devel] [PATCH v3 02/10] 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 | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index c6580da..6d91a0d 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_get_slot_status_descriptor: "
                    "device [%s] returned invalid memory slot[%d]",
-                    dev_path, slot);
+                   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] [PATCH v3 03/10] acpi, mem-hotplug: Add acpi_send_gpe_event() to rise sci for memory hotplug.

[Zhu Guihua]

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

Add a new API named acpi_send_gpe_event() to send memory hotplug SCI.
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/core.c           | 7 +++++++
 hw/acpi/memory_hotplug.c | 6 ++----
 include/hw/acpi/acpi.h   | 3 +++
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 51913d6..98ca994 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -666,6 +666,13 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
     return val;
 }
 
+void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
+                         unsigned int hotplug_status)
+{
+    ar->gpe.sts[0] |= hotplug_status;
+    acpi_update_sci(ar, irq);
+}
+
 void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
 {
     int sci_level, pm1a_sts;
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 6d91a0d..5b13baa 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -202,10 +202,8 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
     mdev->is_enabled = true;
     mdev->is_inserting = true;
 
-    /* do ACPI magic */
-    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
-    acpi_update_sci(ar, irq);
-    return;
+    /* Do ACPI magic */
+    acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
 }
 
 static const VMStateDescription vmstate_memhp_sts = {
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 1f678b4..7a0a209 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -172,6 +172,9 @@ void acpi_gpe_reset(ACPIREGS *ar);
 void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val);
 uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
 
+void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
+                         unsigned int hotplug_status);
+
 void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
 
 /* acpi.c */
-- 
1.9.3

[Qemu-devel] [PATCH v3 04/10] acpi, mem-hotplug: Add unplug request cb for memory device.

[Zhu Guihua]

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

Memory hot unplug are both asynchronize procedures.
When the unplug operation happens, unplug request cb is called first.
And when ghest 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. Add a new bool
member named is_removing to MemStatus indicating that the memory slot
is being removed. Set it to true in acpi_memory_unplug_request_cb(),
and send SCI to guest.

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

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 5b13baa..02231d2 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -206,6 +206,23 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
     acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
 }
 
+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 */
+    acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
+}
+
 static const VMStateDescription vmstate_memhp_sts = {
     .name = "memory hotplug device state",
     .version_id = 1,
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] [PATCH v3 05/10] ich9, piix4, pc-dimm: Add memory hot unplug request support

[Zhu Guihua]

From: Hu Tao <hutao@cn.fujitsu.com>

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

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
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/piix4.c |  6 +++++-
 hw/i386/pc.c    | 28 ++++++++++++++++++++++++++--
 3 files changed, 39 insertions(+), 5 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/piix4.c b/hw/acpi/piix4.c
index 14d40a0..8bd9007 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/hw/i386/pc.c b/hw/i386/pc.c
index 850b6b5..ddc0190 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1641,6 +1641,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)
 {
@@ -1683,8 +1703,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] [PATCH v3 06/10] acpi, mem-hotplug: Add unplug cb for memory device.

[Zhu Guihua]

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

Reset all memory status, and unparent the memory device.

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

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 02231d2..ffacecf 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -223,6 +223,20 @@ void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
     acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
 }
 
+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/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] [PATCH v3 07/10] ich9, piix4, pc-dimm: Add memory hot unplug support

[Zhu Guihua]

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

Call memory unplug cb in ich9_pm_device_unplug_cb() and
piix4_device_unplug_cb().
Implement unplug cb for pc-dimm. It remove the corresponding
memory region, and unregister vmstat. At last, it calls memory
unplug cb to reset memory status and do unparenting.

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/piix4.c | 10 ++++++++--
 hw/i386/pc.c    | 26 ++++++++++++++++++++++++--
 3 files changed, 39 insertions(+), 6 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/piix4.c b/hw/acpi/piix4.c
index 8bd9007..116c163 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/hw/i386/pc.c b/hw/i386/pc.c
index ddc0190..05e9058 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1661,6 +1661,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)
 {
@@ -1714,8 +1731,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] [PATCH v3 08/10] qdev: make qdev_get_hotplug_handler() non-static

[Zhu Guihua]

Memory hotplug code will use qdev_get_hotplug_handler().

Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/core/qdev.c         | 2 +-
 include/hw/qdev-core.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 2eacac0..2f3d1df 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/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 15a226f..03d6239 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);
-- 
1.9.3

[Qemu-devel] [PATCH v3 09/10] ssdt-mem: add MEMORY_SLOT_EJECT_METHOD

[Zhu Guihua]

Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/i386/ssdt-mem.dsl         | 5 +++++
 include/hw/acpi/pc-hotplug.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/i386/ssdt-mem.dsl b/hw/i386/ssdt-mem.dsl
index 22ff5dd..f0a56de 100644
--- a/hw/i386/ssdt-mem.dsl
+++ b/hw/i386/ssdt-mem.dsl
@@ -43,6 +43,7 @@ DefinitionBlock ("ssdt-mem.aml", "SSDT", 0x02, "BXPC", "CSSDT", 0x1)
     External(\_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_STATUS_METHOD, MethodObj)
     External(\_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_OST_METHOD, MethodObj)
     External(\_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_PROXIMITY_METHOD, MethodObj)
+    External(\_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_EJECT_METHOD, MethodObj)
 
     Scope(\_SB) {
 /*  v------------------ DO NOT EDIT ------------------v */
@@ -72,6 +73,10 @@ DefinitionBlock ("ssdt-mem.aml", "SSDT", 0x02, "BXPC", "CSSDT", 0x1)
             Method(_OST, 3) {
                 \_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_OST_METHOD(_UID, Arg0, Arg1, Arg2)
             }
+
+            Method(_EJ0, 1) {
+                 \_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_EJECT_METHOD(_UID, Arg0)
+            }
         }
     }
 }
diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
index b9db295..ae30ff1 100644
--- a/include/hw/acpi/pc-hotplug.h
+++ b/include/hw/acpi/pc-hotplug.h
@@ -50,6 +50,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
 
-- 
1.9.3

[Qemu-devel] [PATCH v3 10/10] acpi: Add hardware implementation for memory hot unplug.

[Zhu Guihua]

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

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: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
 hw/acpi/memory_hotplug.c          | 24 ++++++++++++++++++++++--
 hw/i386/acpi-dsdt-mem-hotplug.dsl | 11 ++++++++++-
 include/hw/acpi/pc-hotplug.h      |  1 +
 trace-events                      |  1 +
 5 files changed, 43 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 ffacecf..64c7b33 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"
 
@@ -75,6 +76,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:
@@ -90,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;
@@ -121,18 +125,34 @@ 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/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
index 2a36c47..b53bf77 100644
--- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
+++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
@@ -50,6 +50,7 @@
                 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
+                MEMORY_SLOT_REMOVE_EVENT, 1, // (read) 1 if has a remove event. (write) 1 to clear event
             }
 
             Mutex (MEMORY_SLOT_LOCK, 0)
@@ -71,8 +72,9 @@
                     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
                 }
                 Release(MEMORY_SLOT_LOCK)
@@ -172,5 +174,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 ae30ff1..b61b6ea 100644
--- a/include/hw/acpi/pc-hotplug.h
+++ b/include/hw/acpi/pc-hotplug.h
@@ -42,6 +42,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
diff --git a/trace-events b/trace-events
index f87b077..4439f45 100644
--- a/trace-events
+++ b/trace-events
@@ -1568,6 +1568,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] [PATCH v3 08/10] qdev: make qdev_get_hotplug_handler() non-static

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 09:16:50AM +0800, Zhu Guihua wrote:
> Memory hotplug code will use qdev_get_hotplug_handler().
> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

don't send patches like this pls.
Just merge this with code that uses it.

> ---
>  hw/core/qdev.c         | 2 +-
>  include/hw/qdev-core.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 2eacac0..2f3d1df 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/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 15a226f..03d6239 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);
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 09/10] ssdt-mem: add MEMORY_SLOT_EJECT_METHOD

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 09:16:51AM +0800, Zhu Guihua wrote:
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

so you add an extern method, but it's not implemented
until the next patch.
anyone bisecting between these patches will get
ospm crashes.

Please just squash this all together.

Also, need to rebase this on my pci branch -
ssdt-mem is gone there.

> ---
>  hw/i386/ssdt-mem.dsl         | 5 +++++
>  include/hw/acpi/pc-hotplug.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/hw/i386/ssdt-mem.dsl b/hw/i386/ssdt-mem.dsl
> index 22ff5dd..f0a56de 100644
> --- a/hw/i386/ssdt-mem.dsl
> +++ b/hw/i386/ssdt-mem.dsl
> @@ -43,6 +43,7 @@ DefinitionBlock ("ssdt-mem.aml", "SSDT", 0x02, "BXPC", "CSSDT", 0x1)
>      External(\_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_STATUS_METHOD, MethodObj)
>      External(\_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_OST_METHOD, MethodObj)
>      External(\_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_PROXIMITY_METHOD, MethodObj)
> +    External(\_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_EJECT_METHOD, MethodObj)
>  
>      Scope(\_SB) {
>  /*  v------------------ DO NOT EDIT ------------------v */
> @@ -72,6 +73,10 @@ DefinitionBlock ("ssdt-mem.aml", "SSDT", 0x02, "BXPC", "CSSDT", 0x1)
>              Method(_OST, 3) {
>                  \_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_OST_METHOD(_UID, Arg0, Arg1, Arg2)
>              }
> +
> +            Method(_EJ0, 1) {
> +                 \_SB.PCI0.MEMORY_HOTPLUG_DEVICE.MEMORY_SLOT_EJECT_METHOD(_UID, Arg0)
> +            }
>          }
>      }
>  }
> diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
> index b9db295..ae30ff1 100644
> --- a/include/hw/acpi/pc-hotplug.h
> +++ b/include/hw/acpi/pc-hotplug.h
> @@ -50,6 +50,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
>  
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 10/10] acpi: Add hardware implementation for memory hot unplug.

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 09:16:52AM +0800, Zhu Guihua wrote:
> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> 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: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

so previous patch adds _EJ0 but no real hardware to
back it up. It's bad for bisect and is not making
review easier.


> ---
>  docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
>  hw/acpi/memory_hotplug.c          | 24 ++++++++++++++++++++++--
>  hw/i386/acpi-dsdt-mem-hotplug.dsl | 11 ++++++++++-
>  include/hw/acpi/pc-hotplug.h      |  1 +
>  trace-events                      |  1 +
>  5 files changed, 43 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 ffacecf..64c7b33 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"
>  
> @@ -75,6 +76,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:
> @@ -90,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;
> @@ -121,18 +125,34 @@ 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/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> index 2a36c47..b53bf77 100644
> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> @@ -50,6 +50,7 @@
>                  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
> +                MEMORY_SLOT_REMOVE_EVENT, 1, // (read) 1 if has a remove event. (write) 1 to clear event
>              }
>  
>              Mutex (MEMORY_SLOT_LOCK, 0)
> @@ -71,8 +72,9 @@
>                      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
>                  }
>                  Release(MEMORY_SLOT_LOCK)
> @@ -172,5 +174,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 ae30ff1..b61b6ea 100644
> --- a/include/hw/acpi/pc-hotplug.h
> +++ b/include/hw/acpi/pc-hotplug.h
> @@ -42,6 +42,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
> diff --git a/trace-events b/trace-events
> index f87b077..4439f45 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1568,6 +1568,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] [PATCH v3 06/10] acpi, mem-hotplug: Add unplug cb for memory device.

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 09:16:48AM +0800, Zhu Guihua wrote:
> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> Reset all memory status, and unparent the memory device.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

Please, stop breaking patches up like this.
You add new a function, with zero documentation, and
no users until later in series.
Does it do what it should do? Why are there now both
acpi_memory_unplug_request_cb and acpi_memory_unplug_cb?
When is this called?
No way to tell from documentation since there's none.
I guess one is supposed to look at the follow-up patches
to figure it out. Just include this in the next patch then.

Same applies everywhere really.

Split patches along functional lines, not by affected file.


> ---
>  hw/acpi/memory_hotplug.c         | 14 ++++++++++++++
>  include/hw/acpi/memory_hotplug.h |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 02231d2..ffacecf 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -223,6 +223,20 @@ void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
>      acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
>  }
>  
> +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/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

Re: [Qemu-devel] [PATCH v3 04/10] acpi, mem-hotplug: Add unplug request cb for memory device.

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 09:16:46AM +0800, Zhu Guihua wrote:
> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> Memory hot unplug are both asynchronize procedures.

asynchronous

> When the unplug operation happens, unplug request cb is called first.
> And when ghest

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.

It does not such thing apparently. It just implements
an unused function - I guess it will be used as a callback
by some other patch.


> Add a new bool
> member named is_removing to MemStatus indicating that the memory slot
> is being removed. Set it to true in acpi_memory_unplug_request_cb(),
> and send SCI to guest.

You describe what code does in great detail but not why
it does it.
There's a new field that is ever only set to true and never
read.
I guess this will make sense eventually with follow-up
patches but why not include it all there?

> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  hw/acpi/memory_hotplug.c         | 17 +++++++++++++++++
>  include/hw/acpi/memory_hotplug.h |  4 ++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 5b13baa..02231d2 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -206,6 +206,23 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>      acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
>  }
>  
> +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 */
> +    acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
> +}
> +
>  static const VMStateDescription vmstate_memhp_sts = {
>      .name = "memory hotplug device state",
>      .version_id = 1,
> 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

Re: [Qemu-devel] [PATCH v3 03/10] acpi, mem-hotplug: Add acpi_send_gpe_event() to rise sci for memory hotplug.

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 09:16:45AM +0800, Zhu Guihua wrote:
> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> Add a new API named acpi_send_gpe_event() to send memory hotplug SCI.
> 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/core.c           | 7 +++++++
>  hw/acpi/memory_hotplug.c | 6 ++----
>  include/hw/acpi/acpi.h   | 3 +++
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 51913d6..98ca994 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -666,6 +666,13 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
>      return val;
>  }
>  
> +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> +                         unsigned int hotplug_status)
> +{
> +    ar->gpe.sts[0] |= hotplug_status;
> +    acpi_update_sci(ar, irq);
> +}
> +
>  void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
>  {
>      int sci_level, pm1a_sts;
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 6d91a0d..5b13baa 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -202,10 +202,8 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>      mdev->is_enabled = true;
>      mdev->is_inserting = true;
>  
> -    /* do ACPI magic */
> -    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> -    acpi_update_sci(ar, irq);
> -    return;
> +    /* Do ACPI magic */
> +    acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
>  }
>  
>  static const VMStateDescription vmstate_memhp_sts = {

This is hardly the only place where we change sts[0].

If you are doing this kind of API work, you need
to fix it all up.

> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index 1f678b4..7a0a209 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -172,6 +172,9 @@ void acpi_gpe_reset(ACPIREGS *ar);
>  void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val);
>  uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
>  
> +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> +                         unsigned int hotplug_status);
> +

need to define legal values for hotplug_status.

>  void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
>  
>  /* acpi.c */
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 02/10] acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus.

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 09:16:44AM +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>

Same comments as everywhere: include implementation with
users.
In this case, I can't tell whether error_setg is
appropriate since I don't know whether the error
ever comes from user.


> ---
>  hw/acpi/memory_hotplug.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index c6580da..6d91a0d 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_get_slot_status_descriptor: "
>                     "device [%s] returned invalid memory slot[%d]",
> -                    dev_path, slot);
> +                   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] [PATCH v3 01/10] acpi, mem-hotplug: Use PC_DIMM_SLOT_PROP in acpi_memory_plug_cb().

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 09:16:43AM +0800, Zhu Guihua wrote:
> From: Tang Chen <tangchen@cn.fujitsu.com>
> 
> Replace string "slot" in acpi_memory_plug_cb() with MACRO PC_DIMM_SLOT_PROP.

Probably should be "macro".

> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

Applied, thanks.

> ---
>  hw/acpi/memory_hotplug.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index ed39241..c6580da 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -168,7 +168,8 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>  {
>      MemStatus *mdev;
>      Error *local_err = NULL;
> -    int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
> +    int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> +                                       &local_err);
>  
>      if (local_err) {
>          error_propagate(errp, local_err);
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 00/10] QEMU memory hot unplug support

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 09:16:42AM +0800, Zhu Guihua wrote:
> 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 series depends on the following patchset.
> [PATCH v2 0/5] Common unplug and unplug request cb for memory and CPU hot-unplug
> https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg03929.html

So this needs a bit more work reorganizing patches, and also needs to be
rebased to latest pci branch.
I'll probably send last non-bugfix pull request tomorrow, most likely
this means this won't be in 2.3.


> 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
> 
> Hu Tao (1):
>   ich9, piix4, pc-dimm: Add memory hot unplug request support
> 
> Tang Chen (7):
>   acpi, mem-hotplug: Use PC_DIMM_SLOT_PROP in acpi_memory_plug_cb().
>   acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus.
>   acpi, mem-hotplug: Add acpi_send_gpe_event() to rise sci for memory
>     hotplug.
>   acpi, mem-hotplug: Add unplug request cb for memory device.
>   acpi, mem-hotplug: Add unplug cb for memory device.
>   ich9, piix4, pc-dimm: Add memory hot unplug support
>   acpi: Add hardware implementation for memory hot unplug.
> 
> Zhu Guihua (2):
>   qdev: make qdev_get_hotplug_handler() non-static
>   ssdt-mem: add MEMORY_SLOT_EJECT_METHOD
> 
>  docs/specs/acpi_mem_hotplug.txt   | 11 ++++-
>  hw/acpi/core.c                    |  7 +++
>  hw/acpi/ich9.c                    | 19 +++++++--
>  hw/acpi/memory_hotplug.c          | 90 +++++++++++++++++++++++++++++++++------
>  hw/acpi/piix4.c                   | 16 +++++--
>  hw/core/qdev.c                    |  2 +-
>  hw/i386/acpi-dsdt-mem-hotplug.dsl | 11 ++++-
>  hw/i386/pc.c                      | 54 +++++++++++++++++++++--
>  hw/i386/ssdt-mem.dsl              |  5 +++
>  include/hw/acpi/acpi.h            |  3 ++
>  include/hw/acpi/memory_hotplug.h  |  6 +++
>  include/hw/acpi/pc-hotplug.h      |  2 +
>  include/hw/qdev-core.h            |  1 +
>  trace-events                      |  1 +
>  14 files changed, 199 insertions(+), 29 deletions(-)
> 
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH v3 03/10] acpi, mem-hotplug: Add acpi_send_gpe_event() to rise sci for memory hotplug.

[Igor Mammedov]

On Sun, 1 Mar 2015 18:29:39 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Feb 26, 2015 at 09:16:45AM +0800, Zhu Guihua wrote:
> > From: Tang Chen <tangchen@cn.fujitsu.com>
> > 
> > Add a new API named acpi_send_gpe_event() to send memory hotplug SCI.
> > 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/core.c           | 7 +++++++
> >  hw/acpi/memory_hotplug.c | 6 ++----
> >  include/hw/acpi/acpi.h   | 3 +++
> >  3 files changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > index 51913d6..98ca994 100644
> > --- a/hw/acpi/core.c
> > +++ b/hw/acpi/core.c
> > @@ -666,6 +666,13 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
> >      return val;
> >  }
> >  
> > +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> > +                         unsigned int hotplug_status)
> > +{
> > +    ar->gpe.sts[0] |= hotplug_status;
> > +    acpi_update_sci(ar, irq);
> > +}
> > +
> >  void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
> >  {
> >      int sci_level, pm1a_sts;
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index 6d91a0d..5b13baa 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -202,10 +202,8 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> >      mdev->is_enabled = true;
> >      mdev->is_inserting = true;
> >  
> > -    /* do ACPI magic */
> > -    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> > -    acpi_update_sci(ar, irq);
> > -    return;
> > +    /* Do ACPI magic */
> > +    acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
> >  }
> >  
> >  static const VMStateDescription vmstate_memhp_sts = {
> 
> This is hardly the only place where we change sts[0].
> 
> If you are doing this kind of API work, you need
> to fix it all up.
I've looked it /i.e. API change/, but it turned out to be rather big
refactoring hence it's too late for 2.3
I'll look at it later when 2.4 cycle opens.

> 
> > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > index 1f678b4..7a0a209 100644
> > --- a/include/hw/acpi/acpi.h
> > +++ b/include/hw/acpi/acpi.h
> > @@ -172,6 +172,9 @@ void acpi_gpe_reset(ACPIREGS *ar);
> >  void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val);
> >  uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
> >  
> > +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> > +                         unsigned int hotplug_status);
> > +
> 
> need to define legal values for hotplug_status.
> 
> >  void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
> >  
> >  /* acpi.c */
> > -- 
> > 1.9.3

Re: [Qemu-devel] [PATCH v3 03/10] acpi, mem-hotplug: Add acpi_send_gpe_event() to rise sci for memory hotplug.

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 10:27:29AM +0100, Igor Mammedov wrote:
> On Sun, 1 Mar 2015 18:29:39 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Feb 26, 2015 at 09:16:45AM +0800, Zhu Guihua wrote:
> > > From: Tang Chen <tangchen@cn.fujitsu.com>
> > > 
> > > Add a new API named acpi_send_gpe_event() to send memory hotplug SCI.
> > > 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/core.c           | 7 +++++++
> > >  hw/acpi/memory_hotplug.c | 6 ++----
> > >  include/hw/acpi/acpi.h   | 3 +++
> > >  3 files changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > > index 51913d6..98ca994 100644
> > > --- a/hw/acpi/core.c
> > > +++ b/hw/acpi/core.c
> > > @@ -666,6 +666,13 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
> > >      return val;
> > >  }
> > >  
> > > +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> > > +                         unsigned int hotplug_status)
> > > +{
> > > +    ar->gpe.sts[0] |= hotplug_status;
> > > +    acpi_update_sci(ar, irq);
> > > +}
> > > +
> > >  void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
> > >  {
> > >      int sci_level, pm1a_sts;
> > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > > index 6d91a0d..5b13baa 100644
> > > --- a/hw/acpi/memory_hotplug.c
> > > +++ b/hw/acpi/memory_hotplug.c
> > > @@ -202,10 +202,8 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> > >      mdev->is_enabled = true;
> > >      mdev->is_inserting = true;
> > >  
> > > -    /* do ACPI magic */
> > > -    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> > > -    acpi_update_sci(ar, irq);
> > > -    return;
> > > +    /* Do ACPI magic */
> > > +    acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
> > >  }
> > >  
> > >  static const VMStateDescription vmstate_memhp_sts = {
> > 
> > This is hardly the only place where we change sts[0].
> > 
> > If you are doing this kind of API work, you need
> > to fix it all up.
> I've looked it /i.e. API change/, but it turned out to be rather big
> refactoring hence it's too late for 2.3
> I'll look at it later when 2.4 cycle opens.

I don't mind who does it but it's a good idea
to sync with Zhu Guihua here to avoid duplicating work.

> > 
> > > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > > index 1f678b4..7a0a209 100644
> > > --- a/include/hw/acpi/acpi.h
> > > +++ b/include/hw/acpi/acpi.h
> > > @@ -172,6 +172,9 @@ void acpi_gpe_reset(ACPIREGS *ar);
> > >  void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val);
> > >  uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
> > >  
> > > +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> > > +                         unsigned int hotplug_status);
> > > +
> > 
> > need to define legal values for hotplug_status.
> > 
> > >  void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
> > >  
> > >  /* acpi.c */
> > > -- 
> > > 1.9.3

Re: [Qemu-devel] [PATCH v3 02/10] acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus.

[Zhu Guihua]

On 03/02/2015 01:31 AM, Michael S. Tsirkin wrote:
> On Thu, Feb 26, 2015 at 09:16:44AM +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>
> Same comments as everywhere: include implementation with
> users.
> In this case, I can't tell whether error_setg is
> appropriate since I don't know whether the error
> ever comes from user.

Do you have any suggestions about this error_setg?

Thanks,
Zhu

>
>> ---
>>   hw/acpi/memory_hotplug.c | 26 +++++++++++++++++++-------
>>   1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> index c6580da..6d91a0d 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_get_slot_status_descriptor: "
>>                      "device [%s] returned invalid memory slot[%d]",
>> -                    dev_path, slot);
>> +                   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] [PATCH v3 02/10] acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus.

[Michael S. Tsirkin]

On Tue, Mar 03, 2015 at 10:18:12AM +0800, Zhu Guihua wrote:
> 
> On 03/02/2015 01:31 AM, Michael S. Tsirkin wrote:
> >On Thu, Feb 26, 2015 at 09:16:44AM +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>
> >Same comments as everywhere: include implementation with
> >users.
> >In this case, I can't tell whether error_setg is
> >appropriate since I don't know whether the error
> >ever comes from user.
> 
> Do you have any suggestions about this error_setg?
> 
> Thanks,
> Zhu

If it's guest triggered, it probably shouldn't
print to monitor. If it's user triggered, then it should.

> >
> >>---
> >>  hw/acpi/memory_hotplug.c | 26 +++++++++++++++++++-------
> >>  1 file changed, 19 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> >>index c6580da..6d91a0d 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_get_slot_status_descriptor: "
> >>                     "device [%s] returned invalid memory slot[%d]",
> >>-                    dev_path, slot);
> >>+                   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] [PATCH v3 02/10] acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus.

[Zhu Guihua]

On 03/03/2015 09:43 PM, Michael S. Tsirkin wrote:
> On Tue, Mar 03, 2015 at 10:18:12AM +0800, Zhu Guihua wrote:
>> On 03/02/2015 01:31 AM, Michael S. Tsirkin wrote:
>>> On Thu, Feb 26, 2015 at 09:16:44AM +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>
>>> Same comments as everywhere: include implementation with
>>> users.
>>> In this case, I can't tell whether error_setg is
>>> appropriate since I don't know whether the error
>>> ever comes from user.
>> Do you have any suggestions about this error_setg?
>>
>> Thanks,
>> Zhu
> If it's guest triggered, it probably shouldn't
> print to monitor. If it's user triggered, then it should.

Oh, I understand.
In this case, the error is not triggered by guest.

Thanks,
Zhu
>>>> ---
>>>>   hw/acpi/memory_hotplug.c | 26 +++++++++++++++++++-------
>>>>   1 file changed, 19 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>>>> index c6580da..6d91a0d 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_get_slot_status_descriptor: "
>>>>                      "device [%s] returned invalid memory slot[%d]",
>>>> -                    dev_path, slot);
>>>> +                   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] [PATCH v3 03/10] acpi, mem-hotplug: Add acpi_send_gpe_event() to rise sci for memory hotplug.

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 10:27:29AM +0100, Igor Mammedov wrote:
> On Sun, 1 Mar 2015 18:29:39 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Feb 26, 2015 at 09:16:45AM +0800, Zhu Guihua wrote:
> > > From: Tang Chen <tangchen@cn.fujitsu.com>
> > > 
> > > Add a new API named acpi_send_gpe_event() to send memory hotplug SCI.
> > > 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/core.c           | 7 +++++++
> > >  hw/acpi/memory_hotplug.c | 6 ++----
> > >  include/hw/acpi/acpi.h   | 3 +++
> > >  3 files changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > > index 51913d6..98ca994 100644
> > > --- a/hw/acpi/core.c
> > > +++ b/hw/acpi/core.c
> > > @@ -666,6 +666,13 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
> > >      return val;
> > >  }
> > >  
> > > +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> > > +                         unsigned int hotplug_status)
> > > +{
> > > +    ar->gpe.sts[0] |= hotplug_status;
> > > +    acpi_update_sci(ar, irq);
> > > +}
> > > +
> > >  void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
> > >  {
> > >      int sci_level, pm1a_sts;
> > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > > index 6d91a0d..5b13baa 100644
> > > --- a/hw/acpi/memory_hotplug.c
> > > +++ b/hw/acpi/memory_hotplug.c
> > > @@ -202,10 +202,8 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> > >      mdev->is_enabled = true;
> > >      mdev->is_inserting = true;
> > >  
> > > -    /* do ACPI magic */
> > > -    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> > > -    acpi_update_sci(ar, irq);
> > > -    return;
> > > +    /* Do ACPI magic */
> > > +    acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
> > >  }
> > >  
> > >  static const VMStateDescription vmstate_memhp_sts = {
> > 
> > This is hardly the only place where we change sts[0].
> > 
> > If you are doing this kind of API work, you need
> > to fix it all up.
> I've looked it /i.e. API change/, but it turned out to be rather big
> refactoring hence it's too late for 2.3
> I'll look at it later when 2.4 cycle opens.

I think we don't have a specific schedule for 2.3,
but I don't mind. However, I'd rather either convert everyone, or no
one. Let's not leave codebase in an inconsistent state.
For now open-coding will work fine.


> > 
> > > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > > index 1f678b4..7a0a209 100644
> > > --- a/include/hw/acpi/acpi.h
> > > +++ b/include/hw/acpi/acpi.h
> > > @@ -172,6 +172,9 @@ void acpi_gpe_reset(ACPIREGS *ar);
> > >  void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val);
> > >  uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
> > >  
> > > +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> > > +                         unsigned int hotplug_status);
> > > +
> > 
> > need to define legal values for hotplug_status.
> > 
> > >  void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
> > >  
> > >  /* acpi.c */
> > > -- 
> > > 1.9.3