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 11:41, Jan Beulich wrote:
> On 11.03.2021 12:05, Andrew Cooper wrote:
>> 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
> And I didn't say so - I said they server a documentation purpose.
>
>> - 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.
> I've not been able to spot a definition of __XEN_TOOLS__ in the
> mini-os sources. Are you perhaps referring to tool stack libraries
> getting built also for it?

Things in stubdom/ which include xenctrl.h get __XEN_TOOLS__ set behind
the scenes, which is the only way that including libxendevicemodel.h
worked before last week.

>
>> 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.
> But this library _is_ part of the tool stack. Looks like it really
> boils down to ...
>
>>>>   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__.
> ... this. If you added half a sentence to this effect to the description,
> you may feel free to add
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> I still would appreciate if you also added the kernel (module) aspect to
> the doc change.

Thanks, will do, and I'll post a v3 just to check that everyone is happy.


However, having laid things out in this way today, it occurs to me that
we should consider further cleanup as well.

I do agree that code wanting to use the libxendevicemodel.h API almost
certainly don't want/need the dmop ABI.  (i.e. an individual consumer
will want one, or the other, but almost certainly not both together).

Should libxendevicemodel.h really be including dm_op.h?  AFAICT, it is
only the ioserverid_t typedef which is API shared between the two
contexts, and we can trivially typedef that locally.

This is something which we should either do now, or not at all.

~Andrew