[v3,2/2] libxl: make creation of xenstore 'suspend event channel' node optional...

Commit Message

Paul Durrant March 13, 2020, 12:19 p.m. UTC

From: Paul Durrant <[e-mail]>

... and make the top level 'device' node in xenstore writable by the
guest

The purpose and semantics of the suspend event channel node are explained
in xenstore-paths.pandoc [1]. It was originally introduced in xend by
commit 17636f47a474 "Teach xc_save to use event-channel-based domain
suspend if available.". Note that, because, the top-level frontend
'device' node was created writable by the guest in xend, there was no
need to explicitly create the 'suspend-event-channel' node as writable
node.

However, libxl creates the 'device' node as read-only by the guest and so
explicit creation of the 'suspend-event-channel' node is necessary to make
it usable. This unfortunately has the side-effect of making some old
Windows PV drivers [2] cease to function. This is because they scan the top
level 'device' node, find the 'suspend' node and expect it to contain the
usual sub-nodes describing a PV frontend. When this is found not to be the
case, enumeration ceases and (because the 'suspend' node is observed before
the 'vbd' node) no system disk is enumerated. Windows will then crash with
bugcheck code 0x7B.

This patch adds a boolean 'suspend_event_channel' field into
libxl_create_info to control whether the xenstore node is created and a
similarly named option in xl.cfg which, for compatibility with previous
libxl behaviour, defaults to true. It also makes the top level device node
writable, as xend did, and updates xenstore-paths.pandoc to say that the
suspend event channel node may not exist and that the guest may create it
if it does not exist.

[1] https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/xenstore-paths.pandoc;hb=HEAD#l177
[2] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/5/html/para-virtualized_windows_drivers_guide/sect-para-virtualized_windows_drivers_guide-installing_and_configuring_the_para_virtualized_drivers-installing_the_para_virtualized_drivers

NOTE: While adding the new LIBXL_HAVE_CREATEINFO_SUSPEND_EVENT_CHANNEL
      definition into libxl.h, this patch corrects the previous stanza
      which erroneously implies libxl_domain_create_info is a function.

Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Ian Jackson <[e-mail]>
Cc: Wei Liu <[e-mail]>
Cc: Andrew Cooper <[e-mail]>
Cc: George Dunlap <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Julien Grall <[e-mail]>
Cc: Stefano Stabellini <[e-mail]>
Cc: Anthony PERARD <[e-mail]>

v3:
 - Actually define LIBXL_HAVE_CREATEINFO_SUSPEND_EVENT_CHANNEL as well
   as commenting on it

v2:
 - Update xenstore-paths.pandoc and squash patch #3
---
 docs/man/xl.cfg.5.pod.in        |  7 +++++++
 docs/misc/xenstore-paths.pandoc |  7 ++++---
 tools/libxl/libxl.h             | 14 +++++++++++++-
 tools/libxl/libxl_create.c      | 14 ++++++++++----
 tools/libxl/libxl_types.idl     |  1 +
 tools/xl/xl_parse.c             |  3 +++
 6 files changed, 38 insertions(+), 8 deletions(-)

Patch

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 0cad561375..5f476f1e1d 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -668,6 +668,13 @@  file.
 
 =back
 
+=item B<suspend_event_channel=BOOLEAN>
+
+Create the xenstore path for the domain's suspend event channel. The
+existence of this path can cause problems with older PV drivers running
+in the guest. If this option is not specified then it will default to
+B<true>.
+
 =back
 
 =head2 Devices
diff --git a/docs/misc/xenstore-paths.pandoc b/docs/misc/xenstore-paths.pandoc
index e2ab5da54e..a8eecdb7ed 100644
--- a/docs/misc/xenstore-paths.pandoc
+++ b/docs/misc/xenstore-paths.pandoc
@@ -176,10 +176,11 @@  The size of the video RAM this domain is configured with.
 
 #### ~/device/suspend/event-channel = ""|EVTCHN [w]
 
-The domain's suspend event channel. The toolstack will create this
-path with an empty value which the guest may choose to overwrite.
+The domain's suspend event channel. The toolstack may create this
+path with an empty value which the guest may choose to overwrite. If
+the path does not exist then the guest may create it.
 
-If the guest overwrites this, it will be with the number of an unbound
+If the guest writes this, it will be with the number of an unbound
 event channel port it has acquired.  The toolstack is expected to use
 an interdomain bind, and then, when it wishes to ask the guest to
 suspend, to signal the event channel.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 35e13428b2..b97823823b 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1272,10 +1272,22 @@  void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
  * LIBXL_HAVE_CREATEINFO_DOMID
  *
  * libxl_domain_create_new() and libxl_domain_create_restore() will use
- * a domid specified in libxl_domain_create_info().
+ * a domid specified in libxl_domain_create_info.
  */
 #define LIBXL_HAVE_CREATEINFO_DOMID
 
+/*
+ * LIBXL_HAVE_CREATEINFO_SUSPEND_EVENT_CHANNEL
+ *
+ * libxl_domain_create_info contains a boolean 'suspend_event_channel'
+ * value to control whether the xenstore path:
+ *
+ * /local/domain/$DOMID/device/suspend/event-channel (RW)
+ *
+ * is created.
+ */
+#define LIBXL_HAVE_CREATEINFO_SUSPEND_EVENT_CHANNEL
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index fb7b3999ae..8afb0ce2be 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -57,6 +57,8 @@  int libxl__domain_create_info_setdefault(libxl__gc *gc,
     if (!c_info->ssidref)
         c_info->ssidref = SECINITSID_DOMU;
 
+    libxl_defbool_setdefault(&c_info->suspend_event_channel, true);
+
     return 0;
 }
 
@@ -750,7 +752,7 @@  retry_transaction:
                     roperm, ARRAY_SIZE(roperm));
     libxl__xs_mknod(gc, t,
                     GCSPRINTF("%s/device", dom_path),
-                    roperm, ARRAY_SIZE(roperm));
+                    rwperm, ARRAY_SIZE(rwperm));
     libxl__xs_mknod(gc, t,
                     GCSPRINTF("%s/control", dom_path),
                     roperm, ARRAY_SIZE(roperm));
@@ -782,9 +784,13 @@  retry_transaction:
     libxl__xs_mknod(gc, t,
                     GCSPRINTF("%s/control/sysrq", dom_path),
                     rwperm, ARRAY_SIZE(rwperm));
-    libxl__xs_mknod(gc, t,
-                    GCSPRINTF("%s/device/suspend/event-channel", dom_path),
-                    rwperm, ARRAY_SIZE(rwperm));
+
+    if (libxl_defbool_val(info->suspend_event_channel))
+        libxl__xs_mknod(gc, t,
+                        GCSPRINTF("%s/device/suspend/event-channel",
+                                  dom_path),
+                        rwperm, ARRAY_SIZE(rwperm));
+
     libxl__xs_mknod(gc, t,
                     GCSPRINTF("%s/data", dom_path),
                     rwperm, ARRAY_SIZE(rwperm));
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index d0d431614f..2bce19bcf0 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -418,6 +418,7 @@  libxl_domain_create_info = Struct("domain_create_info",[
     ("run_hotplug_scripts",libxl_defbool),
     ("driver_domain",libxl_defbool),
     ("passthrough",  libxl_passthrough),
+    ("suspend_event_channel",libxl_defbool),
     ], dir=DIR_IN)
 
 libxl_domain_restore_params = Struct("domain_restore_params", [
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index b881184804..122c6eb641 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2725,6 +2725,9 @@  skip_usbdev:
 
     parse_vkb_list(config, d_config);
 
+    xlu_cfg_get_defbool(config, "suspend_event_channel",
+                        &c_info->suspend_event_channel, 0);
+
     xlu_cfg_destroy(config);
 }