Patchwork [RFC,v3,11/19] Implement qmp and hmp commands for notification lists

login
register
mail settings
Submitter Vasilis Liaskovitis
Date Sept. 21, 2012, 11:17 a.m.
Message ID <1348226255-4226-12-git-send-email-vasilis.liaskovitis@profitbricks.com>
Download mbox | patch
Permalink /patch/185715/
State New
Headers show

Comments

Vasilis Liaskovitis - Sept. 21, 2012, 11:17 a.m.
Guest can respond to ACPI hotplug events e.g. with _EJ or _OST method.
This patch implements a tail queue to store guest notifications for memory
hot-add and hot-remove requests.

Guest responses for memory hotplug command on a per-dimm basis can be detected
with the new hmp command "info memhp" or the new qmp command "query-memhp"
Examples:

(qemu) device_add dimm,id=ram0
(qemu) info memory-hotplug
dimm: ram0 hot-add success
or
dimm: ram0 hot-add failure

(qemu) device_del ram3
(qemu) info memory-hotplug
dimm: ram3 hot-remove success
or
dimm: ram3 hot-remove failure

Results are removed from the queue once read.

This patch only queues _EJ events that signal hot-remove success.
For  _OST event queuing, which cover the hot-remove failure and
hot-add success/failure cases, the _OST patches in this series are  are also
needed.

These notification items should probably be part of migration state (not yet
implemented).

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
---
 hmp-commands.hx  |    2 +
 hmp.c            |   17 ++++++++++++++
 hmp.h            |    1 +
 hw/dimm.c        |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/dimm.h        |    2 +-
 monitor.c        |    7 ++++++
 qapi-schema.json |   26 ++++++++++++++++++++++
 qmp-commands.hx  |   37 ++++++++++++++++++++++++++++++++
 8 files changed, 152 insertions(+), 2 deletions(-)
Eric Blake - Sept. 21, 2012, 10:03 p.m.
On 09/21/2012 05:17 AM, Vasilis Liaskovitis wrote:
> Guest can respond to ACPI hotplug events e.g. with _EJ or _OST method.
> This patch implements a tail queue to store guest notifications for memory
> hot-add and hot-remove requests.
> 
> Guest responses for memory hotplug command on a per-dimm basis can be detected
> with the new hmp command "info memhp" or the new qmp command "query-memhp"

Naming doesn't match the QMP code.

> Examples:
> 
> (qemu) device_add dimm,id=ram0

> 
> These notification items should probably be part of migration state (not yet
> implemented).

In the case of libvirt driving migration, you already said in 10/19 that
libvirt has to start the destination with the populated=on|off fields
correct for each dimm according to the state it was in at the time the
host started the update.  Can the host hot unplug memory after migration
has started?

> +
> +##
> +# @MemHpInfo:
> +#
> +# Information about status of a memory hotplug command
> +#
> +# @dimm: the Dimm associated with the result
> +#
> +# @result: the result of the hotplug command
> +#
> +# Since: 1.3
> +#
> +##
> +{ 'type': 'MemHpInfo',
> +  'data': {'dimm': 'str', 'request': 'str', 'result': 'str'} }

Should 'result' be a bool (true for success, false for still pending) or
an enum, instead of a free-form string?  Likewise, isn't 'request' going
to be exactly one of two values (plug or unplug)?
Vasilis Liaskovitis - Sept. 24, 2012, 2:45 p.m.
Hi,

On Fri, Sep 21, 2012 at 04:03:26PM -0600, Eric Blake wrote:
> On 09/21/2012 05:17 AM, Vasilis Liaskovitis wrote:
> > Guest can respond to ACPI hotplug events e.g. with _EJ or _OST method.
> > This patch implements a tail queue to store guest notifications for memory
> > hot-add and hot-remove requests.
> > 
> > Guest responses for memory hotplug command on a per-dimm basis can be detected
> > with the new hmp command "info memhp" or the new qmp command "query-memhp"
> 
> Naming doesn't match the QMP code.

will fix.

> 
> > Examples:
> > 
> > (qemu) device_add dimm,id=ram0
> 
> > 
> > These notification items should probably be part of migration state (not yet
> > implemented).
> 
> In the case of libvirt driving migration, you already said in 10/19 that
> libvirt has to start the destination with the populated=on|off fields
> correct for each dimm according to the state it was in at the time the

That patch actually alleviates this restriction for the off->on direction i.e.
it allows for the target-VM to not have its args updated for dimm hot-add.
(e.g. Let's say the source was started with a dimm, initialy off. The dimm is
 hot-plugged, and then migrated . WIth patch 10/19, the populated arg doesn't
 have to be updated on the target)
The other direction (off->on) still needs correct arg change.

If libvirt/management layers guarantee the dimm arguments are correctly changed,
I don't see that we need 10/19 patch eventually.

What I think is needed is another hmp/qmp command, that will report
which dimms are on/off at any given time e.g.

(monitor) info memory-hotplug

dimm0: off
dimm1: on
...
dimmN: off

This can be used on the source by libvirt / other layers to find out the
populated dimms, and construct the correct command line on the destination.
Does this make sense to you?

The current patch only deals with success/failure event notifications (not
on-off state of dimms) and should probably be renamed to
"query-memory-hotplug-events".

> host started the update.  Can the host hot unplug memory after migration
> has started?

Good testcase. I would rather not allow any  hotplug operations while the migration
is happening. 

What do we do with pci hotplug during migration currently?  I found a discussion
dating from a year ago, suggesting the same as the simplest solution, but I
don't know what's currently implemented.
http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg01204.html

> 
> > +
> > +##
> > +# @MemHpInfo:
> > +#
> > +# Information about status of a memory hotplug command
> > +#
> > +# @dimm: the Dimm associated with the result
> > +#
> > +# @result: the result of the hotplug command
> > +#
> > +# Since: 1.3
> > +#
> > +##
> > +{ 'type': 'MemHpInfo',
> > +  'data': {'dimm': 'str', 'request': 'str', 'result': 'str'} }
> 
> Should 'result' be a bool (true for success, false for still pending) or
> an enum, instead of a free-form string?  Likewise, isn't 'request' going
> to be exactly one of two values (plug or unplug)?

agreed with 'request'.

For 'result' it is also a boolean, but with 'success' and 'failure' (rather than
'pending'). Items are only queued when the guest has given us a definite _OST
or _EJ result wich is either success or fail. If an operation is pending, nothing
is queued here. 

Perhaps queueing pending operations also has a usecase, but this isn't addressed
in this patch.
 
thanks,

- Vasilis
Stefan Hajnoczi - Oct. 23, 2012, 12:15 p.m.
On Fri, Sep 21, 2012 at 01:17:27PM +0200, Vasilis Liaskovitis wrote:
> +MemHpInfoList *qmp_query_memory_hotplug(Error **errp)
> +{
> +    DimmBus *bus = main_memory_bus;
> +    MemHpInfoList *head = NULL, *cur_item = NULL, *info;
> +    struct dimm_hp_result *item, *nextitem;
> +
> +    QTAILQ_FOREACH_SAFE(item, &bus->dimm_hp_result_queue, next, nextitem) {
> +
> +        info = g_malloc0(sizeof(*info));
> +        info->value = g_malloc0(sizeof(*info->value));
> +        info->value->dimm = g_malloc0(sizeof(char) * 32);
> +        info->value->request = g_malloc0(sizeof(char) * 16);
> +        info->value->result = g_malloc0(sizeof(char) * 16);
> +        switch (item->ret) {
> +            case DIMM_REMOVE_SUCCESS:
> +                strcpy(info->value->request, "hot-remove");
> +                strcpy(info->value->result, "success");
> +                break;
> +            case DIMM_REMOVE_FAIL:
> +                strcpy(info->value->request, "hot-remove");
> +                strcpy(info->value->result, "failure");
> +                break;
> +            case DIMM_ADD_SUCCESS:
> +                strcpy(info->value->request, "hot-add");
> +                strcpy(info->value->result, "success");
> +                break;
> +            case DIMM_ADD_FAIL:
> +                strcpy(info->value->request, "hot-add");
> +                strcpy(info->value->result, "failure");
> +                break;
> +            default:
> +                break;    
> +        }

Any reason to use fixed-size malloc + strcpy() instead of just
info->value->X = g_strdup("foo")?

Stefan

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index ed67e99..cfb1b67 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1462,6 +1462,8 @@  show device tree
 show qdev device model list
 @item info roms
 show roms
+@item info memory-hotplug
+show memory-hotplug
 @end table
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index ba6fbd3..4b3d63d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1168,3 +1168,20 @@  void hmp_screen_dump(Monitor *mon, const QDict *qdict)
     qmp_screendump(filename, &err);
     hmp_handle_error(mon, &err);
 }
+
+void hmp_info_memory_hotplug(Monitor *mon)
+{
+    MemHpInfoList *info;
+    MemHpInfoList *item;
+    MemHpInfo *dimm;
+
+    info = qmp_query_memory_hotplug(NULL);
+    for (item = info; item; item = item->next) {
+        dimm = item->value;
+        monitor_printf(mon, "dimm: %s %s %s\n", dimm->dimm,
+                dimm->request, dimm->result);
+        dimm->dimm = NULL;
+    }
+
+    qapi_free_MemHpInfoList(info);
+}
diff --git a/hmp.h b/hmp.h
index 48b9c59..986705a 100644
--- a/hmp.h
+++ b/hmp.h
@@ -73,5 +73,6 @@  void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
 void hmp_send_key(Monitor *mon, const QDict *qdict);
 void hmp_screen_dump(Monitor *mon, const QDict *qdict);
+void hmp_info_memory_hotplug(Monitor *mon);
 
 #endif
diff --git a/hw/dimm.c b/hw/dimm.c
index 288b997..fbd93a8 100644
--- a/hw/dimm.c
+++ b/hw/dimm.c
@@ -65,6 +65,7 @@  static void dimm_bus_initfn(Object *obj)
     DimmBus *bus = DIMM_BUS(obj);
     QTAILQ_INIT(&bus->dimmconfig_list);
     QTAILQ_INIT(&bus->dimmlist);
+    QTAILQ_INIT(&bus->dimm_hp_result_queue);
 
     QTAILQ_FOREACH_SAFE(dimm_cfg, &dimmconfig_list, nextdimmcfg, next_dimm_cfg) {
         QTAILQ_REMOVE(&dimmconfig_list, dimm_cfg, nextdimmcfg);
@@ -236,20 +237,78 @@  void dimm_notify(uint32_t idx, uint32_t event)
 {
     DimmBus *bus = main_memory_bus;
     DimmDevice *s;
+    DimmConfig *slotcfg;
+    struct dimm_hp_result *result;
+
     s = dimm_find_from_idx(idx);
     assert(s != NULL);
+    result = g_malloc0(sizeof(*result));
+    slotcfg = dimmcfg_find_from_name(DEVICE(s)->id);
+    result->dimmname = slotcfg->name;
 
     switch(event) {
         case DIMM_REMOVE_SUCCESS:
             dimm_depopulate(s);
-            qdev_simple_unplug_cb((DeviceState*)s);
             QTAILQ_REMOVE(&bus->dimmlist, s, nextdimm);
+            qdev_simple_unplug_cb((DeviceState*)s);
+            QTAILQ_INSERT_TAIL(&bus->dimm_hp_result_queue, result, next);
             break;
         default:
+            g_free(result);
             break;
     }
 }
 
+MemHpInfoList *qmp_query_memory_hotplug(Error **errp)
+{
+    DimmBus *bus = main_memory_bus;
+    MemHpInfoList *head = NULL, *cur_item = NULL, *info;
+    struct dimm_hp_result *item, *nextitem;
+
+    QTAILQ_FOREACH_SAFE(item, &bus->dimm_hp_result_queue, next, nextitem) {
+
+        info = g_malloc0(sizeof(*info));
+        info->value = g_malloc0(sizeof(*info->value));
+        info->value->dimm = g_malloc0(sizeof(char) * 32);
+        info->value->request = g_malloc0(sizeof(char) * 16);
+        info->value->result = g_malloc0(sizeof(char) * 16);
+        switch (item->ret) {
+            case DIMM_REMOVE_SUCCESS:
+                strcpy(info->value->request, "hot-remove");
+                strcpy(info->value->result, "success");
+                break;
+            case DIMM_REMOVE_FAIL:
+                strcpy(info->value->request, "hot-remove");
+                strcpy(info->value->result, "failure");
+                break;
+            case DIMM_ADD_SUCCESS:
+                strcpy(info->value->request, "hot-add");
+                strcpy(info->value->result, "success");
+                break;
+            case DIMM_ADD_FAIL:
+                strcpy(info->value->request, "hot-add");
+                strcpy(info->value->result, "failure");
+                break;
+            default:
+                break;    
+        }
+        strcpy(info->value->dimm, item->dimmname);
+        /* XXX: waiting for the qapi to support GSList */
+        if (!cur_item) {
+            head = cur_item = info;
+        } else {
+            cur_item->next = info;
+            cur_item = info;
+        }
+
+        /* hotplug notification copied to qmp list, delete original item */
+        QTAILQ_REMOVE(&bus->dimm_hp_result_queue, item, next);
+        g_free(item);
+    }
+
+    return head;
+}
+
 static int dimm_init(DeviceState *s)
 {
     DimmBus *bus = main_memory_bus;
@@ -286,6 +345,7 @@  static void dimm_class_init(ObjectClass *klass, void *data)
 
     dc->props = dimm_properties;
     dc->unplug = dimm_unplug_device;
+    dc->bus_type = TYPE_DIMM_BUS;
     dc->init = dimm_init;
 }
 
diff --git a/hw/dimm.h b/hw/dimm.h
index 5e991a6..95251ba 100644
--- a/hw/dimm.h
+++ b/hw/dimm.h
@@ -69,6 +69,7 @@  typedef struct DimmBus {
     dimm_calcoffset_fn dimm_calcoffset;
     DimmConfiglist dimmconfig_list;
     QTAILQ_HEAD(Dimmlist, DimmDevice) dimmlist;
+    QTAILQ_HEAD(dimm_hp_result_head, dimm_hp_result)  dimm_hp_result_queue;
 } DimmBus;
 
 struct dimm_hp_result {
@@ -86,5 +87,4 @@  void main_memory_bus_create(Object *parent);
 void dimm_config_create(char *id, uint64_t size, uint64_t node,
         uint32_t dimm_idx, uint32_t populated);
 
-
 #endif
diff --git a/monitor.c b/monitor.c
index 67064e2..be9a1d9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2740,6 +2740,13 @@  static mon_cmd_t info_cmds[] = {
         .mhandler.info = do_trace_print_events,
     },
     {
+        .name       = "memory-hotplug",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show memory hotplug status",
+        .mhandler.info = hmp_info_memory_hotplug,
+    },
+    {
         .name       = NULL,
     },
 };
diff --git a/qapi-schema.json b/qapi-schema.json
index a9f465a..3706a2a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2555,3 +2555,29 @@ 
 # Since: 0.14.0
 ##
 { 'command': 'screendump', 'data': {'filename': 'str'} }
+
+##
+# @MemHpInfo:
+#
+# Information about status of a memory hotplug command
+#
+# @dimm: the Dimm associated with the result
+#
+# @result: the result of the hotplug command
+#
+# Since: 1.3
+#
+##
+{ 'type': 'MemHpInfo',
+  'data': {'dimm': 'str', 'request': 'str', 'result': 'str'} }
+
+##
+# @query-memory-hotplug:
+#
+# Returns a list of information about pending hotplug commands
+#
+# Returns: a list of @MemhpInfo
+#
+# Since: 1.3
+##
+{ 'command': 'query-memory-hotplug', 'returns': ['MemHpInfo'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6e21ddb..e50dcc2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2539,3 +2539,40 @@  EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_target,
     },
+    {
+        .name       = "query-memory-hotplug",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_memory_hotplug
+    },
+SQMP
+query-memory-hotplug
+----------
+
+Show memory hotplug command notifications.
+
+Return a json-array. Each DIMM that has a pending notification is represented
+by a json-object, which contains:
+
+- "dimm": Dimm name (json-str)
+- "request": type of hot request: hot-add or hot-remove  (json-str)
+- "result": result of the hotplug request for this Dimm success or failure (json-str)
+
+Example:
+
+-> { "execute": "query-memory-hotplug" }
+<- {
+      "return":[
+         {
+            "result": "failure",
+            "request": "hot-remove",
+            "dimm": "dimm10"
+         },
+         {
+            "result": "success",
+            "request": "hot-add",
+            "dimm": "dimm3"
+         }
+      ]
+   }
+
+EQMP