[Xen-devel] [PATCH 0/3] PV driver compatibility fixes

[Xen-devel] [PATCH 0/3] PV driver compatibility fixes

[Paul Durrant]

Paul Durrant (3):
  libxl: create domain 'error' node in xenstore
  libxl: make creation of xenstore suspend event channel node optional
  libxl: make the top level 'device' node in xenstore writable...

 docs/man/xl.cfg.5.pod.in        |  7 +++++++
 docs/misc/xenstore-paths.pandoc |  5 +++++
 tools/libxl/libxl.h             | 13 ++++++++++++-
 tools/libxl/libxl_create.c      | 17 +++++++++++++----
 tools/libxl/libxl_types.idl     |  1 +
 tools/xl/xl_parse.c             |  3 +++
 6 files changed, 41 insertions(+), 5 deletions(-)

-- 
2.20.1


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

[Xen-devel] [PATCH 1/3] libxl: create domain 'error' node in xenstore

[Paul Durrant]

Several PV drivers (both historically and currently [1]) report errors
by writing text into /local/domain/$DOMID/error. This patch creates the
node in libxl and makes it writable by the domain, and also adds some
text into xenstore-paths.pandoc to state what the node is for.

[1] https://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenvif.git;a=blob;f=src/xenvif/frontend.c;hb=HEAD#l459

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
 docs/misc/xenstore-paths.pandoc | 5 +++++
 tools/libxl/libxl_create.c      | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/docs/misc/xenstore-paths.pandoc b/docs/misc/xenstore-paths.pandoc
index 0a6b36146e..e2ab5da54e 100644
--- a/docs/misc/xenstore-paths.pandoc
+++ b/docs/misc/xenstore-paths.pandoc
@@ -539,6 +539,11 @@ address written in one of these paths to, for example, establish a VNC
 session to the guest (although clearly some level of trust is placed
 in the value supplied by the guest in this case).
 
+#### ~/error [w]
+
+A domain writable path used by some PV drivers to pass error messages
+to the toolstack.
+
 ### Paths private to the toolstack
 
 #### ~/device-model/$DOMID/state [w]
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index ccc9e70990..27627cb199 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -797,6 +797,9 @@ retry_transaction:
     libxl__xs_mknod(gc, t,
                     GCSPRINTF("%s/attr", dom_path),
                     rwperm, ARRAY_SIZE(rwperm));
+    libxl__xs_mknod(gc, t,
+                    GCSPRINTF("%s/error", dom_path),
+                    rwperm, ARRAY_SIZE(rwperm));
 
     if (libxl_defbool_val(info->driver_domain)) {
         /*
-- 
2.20.1


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

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

[Paul Durrant]

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 +++
 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>.
+
 =back
 
 =head2 Devices
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 27627cb199..7119e95412 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;
 }
 
@@ -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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/3] libxl: make the top level 'device' node in xenstore writable...

[Paul Durrant]

... by the guest.

Since this node is created largely to host the frontend areas for PV
devices, all of which are fully guest-writable, there seems little point
in making the top level node read-only. Other toolstacks, such as xend,
did make the node writable by the guest and some PV drivers [1] relied
upon this to stash information.

[1] 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

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>
---
 tools/libxl/libxl_create.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 7119e95412..bc8e525821 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -752,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));
-- 
2.20.1


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

Re: [Xen-devel] [PATCH 1/3] libxl: create domain 'error' node in xenstore

[Ian Jackson]

Paul Durrant writes ("[PATCH 1/3] libxl: create domain 'error' node in xenstore"):
> Several PV drivers (both historically and currently [1]) report errors
> by writing text into /local/domain/$DOMID/error. This patch creates the
> node in libxl and makes it writable by the domain, and also adds some
> text into xenstore-paths.pandoc to state what the node is for.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

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

[Julien Grall]

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

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

[Durrant, Paul]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 27 February 2020 22:52
> To: Durrant, Paul <pdurrant@amazon.co.uk>; 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
> 
> 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.
> 

The doc already says that:

-----
#### ~/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.
-----

> >   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?

Well that's the problem. Some of them may have become accustomed to it being present. Also the documentation does say it is created by the toolstack (but not when). Perhaps I should update the doc to say the toolstack *may* create this path when the domain is created.

> 
> 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. :).

Stable? That'll be the day :-)

  Paul

> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Julien Grall]

Hi Paul,

On 28/02/2020 09:28, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 27 February 2020 22:52
>> To: Durrant, Paul <pdurrant@amazon.co.uk>; 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
>>
>> 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.
>>
> 
> The doc already says that:
> 
> -----
> #### ~/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 paragraph you quoted does not suggest the node may not exist. So I 
think you want to update the documentation to reflect the node may not 
exist.

>>>    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?
> 
> Well that's the problem. Some of them may have become accustomed to it being present. Also the documentation does say it is created by the toolstack (but not when). Perhaps I should update the doc to say the toolstack *may* create this path when the domain is created.

Hmmm fair point. However, this now means you may need to modify your 
configuration file depending on the PV driver installed.

This is not a very ideal situation to be in when you have to upgrade 
your OS (imagine the new PV driver requires the path).

How do you envision this to work?

> 
>>
>> 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. :).
> 
> Stable? That'll be the day :-)

Thank you for the confirmation :).

Cheers,

-- 
Julien Grall

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

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

[Durrant, Paul]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 28 February 2020 10:26
> To: Durrant, Paul <pdurrant@amazon.co.uk>; 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
> 
> Hi Paul,
> 
> On 28/02/2020 09:28, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: 27 February 2020 22:52
> >> To: Durrant, Paul <pdurrant@amazon.co.uk>; 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
> >>
> >> 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.
> >>
> >
> > The doc already says that:
> >
> > -----
> > #### ~/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 paragraph you quoted does not suggest the node may not exist. So I
> think you want to update the documentation to reflect the node may not
> exist.
> 
> >>>    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?
> >
> > Well that's the problem. Some of them may have become accustomed to it
> being present. Also the documentation does say it is created by the
> toolstack (but not when). Perhaps I should update the doc to say the
> toolstack *may* create this path when the domain is created.
> 
> Hmmm fair point. However, this now means you may need to modify your
> configuration file depending on the PV driver installed.
> 
> This is not a very ideal situation to be in when you have to upgrade
> your OS (imagine the new PV driver requires the path).
> 
> How do you envision this to work?
> 

Indeed. Basically this is a control knob you can try if you have drivers that don't cope. The scenario is that you have a guest image imported from an old (pre-libxl) Xen, you fire it up, it BSODs... what do you do? The bug is in the guest drivers - they ought to cope whether the node is there or not - but you can't boot the VM to upgrade them. This option allows you to get the VM going so you can fix it. Your only other option would be to start the guest paused, xenstore-rm the node, and then unpause. 
We could just document that procedure, but where? Hopefully an admin might read the xl.cfg manpage when moving to the newer Xen and would see this option was available to them.

The fact that xenstore is largely a wild west is not an admin's fault and we really ought to try to help them where we can.

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