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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.11.2020 12:10, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 25 November 2020 09:20
>>
>> 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.

Making use of EVTCHNOP_reset indeed would require a change to the
kernel. But Linux has a command line option to suppress use of
FIFO event channels, so I can't see why the boot kernel couldn't
be passed this flag without any modification to the binary.

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

I don't think specific error codes can be considered "ABI". Not
the least because, if there are multiple causes for an error, it
ought to be undefined which error gets returned. A guest not
falling back to 2-level on _any_ error here is basically setting
itself up for eventual failure because of e.g. getting back
-ENOMEM. Or someone deciding to add an XSM check to the function.

As said, I'm of the opinion that the other -ENOSYS ought to be
replaced as well, which of course would be precluded if this was
considered "ABI".

Jan