RE: [PATCH v4 1/3] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_disable_fifo, ...

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 25 November 2020 09:20
> To: Paul Durrant <paul@xxxxxxx>
> Cc: Paul Durrant <pdurrant@xxxxxxxxxx>; Eslam Elnikety <elnikety@xxxxxxxxxx>; 
> Christian Lindig
> <christian.lindig@xxxxxxxxxx>; David Scott <dave@xxxxxxxxxx>; Ian Jackson 
> <iwj@xxxxxxxxxxxxxx>; Wei
> Liu <wl@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap 
> <george.dunlap@xxxxxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; 
> xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 1/3] domctl: introduce a new domain create flag, 
> XEN_DOMCTL_CDF_disable_fifo,
> ...
> 
> On 24.11.2020 20:17, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@xxxxxxxxxx>
> >
> > ...to control the visibility of the FIFO event channel operations
> > (EVTCHNOP_init_control, EVTCHNOP_expand_array, and EVTCHNOP_set_priority) to
> > the guest.
> >
> > These operations were added to the public header in commit d2d50c2f308f
> > ("evtchn: add FIFO-based event channel ABI") and the first implementation
> > appeared in the two subsequent commits: edc8872aeb4a ("evtchn: implement
> > EVTCHNOP_set_priority and add the set_priority hook") and 88910061ec61
> > ("evtchn: add FIFO-based event channel hypercalls and port ops"). Prior to
> > that, a guest issuing those operations would receive a return value of
> > -ENOSYS (not implemented) from Xen. Guests aware of the FIFO operations but
> > running on an older (pre-4.4) Xen would fall back to using the 2-level event
> > channel interface upon seeing this return value.
> >
> > Unfortunately the uncontrolable appearance of these new operations in Xen 
> > 4.4
> > onwards has implications for hibernation of some Linux guests. During resume
> > from hibernation, there are two kernels involved: the "boot" kernel and the
> > "resume" kernel. The guest boot kernel may default to use FIFO operations 
> > and
> > instruct Xen via EVTCHNOP_init_control to switch from 2-level to FIFO. On 
> > the
> > other hand, the resume kernel keeps assuming 2-level, because it was 
> > hibernated
> > on a version of Xen that did not support the FIFO operations.
> 
> And the alternative of the boot kernel issuing EVTCHNOP_reset has
> other unwanted consequences. Maybe worth mentioning here, as
> otherwise this would look like the obvious way to return to 2-level
> mode?
> 
> Also, why can't the boot kernel be instructed to avoid engaging
> FIFO mode?
> 

Both of those are, of course, viable alternatives if the guest can be modified. 
The problem we need to work around is guest that are already out there and 
cannot be updated.

> > To maintain compatibility it is necessary to make Xen behave as it did
> > before the new operations were added and hence the code in this patch 
> > ensures
> > that, if XEN_DOMCTL_CDF_disable_fifo is set, the FIFO event channel 
> > operations
> > will again result in -ENOSYS being returned to the guest.
> 
> Are there indeed dependencies on the precise return value anywhere?
> If so, the generally inappropriate use (do_event_channel_op()'s
> default case really would also need switching) would want a brief
> comment, so it'll be understood by readers that this isn't code to
> derive other code from. If not, -EPERM or -EACCES perhaps?
> 

The patch, as stated, is reverting behaviour and so the -ENOSYS really needs to 
stay since it is essentially ABI now. I am not aware of guest code that will, 
in fact, die if it sees -EPERM or -EACCES instead but there may be such code. 
The only safe thing to do is to make things look like the used to.

> Also, now that we gain a runtime control, do we perhaps also want a
> build time one?

Yes, a Kconfig flag to compile out the code seems like a worthy addition. I'll 
spin up a follow-up patch as soon as I get time.

  Paul

> 
> > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> > Signed-off-by: Eslam Elnikety <elnikety@xxxxxxxxxx>
> 
> Are this order as well as the From: tag above correct? Or
> alternatively, are there actually any pieces left at all from
> Eslam's earlier patch?
> 
> > v4:
> >  - New in v4
> 
> (Just as an aside: That's quite interesting for a previously
> standalone patch. I guess that patch was really split, considering
> you've retained Eslam's S-o-b? But perhaps there are different ways
> to look at things ...)
> 
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -70,9 +70,11 @@ struct xen_domctl_createdomain {
> >  #define XEN_DOMCTL_CDF_iommu          (1U<<_XEN_DOMCTL_CDF_iommu)
> >  #define _XEN_DOMCTL_CDF_nested_virt   6
> >  #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
> > +#define _XEN_DOMCTL_CDF_disable_fifo  7
> > +#define XEN_DOMCTL_CDF_disable_fifo   (1U << _XEN_DOMCTL_CDF_disable_fifo)
> 
> Despite getting longish, I think this needs "evtchn" somewhere in
> the name. To keep size bounded, maybe XEN_DOMCTL_CDF_no_fifo_evtchn?
> 
> >  /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
> > -#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_nested_virt
> > +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_disable_fifo
> 
> While not directly related to this patch, I'm puzzled by the
> presence of this constant: I've not been able to find any use of
> it. In particular you did have a need to modify
> sanitise_domain_config().
> 
> Jan