Re: [Qemu-devel] [PATCH 33/35] pc: ACPI BIOS: reserve SRAT entry for hotplug mem hole

[Vasilis Liaskovitis]

On Wed, May 28, 2014 at 10:07:22AM +0200, Igor Mammedov wrote:
> On Tue, 27 May 2014 17:57:31 +0200
> Anshul Makkar <address@hidden> wrote:
> 
> > Hi,
> > 
> > I tested the hot unplug patch and doesn't seem to work properly with Debian
> > 6 and Ubuntu host.
> > 
> > Scenario:
> > I added 3 dimm devices of 1G each:
> > 
> > object_add memory-ram,id=ram0,size=1G, device_add dimm,id=dimm1,memdev=ram0
> > 
> > object_add memory-ram,id=ram1,size=1G, device_add dimm,id=dimm2,memdev=ram1
> > 
> > object_add memory-ram,id=ram2,size=1G, device_add dimm,id=dimm3,memdev=ram2
> > 
> > device_del dimm3: I get the OST EVENT EJECT 0x3 and OST STATUS as 0x84(IN
> > PROGRESS) If I check on the guest, the device has been successfully
> > removed. But no OST EJECT SUCCESS event was received.
> I think there should be a SUCCESS event,
> it should be investigated from the guest side first, OST support in kernel
> is relatively new.

When testing older guest kernel (3.11), _OST success events are not sent
from the guest. I haven't tried newer versions yet.

In terms of OSPM _OST behaviour, I am not sure if returning OST success status
on succcesful removal is *required*. Figure 6-37, page 306 of ACPI spec5.0
shows that on succcesfull OS ejection ejection, _EJ0 is evaluated. Evaluating
_OST does not seem to be a requirement, is it? (cc'ing linux-acpi for input)

In linux guests, on successful removal, _EJ0 is always evaluated. I believe we
should be handling _EJ0 and doing the dimm removal (object_unparent) there.
Currently OST successes are never received and dimm devices remain in QEMU even
when successfully ejected from guest.
E.g. a quick patch for _EJ0 handling, on top of Hu Tao's series:

acpi, memory-hotplug: Add _EJ0 handling

---
 docs/specs/acpi_mem_hotplug.txt  |    3 ++-
 hw/acpi/memory_hotplug.c         |   13 +++++++------
 hw/i386/ssdt-misc.dsl            |    3 ++-
 include/hw/acpi/memory_hotplug.h |    1 +
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 1290994..1352962 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -28,7 +28,8 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte 
access):
                 region will read/store data from/to selected memory device.
       [0x4-0x7] OST event code reported by OSPM
       [0x8-0xb] OST status code reported by OSPM
-      [0xc-0x13] reserved, writes into it are ignored
+      [0xc]    EJ device if written to
+      [0xd-0x13] reserved, writes into it are ignored
       [0x14] Memory device control fields
           bits:
               0: reserved, OSPM must clear it before writing to register
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 8aa829d..d3edd28 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -93,9 +93,6 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
         case 0x03: /* EJECT */
             switch (mdev->ost_status) {
             case 0x0: /* SUCCESS */
-                object_unparent(OBJECT(mdev->dimm));
-                mdev->is_removing = false;
-                mdev->dimm = NULL;
                 break;
             case 0x1: /* FAILURE */
             case 0x2: /* UNRECOGNIZED NOTIFY */
@@ -115,9 +112,6 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
         case 0x103: /* OSPM EJECT */
             switch (mdev->ost_status) {
             case 0x0: /* SUCCESS */
-                object_unparent(OBJECT(mdev->dimm));
-                mdev->is_removing = false;
-                mdev->dimm = NULL;
                 break;
             case 0x84: /* EJECTION IN PROGRESS */
                 mdev->is_enabled = false;
@@ -137,6 +131,12 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
             mdev->is_enabled = false;
         }
         break;
+    case 0x0c:
+        mdev = &mem_st->devs[mem_st->selector];
+        object_unparent(OBJECT(mdev->dimm));
+        mdev->is_removing = false;
+        mdev->dimm = NULL;
+        break;
     }
 }
 
@@ -238,6 +238,7 @@ static const VMStateDescription vmstate_memhp_sts = {
         VMSTATE_BOOL(is_inserting, MemStatus),
         VMSTATE_UINT32(ost_event, MemStatus),
         VMSTATE_UINT32(ost_status, MemStatus),
+        VMSTATE_UINT32(ej_status, MemStatus),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
index 927e503..d20c6f0 100644
--- a/hw/i386/ssdt-misc.dsl
+++ b/hw/i386/ssdt-misc.dsl
@@ -163,6 +163,7 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", 
"BXSSDTSUSP", 0x1)
                 MSEL, 32,  // DIMM selector, write only
                 MOEV, 32,  // _OST event code, write only
                 MOSC, 32,  // _OST status code, write only
+                MEJE, 8, // if written to, eject DIMM
             }
 
             Method(MESC, 0, Serialized) {
@@ -283,7 +284,7 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", 
"BXSSDTSUSP", 0x1)
             Method(MDEJ, 2) {
                 Acquire(MLCK, 0xFFFF)
                 Store(ToInteger(Arg0), MSEL) // select DIMM
-                Store(One, MRMV)
+                Store(1, MEJE)
                 Release(MLCK)
             }
         } // Device()
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index 66dbe96..6a9f378 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -14,6 +14,7 @@ typedef struct MemStatus {
     bool is_removing;
     uint32_t ost_event;
     uint32_t ost_status;
+    uint32_t ej_status;
 } MemStatus;
 
 typedef struct MemHotplugState {
-- 
1.7.10.4