diff mbox series

[2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API

Message ID 20210305124949.6719-3-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series tools/libs: Multiple fixes to header handling | expand

Commit Message

Andrew Cooper March 5, 2021, 12:49 p.m. UTC
Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.

That change actually broke the build with:

  include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
       ioservid_t *id);
       ^

as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
nothing noticed because the header.chk logic is also broken (fixed
subsequently).

Strip the guard from the public header, and remove compensation from
devicemodel's private.h

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Paul Durrant <paul@xen.org>
CC: Ian Jackson <iwj@xenproject.org>

For 4.15.  This is a build fix, even if current staging can't spot the
breakage.

These two issues highlight that libxendevcemodel.h has never been checked
since its introduction, because the checking logic only saw an empty file.
---
 tools/libs/devicemodel/private.h | 2 --
 xen/include/public/hvm/dm_op.h   | 5 -----
 2 files changed, 7 deletions(-)

Comments

Ian Jackson March 5, 2021, 1:26 p.m. UTC | #1
Andrew Cooper writes ("[PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
> 
> That change actually broke the build with:
> 
>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>        ioservid_t *id);
>        ^
> 
> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
> nothing noticed because the header.chk logic is also broken (fixed
> subsequently).
> 
> Strip the guard from the public header, and remove compensation from
> devicemodel's private.h

Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Jan Beulich March 5, 2021, 1:53 p.m. UTC | #2
On 05.03.2021 13:49, Andrew Cooper wrote:
> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
> 
> That change actually broke the build with:
> 
>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>        ioservid_t *id);
>        ^
> 
> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
> nothing noticed because the header.chk logic is also broken (fixed
> subsequently).

While I agree up to here, ...

> Strip the guard from the public header, and remove compensation from
> devicemodel's private.h

... I'm unconvinced that entirely dropping the guard from the
public header is wanted (or needed): We use these to make clear
that in particular kernels aren't supposed to make use of the
enclosed entities. If a type needs exposing, it (and only it)
wants moving ou of the guarded region imo.

Jan
Andrew Cooper March 5, 2021, 2:12 p.m. UTC | #3
On 05/03/2021 13:53, Jan Beulich wrote:
> On 05.03.2021 13:49, Andrew Cooper wrote:
>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
>>
>> That change actually broke the build with:
>>
>>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>>        ioservid_t *id);
>>        ^
>>
>> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
>> nothing noticed because the header.chk logic is also broken (fixed
>> subsequently).
> While I agree up to here, ...
>
>> Strip the guard from the public header, and remove compensation from
>> devicemodel's private.h
> ... I'm unconvinced that entirely dropping the guard from the
> public header is wanted (or needed): We use these to make clear
> that in particular kernels aren't supposed to make use of the
> enclosed entities. If a type needs exposing, it (and only it)
> wants moving ou of the guarded region imo.

DMOP was invented specifically so a kernel module (i915, for Intel
gVT-g) was independent of the domctl ABI version.

Improving the life of dom0 userspace was an intended consequence, but
not the driving force behind the change.

Exactly the same is true for stubdoms currently, and I am very serious
about purging unstable interfaces eventually.

~Andrew
Jan Beulich March 5, 2021, 2:18 p.m. UTC | #4
On 05.03.2021 15:12, Andrew Cooper wrote:
> On 05/03/2021 13:53, Jan Beulich wrote:
>> On 05.03.2021 13:49, Andrew Cooper wrote:
>>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
>>>
>>> That change actually broke the build with:
>>>
>>>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>>>        ioservid_t *id);
>>>        ^
>>>
>>> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
>>> nothing noticed because the header.chk logic is also broken (fixed
>>> subsequently).
>> While I agree up to here, ...
>>
>>> Strip the guard from the public header, and remove compensation from
>>> devicemodel's private.h
>> ... I'm unconvinced that entirely dropping the guard from the
>> public header is wanted (or needed): We use these to make clear
>> that in particular kernels aren't supposed to make use of the
>> enclosed entities. If a type needs exposing, it (and only it)
>> wants moving ou of the guarded region imo.
> 
> DMOP was invented specifically so a kernel module (i915, for Intel
> gVT-g) was independent of the domctl ABI version.
> 
> Improving the life of dom0 userspace was an intended consequence, but
> not the driving force behind the change.

This is news to me - so far it had been my understanding that it
was introduced to have a way for the kernel to audit and hand on
requests to the hypervisor without needing to know all the inner
details. I wasn't even aware a kernel module was using any of
these.

Jan
Jan Beulich March 5, 2021, 2:21 p.m. UTC | #5
On 05.03.2021 15:18, Jan Beulich wrote:
> On 05.03.2021 15:12, Andrew Cooper wrote:
>> On 05/03/2021 13:53, Jan Beulich wrote:
>>> On 05.03.2021 13:49, Andrew Cooper wrote:
>>>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
>>>>
>>>> That change actually broke the build with:
>>>>
>>>>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>>>>        ioservid_t *id);
>>>>        ^
>>>>
>>>> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
>>>> nothing noticed because the header.chk logic is also broken (fixed
>>>> subsequently).
>>> While I agree up to here, ...
>>>
>>>> Strip the guard from the public header, and remove compensation from
>>>> devicemodel's private.h
>>> ... I'm unconvinced that entirely dropping the guard from the
>>> public header is wanted (or needed): We use these to make clear
>>> that in particular kernels aren't supposed to make use of the
>>> enclosed entities. If a type needs exposing, it (and only it)
>>> wants moving ou of the guarded region imo.
>>
>> DMOP was invented specifically so a kernel module (i915, for Intel
>> gVT-g) was independent of the domctl ABI version.
>>
>> Improving the life of dom0 userspace was an intended consequence, but
>> not the driving force behind the change.
> 
> This is news to me - so far it had been my understanding that it
> was introduced to have a way for the kernel to audit and hand on
> requests to the hypervisor without needing to know all the inner
> details. I wasn't even aware a kernel module was using any of
> these.

And indeed, quote from docs/designs/dmop.markdown:

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

And it goes on discussing only the purpose that I've been aware
of.

Jan
Ian Jackson March 5, 2021, 2:28 p.m. UTC | #6
Jan Beulich writes ("Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> This is news to me - so far it had been my understanding that it
> was introduced to have a way for the kernel to audit and hand on
> requests to the hypervisor without needing to know all the inner
> details. I wasn't even aware a kernel module was using any of
> these.

Quite so.

Ian.
Andrew Cooper March 5, 2021, 3:13 p.m. UTC | #7
On 05/03/2021 14:21, Jan Beulich wrote:
> On 05.03.2021 15:18, Jan Beulich wrote:
>> On 05.03.2021 15:12, Andrew Cooper wrote:
>>> On 05/03/2021 13:53, Jan Beulich wrote:
>>>> On 05.03.2021 13:49, Andrew Cooper wrote:
>>>>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
>>>>>
>>>>> That change actually broke the build with:
>>>>>
>>>>>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>>>>>        ioservid_t *id);
>>>>>        ^
>>>>>
>>>>> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
>>>>> nothing noticed because the header.chk logic is also broken (fixed
>>>>> subsequently).
>>>> While I agree up to here, ...
>>>>
>>>>> Strip the guard from the public header, and remove compensation from
>>>>> devicemodel's private.h
>>>> ... I'm unconvinced that entirely dropping the guard from the
>>>> public header is wanted (or needed): We use these to make clear
>>>> that in particular kernels aren't supposed to make use of the
>>>> enclosed entities. If a type needs exposing, it (and only it)
>>>> wants moving ou of the guarded region imo.
>>> DMOP was invented specifically so a kernel module (i915, for Intel
>>> gVT-g) was independent of the domctl ABI version.
>>>
>>> Improving the life of dom0 userspace was an intended consequence, but
>>> not the driving force behind the change.
>> This is news to me - so far it had been my understanding that it
>> was introduced to have a way for the kernel to audit and hand on
>> requests to the hypervisor without needing to know all the inner
>> details. I wasn't even aware a kernel module was using any of
>> these.
> And indeed, quote from docs/designs/dmop.markdown:
>
> "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)."
>
> And it goes on discussing only the purpose that I've been aware
> of.

The use in the dom0 kernel wasn't kept secret in the slightest.  It was
discussed on at the time, and at dev summits.

But upstream tends to only remember/care about the bits which pertain
directly to upstream, and the design particulars of the DMOP ABI were
specifically for userspace.

~Andrew
Ian Jackson March 5, 2021, 3:20 p.m. UTC | #8
Andrew Cooper writes ("Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> The use in the dom0 kernel wasn't kept secret in the slightest.  It was
> discussed on at the time, and at dev summits.

No-one is accusing anyone of keeping anything secret.

> But upstream tends to only remember/care about the bits which pertain
> directly to upstream,

I would prefer to say that upstream only tends to remember things
which are WRITTEN DOWN.

Ian.
diff mbox series

Patch

diff --git a/tools/libs/devicemodel/private.h b/tools/libs/devicemodel/private.h
index c4a225f8af..c24f3396bb 100644
--- a/tools/libs/devicemodel/private.h
+++ b/tools/libs/devicemodel/private.h
@@ -1,8 +1,6 @@ 
 #ifndef XENDEVICEMODEL_PRIVATE_H
 #define XENDEVICEMODEL_PRIVATE_H
 
-#define __XEN_TOOLS__ 1
-
 #include <xentoollog.h>
 #include <xendevicemodel.h>
 #include <xencall.h>
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index ef7fbc0d3d..fa3f083fed 100644
--- 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;