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

From: Julien Grall <julien@xen.org>
To: Paul Durrant <pdurrant@amazon.com>, xen-devel@lists.xenproject.org
Cc: Anthony PERARD <anthony.perard@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Wei Liu <wl@xen.org>
Subject: Re: [Xen-devel] [PATCH 2/3] libxl: make creation of xenstore suspend event channel node optional
Date: Thu, 27 Feb 2020 22:51:42 +0000	[thread overview]
Message-ID: <4ad6fe4e-40bd-7a04-54d2-38edb56346e9@xen.org> (raw)
In-Reply-To: <20200226160848.1854-3-pdurrant@amazon.com>

Hi,

On 26/02/2020 16:08, Paul Durrant wrote:
> The purpose and semantics of the 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.
> 
> [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 ibxl_domain_create_info is a function.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
>   docs/man/xl.cfg.5.pod.in    |  7 +++++++
>   tools/libxl/libxl.h         | 13 ++++++++++++-
>   tools/libxl/libxl_create.c  | 12 +++++++++---
>   tools/libxl/libxl_types.idl |  1 +
>   tools/xl/xl_parse.c         |  3 +++

You may want to update xenstore-paths.pandoc as the document mention the 
node will be created by the toolstack.

>   5 files changed, 32 insertions(+), 4 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>.

In the next patch you are going to make device/ rw. Do you see any issue 
with just not creating the node for everyone? Are PV drivers allowed to 
assume a node will be present?

My knowledge of xenstore is limited, so I thought I would ask the 
questions to understand a bit more how stable the ABI is meant to be. :).

Cheers,

-- 
Julien Grall

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