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 <address@hidden> 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 <address@hidden>
> Signed-off-by: Tang Chen <address@hidden>
> ---
>  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;