Re: [Xen-devel] [PATCH v2 2/2] libxl: make creation of xenstore 'suspend event channel' node optional...

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

Ping?

> -----Original Message-----
> From: pdurrant@xxxxxxxx <pdurrant@xxxxxxxx>
> Sent: 05 March 2020 12:14
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Durrant, Paul <pdurrant@xxxxxxxxxxxx>; Ian Jackson 
> <ian.jackson@xxxxxxxxxxxxx>; Wei Liu
> <wl@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap 
> <george.dunlap@xxxxxxxxxx>; Jan
> Beulich <jbeulich@xxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Subject: [PATCH v2 2/2] libxl: make creation of xenstore 'suspend event 
> channel' node optional...
> 
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> ... 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 <pdurrant@xxxxxxxxxx>
> ---
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 
> 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             | 13 ++++++++++++-
>  tools/libxl/libxl_create.c      | 14 ++++++++++----
>  tools/libxl/libxl_types.idl     |  1 +
>  tools/xl/xl_parse.c             |  3 +++
>  6 files changed, 37 insertions(+), 8 deletions(-)
> 
> 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..d2afe48512 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1272,10 +1272,21 @@ 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.
> + */
> +
>  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);
>  }
> 
> --
> 2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel