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

[Michael S. Tsirkin]

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

asynchronous

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

guest

> OS finished handling unplug, unplug cb will be called
> to do the real removal of device.
> 
> This patch adds unplug request cb for memory device.

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


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

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

> 
> Signed-off-by: Tang Chen <address@hidden>
> Signed-off-by: Zhu Guihua <address@hidden>
> ---
>  hw/acpi/memory_hotplug.c         | 17 +++++++++++++++++
>  include/hw/acpi/memory_hotplug.h |  4 ++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 5b13baa..02231d2 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -206,6 +206,23 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, 
> MemHotplugState *mem_st,
>      acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
>  }
>  
> +void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
> +                                   MemHotplugState *mem_st,
> +                                   DeviceState *dev, Error **errp)
> +{
> +    MemStatus *mdev;
> +
> +    mdev = acpi_memory_slot_status(mem_st, dev, errp);
> +    if (!mdev) {
> +        return;
> +    }
> +
> +    mdev->is_removing = true;
> +
> +    /* Do ACPI magic */
> +    acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
> +}
> +
>  static const VMStateDescription vmstate_memhp_sts = {
>      .name = "memory hotplug device state",
>      .version_id = 1,
> diff --git a/include/hw/acpi/memory_hotplug.h 
> b/include/hw/acpi/memory_hotplug.h
> index 7bbf8a0..c437a85 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -11,6 +11,7 @@ typedef struct MemStatus {
>      DeviceState *dimm;
>      bool is_enabled;
>      bool is_inserting;
> +    bool is_removing;
>      uint32_t ost_event;
>      uint32_t ost_status;
>  } MemStatus;
> @@ -28,6 +29,9 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object 
> *owner,
>  
>  void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
>                           DeviceState *dev, Error **errp);
> +void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
> +                                   MemHotplugState *mem_st,
> +                                   DeviceState *dev, Error **errp);
>  
>  extern const VMStateDescription vmstate_memory_hotplug;
>  #define VMSTATE_MEMORY_HOTPLUG(memhp, state) \
> -- 
> 1.9.3