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

[Vasilis Liaskovitis]

On Thu, May 29, 2014 at 11:12:37AM +0200, Igor Mammedov wrote:
> On Wed, 28 May 2014 18:38:13 +0200
> Vasilis Liaskovitis <address@hidden> wrote:
> 
> > On Wed, May 28, 2014 at 03:26:42PM +0200, Igor Mammedov wrote:
> > > On Wed, 28 May 2014 14:23:13 +0200
> > > Vasilis Liaskovitis <address@hidden> wrote:
> > > 
> > > > 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:
> > > > 
> The same register [0x14] is control register when writing there, so we can 
> use-reuse
> the same bit position as MRMV for signaling QEMU to perform eject on writing 
> 1 there,
> similarly like it's done for insert event.

thanks, on a closer look that makes sense. MRMV is sufficient, example patch
below.

With the new "query-acpi-ospm-status" we can read OST status from QMP. However
as I mentioned, on succesful removal, the guest kernel does not send the OST 
success
status (0x0).

This leads to the scenario where memory device is succesfully ejected, _EJ0 is
evaluated in guest and device is deleted in qemu (with the patch below). The
dimm will no longer show up in "query-memory-devices", however the ospm status
of the dimm slot will still remain  at 0x84 (ejection in progress).  This can
be confusing to management layers. Do you have any suggestion for how to
report the succesful _EJ0? We could simply write a succesful status:

  mdev->ost_status = 0x0 

when MRMV is written to, but it is a bit hacky. On the other hand, having 
another
command only for _EJ0 notification sounds like overkill.

In terms of linux kernel ACPI conformity, Figure 6-37 in the spec implies that
_OST evaluation is not required after succesfull _EJ0 evaluation. Also see
relevant comment in drivers/acpi/scan.c: acpi_scan_hot_remove() line 378
(3.15-rc8):
    /*
    * Verify if eject was indeed successful.  If not, log an error
    * message.  No need to call _OST since _EJ0 call was made
    * OK.
    */



acpi, memory-hotplug: Add _EJ0 handling

---
 docs/specs/acpi_mem_hotplug.txt |    3 ++-
 hw/acpi/memory_hotplug.c        |    9 +++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 1290994..4501774 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -35,7 +35,8 @@ 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: Device no longer used by guest, can be safely removed  
+              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 8aa829d..24989d3 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;
@@ -135,6 +129,9 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
             trace_mhp_acpi_clear_insert_evt(mem_st->selector);
         } else if (data & 4) { /* MRMV */
             mdev->is_enabled = false;
+            object_unparent(OBJECT(mdev->dimm));
+            mdev->is_removing = false;
+            mdev->dimm = NULL;
         }
         break;
     }