diff mbox

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

Message ID 1409126919-22233-2-git-send-email-tangchen@cn.fujitsu.com
State New
Headers show

Commit Message

Tang Chen Aug. 27, 2014, 8:08 a.m. UTC 
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(+)

Comments

Igor Mammedov Sept. 4, 2014, 12:15 p.m. UTC | #1 
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) \
Tang Chen Sept. 16, 2014, 3:17 a.m. UTC | #2 
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) \
> .
>
diff mbox

Patch

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) \