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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. Outside of our own build
system they really mainly serve a documentation purpose.

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

Jan