qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation


From: Zhu Guihua
Subject: Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug
Date: Tue, 24 Mar 2015 17:38:53 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0


On 03/16/2015 10:59 PM, Igor Mammedov wrote:
On Mon, 16 Mar 2015 16:58:18 +0800
Zhu Guihua <address@hidden> wrote:

This patch adds a new bit to memory hotplug IO port indicating that
actually bit was added in 2/6 where is_removing had been added.

EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do
the real removal.

Signed-off-by: Zhu Guihua <address@hidden>
---
  docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
  hw/acpi/memory_hotplug.c          | 21 +++++++++++++++++++--
  hw/core/qdev.c                    |  2 +-
  hw/i386/acpi-build.c              |  9 +++++++++
  hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 ++++++++++
  include/hw/acpi/pc-hotplug.h      |  2 ++
  include/hw/qdev-core.h            |  1 +
  trace-events                      |  1 +
  8 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 1290994..85cd4b8 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,12 @@ 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. if guest fails to eject device, it
+                 should send OST event about it and forget about device
+                 removal.
+              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 687b2f1..d6b8c89 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -2,6 +2,7 @@
  #include "hw/acpi/pc-hotplug.h"
  #include "hw/mem/pc-dimm.h"
  #include "hw/boards.h"
+#include "hw/qdev-core.h"
  #include "trace.h"
  #include "qapi-event.h"
@@ -91,6 +92,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,19 +125,33 @@ 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 */
fix comment to match docs above.

+            mdev->is_removing = false;
+            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
just clear event here and don't do removal part as it doesn't match
documentation you've written above regarding this field.

It would be better to move is_removing handling from here to 2/6
+ related ASL code from DSDT which should clear it after sending device check.

+            /*
+             * QEMU memory hot unplug is an asynchronous 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.
+             */
something like this comment, should be in acpi_mem_hotplug.txt not here.


There is 'is_enabled' field, which is 1 if device is present, we can use it
for triggering actual ejecting in QEMU from EJ0(), something like:

} else if (data & 1) { /* eject device */

I think this is not correct. When you clear insert event, the 'is_enabled' filed was also 1.
And when we hot remove memory, the addr 0x14 will be written only once.

Thanks,
Zhu

+            dev = DEVICE(mdev->dimm);
potential NULL dereference, dimm could be NULL if guest does eject twice
or does eject of empty slot.
Perhaps add check before accessing dimm.

  if(!mdev->is_enabled) {
     trace_..._ejecting_invalid_slot(...)
     break;
  }

+            hotplug_ctrl = qdev_get_hotplug_handler(dev);
+            /* Call pc-dimm unplug cb. */
+            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
It's not that we can do anything about error at this point
but instead of forgetting it silently at least log error in trace,
the best would be in addition to that send QMP event to notify mgmt
about it. (sending QMP event could be a separate patch)


          }
          break;
+    default:
+        break;
      }
}

[...]



reply via email to

[Prev in Thread] Current Thread [Next in Thread]