[Qemu-devel] [RESEND PATCH v3 0/8] QEmu memory hot unplug support.

[Qemu-devel] [RESEND PATCH v3 0/8] QEmu memory hot unplug support.

[Tang Chen]

This patch-set implements memory hot-remove for QEmu.

Approach: QEmu sets GPE status bit, then triggers SCI to notify guest os.
Guest os checks device status, and free memory resource if possible,
then generate OST. Finally, qemu handles OST events to free dimm device.

Change log v2 -> v3:
1. Divide the large patch into 7 small patches. 
   (patch 1, 3~8)
2. Add new patch 2/8, implementing memory hot-remove for ich9.
3. Sanitize ACPI hardware implement code in patch 6/8.

Hu Tao (6):
  acpi, piix4: Add memory hot unplug support for piix4.
  pc: Add memory hot unplug support for pc machine.
  qdev: Add memory hot unplug support for bus-less devices.
  pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support.
  pc, acpi bios: Add memory hot unplug interface.
  monitor: Add memory hot unplug support for device_del command.

Tang Chen (2):
  acpi, ich9: Add memory hot unplug support for ich9.
  acpi: Add hardware implementation for memory hot unplug.

 hw/acpi/ich9.c                   | 12 ++++++
 hw/acpi/memory_hotplug.c         | 87 ++++++++++++++++++++++++++++++++++++++--
 hw/acpi/piix4.c                  |  3 ++
 hw/core/qdev.c                   |  8 ++++
 hw/i386/pc.c                     | 32 +++++++++++++++
 hw/i386/ssdt-mem.dsl             |  5 +++
 hw/i386/ssdt-misc.dsl            | 15 ++++++-
 hw/isa/lpc_ich9.c                |  5 ++-
 hw/mem/pc-dimm.c                 | 10 +++++
 include/hw/acpi/acpi.h           | 14 +++++++
 include/hw/acpi/ich9.h           |  2 +
 include/hw/acpi/memory_hotplug.h |  3 ++
 include/hw/acpi/pc-hotplug.h     |  2 +
 include/qom/object.h             |  1 +
 qdev-monitor.c                   | 23 +++++++++++
 qom/object.c                     |  2 +-
 16 files changed, 217 insertions(+), 7 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [RESEND PATCH v3 1/8] acpi, piix4: Add memory hot unplug support for piix4.

[Tang Chen]

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

Implement acpi_memory_unplug_cb(), sending an sci to guest to trigger
memory hot-remove, and call it in piix4_device_unplug_cb().

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 hw/acpi/memory_hotplug.c         | 25 +++++++++++++++++++++++++
 hw/acpi/piix4.c                  |  3 +++
 include/hw/acpi/memory_hotplug.h |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index ed39241..c310b44 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -195,6 +195,31 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
     return;
 }
 
+void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
+                           DeviceState *dev, Error **errp)
+{
+    Error *local_err = NULL;
+    int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (slot >= mem_st->dev_count) {
+        char *dev_path = object_get_canonical_path(OBJECT(dev));
+        error_setg(errp, "acpi_memory_plug_cb: "
+                   "device [%s] returned invalid memory slot[%d]",
+                   dev_path, slot);
+        g_free(dev_path);
+        return;
+    }
+
+    /* 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 b72b34e..c1d5187 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -362,6 +362,9 @@ static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
     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 if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        acpi_memory_unplug_cb(&s->ar, s->irq, &s->acpi_memory_hotplug, dev,
+                              errp);
     } else {
         error_setg(errp, "acpi: device unplug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index 7bbf8a0..fc6b868 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -28,6 +28,8 @@ 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_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.8.4.2

Re: [Qemu-devel] [RESEND PATCH v3 1/8] acpi, piix4: Add memory hot unplug support for piix4.

[Igor Mammedov]

On Wed, 27 Aug 2014 16:08:32 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> From: Hu Tao <hutao@cn.fujitsu.com>
> 
> Implement acpi_memory_unplug_cb(), sending an sci to guest to trigger
> memory hot-remove, and call it in piix4_device_unplug_cb().
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  hw/acpi/memory_hotplug.c         | 25 +++++++++++++++++++++++++
>  hw/acpi/piix4.c                  |  3 +++
>  include/hw/acpi/memory_hotplug.h |  2 ++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index ed39241..c310b44 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -195,6 +195,31 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>      return;
>  }
>  
> +void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> +                           DeviceState *dev, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
might be worth to use PC_DIMM_SLOT_PROP here, and while at it
do the same to acpi_memory_plug_cb() as well in separate patch.

> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (slot >= mem_st->dev_count) {
> +        char *dev_path = object_get_canonical_path(OBJECT(dev));
> +        error_setg(errp, "acpi_memory_plug_cb: "
> +                   "device [%s] returned invalid memory slot[%d]",
> +                   dev_path, slot);
> +        g_free(dev_path);
> +        return;
> +    }
move 3 above blocks into separate function to reduce code duplication
with acpi_memory_plug_cb(), perhaps something like following:

MemStatus *
acpi_memory_get_slot_status_descriptor(MemHotplugState *mem_st,
                                       DeviceState *dev, Error **errp);

>+
place here also missing removal signaling for selected mdev,
perhaps it's due to wrong patch ordering

> +    /* do ACPI magic */
> +    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> +    acpi_update_sci(ar, irq);
worth to make this block a separate function and reuse in
acpi_memory_plug_cb()

> +}
> +
>  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 b72b34e..c1d5187 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -362,6 +362,9 @@ static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
>      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 if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        acpi_memory_unplug_cb(&s->ar, s->irq, &s->acpi_memory_hotplug, dev,
> +                              errp);
>      } else {
>          error_setg(errp, "acpi: device unplug request for not supported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index 7bbf8a0..fc6b868 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -28,6 +28,8 @@ 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_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 v3 1/8] acpi, piix4: Add memory hot unplug support for piix4.

[Tang Chen]

Hi Igor,

On 09/04/2014 08:15 PM, Igor Mammedov wrote:
> On Wed, 27 Aug 2014 16:08:32 +0800
> Tang Chen <tangchen@cn.fujitsu.com> wrote:
>
>> From: Hu Tao <hutao@cn.fujitsu.com>
>>
>> Implement acpi_memory_unplug_cb(), sending an sci to guest to trigger
>> memory hot-remove, and call it in piix4_device_unplug_cb().
>>
>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
>> ---
>>   hw/acpi/memory_hotplug.c         | 25 +++++++++++++++++++++++++
>>   hw/acpi/piix4.c                  |  3 +++
>>   include/hw/acpi/memory_hotplug.h |  2 ++
>>   3 files changed, 30 insertions(+)
>>
>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> index ed39241..c310b44 100644
>> --- a/hw/acpi/memory_hotplug.c
>> +++ b/hw/acpi/memory_hotplug.c
>> @@ -195,6 +195,31 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>>       return;
>>   }
>>   
>> +void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>> +                           DeviceState *dev, Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
> might be worth to use PC_DIMM_SLOT_PROP here, and while at it
> do the same to acpi_memory_plug_cb() as well in separate patch.
>

Followed.

>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    if (slot >= mem_st->dev_count) {
>> +        char *dev_path = object_get_canonical_path(OBJECT(dev));
>> +        error_setg(errp, "acpi_memory_plug_cb: "
>> +                   "device [%s] returned invalid memory slot[%d]",
>> +                   dev_path, slot);
>> +        g_free(dev_path);
>> +        return;
>> +    }
> move 3 above blocks into separate function to reduce code duplication
> with acpi_memory_plug_cb(), perhaps something like following:
>
> MemStatus *
> acpi_memory_get_slot_status_descriptor(MemHotplugState *mem_st,
>                                         DeviceState *dev, Error **errp);

Followed.

>> +
> place here also missing removal signaling for selected mdev,
> perhaps it's due to wrong patch ordering

It is not a wrong ordering of patches. I meant to define mdev->is_removing
in patch 6/8. But it is OK to define it here. So followed.

>> +    /* do ACPI magic */
>> +    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
>> +    acpi_update_sci(ar, irq);
> worth to make this block a separate function and reuse in
> acpi_memory_plug_cb()

Followed.

Thanks.

>> +}
>> +
>>   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 b72b34e..c1d5187 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -362,6 +362,9 @@ static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
>>       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 if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> +        acpi_memory_unplug_cb(&s->ar, s->irq, &s->acpi_memory_hotplug, dev,
>> +                              errp);
>>       } else {
>>           error_setg(errp, "acpi: device unplug request for not supported device"
>>                      " type: %s", object_get_typename(OBJECT(dev)));
>> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
>> index 7bbf8a0..fc6b868 100644
>> --- a/include/hw/acpi/memory_hotplug.h
>> +++ b/include/hw/acpi/memory_hotplug.h
>> @@ -28,6 +28,8 @@ 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_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) \
> .
>

[Qemu-devel] [RESEND PATCH v3 2/8] acpi, ich9: Add memory hot unplug support for ich9.

[Tang Chen]

Implement ich9_pm_device_unplug_cb() to support memory hot-remove,
calling acpi_memory_unplug_cb(). And itself will be called in
ich9_device_unplug_cb().

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 hw/acpi/ich9.c         | 12 ++++++++++++
 hw/isa/lpc_ich9.c      |  5 +++--
 include/hw/acpi/ich9.h |  2 ++
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 7b14bbb..d369151 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -310,6 +310,18 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
     }
 }
 
+void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
+{
+    if (pm->acpi_memory_hotplug.is_enabled &&
+        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        acpi_memory_unplug_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_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
 {
     ICH9LPCState *s = ICH9_LPC_DEVICE(adev);
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 177023b..2c0761a 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -610,8 +610,9 @@ static void ich9_device_plug_cb(HotplugHandler *hotplug_dev,
 static void ich9_device_unplug_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)));
+    ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
+
+    ich9_pm_device_unplug_cb(&lpc->pm, dev, errp);
 }
 
 static bool ich9_rst_cnt_needed(void *opaque)
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 7e42448..029576f 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -60,6 +60,8 @@ extern const VMStateDescription vmstate_ich9_pm;
 void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp);
 
 void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp);
+void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
+                              Error **errp);
 
 void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
 #endif /* HW_ACPI_ICH9_H */
-- 
1.8.4.2

Re: [Qemu-devel] [RESEND PATCH v3 2/8] acpi, ich9: Add memory hot unplug support for ich9.

[Igor Mammedov]

On Wed, 27 Aug 2014 16:08:33 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> Implement ich9_pm_device_unplug_cb() to support memory hot-remove,
> calling acpi_memory_unplug_cb(). And itself will be called in
> ich9_device_unplug_cb().
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  hw/acpi/ich9.c         | 12 ++++++++++++
>  hw/isa/lpc_ich9.c      |  5 +++--
>  include/hw/acpi/ich9.h |  2 ++
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 7b14bbb..d369151 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -310,6 +310,18 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
>      }
>  }
>  
> +void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
> +{
> +    if (pm->acpi_memory_hotplug.is_enabled &&
why checking ^^^^^^^^^^^ here but not doing so in piix4 too?

> +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        acpi_memory_unplug_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_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
>  {
>      ICH9LPCState *s = ICH9_LPC_DEVICE(adev);
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 177023b..2c0761a 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -610,8 +610,9 @@ static void ich9_device_plug_cb(HotplugHandler *hotplug_dev,
>  static void ich9_device_unplug_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)));
> +    ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
> +
> +    ich9_pm_device_unplug_cb(&lpc->pm, dev, errp);
>  }
>  
>  static bool ich9_rst_cnt_needed(void *opaque)
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 7e42448..029576f 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -60,6 +60,8 @@ extern const VMStateDescription vmstate_ich9_pm;
>  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp);
>  
>  void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp);
> +void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
> +                              Error **errp);
>  
>  void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
>  #endif /* HW_ACPI_ICH9_H */

Re: [Qemu-devel] [RESEND PATCH v3 2/8] acpi, ich9: Add memory hot unplug support for ich9.

[Tang Chen]

Hi Igor,

On 09/04/2014 08:25 PM, Igor Mammedov wrote:
> On Wed, 27 Aug 2014 16:08:33 +0800
> Tang Chen <tangchen@cn.fujitsu.com> wrote:
>
>> Implement ich9_pm_device_unplug_cb() to support memory hot-remove,
>> calling acpi_memory_unplug_cb(). And itself will be called in
>> ich9_device_unplug_cb().
>>
>> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
>> ---
>>   hw/acpi/ich9.c         | 12 ++++++++++++
>>   hw/isa/lpc_ich9.c      |  5 +++--
>>   include/hw/acpi/ich9.h |  2 ++
>>   3 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index 7b14bbb..d369151 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -310,6 +310,18 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
>>       }
>>   }
>>   
>> +void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
>> +{
>> +    if (pm->acpi_memory_hotplug.is_enabled &&
> why checking ^^^^^^^^^^^ here but not doing so in piix4 too?

Yes, we should also check it in piix4. Fixed.

Thanks.

>
>> +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> +        acpi_memory_unplug_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_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
>>   {
>>       ICH9LPCState *s = ICH9_LPC_DEVICE(adev);
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 177023b..2c0761a 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -610,8 +610,9 @@ static void ich9_device_plug_cb(HotplugHandler *hotplug_dev,
>>   static void ich9_device_unplug_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)));
>> +    ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
>> +
>> +    ich9_pm_device_unplug_cb(&lpc->pm, dev, errp);
>>   }
>>   
>>   static bool ich9_rst_cnt_needed(void *opaque)
>> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
>> index 7e42448..029576f 100644
>> --- a/include/hw/acpi/ich9.h
>> +++ b/include/hw/acpi/ich9.h
>> @@ -60,6 +60,8 @@ extern const VMStateDescription vmstate_ich9_pm;
>>   void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp);
>>   
>>   void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp);
>> +void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
>> +                              Error **errp);
>>   
>>   void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
>>   #endif /* HW_ACPI_ICH9_H */
> .
>

[Qemu-devel] [RESEND PATCH v3 3/8] pc: Add memory hot unplug support for pc machine.

[Tang Chen]

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

Implement device unplug callback for PCMachine. And it now only support
pc-dimm hot-remove. The callback will call piix4 or ich9 callbacks introduced
in previous patches.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 hw/i386/pc.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8fa8d2f..8bafe7c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -61,6 +61,8 @@
 #include "hw/mem/pc-dimm.h"
 #include "trace.h"
 #include "qapi/visitor.h"
+#include "hw/acpi/piix4.h"
+#include "hw/i386/ich9.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -1607,6 +1609,27 @@ out:
     error_propagate(errp, local_err);
 }
 
+static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
+                           DeviceState *dev, Error **errp)
+{
+    Object *acpi_dev;
+    HotplugHandlerClass *hhc;
+    Error *local_err = NULL;
+
+    acpi_dev = (acpi_dev = piix4_pm_find()) ? acpi_dev : ich9_lpc_find();
+    if (!acpi_dev) {
+        error_setg(&local_err,
+                   "memory hotplug is not enabled: missing acpi device");
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    hhc = HOTPLUG_HANDLER_GET_CLASS(acpi_dev);
+    hhc->unplug(HOTPLUG_HANDLER(acpi_dev), dev, &local_err);
+
+    error_propagate(errp, local_err);
+}
+
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
@@ -1615,6 +1638,14 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
     }
 }
 
+static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        pc_dimm_unplug(hotplug_dev, dev, errp);
+    }
+}
+
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
                                              DeviceState *dev)
 {
@@ -1701,6 +1732,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     pcmc->get_hotplug_handler = mc->get_hotplug_handler;
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     hc->plug = pc_machine_device_plug_cb;
+    hc->unplug = pc_machine_device_unplug_cb;
 }
 
 static const TypeInfo pc_machine_info = {
-- 
1.8.4.2

Re: [Qemu-devel] [RESEND PATCH v3 3/8] pc: Add memory hot unplug support for pc machine.

[Igor Mammedov]

On Wed, 27 Aug 2014 16:08:34 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> From: Hu Tao <hutao@cn.fujitsu.com>
> 
> Implement device unplug callback for PCMachine. And it now only support
> pc-dimm hot-remove. The callback will call piix4 or ich9 callbacks introduced
> in previous patches.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  hw/i386/pc.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8fa8d2f..8bafe7c 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -61,6 +61,8 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "trace.h"
>  #include "qapi/visitor.h"
> +#include "hw/acpi/piix4.h"
> +#include "hw/i386/ich9.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -1607,6 +1609,27 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> +static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> +                           DeviceState *dev, Error **errp)
> +{
> +    Object *acpi_dev;
> +    HotplugHandlerClass *hhc;
> +    Error *local_err = NULL;
> +
> +    acpi_dev = (acpi_dev = piix4_pm_find()) ? acpi_dev : ich9_lpc_find();
> +    if (!acpi_dev) {
> +        error_setg(&local_err,
> +                   "memory hotplug is not enabled: missing acpi device");
> +        error_propagate(errp, local_err);
> +        return;
> +    }
drop/replace above block, see: "if (!pcms->acpi_dev) {" for reference

> +
> +    hhc = HOTPLUG_HANDLER_GET_CLASS(acpi_dev);
> +    hhc->unplug(HOTPLUG_HANDLER(acpi_dev), dev, &local_err);

you should take care about dev's vmstate here, that was registered in pc_dimm_plug()
> +
> +    error_propagate(errp, local_err);
> +}
> +
>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> @@ -1615,6 +1638,14 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>      }
>  }
>  
> +static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
> +                                        DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        pc_dimm_unplug(hotplug_dev, dev, errp);
> +    }
> +}
> +
>  static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
>                                               DeviceState *dev)
>  {
> @@ -1701,6 +1732,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      pcmc->get_hotplug_handler = mc->get_hotplug_handler;
>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>      hc->plug = pc_machine_device_plug_cb;
> +    hc->unplug = pc_machine_device_unplug_cb;
>  }
>  
>  static const TypeInfo pc_machine_info = {

Re: [Qemu-devel] [RESEND PATCH v3 3/8] pc: Add memory hot unplug support for pc machine.

[Igor Mammedov]

On Thu, 4 Sep 2014 14:44:41 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed, 27 Aug 2014 16:08:34 +0800
> Tang Chen <tangchen@cn.fujitsu.com> wrote:
> 
> > From: Hu Tao <hutao@cn.fujitsu.com>
> > 
> > Implement device unplug callback for PCMachine. And it now only support
> > pc-dimm hot-remove. The callback will call piix4 or ich9 callbacks introduced
> > in previous patches.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> > ---
> >  hw/i386/pc.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 8fa8d2f..8bafe7c 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -61,6 +61,8 @@
> >  #include "hw/mem/pc-dimm.h"
> >  #include "trace.h"
> >  #include "qapi/visitor.h"
> > +#include "hw/acpi/piix4.h"
> > +#include "hw/i386/ich9.h"
> >  
> >  /* debug PC/ISA interrupts */
> >  //#define DEBUG_IRQ
> > @@ -1607,6 +1609,27 @@ out:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > +static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> > +                           DeviceState *dev, Error **errp)
> > +{
> > +    Object *acpi_dev;
> > +    HotplugHandlerClass *hhc;
> > +    Error *local_err = NULL;
> > +
> > +    acpi_dev = (acpi_dev = piix4_pm_find()) ? acpi_dev : ich9_lpc_find();
> > +    if (!acpi_dev) {
> > +        error_setg(&local_err,
> > +                   "memory hotplug is not enabled: missing acpi device");
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> drop/replace above block, see: "if (!pcms->acpi_dev) {" for reference
> 
> > +
> > +    hhc = HOTPLUG_HANDLER_GET_CLASS(acpi_dev);
> > +    hhc->unplug(HOTPLUG_HANDLER(acpi_dev), dev, &local_err);
> 
> you should take care about dev's vmstate here, that was registered in pc_dimm_plug()
I'm wrong here, vmstate_unregister should take place at pc_dimm unrealize time,

hhc->unplug handler is more like unplug_request handler,
perhaps we need to give some thought to renaming it to unplug_request to avoid confusion
and have an optional unplug callback that could do tear-down with HotplugHandler if needed
before device is unrealized.

[Qemu-devel] [RESEND PATCH v3 4/8] qdev: Add memory hot unplug support for bus-less devices.

[Tang Chen]

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

Implement bus-less device hot-remove in qdev_unplug(). It will call PCMachine
callback introduced in previous patch.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 hw/core/qdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index da1ba48..e365a74 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -228,6 +228,14 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
     if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
         hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
+    } else if (*errp == NULL) {
+        HotplugHandler *hotplug_ctrl;
+        MachineState *machine = MACHINE(qdev_get_machine());
+        MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+        hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
+        if (hotplug_ctrl)
+            hotplug_handler_unplug(hotplug_ctrl, dev, errp);
     } else {
         assert(dc->unplug != NULL);
         if (dc->unplug(dev) < 0) { /* legacy handler */
-- 
1.8.4.2

Re: [Qemu-devel] [RESEND PATCH v3 4/8] qdev: Add memory hot unplug support for bus-less devices.

[Igor Mammedov]

On Wed, 27 Aug 2014 16:08:35 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> From: Hu Tao <hutao@cn.fujitsu.com>
> 
> Implement bus-less device hot-remove in qdev_unplug(). It will call PCMachine
> callback introduced in previous patch.
> 
subject/commit message doesn't need to mention memory hotplug/PCMachine,
it's generic handling that applies to other devices even though
pc-dimm is the only bus-less device handled here.


> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  hw/core/qdev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index da1ba48..e365a74 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -228,6 +228,14 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>  
>      if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>          hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
> +    } else if (*errp == NULL) {
what's the reason for ^^^ condition here?

> +        HotplugHandler *hotplug_ctrl;
> +        MachineState *machine = MACHINE(qdev_get_machine());
> +        MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +        hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> +        if (hotplug_ctrl)
> +            hotplug_handler_unplug(hotplug_ctrl, dev, errp);
>      } else {
>          assert(dc->unplug != NULL);
>          if (dc->unplug(dev) < 0) { /* legacy handler */

Re: [Qemu-devel] [RESEND PATCH v3 4/8] qdev: Add memory hot unplug support for bus-less devices.

[Tang Chen]

Hi Igor,

On 09/04/2014 09:22 PM, Igor Mammedov wrote:
> On Wed, 27 Aug 2014 16:08:35 +0800
> Tang Chen <tangchen@cn.fujitsu.com> wrote:
>
>> From: Hu Tao <hutao@cn.fujitsu.com>
>>
>> Implement bus-less device hot-remove in qdev_unplug(). It will call PCMachine
>> callback introduced in previous patch.
>>
> subject/commit message doesn't need to mention memory hotplug/PCMachine,
> it's generic handling that applies to other devices even though
> pc-dimm is the only bus-less device handled here.

Followed.

>
>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
>> ---
>>   hw/core/qdev.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index da1ba48..e365a74 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -228,6 +228,14 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>>   
>>       if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>>           hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
>> +    } else if (*errp == NULL) {
> what's the reason for ^^^ condition here?
It should be "else if (object_dynamic_cast(qdev_get_machine(), 
TYPE_MACHINE)) "

Just like we do in device_set_realized().

Thanks.
>> +        HotplugHandler *hotplug_ctrl;
>> +        MachineState *machine = MACHINE(qdev_get_machine());
>> +        MachineClass *mc = MACHINE_GET_CLASS(machine);
>> +
>> +        hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
>> +        if (hotplug_ctrl)
>> +            hotplug_handler_unplug(hotplug_ctrl, dev, errp);
>>       } else {
>>           assert(dc->unplug != NULL);
>>           if (dc->unplug(dev) < 0) { /* legacy handler */
>
> .
>

[Qemu-devel] [RESEND PATCH v3 5/8] pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support.

[Tang Chen]

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

Implement unrealize function for pc-dimm device. It delete subregion from
hotplug region, and delete ram address range from guest ram list.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 hw/mem/pc-dimm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 20fe0dc..34109a2 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -270,12 +270,22 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
     return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
 }
 
+static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
+{
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+    MemoryRegion *mr = pc_dimm_get_memory_region(dimm);
+
+    memory_region_del_subregion(mr->container, mr);
+    vmstate_unregister_ram(mr, dev);
+}
+
 static void pc_dimm_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
     PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
 
     dc->realize = pc_dimm_realize;
+    dc->unrealize = pc_dimm_unrealize;
     dc->props = pc_dimm_properties;
 
     ddc->get_memory_region = pc_dimm_get_memory_region;
-- 
1.8.4.2

Re: [Qemu-devel] [RESEND PATCH v3 5/8] pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support.

[Igor Mammedov]

On Wed, 27 Aug 2014 16:08:36 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> From: Hu Tao <hutao@cn.fujitsu.com>
> 
> Implement unrealize function for pc-dimm device. It delete subregion from
s/delete/removes/

> hotplug region, and delete ram address range from guest ram list.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  hw/mem/pc-dimm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 20fe0dc..34109a2 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -270,12 +270,22 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
>      return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
>  }
>  
> +static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
> +{
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    MemoryRegion *mr = pc_dimm_get_memory_region(dimm);
> +
> +    memory_region_del_subregion(mr->container, mr);
usually MemoryRegion is treated as opaque and it's fields
accessed via memory_region_foo() helpers.

> +    vmstate_unregister_ram(mr, dev);
these 2 lines look like a job for PCMachine which did original mapping/vmstate registration

> +}
> +
>  static void pc_dimm_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
>  
>      dc->realize = pc_dimm_realize;
> +    dc->unrealize = pc_dimm_unrealize;
>      dc->props = pc_dimm_properties;
>  
>      ddc->get_memory_region = pc_dimm_get_memory_region;

Re: [Qemu-devel] [RESEND PATCH v3 5/8] pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support.

[zhanghailiang]

On 2014/9/4 21:28, Igor Mammedov wrote:
> On Wed, 27 Aug 2014 16:08:36 +0800
> Tang Chen<tangchen@cn.fujitsu.com>  wrote:
>
>> From: Hu Tao<hutao@cn.fujitsu.com>
>>
>> Implement unrealize function for pc-dimm device. It delete subregion from
> s/delete/removes/
>
>> hotplug region, and delete ram address range from guest ram list.
>>
>> Signed-off-by: Hu Tao<hutao@cn.fujitsu.com>
>> Signed-off-by: Tang Chen<tangchen@cn.fujitsu.com>
>> ---
>>   hw/mem/pc-dimm.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index 20fe0dc..34109a2 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -270,12 +270,22 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
>>       return host_memory_backend_get_memory(dimm->hostmem,&error_abort);
>>   }
>>
>> +static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
>> +{
>> +    PCDIMMDevice *dimm = PC_DIMM(dev);
>> +    MemoryRegion *mr = pc_dimm_get_memory_region(dimm);
>> +
>> +    memory_region_del_subregion(mr->container, mr);
> usually MemoryRegion is treated as opaque and it's fields
> accessed via memory_region_foo() helpers.
>
>> +    vmstate_unregister_ram(mr, dev);
> these 2 lines look like a job for PCMachine which did original mapping/vmstate registration
>

Actually, this patch also fix the bug *when hotplug memory failing in
the place where after pc_dimm_plug but before the end of device_set_realized,
it does not clear the work done by pc_dimm_plug*.

For there is no callback like pc_dimm_plug_fail_cb for us to call when fail,
Maybe pc_dimm_unrealize is the only place where we can do the clear work...

Thanks,
zhanghailiang

>> +}
>> +
>>   static void pc_dimm_class_init(ObjectClass *oc, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(oc);
>>       PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
>>
>>       dc->realize = pc_dimm_realize;
>> +    dc->unrealize = pc_dimm_unrealize;
>>       dc->props = pc_dimm_properties;
>>
>>       ddc->get_memory_region = pc_dimm_get_memory_region;
>
>
>
>

Re: [Qemu-devel] [RESEND PATCH v3 5/8] pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support.

[Igor Mammedov]

On Fri, 12 Sep 2014 13:30:36 +0800
zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:

> On 2014/9/4 21:28, Igor Mammedov wrote:
> > On Wed, 27 Aug 2014 16:08:36 +0800
> > Tang Chen<tangchen@cn.fujitsu.com>  wrote:
> >
> >> From: Hu Tao<hutao@cn.fujitsu.com>
> >>
> >> Implement unrealize function for pc-dimm device. It delete subregion from
> > s/delete/removes/
> >
> >> hotplug region, and delete ram address range from guest ram list.
> >>
> >> Signed-off-by: Hu Tao<hutao@cn.fujitsu.com>
> >> Signed-off-by: Tang Chen<tangchen@cn.fujitsu.com>
> >> ---
> >>   hw/mem/pc-dimm.c | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> >> index 20fe0dc..34109a2 100644
> >> --- a/hw/mem/pc-dimm.c
> >> +++ b/hw/mem/pc-dimm.c
> >> @@ -270,12 +270,22 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
> >>       return host_memory_backend_get_memory(dimm->hostmem,&error_abort);
> >>   }
> >>
> >> +static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
> >> +{
> >> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> >> +    MemoryRegion *mr = pc_dimm_get_memory_region(dimm);
> >> +
> >> +    memory_region_del_subregion(mr->container, mr);
> > usually MemoryRegion is treated as opaque and it's fields
> > accessed via memory_region_foo() helpers.
> >
> >> +    vmstate_unregister_ram(mr, dev);
> > these 2 lines look like a job for PCMachine which did original mapping/vmstate registration
> >
> 
> Actually, this patch also fix the bug *when hotplug memory failing in
> the place where after pc_dimm_plug but before the end of device_set_realized,
> it does not clear the work done by pc_dimm_plug*.
> 
> For there is no callback like pc_dimm_plug_fail_cb for us to call when fail,
> Maybe pc_dimm_unrealize is the only place where we can do the clear work...
Looking at device_set_realized() and pc-dimm case in patrticular
there is no point where it could fail after hotplug_handler_plug() is called.

But even if there where, one should use pc_dimm_unplug() first to
reverse actions performed by successful pc_dimm_plug().

The problem here is that currently unplug callback is actually
doing only unplug request part asking guest to eject memory,
but we also have destroy device when guest tells via ACPI to
ejct memory. You are doing it implicitly by unparenting pc-dimm
from ACPI code and pulling in pc-dimm.unrealize() unrelated
stuff that should be done by PCMachine.

I'm suggesting that we extend hotplug-handler API to handle
this async/split unplug workflow. By converting current
hotplug_handler_unplug() and related code to
hotplug_handler_unplug_request() that would do the first part
of unplug sequence (see/review http://patchwork.ozlabs.org/patch/387018/)

And then on top of it add hotplug_handler_unplug() that would
handle actual device removal when ACPI asks for it.

I'm working now on doing above for PCIDevices since they have
the same workflow (expect to submit patches next week) and
it looks like we would need to use the same approach for CPU
unplug as well.


> 
> Thanks,
> zhanghailiang
> 
> >> +}
> >> +
> >>   static void pc_dimm_class_init(ObjectClass *oc, void *data)
> >>   {
> >>       DeviceClass *dc = DEVICE_CLASS(oc);
> >>       PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
> >>
> >>       dc->realize = pc_dimm_realize;
> >> +    dc->unrealize = pc_dimm_unrealize;
> >>       dc->props = pc_dimm_properties;
> >>
> >>       ddc->get_memory_region = pc_dimm_get_memory_region;
> >
> >
> >
> >
> 
> 

Re: [Qemu-devel] [RESEND PATCH v3 5/8] pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support.

[Tang Chen]

Hi Igor, Zhang,

On 09/12/2014 09:17 PM, Igor Mammedov wrote:
> ......
> Actually, this patch also fix the bug *when hotplug memory failing in
> the place where after pc_dimm_plug but before the end of device_set_realized,
> it does not clear the work done by pc_dimm_plug*.
>
> For there is no callback like pc_dimm_plug_fail_cb for us to call when fail,
> Maybe pc_dimm_unrealize is the only place where we can do the clear work...
> Looking at device_set_realized() and pc-dimm case in patrticular
> there is no point where it could fail after hotplug_handler_plug() is called.
>
> But even if there where, one should use pc_dimm_unplug() first to
> reverse actions performed by successful pc_dimm_plug().
>
> The problem here is that currently unplug callback is actually
> doing only unplug request part asking guest to eject memory,
> but we also have destroy device when guest tells via ACPI to
> ejct memory. You are doing it implicitly by unparenting pc-dimm
> from ACPI code and pulling in pc-dimm.unrealize() unrelated
> stuff that should be done by PCMachine.
>
> I'm suggesting that we extend hotplug-handler API to handle
> this async/split unplug workflow. By converting current
> hotplug_handler_unplug() and related code to
> hotplug_handler_unplug_request() that would do the first part
> of unplug sequence (see/review http://patchwork.ozlabs.org/patch/387018/)

I've read the above patch.

1. I think you proposal would help to resolve the following problem:

     If guest OS failed to handle hoplug sci, QEmu does not know it, and 
could
     release resources incorrectly.

     Would you please have a look at the following patch.
https://www.mail-archive.com/qemu-devel%40nongnu.org/msg253025.html

     I added a pthread wait condition to synchronize :
     sci request -> guest OS handling -> _OST -> QEmu handling
     (Of course, according to the previous discussing, _OST is optional.)

     And IIUC, your proposal may be :
     hotplug request -> guest OS handling -> real hotplug handling

     right ?

     I think it is the same. How do you think synchronize it with a wait 
condition ?
     Or you have any better idea ?
     Since no one has replied the patch, I'm not sure if it is OK.


2. Let's finish memory and cpu hotplug job based on the current 
framework, shall we ?
     In the above patch, it renames a lot of functions that are being 
used in memory and
     cpu hotplug patches. I think we can push memory and cpu hotplug 
jobs, and in the
     next phase, let's improve the framework.

     And of course, the problem I mentioned above should also be put in 
the next phase.

     So I want to submit the next version memory hotplug patches based 
on the original
     framework, and help to improve it in the next phase.

How do you think ?

>
> And then on top of it add hotplug_handler_unplug() that would
> handle actual device removal when ACPI asks for it.
>
> I'm working now on doing above for PCIDevices since they have
> the same workflow (expect to submit patches next week) and
> it looks like we would need to use the same approach for CPU
> unplug as well.
>
>
>

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

[Tang Chen]

This patch adds a bool member named "is_removing" to MemStatus indicating if
the memory device is being removed. It is set to true in acpi_memory_unplug_cb()
when doing memory hot-remove with device_del command, or write an
ACPI_EJECTION_IN_PROGRESS status to ACPI register when triggering memory
hot-remove in guest.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 hw/acpi/memory_hotplug.c         | 62 ++++++++++++++++++++++++++++++++++++++--
 include/hw/acpi/acpi.h           | 14 +++++++++
 include/hw/acpi/memory_hotplug.h |  1 +
 3 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index c310b44..9074e7c 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:
@@ -84,6 +85,51 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
     return val;
 }
 
+static void acpi_handle_eject(MemStatus *mdev)
+{
+    switch (mdev->ost_status) {
+    case ACPI_SUCCESS:
+        object_unparent(OBJECT(mdev->dimm));
+        mdev->is_removing = false;
+        mdev->dimm = NULL;
+        break;
+    case ACPI_EJECT_IN_PROGRESS:
+        /* For ejection triggered by hardware (device_del command),
+         * mdev->is_removing is set to true by acpi_memory_unplug_cb()
+         * before sending SCI. So we only handle ejection triggered by
+         * guest OS.
+	 */
+        if (mdev->ost_event == ACPI_OSPM_EJECT) {
+            mdev->is_enabled = false;
+            mdev->is_removing = true;
+        }
+        break;
+    case ACPI_FAILURE:
+    case ACPI_UNRECOGNIZED_NOTIFY:
+    case ACPI_EJECT_NOT_SUPPORTED:
+    case ACPI_EJECT_DEVICE_IN_USE:
+    case ACPI_EJECT_DEVICE_BUSY:
+    case ACPI_EJECT_DEPENDENCY_BUSY:
+        mdev->is_removing = false;
+        mdev->is_enabled = true;
+        break;
+    default:
+        break;
+    }
+}
+
+static void acpi_handle_ost_event(MemStatus *mdev)
+{
+    switch (mdev->ost_event) {
+    case ACPI_NOTIFY_EJECT_REQUEST:    /* Ejection triggered by hardware. */
+    case ACPI_OSPM_EJECT:              /* Ejection triggered by guest OS. */
+        acpi_handle_eject(mdev);
+        break;
+    default:
+        break;
+    }
+}
+
 static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
                                       unsigned int size)
 {
@@ -121,21 +167,27 @@ 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);
+
+        acpi_handle_ost_event(mdev);
         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_enabled = false;
         }
+
+        break;
+    default:
         break;
     }
-
 }
 static const MemoryRegionOps acpi_memory_hotplug_ops = {
     .read = acpi_memory_hotplug_read,
@@ -198,6 +250,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
 void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
                            DeviceState *dev, Error **errp)
 {
+    MemStatus *mdev;
     Error *local_err = NULL;
     int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
 
@@ -215,6 +268,9 @@ void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
         return;
     }
 
+    mdev = &mem_st->devs[slot];
+    mdev->is_removing = true;
+
     /* do ACPI magic */
     ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
     acpi_update_sci(ar, irq);
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 1f678b4..e105e45 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -91,6 +91,20 @@
 /* PM2_CNT */
 #define ACPI_BITMASK_ARB_DISABLE                0x0001
 
+/* OST_EVENT */
+#define ACPI_NOTIFY_EJECT_REQUEST               0x03
+#define ACPI_OSPM_EJECT                         0x103
+
+/* OST_STATUS */
+#define ACPI_SUCCESS                            0x0
+#define ACPI_FAILURE                            0x1
+#define ACPI_UNRECOGNIZED_NOTIFY                0x2
+#define ACPI_EJECT_NOT_SUPPORTED                0x80
+#define ACPI_EJECT_DEVICE_IN_USE                0x81
+#define ACPI_EJECT_DEVICE_BUSY                  0x82
+#define ACPI_EJECT_DEPENDENCY_BUSY              0x83
+#define ACPI_EJECT_IN_PROGRESS                  0x84
+
 /* structs */
 typedef struct ACPIPMTimer ACPIPMTimer;
 typedef struct ACPIPM1EVT ACPIPM1EVT;
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index fc6b868..fe41268 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;
-- 
1.8.4.2

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

[Igor Mammedov]

On Wed, 27 Aug 2014 16:08:37 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> This patch adds a bool member named "is_removing" to MemStatus indicating if
> the memory device is being removed. It is set to true in acpi_memory_unplug_cb()
> when doing memory hot-remove with device_del command, or write an
> ACPI_EJECTION_IN_PROGRESS status to ACPI register when triggering memory
> hot-remove in guest.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  hw/acpi/memory_hotplug.c         | 62 ++++++++++++++++++++++++++++++++++++++--
>  include/hw/acpi/acpi.h           | 14 +++++++++
>  include/hw/acpi/memory_hotplug.h |  1 +
>  3 files changed, 74 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index c310b44..9074e7c 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;
update docs/specs/acpi_mem_hotplug.txt with new bit field

>          trace_mhp_acpi_read_flags(mem_st->selector, val);
>          break;
>      default:
> @@ -84,6 +85,51 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
>      return val;
>  }
>  
> +static void acpi_handle_eject(MemStatus *mdev)
> +{
> +    switch (mdev->ost_status) {
> +    case ACPI_SUCCESS:
> +        object_unparent(OBJECT(mdev->dimm));
> +        mdev->is_removing = false;
> +        mdev->dimm = NULL;
> +        break;
> +    case ACPI_EJECT_IN_PROGRESS:
> +        /* For ejection triggered by hardware (device_del command),
> +         * mdev->is_removing is set to true by acpi_memory_unplug_cb()
> +         * before sending SCI. So we only handle ejection triggered by
> +         * guest OS.
> +	 */
> +        if (mdev->ost_event == ACPI_OSPM_EJECT) {
> +            mdev->is_enabled = false;
> +            mdev->is_removing = true;
> +        }
> +        break;
> +    case ACPI_FAILURE:
> +    case ACPI_UNRECOGNIZED_NOTIFY:
> +    case ACPI_EJECT_NOT_SUPPORTED:
> +    case ACPI_EJECT_DEVICE_IN_USE:
> +    case ACPI_EJECT_DEVICE_BUSY:
> +    case ACPI_EJECT_DEPENDENCY_BUSY:
> +        mdev->is_removing = false;
> +        mdev->is_enabled = true;
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void acpi_handle_ost_event(MemStatus *mdev)
> +{
> +    switch (mdev->ost_event) {
> +    case ACPI_NOTIFY_EJECT_REQUEST:    /* Ejection triggered by hardware. */
> +    case ACPI_OSPM_EJECT:              /* Ejection triggered by guest OS. */
> +        acpi_handle_eject(mdev);
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
>  static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>                                        unsigned int size)
>  {
> @@ -121,21 +167,27 @@ 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);
> +
> +        acpi_handle_ost_event(mdev);
_OST is optional and OSPM doesn't have to call it at all,
it was already discussed on list and using _OST for device removal
was evaluated as not usable.
We use _OST here as supplementary status information for management tools
and no more /i.e. as LEDs on real hardware/.

See "Figure 6-37 Device Ejection Flow Example Using _OST." in ACPI 5.1 spec
and note below it.


>          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_enabled = false;
device tear-down should be initiated here, when OSPM calls _EJ0 method
and when we return from this function it should be destroyed.

>          }
> +
> +        break;
> +    default:
>          break;
>      }
> -
>  }
>  static const MemoryRegionOps acpi_memory_hotplug_ops = {
>      .read = acpi_memory_hotplug_read,
> @@ -198,6 +250,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>  void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>                             DeviceState *dev, Error **errp)
>  {
> +    MemStatus *mdev;
>      Error *local_err = NULL;
>      int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
>  
> @@ -215,6 +268,9 @@ void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>          return;
>      }
>  
> +    mdev = &mem_st->devs[slot];
> +    mdev->is_removing = true;
> +
>      /* do ACPI magic */
>      ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
>      acpi_update_sci(ar, irq);
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index 1f678b4..e105e45 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -91,6 +91,20 @@
>  /* PM2_CNT */
>  #define ACPI_BITMASK_ARB_DISABLE                0x0001
>  
> +/* OST_EVENT */
> +#define ACPI_NOTIFY_EJECT_REQUEST               0x03
> +#define ACPI_OSPM_EJECT                         0x103
> +
> +/* OST_STATUS */
> +#define ACPI_SUCCESS                            0x0
> +#define ACPI_FAILURE                            0x1
> +#define ACPI_UNRECOGNIZED_NOTIFY                0x2
> +#define ACPI_EJECT_NOT_SUPPORTED                0x80
> +#define ACPI_EJECT_DEVICE_IN_USE                0x81
> +#define ACPI_EJECT_DEVICE_BUSY                  0x82
> +#define ACPI_EJECT_DEPENDENCY_BUSY              0x83
> +#define ACPI_EJECT_IN_PROGRESS                  0x84
> +
>  /* structs */
>  typedef struct ACPIPMTimer ACPIPMTimer;
>  typedef struct ACPIPM1EVT ACPIPM1EVT;
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index fc6b868..fe41268 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;

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

[Tang Chen]

Hi Igor,

On 09/04/2014 10:20 PM, Igor Mammedov wrote:

> ......
> +
> +        acpi_handle_ost_event(mdev);
> _OST is optional and OSPM doesn't have to call it at all,
> it was already discussed on list and using _OST for device removal
> was evaluated as not usable.
> We use _OST here as supplementary status information for management tools
> and no more /i.e. as LEDs on real hardware/.
>
> See "Figure 6-37 Device Ejection Flow Example Using _OST." in ACPI 5.1 spec
> and note below it.

OK, I agree we should not make memory hotplug process depend on _OST.

But we do need to handle one problem:   When guest OS failed to remove 
device,
we should not clear QEmu data of that device.

As you and ACPI spec said, Platform (which is QEmu here) should reply _OST
rised by guest OS except things such as LEDs on real hardware. But _OST is
the only way I can find to get status from guest OS.

So, let's do it in the following way:
1. Make device hotplug process independent from _OST for the 
compatibility reason.

2. For OSPMs that support _OST, QEmu use _OST to catch error from guest OS.
     May be report an error message only, and stop QEmu from remove 
related data.
     (Please see/review patch-set:
https://www.mail-archive.com/qemu-devel%40nongnu.org/msg253025.html)

3. For OSPMs that do not support _OST, do not handle guest OS error for now.

How do you think ?

Thanks.

>
>
>>           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_enabled = false;
> device tear-down should be initiated here, when OSPM calls _EJ0 method
> and when we return from this function it should be destroyed.
>
>>           }
>> +
>> +        break;
>> +    default:
>>           break;
>>       }
>> -
>>   }
>>   static const MemoryRegionOps acpi_memory_hotplug_ops = {
>>       .read = acpi_memory_hotplug_read,
>> @@ -198,6 +250,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>>   void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>>                              DeviceState *dev, Error **errp)
>>   {
>> +    MemStatus *mdev;
>>       Error *local_err = NULL;
>>       int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
>>   
>> @@ -215,6 +268,9 @@ void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>>           return;
>>       }
>>   
>> +    mdev = &mem_st->devs[slot];
>> +    mdev->is_removing = true;
>> +
>>       /* do ACPI magic */
>>       ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
>>       acpi_update_sci(ar, irq);
>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>> index 1f678b4..e105e45 100644
>> --- a/include/hw/acpi/acpi.h
>> +++ b/include/hw/acpi/acpi.h
>> @@ -91,6 +91,20 @@
>>   /* PM2_CNT */
>>   #define ACPI_BITMASK_ARB_DISABLE                0x0001
>>   
>> +/* OST_EVENT */
>> +#define ACPI_NOTIFY_EJECT_REQUEST               0x03
>> +#define ACPI_OSPM_EJECT                         0x103
>> +
>> +/* OST_STATUS */
>> +#define ACPI_SUCCESS                            0x0
>> +#define ACPI_FAILURE                            0x1
>> +#define ACPI_UNRECOGNIZED_NOTIFY                0x2
>> +#define ACPI_EJECT_NOT_SUPPORTED                0x80
>> +#define ACPI_EJECT_DEVICE_IN_USE                0x81
>> +#define ACPI_EJECT_DEVICE_BUSY                  0x82
>> +#define ACPI_EJECT_DEPENDENCY_BUSY              0x83
>> +#define ACPI_EJECT_IN_PROGRESS                  0x84
>> +
>>   /* structs */
>>   typedef struct ACPIPMTimer ACPIPMTimer;
>>   typedef struct ACPIPM1EVT ACPIPM1EVT;
>> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
>> index fc6b868..fe41268 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;
> .
>

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

[Igor Mammedov]

On Tue, 16 Sep 2014 18:12:02 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> Hi Igor,
> 
> On 09/04/2014 10:20 PM, Igor Mammedov wrote:
> 
> > ......
> > +
> > +        acpi_handle_ost_event(mdev);
> > _OST is optional and OSPM doesn't have to call it at all,
> > it was already discussed on list and using _OST for device removal
> > was evaluated as not usable.
> > We use _OST here as supplementary status information for management tools
> > and no more /i.e. as LEDs on real hardware/.
> >
> > See "Figure 6-37 Device Ejection Flow Example Using _OST." in ACPI 5.1 spec
> > and note below it.
> 
> OK, I agree we should not make memory hotplug process depend on _OST.
> 
> But we do need to handle one problem:   When guest OS failed to remove 
> device,
> we should not clear QEmu data of that device.
> 
> As you and ACPI spec said, Platform (which is QEmu here) should reply _OST
> rised by guest OS except things such as LEDs on real hardware. But _OST is
> the only way I can find to get status from guest OS.
> 
> So, let's do it in the following way:
> 1. Make device hotplug process independent from _OST for the 
> compatibility reason.
> 
> 2. For OSPMs that support _OST, QEmu use _OST to catch error from guest OS.
>      May be report an error message only, and stop QEmu from remove 
> related data.
>      (Please see/review patch-set:
> https://www.mail-archive.com/qemu-devel%40nongnu.org/msg253025.html)
> 
> 3. For OSPMs that do not support _OST, do not handle guest OS error for now.
> 
> How do you think ?
I'd keep _OST handling in ACPI write handler only for notification purposes
only. This way management tools/end user will see guest OS error if supported
, otherwise we do not care.

Make '_OST unplug handler' reset removing bit in case of error, so that guest
won't see DIMM stuck in continuous removing state when following hotplug events
happen.


> 
> Thanks.
> 
> >
> >
> >>           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_enabled = false;
> > device tear-down should be initiated here, when OSPM calls _EJ0 method
> > and when we return from this function it should be destroyed.
> >
> >>           }
> >> +
> >> +        break;
> >> +    default:
> >>           break;
> >>       }
> >> -
> >>   }
> >>   static const MemoryRegionOps acpi_memory_hotplug_ops = {
> >>       .read = acpi_memory_hotplug_read,
> >> @@ -198,6 +250,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> >>   void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> >>                              DeviceState *dev, Error **errp)
> >>   {
> >> +    MemStatus *mdev;
> >>       Error *local_err = NULL;
> >>       int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
> >>   
> >> @@ -215,6 +268,9 @@ void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> >>           return;
> >>       }
> >>   
> >> +    mdev = &mem_st->devs[slot];
> >> +    mdev->is_removing = true;
> >> +
> >>       /* do ACPI magic */
> >>       ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> >>       acpi_update_sci(ar, irq);
> >> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> >> index 1f678b4..e105e45 100644
> >> --- a/include/hw/acpi/acpi.h
> >> +++ b/include/hw/acpi/acpi.h
> >> @@ -91,6 +91,20 @@
> >>   /* PM2_CNT */
> >>   #define ACPI_BITMASK_ARB_DISABLE                0x0001
> >>   
> >> +/* OST_EVENT */
> >> +#define ACPI_NOTIFY_EJECT_REQUEST               0x03
> >> +#define ACPI_OSPM_EJECT                         0x103
> >> +
> >> +/* OST_STATUS */
> >> +#define ACPI_SUCCESS                            0x0
> >> +#define ACPI_FAILURE                            0x1
> >> +#define ACPI_UNRECOGNIZED_NOTIFY                0x2
> >> +#define ACPI_EJECT_NOT_SUPPORTED                0x80
> >> +#define ACPI_EJECT_DEVICE_IN_USE                0x81
> >> +#define ACPI_EJECT_DEVICE_BUSY                  0x82
> >> +#define ACPI_EJECT_DEPENDENCY_BUSY              0x83
> >> +#define ACPI_EJECT_IN_PROGRESS                  0x84
> >> +
> >>   /* structs */
> >>   typedef struct ACPIPMTimer ACPIPMTimer;
> >>   typedef struct ACPIPM1EVT ACPIPM1EVT;
> >> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> >> index fc6b868..fe41268 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;
> > .
> >
> 

[Qemu-devel] [RESEND PATCH v3 7/8] pc, acpi bios: Add memory hot unplug interface.

[Tang Chen]

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

This patch implements MEMORY_SLOT_EJECT_METHOD according to ACPI spec.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 hw/i386/ssdt-mem.dsl         |  5 +++++
 hw/i386/ssdt-misc.dsl        | 15 ++++++++++++++-
 include/hw/acpi/pc-hotplug.h |  2 ++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hw/i386/ssdt-mem.dsl b/hw/i386/ssdt-mem.dsl
index 22ff5dd..1416639 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/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
index 0fd4480..79228ac 100644
--- a/hw/i386/ssdt-misc.dsl
+++ b/hw/i386/ssdt-misc.dsl
@@ -156,6 +156,7 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
                 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, // 1 if DIMM has a remove request, read only
             }
 
             Mutex (MEMORY_SLOT_LOCK, 0)
@@ -174,11 +175,16 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
                 Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
                 while (LLess(Local0, MEMORY_SLOTS_NUMBER)) {
                     Store(Local0, MEMORY_SLOT_SLECTOR) // select Local0 DIMM
+
                     If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
                         MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
                         Store(1, MEMORY_SLOT_INSERT_EVENT)
                     }
-                    // TODO: handle memory eject request
+
+                    If (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
+                        MTFY(Local0, 3)
+                    }
+
                     Add(Local0, One, Local0) // goto next DIMM
                 }
                 Release(MEMORY_SLOT_LOCK)
@@ -278,6 +284,13 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
                 Store(Arg2, MEMORY_SLOT_OST_STATUS)
                 Release(MEMORY_SLOT_LOCK)
             }
+
+            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
+                Acquire(MLCK, 0xFFFF)
+                Store(ToInteger(Arg0), MSEL) // select DIMM
+                Store(One, MEMORY_SLOT_REMOVE_EVENT)
+                Release(MLCK)
+            }
         } // Device()
     } // Scope()
 }
diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
index b9db295..b3770a1 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
@@ -50,6 +51,7 @@
 #define MEMORY_SLOT_CRS_METHOD       MCRS
 #define MEMORY_SLOT_OST_METHOD       MOST
 #define MEMORY_SLOT_PROXIMITY_METHOD MPXM
+#define MEMORY_SLOT_EJECT_METHOD     MDEJ
 #define MEMORY_SLOT_NOTIFY_METHOD    MTFY
 #define MEMORY_SLOT_SCAN_METHOD      MSCN
 
-- 
1.8.4.2

Re: [Qemu-devel] [RESEND PATCH v3 7/8] pc, acpi bios: Add memory hot unplug interface.

[Igor Mammedov]

On Wed, 27 Aug 2014 16:08:38 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> From: Hu Tao <hutao@cn.fujitsu.com>
> 
> This patch implements MEMORY_SLOT_EJECT_METHOD according to ACPI spec.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  hw/i386/ssdt-mem.dsl         |  5 +++++
>  hw/i386/ssdt-misc.dsl        | 15 ++++++++++++++-
>  include/hw/acpi/pc-hotplug.h |  2 ++
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/ssdt-mem.dsl b/hw/i386/ssdt-mem.dsl
> index 22ff5dd..1416639 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/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> index 0fd4480..79228ac 100644
> --- a/hw/i386/ssdt-misc.dsl
> +++ b/hw/i386/ssdt-misc.dsl
> @@ -156,6 +156,7 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
>                  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, // 1 if DIMM has a remove request, read only
>              }
>  
>              Mutex (MEMORY_SLOT_LOCK, 0)
> @@ -174,11 +175,16 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
>                  Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
>                  while (LLess(Local0, MEMORY_SLOTS_NUMBER)) {
>                      Store(Local0, MEMORY_SLOT_SLECTOR) // select Local0 DIMM
> +
>                      If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check
>                          MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
>                          Store(1, MEMORY_SLOT_INSERT_EVENT)
>                      }
> -                    // TODO: handle memory eject request
> +
> +                    If (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request
please use ELSE IF here, otherwise OSPM could try to do insert/remove simultaneously

> +                        MTFY(Local0, 3)
use MEMORY_SLOT_NOTIFY_METHOD here

> +                    }
> +
>                      Add(Local0, One, Local0) // goto next DIMM
>                  }
>                  Release(MEMORY_SLOT_LOCK)
> @@ -278,6 +284,13 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
>                  Store(Arg2, MEMORY_SLOT_OST_STATUS)
>                  Release(MEMORY_SLOT_LOCK)
>              }
> +
> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
> +                Acquire(MLCK, 0xFFFF)
> +                Store(ToInteger(Arg0), MSEL) // select DIMM
> +                Store(One, MEMORY_SLOT_REMOVE_EVENT)
> +                Release(MLCK)

use macros here too, see Method(MEMORY_SLOT_OST_METHOD, 4) for example

> +            }
>          } // Device()
>      } // Scope()
>  }
> diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
> index b9db295..b3770a1 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
> @@ -50,6 +51,7 @@
>  #define MEMORY_SLOT_CRS_METHOD       MCRS
>  #define MEMORY_SLOT_OST_METHOD       MOST
>  #define MEMORY_SLOT_PROXIMITY_METHOD MPXM
> +#define MEMORY_SLOT_EJECT_METHOD     MDEJ
maybe s/MDEJ/MEJ0/ to be clearer what eject method is implied

>  #define MEMORY_SLOT_NOTIFY_METHOD    MTFY
>  #define MEMORY_SLOT_SCAN_METHOD      MSCN
>  

[Qemu-devel] [RESEND PATCH v3 8/8] monitor: Add memory hot unplug support for device_del command.

[Tang Chen]

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

Implement find_peripheral_device() to find bus-less device, and call it in
qmp_device_del() so that device_del command will be able to remove memory
device.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 include/qom/object.h |  1 +
 qdev-monitor.c       | 23 +++++++++++++++++++++++
 qom/object.c         |  2 +-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 8a05a81..a352beb 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1300,5 +1300,6 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
  */
 Object *container_get(Object *root, const char *path);
 
+bool object_property_is_child(ObjectProperty *prop);
 
 #endif
diff --git a/qdev-monitor.c b/qdev-monitor.c
index fb9ee24..effd928 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -24,6 +24,7 @@
 #include "qmp-commands.h"
 #include "sysemu/arch_init.h"
 #include "qemu/config-file.h"
+#include "qom/object.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -683,12 +684,34 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return 0;
 }
 
+static DeviceState *find_peripheral_device(const char *id)
+{
+    Object *peripheral = qdev_get_peripheral();
+    DeviceState *dev = NULL;
+    ObjectProperty *prop;
+
+    QTAILQ_FOREACH(prop, &peripheral->properties, node) {
+        if (object_property_is_child(prop)) {
+            dev = DEVICE(prop->opaque);
+            if (!strcmp(dev->id, id)) {
+                return dev;
+            }
+        }
+    }
+
+    return NULL;
+}
+
 void qmp_device_del(const char *id, Error **errp)
 {
     DeviceState *dev;
 
     dev = qdev_find_recursive(sysbus_get_default(), id);
     if (!dev) {
+        dev = find_peripheral_device(id);
+    }
+
+    if (!dev) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, id);
         return;
     }
diff --git a/qom/object.c b/qom/object.c
index 0e8267b..1183e5d 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -351,7 +351,7 @@ void object_initialize(void *data, size_t size, const char *typename)
     object_initialize_with_type(data, size, type);
 }
 
-static inline bool object_property_is_child(ObjectProperty *prop)
+bool object_property_is_child(ObjectProperty *prop)
 {
     return strstart(prop->type, "child<", NULL);
 }
-- 
1.8.4.2

Re: [Qemu-devel] [RESEND PATCH v3 8/8] monitor: Add memory hot unplug support for device_del command.

[Igor Mammedov]

On Wed, 27 Aug 2014 16:08:39 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> From: Hu Tao <hutao@cn.fujitsu.com>
> 
> Implement find_peripheral_device() to find bus-less device, and call it in
> qmp_device_del() so that device_del command will be able to remove memory
> device.
probably patch should go before 4/8

> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  include/qom/object.h |  1 +
>  qdev-monitor.c       | 23 +++++++++++++++++++++++
>  qom/object.c         |  2 +-
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 8a05a81..a352beb 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1300,5 +1300,6 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
>   */
>  Object *container_get(Object *root, const char *path);
>  
> +bool object_property_is_child(ObjectProperty *prop);
>  
>  #endif
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index fb9ee24..effd928 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -24,6 +24,7 @@
>  #include "qmp-commands.h"
>  #include "sysemu/arch_init.h"
>  #include "qemu/config-file.h"
> +#include "qom/object.h"
>  
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> @@ -683,12 +684,34 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      return 0;
>  }
>  
> +static DeviceState *find_peripheral_device(const char *id)
> +{
> +    Object *peripheral = qdev_get_peripheral();
> +    DeviceState *dev = NULL;
> +    ObjectProperty *prop;
> +
> +    QTAILQ_FOREACH(prop, &peripheral->properties, node) {
> +        if (object_property_is_child(prop)) {
> +            dev = DEVICE(prop->opaque);
> +            if (!strcmp(dev->id, id)) {
> +                return dev;
> +            }
> +        }
> +    }
It's not permitted to access internal fields of Object outside of object.c

checkout usage of object_resolve_path()/object_resolve_path_type(),
that's what you need to use.

> +
> +    return NULL;
> +}
> +
>  void qmp_device_del(const char *id, Error **errp)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_find_recursive(sysbus_get_default(), id);
>      if (!dev) {
> +        dev = find_peripheral_device(id);
> +    }
> +
> +    if (!dev) {
>          error_set(errp, QERR_DEVICE_NOT_FOUND, id);
>          return;
>      }
> diff --git a/qom/object.c b/qom/object.c
> index 0e8267b..1183e5d 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -351,7 +351,7 @@ void object_initialize(void *data, size_t size, const char *typename)
>      object_initialize_with_type(data, size, type);
>  }
>  
> -static inline bool object_property_is_child(ObjectProperty *prop)
> +bool object_property_is_child(ObjectProperty *prop)
>  {
>      return strstart(prop->type, "child<", NULL);
>  }