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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

Jan