| Message ID | 20200305121417.16583-3-pdurrant@amzn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PV driver compatibility fixes | expand |
Ping? > -----Original Message----- > From: pdurrant@amzn.com <pdurrant@amzn.com> > Sent: 05 March 2020 12:14 > To: xen-devel@lists.xenproject.org > Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu > <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Jan > Beulich <jbeulich@suse.com>; Julien Grall <julien@xen.org>; Stefano Stabellini > <sstabellini@kernel.org>; Anthony PERARD <anthony.perard@citrix.com> > Subject: [PATCH v2 2/2] libxl: make creation of xenstore 'suspend event channel' node optional... > > From: Paul Durrant <pdurrant@amazon.com> > > ... 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@amazon.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Julien Grall <julien@xen.org> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Anthony PERARD <anthony.perard@citrix.com> > > 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
pdurrant@amzn.com writes ("[PATCH v2 2/2] libxl: make creation of xenstore 'suspend event channel' node optional..."):
> From: Paul Durrant <pdurrant@amazon.com>
>
> ... and make the top level 'device' node in xenstore writable by the
> guest
Sorry for taking a long time to review this. Thanks for your
proposal.
> 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 is quite a thorny problem.
I am uncomfortable with making ~/device writeable by the guest. From
what you say it is necessary for at least these guests but I worry
that there might be something out there somewhere (maybe not even in
our tree) which trusts it too much. (libxl used to be in this
category, before XSA-175/178, so this is sadly not a theoretical
concern.) It's true that xend apparently make this writeable but
we have been making it readonly for a while now and maybe people
read xenstore-ls to see, or just didn't care...
I'm not sure how we could conduct an audit to find problems. It seems
like that would be hard to do thoroughly (and a disproportionate
effort). But we could at least
- make this path writeable only as a bug compatibility feature,
ie when needed to support this old guest
- make sure we document it clearly in xenstore-paths as that
is the arch reference that people will use when coding
- document the fact that there may be security implications
of setting thsi compat option
> 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.
So my inclination is to ask you to rework this patch so that:
- the name of the config option more clearly indicates its status
as a bug compat workaround
- the ~/device writeability is controlled by the same compat flag,
with corresponding update to the docs for the compat flag
- adding an entry for the top-level ~/device to
xenstore-paths.pandoc
But I am open to other approaches.
> 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.
Normally we like things broken out into their own patches but this one
is trivial I won't insist on that.
Ian.
> -----Original Message----- > From: Ian Jackson <ian.jackson@citrix.com> > Sent: 17 March 2020 16:51 > To: Paul Durrant <paul@xenproject.org> > Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Andrew Cooper <Andrew.Cooper3@citrix.com>; > George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall > <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard > <anthony.perard@citrix.com> > Subject: Re: [PATCH v2 2/2] libxl: make creation of xenstore 'suspend event channel' node optional... > > pdurrant@amzn.com writes ("[PATCH v2 2/2] libxl: make creation of xenstore 'suspend event channel' > node optional..."): > > From: Paul Durrant <pdurrant@amazon.com> > > > > ... and make the top level 'device' node in xenstore writable by the > > guest > > Sorry for taking a long time to review this. Thanks for your > proposal. > > > 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 is quite a thorny problem. > Indeed. > I am uncomfortable with making ~/device writeable by the guest. From > what you say it is necessary for at least these guests but I worry > that there might be something out there somewhere (maybe not even in > our tree) which trusts it too much. (libxl used to be in this > category, before XSA-175/178, so this is sadly not a theoretical > concern.) It's true that xend apparently make this writeable but > we have been making it readonly for a while now and maybe people > read xenstore-ls to see, or just didn't care... > That's true. I imagine most things don't care but there is a risk they might. > I'm not sure how we could conduct an audit to find problems. It seems > like that would be hard to do thoroughly (and a disproportionate > effort). Impossible really. We don't have code for all frontends :-( > But we could at least > - make this path writeable only as a bug compatibility feature, > ie when needed to support this old guest > - make sure we document it clearly in xenstore-paths as that > is the arch reference that people will use when coding > - document the fact that there may be security implications > of setting thsi compat option I'm ok with that approach. > > > 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. > > So my inclination is to ask you to rework this patch so that: > > - the name of the config option more clearly indicates its status > as a bug compat workaround So, naming-wise... 'xend_compat', or is that too vague? > - the ~/device writeability is controlled by the same compat flag, > with corresponding update to the docs for the compat flag > - adding an entry for the top-level ~/device to > xenstore-paths.pandoc > > But I am open to other approaches. > That all sounds fine. > > 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. > > Normally we like things broken out into their own patches but this one > is trivial I won't insist on that. OK. It did seem too trivial to break out. Paul > > Ian.
Paul Durrant writes ("RE: [PATCH v2 2/2] libxl: make creation of xenstore 'suspend event channel' node optional..."):
> So, naming-wise... 'xend_compat', or is that too vague?
> > - the ~/device writeability is controlled by the same compat flag,
> > with corresponding update to the docs for the compat flag
> > - adding an entry for the top-level ~/device to
> > xenstore-paths.pandoc
> >
> > But I am open to other approaches.
>
> That all sounds fine.
We had this conversation on IRC:
17:01 * Diziet reads.
17:02 <Diziet> I think "xend_compat" is a bit of a vague name, yes.
"xend_suspend_evtchn_compat" maybe ?
17:03 <Diziet> xadimgnik: Would it be possible to put into the xl.cfg(5) doc
bit for this something about *which* pv drivers are likely to be
affected ? I think that would help users (at the very least, it
would help them dismiss it if it wasn't relevant to them).
17:23 <xadimgnik> Diziet: sure, I can add some text and the name is ok
17:31 <Diziet> Thanks.
17:31 <Diziet> Shall I c&p this into an email or will you do that ?
17:35 <xadimgnik> Diziet: it'd be helpful if you did it, thanks
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); }