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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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

> 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