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

[Igor Mammedov]

On Wed, 4 Feb 2015 10:51:26 +0800
Zhu Guihua <address@hidden> wrote:

> From: Tang Chen <address@hidden>
> 
> This patch adds a new bit to memory hotplug IO port indicating that
> ej0 has been evaluated by guest OS. And call pc-dimm unplug cb to do
> the real removal.
> 
> Signed-off-by: Hu Tao <address@hidden>
> Signed-off-by: Tang Chen <address@hidden>
> Signed-off-by: Zhu Guihua <address@hidden>
> ---
>  docs/specs/acpi_mem_hotplug.txt   |  9 +++++++--
>  hw/acpi/memory_hotplug.c          | 25 ++++++++++++++++++++++---
>  hw/i386/acpi-dsdt-mem-hotplug.dsl | 11 ++++++++++-
>  hw/i386/ssdt-mem.dsl              |  5 +++++
>  include/hw/acpi/pc-hotplug.h      |  2 ++
>  5 files changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> index 1290994..9805f1a 100644
> --- a/docs/specs/acpi_mem_hotplug.txt
> +++ b/docs/specs/acpi_mem_hotplug.txt
> @@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte 
> access):
>                1: Device insert event, used to distinguish device for which
>                   no device check event to OSPM was issued.
>                   It's valid only when bit 1 is set.
> -              2-7: reserved and should be ignored by OSPM
> +              2: Device remove event, used to distinguish device for which
> +                 no device check event to OSPM was issued.
> +              3-7: reserved and should be ignored by OSPM
>        [0x15-0x17] reserved
>  
>    write access:
> @@ -35,7 +37,10 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte 
> access):
>                1: if set to 1 clears device insert event, set by OSPM
>                   after it has emitted device check event for the
>                   selected memory device
> -              2-7: reserved, OSPM must clear them before writing to register
> +              2: if set to 1 clears device remove event, set by OSPM
> +                 after it has emitted device check event for the
> +                 selected memory device
it doesn't match what this patch does, it clears removal event when
OSPM evaluates EJ0 handler.

What if OSPM fails to unplug device and therefore won't evaluate EJ0
method at all. So when next memory hotplug SCI arrives it will 
send removal notify to OSPM again.
I doesn't seem right, pls fix patch to behave as documented above,
i.e. clear event after device check event.
If guest fails to eject device it should send OST event about it
and forget about device removal. It's up to management tools to repeat
device_del if it's failed and tools still wish to do so.


> +              3-7: reserved, OSPM must clear them before writing to register
>  
>  Selecting memory device slot beyond present range has no effect on platform:
>     - write accesses to memory hot-plug registers not documented above are
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 3ae9629..a6fc3b3 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -3,6 +3,7 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/i386/pc.h"
>  #include "hw/boards.h"
> +#include "hw/qdev-core.h"
>  #include "trace.h"
>  #include "qapi-event.h"
>  
> @@ -76,6 +77,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:
> @@ -91,6 +93,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
> addr, uint64_t data,
>      MemHotplugState *mem_st = opaque;
>      MemStatus *mdev;
>      ACPIOSTInfo *info;
> +    DeviceState *dev = NULL;
> +    HotplugHandler *hotplug_ctrl = NULL;
>  
>      if (!mem_st->dev_count) {
>          return;
> @@ -122,21 +126,36 @@ static void acpi_memory_hotplug_write(void *opaque, 
> hwaddr addr, uint64_t data,
>          mdev = &mem_st->devs[mem_st->selector];
>          mdev->ost_status = data;
>          trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
> -        /* TODO: implement memory removal on guest signal */
>  
>          info = acpi_memory_device_status(mem_st->selector, mdev);
>          qapi_event_send_acpi_device_ost(info, &error_abort);
>          qapi_free_ACPIOSTInfo(info);
>          break;
> -    case 0x14:
> +    case 0x14: /* set is_* fields */
>          mdev = &mem_st->devs[mem_st->selector];
> +
>          if (data & 2) { /* clear insert event */
>              mdev->is_inserting  = false;
>              trace_mhp_acpi_clear_insert_evt(mem_st->selector);
> +        } else if (data & 4) { /* request removal of device */
> +            mdev->is_removing = false;
> +            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
> +            /*
> +             * QEmu memory hot unplug is an asynchronized procedure. QEmu 
> first
> +             * calls pc-dimm unplug request cb to send a SCI to guest. When 
> the
> +             * Guest OS finished handling the SCI, it evaluates ACPI ej0, and
> +             * QEmu calls pc-dimm unplug cb to remove memory device.
> +             */
> +            dev = DEVICE(mdev->dimm);
> +            hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +            /* Call pc-dimm unplug cb. */
> +            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
>          }
> +
> +        break;
> +    default:
>          break;
>      }
> -
>  }
>  static const MemoryRegionOps acpi_memory_hotplug_ops = {
>      .read = acpi_memory_hotplug_read,
> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl 
> b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> index 2a36c47..b53bf77 100644
> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> @@ -50,6 +50,7 @@
>                  Offset(20),
>                  MEMORY_SLOT_ENABLED,  1, // 1 if enabled, read only
>                  MEMORY_SLOT_INSERT_EVENT, 1, // (read) 1 if has a insert 
> event. (write) 1 to clear event
> +                MEMORY_SLOT_REMOVE_EVENT, 1, // (read) 1 if has a remove 
> event. (write) 1 to clear event
>              }
>  
>              Mutex (MEMORY_SLOT_LOCK, 0)
> @@ -71,8 +72,9 @@
>                      If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory 
> device needs check
>                          MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
>                          Store(1, MEMORY_SLOT_INSERT_EVENT)
> +                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // 
> Ejection request
> +                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
>                      }
> -                    // TODO: handle memory eject request
>                      Add(Local0, One, Local0) // goto next DIMM
>                  }
>                  Release(MEMORY_SLOT_LOCK)
> @@ -172,5 +174,12 @@
>                  Store(Arg2, MEMORY_SLOT_OST_STATUS)
>                  Release(MEMORY_SLOT_LOCK)
>              }
> +
> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
> +                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
> +                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
> +                Store(One, MEMORY_SLOT_REMOVE_EVENT)
> +                Release(MEMORY_SLOT_LOCK)
> +            }
>          } // Device()
>      } // Scope()
> diff --git a/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)
> +            }
could you split changes to hw/i386/ssdt-mem.dsl into a separate patch
so we could simply swap it with AML API version if it's merged first.


>          }
>      }
>  }
> diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
> index b9db295..b61b6ea 100644
> --- a/include/hw/acpi/pc-hotplug.h
> +++ b/include/hw/acpi/pc-hotplug.h
> @@ -42,6 +42,7 @@
>  #define MEMORY_SLOT_PROXIMITY        MPX
>  #define MEMORY_SLOT_ENABLED          MES
>  #define MEMORY_SLOT_INSERT_EVENT     MINS
> +#define MEMORY_SLOT_REMOVE_EVENT     MRMV
>  #define MEMORY_SLOT_SLECTOR          MSEL
>  #define MEMORY_SLOT_OST_EVENT        MOEV
>  #define MEMORY_SLOT_OST_STATUS       MOSC
> @@ -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     MEJ0
>  #define MEMORY_SLOT_NOTIFY_METHOD    MTFY
>  #define MEMORY_SLOT_SCAN_METHOD      MSCN
>