Re: [PATCH for-4.15 v2] xen/dmop: Strip __XEN_TOOLS__ header guard from public API

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/03/2021 08:27, Jan Beulich wrote:
> On 10.03.2021 18:22, Andrew Cooper wrote:
>> On 10/03/2021 17:12, Jan Beulich wrote:
>>> On 10.03.2021 16:07, Andrew Cooper wrote:
>>>> --- a/docs/designs/dmop.pandoc
>>>> +++ b/docs/designs/dmop.pandoc
>>>> @@ -4,9 +4,13 @@ DMOP
>>>>  Introduction
>>>>  ------------
>>>>  
>>>> -The aim of DMOP is to prevent a compromised device model from compromising
>>>> -domains other than the one it is providing emulation for (which is 
>>>> therefore
>>>> -likely already compromised).
>>>> +The DMOP hypercall has a new ABI design to solve problems in the Xen
>>>> +ecosystem.  First, the ABI is fully stable, to reduce the coupling between
>>>> +device models and the version of Xen.
>>>> +
>>>> +Secondly, for device models in userspace, the ABI is designed 
>>>> specifically to
>>>> +allow a kernel to audit the memory ranges used, without having to know the
>>>> +internal structure of sub-ops.
>>> I appreciate the editing, but the cited points still don't justify ...
>>>
>>>> --- a/xen/include/public/hvm/dm_op.h
>>>> +++ b/xen/include/public/hvm/dm_op.h
>>>> @@ -25,9 +25,6 @@
>>>>  #define __XEN_PUBLIC_HVM_DM_OP_H__
>>>>  
>>>>  #include "../xen.h"
>>>> -
>>>> -#if defined(__XEN__) || defined(__XEN_TOOLS__)
>>>> -
>>>>  #include "../event_channel.h"
>>>>  
>>>>  #ifndef uint64_aligned_t
>>>> @@ -491,8 +488,6 @@ struct xen_dm_op {
>>>>      } u;
>>>>  };
>>>>  
>>>> -#endif /* __XEN__ || __XEN_TOOLS__ */
>>>> -
>>>>  struct xen_dm_op_buf {
>>>>      XEN_GUEST_HANDLE(void) h;
>>>>      xen_ulong_t size;
>>> ... this removal: What the kernel needs for its auditing (your 2nd
>>> point) is already outside of this guarded region, as you can see
>>> from the context above. You said there was a design goal of allowing
>>> use of this interface by (and not only through) the kernel, e.g. by
>>> a kernel module (which I don't think you mean to be covered by
>>> "device models"). If that was indeed a goal (Paul - can you confirm
>>> this?), this would now want listing as a 3rd item. Without such a
>>> statement I'd call it a bug to not have the guards there, and hence
>>> might either feel tempted myself to add them back at some point, or
>>> would ack a patch doing so.
>> Xen has absolutely no business dictating stuff like this.
> Actually there's no "dictating" here anyway: People are free to clone
> the headers and omit the guards anyway.

That is somewhere between busywork and just plain rude to 3rd parties. 
"here are some headers but you need to unbreak them locally before use".

> Outside of our own build
> system they really mainly serve a documentation purpose.

Header guards are not documentation - they are active attempt to
segregate hypercalls by "who we think is supposed to use them".

MiniOS, which uses this header within our build system, is not a part of
the toolstack, and should not need to define __XEN_TOOLS__ to be able to
use stable-ABI hypercalls.

Equally, the fact that libxendevicemodel's private.h needed to define
__XEN_TOOLS__ demonstrates the problem - dm_op.h (the structs and
defines - not just the types) are necessary to use the ioctl() on
/dev/xen/devicemodel in the first place.

>>   It is an
>> internal and opaque property of the domain if the hypercalls happen to
>> originate from logic in user mode or kernel mode.  Stubdomains would
>> fall into the same "kernel" category as xengt in the dom0 i915 driver.
>>
>> However, the actual bug I'm trying to fix with this is the need for
>> userspace, which doesn't link against libxc, to do
>>
>> #define __XEN_TOOLS__
>> #include <xendevicemodel.h>
>>
>> to be able to use the libxendevicemodel stable library.
>>
>> The __XEN_TOOLS__ guard is buggy even ignoring the kernel device model
>> aspect.
> Depends on what __XEN_TOOLS__ really means - to guard things accessible
> to any part of the tool stack, or to guard unstable interfaces only.

As far as I'm concerned, __XEN_TOOLS__ should always have been spelled
__XEN_UNSTABLE_ABI__.

For better or worse, the fact that we currently do have unstable
interfaces, which aren't in an obvious namespace such as
xen/public/unstable/, means that there is some value in some form of
protection to prevent users from inadvertently using an interface which
will explode on them with a mismatched hypervisor.

>From numerous complains and problems from donwstreams, we are
deliberately taking steps to stabilise the interfaces, specifically to
decouple the software in domains from the version of the hypervisor.

dmop was the first serious crack at this, and all these patches from me
are literally trying to get https://github.com/xapi-project/varstored
(which is a trivial IO port device model for UEFI secure variables) onto
fully stable hypercalls, so it doesn't need rebuilding simply because
Xen changed.

Relatedly, xenstored.  At the moment, xenstored (which requires one bit
of information per domain via its final unstable hypercall) breaks
whenever the domctl and sysctl interface versions change, despite the
fact that the ABI hasn't actually changed in years and years AFAICT.

xenstored is arguably classified as "toolstack", while varstored (or its
stubdomain equivalent) is not.  Neither should be forced to jump through
hoops to see stable API/ABIs, even after they've included the
appropriate headers.

~Andrew