docs/designs/dmop.pandoc | 10 +++++++--- tools/libs/devicemodel/private.h | 2 -- xen/include/public/hvm/dm_op.h | 5 ----- 3 files changed, 7 insertions(+), 10 deletions(-)
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. Fix the dmop design doc to discuss both reasons
behind the the ABI design.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
---
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>
v2:
* Patch dmop.pandoc as well.
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.
---
docs/designs/dmop.pandoc | 10 +++++++---
tools/libs/devicemodel/private.h | 2 --
xen/include/public/hvm/dm_op.h | 5 -----
3 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/docs/designs/dmop.pandoc b/docs/designs/dmop.pandoc
index 8e9f95af47..336be64397 100644
--- 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.
The problem occurs when you a device model issues an hypercall that
includes references to user memory other than the operation structure
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;
--
2.11.0
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.
Jan
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. 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.
~Andrew
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
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
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@suse.com>
I still would appreciate if you also added the kernel (module) aspect to
the doc change.
Jan
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@suse.com>
>
> 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
On 11.03.2021 14:00, Andrew Cooper wrote: > 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? I was indeed wondering. > AFAICT, it is > only the ioserverid_t typedef which is API shared between the two > contexts, and we can trivially typedef that locally. Hmm, a local typedef isn't nice - there should be one central point. Granted there's no risk for this to change in anywhere halfway foreseeable future, but still. Also neither C89 nor C99 allow a typedef to be repeated - in those versions we'd then rely on an extension. > This is something which we should either do now, or not at all. Well, yes, would be better. Jan
On 11/03/2021 13:28, Jan Beulich wrote: > On 11.03.2021 14:00, Andrew Cooper wrote: >> 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? > I was indeed wondering. > >> AFAICT, it is >> only the ioserverid_t typedef which is API shared between the two >> contexts, and we can trivially typedef that locally. > Hmm, a local typedef isn't nice - there should be one central point. > Granted there's no risk for this to change in anywhere halfway > foreseeable future, but still. Also neither C89 nor C99 allow a > typedef to be repeated - in those versions we'd then rely on an > extension. I wonder if we're depending on that extension elsewhere. As far as the stable libraries go, we are dependent on a Linux or BSD environment currently. Alternatively we can drop the typedef and use uint16_t instead without breaking things in practice. (As long as we make the change in 4.15 and we lose the wiggle room afforded us by the entire interface being behind __XEN_TOOLS__ previously). Thoughts? I can't think of any ifdefary which would help, and swapping to uint16_t would reduce the use of an improperly namespaced identifier. ~Andrew
On 11.03.2021 14:43, Andrew Cooper wrote: > On 11/03/2021 13:28, Jan Beulich wrote: >> On 11.03.2021 14:00, Andrew Cooper wrote: >>> 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? >> I was indeed wondering. >> >>> AFAICT, it is >>> only the ioserverid_t typedef which is API shared between the two >>> contexts, and we can trivially typedef that locally. >> Hmm, a local typedef isn't nice - there should be one central point. >> Granted there's no risk for this to change in anywhere halfway >> foreseeable future, but still. Also neither C89 nor C99 allow a >> typedef to be repeated - in those versions we'd then rely on an >> extension. > > I wonder if we're depending on that extension elsewhere. As far as the > stable libraries go, we are dependent on a Linux or BSD environment > currently. Right, but we'd like the headers to be consumable by any environment. > Alternatively we can drop the typedef and use uint16_t instead without > breaking things in practice. (As long as we make the change in 4.15 and > we lose the wiggle room afforded us by the entire interface being behind > __XEN_TOOLS__ previously). > > Thoughts? I can't think of any ifdefary which would help, and swapping > to uint16_t would reduce the use of an improperly namespaced identifier. I'm not outright against, but this might inspire people to use uint16_t elsewhere too, when they should use the typedef. How about a transient #define (suitably commented)? Jan
On 11/03/2021 14:44, Jan Beulich wrote: > On 11.03.2021 14:43, Andrew Cooper wrote: >> On 11/03/2021 13:28, Jan Beulich wrote: >>> On 11.03.2021 14:00, Andrew Cooper wrote: >>>> 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? >>> I was indeed wondering. >>> >>>> AFAICT, it is >>>> only the ioserverid_t typedef which is API shared between the two >>>> contexts, and we can trivially typedef that locally. >>> Hmm, a local typedef isn't nice - there should be one central point. >>> Granted there's no risk for this to change in anywhere halfway >>> foreseeable future, but still. Also neither C89 nor C99 allow a >>> typedef to be repeated - in those versions we'd then rely on an >>> extension. >> I wonder if we're depending on that extension elsewhere. As far as the >> stable libraries go, we are dependent on a Linux or BSD environment >> currently. > Right, but we'd like the headers to be consumable by any environment. > >> Alternatively we can drop the typedef and use uint16_t instead without >> breaking things in practice. (As long as we make the change in 4.15 and >> we lose the wiggle room afforded us by the entire interface being behind >> __XEN_TOOLS__ previously). >> >> Thoughts? I can't think of any ifdefary which would help, and swapping >> to uint16_t would reduce the use of an improperly namespaced identifier. > I'm not outright against, but this might inspire people to use > uint16_t elsewhere too, when they should use the typedef. How > about a transient #define (suitably commented)? Having actually experimented, it is more complicated. /local/xen.git/tools/libs/devicemodel/../../../tools/include/xendevicemodel.h:283:45: error: array type has incomplete element type ‘struct xen_dm_op_modified_memory_extent’ struct xen_dm_op_modified_memory_extent extents[], uint32_t nr); so the dm_op include is necessary to use the libxendevicemodel API. Also hvmmem_type_t and evtchn_port_t get used. ~Andrew
On Thu, Mar 11, 2021 at 01:43:05PM +0000, Andrew Cooper wrote: > On 11/03/2021 13:28, Jan Beulich wrote: > > On 11.03.2021 14:00, Andrew Cooper wrote: > >> 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? FTR, this is xendevicemodel.h. Just saying because it took me a bit to find the header. I'm dense today. > > I was indeed wondering. > > > >> AFAICT, it is > >> only the ioserverid_t typedef which is API shared between the two > >> contexts, and we can trivially typedef that locally. > > Hmm, a local typedef isn't nice - there should be one central point. > > Granted there's no risk for this to change in anywhere halfway > > foreseeable future, but still. Also neither C89 nor C99 allow a > > typedef to be repeated - in those versions we'd then rely on an > > extension. > > I wonder if we're depending on that extension elsewhere. As far as the > stable libraries go, we are dependent on a Linux or BSD environment > currently. > > Alternatively we can drop the typedef and use uint16_t instead without > breaking things in practice. (As long as we make the change in 4.15 and > we lose the wiggle room afforded us by the entire interface being behind > __XEN_TOOLS__ previously). > > Thoughts? I can't think of any ifdefary which would help, and swapping > to uint16_t would reduce the use of an improperly namespaced identifier. I don't see much problem in switching to uint16_t, it's likely what should have been used from the start in order to avoid bits of dm_op.h leaking into xendevicemodel.h. Or alternatively a new type that maps to uint16_t if we think that would be more descriptive from a header PoV: server_t or some such. At the end of day it should be an opaque handler from the caller PoV, or is it expected that the ioserverid_t obtained from xendevicemodel will be used as a parameter to other libraries? If you end up changing the type to uint16_t it might help to expand the parameter name to server_id or some such, as an id parameter with type uint16_t is kind of ambiguous. We already have some comment blocks to describe the purpose of the parameters, so I don't think it's a big deal if you also leave then as 'id'. Thanks, Roger.
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. 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.
There's imo a good reason to prevent kernel to use e.g. sysctl and
domctl. Those are unstable interfaces, yes, but still.
> 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.
Aiui to address this all that would be needed would be to move the
one typedef out of the guarded region.
Jan
Andrew Cooper writes ("[PATCH for-4.15 v2] 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. Fix the dmop design doc to discuss both reasons
> behind the the ABI design.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> ---
> 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>
>
> v2:
> * Patch dmop.pandoc as well.
>
> For 4.15. This is a build fix, even if current staging can't spot the
> breakage.
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
I am going to stop giving acks for this kind of thing fairly shorty.
Ian.
On 10/03/2021 15:14, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.15 v2] 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. Fix the dmop design doc to discuss both reasons
>> behind the the ABI design.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
>> ---
>> 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>
>>
>> v2:
>> * Patch dmop.pandoc as well.
>>
>> For 4.15. This is a build fix, even if current staging can't spot the
>> breakage.
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
>
> I am going to stop giving acks for this kind of thing fairly shorty.
Thanks, but you already release acked it. This is the requested update
including the documentation change.
~Andrew
Andrew Cooper writes ("Re: [PATCH for-4.15 v2] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> Thanks, but you already release acked it. This is the requested update
> including the documentation change.
Oh yes so I did.
Ian.
© 2016 - 2024 Red Hat, Inc.