Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Jan Beulich]

>>> On 17.01.17 at 16:06,  wrote:
> On 17/01/17 12:29, George Dunlap wrote:
>> On Tue, Jan 17, 2017 at 11:22 AM, Andrew Cooper
>>  wrote:
>>> On 16/01/17 16:16, Jan Beulich wrote:
>>> On 16.01.17 at 17:05,  wrote:
> On 13/01/17 12:47, Jan Beulich wrote:
>> The kernel already has to parse this structure anyway, and will know 
>> the
>> bitness of its userspace process.  We could easily (at this point)
>> require the kernel to turn it into the kernels bitness for 
>> forwarding on
>> to Xen, which covers the 32bit userspace under a 64bit kernel 
>> problem,
>> in a way which won't break the hypercall ABI when 128bit comes along.
 But that won't cover a 32-bit kernel.
>>> Yes it will.
>> How that, without a compat translation layer in Xen?
> Why shouldn't there be a compat layer?
 Because the compat layer we have is kind of ugly to maintain. Hence
 I would expect additions to it to not make the situation any better.
>>> This is because our compat handling is particularly ugly (partially
>>> because our ABI has varying-size fields at random places in the middle
>>> of structures).  Not because a compat layer is the wrong thing to do.
>>>
 And I'm not sure we really need to bother considering hypothetical
 128-bit architectures at this point in time.
>>> Because considering this case will avoid us painting ourselves into a
>>> corner.
>> Why would we consider this case here, when all other part of the
>> public interface don't do so?
> This is asking why we should continue to shoot ourselves in the foot,
> ABI wise, rather than trying to do something better.
>
> And the answer is that I'd prefer that we started fixing the problem,
> rather than making it worse.
 Okay, so 128 bit handles then. But wait, we should be prepared for
 256-bit environments to, so 256-bit handles then. But wait, ...
>>> Precisely. A fixed bit width doesn't work, and cannot work going
>>> forwards.  Using a fixed bitsize will force is to burn a hypercall
>>> number every time we want to implement this ABI at a larger bit size.
>> Are we running so low on hypercall numbers that "burning" them when
>> the dominant bit width doubles in size is going to be an issue?
> 
> There is a fixed ABI of 63 hypercalls.
> 
> This can compatibly be extend up to 255 (the amount of extra room in the
> hypercall page), but no further, as c/s 2a33551d in 2008 added:
> 
> /*
>  * Leaf 3 (0x4002)
>  * EAX: Number of hypercall transfer pages. This register is always
> guaranteed
>  *  to specify one hypercall page.
> 
> to our public ABI.

As said in the other reply - there's nothing keeping us from making
the hypervisor fill stub N such that it passes N+0x1000 as hypercall
number.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Jan Beulich]

>>> On 17.01.17 at 16:13,  wrote:
> On 17/01/17 12:42, Jan Beulich wrote:
>>
 And I'm not sure we really need to bother considering hypothetical
 128-bit architectures at this point in time.
>>> Because considering this case will avoid us painting ourselves into a
>>> corner.
>> Why would we consider this case here, when all other part of the
>> public interface don't do so?
> This is asking why we should continue to shoot ourselves in the foot,
> ABI wise, rather than trying to do something better.
>
> And the answer is that I'd prefer that we started fixing the problem,
> rather than making it worse.
 Okay, so 128 bit handles then. But wait, we should be prepared for
 256-bit environments to, so 256-bit handles then. But wait, ...
>>> Precisely. A fixed bit width doesn't work, and cannot work going
>>> forwards.  Using a fixed bitsize will force is to burn a hypercall
>>> number every time we want to implement this ABI at a larger bit size.
>> With wider machine word width the number space of hypercalls
>> widens too, so I would not be worried at all using new hypercall
>> numbers, or even wholesale new hypercall number ranges.
> 
> I will leave this to the other fork of the thread, but our hypercall
> space does extend.  It is currently fixed at an absolute maximum of 4k/32.

If we were to introduce a new range, we'd likely still fit the stubs
all in one page, just that the numbers the stubs put into whatever
the equivalent of %rax would be would then have some higher
bit set.

 Or maybe I'm simply not getting what you mean to put in place here.
>>> The interface should be in terms of void * (and where appropriate,
>>> size_t), from the guests point of view, and is what a plain
>>> GUEST_HANDLE() gives you.
>> As said - that'll further break 32-bit tool stacks on 64-bit kernels.
> 
> dmop is not a tool api.  It can only be issued by a kernel, after the
> kernel has audited the internals.

To me qemu, for which the interface is mainly being made (hence
the naming of it), is very much a tool. And fundamentally I don't
see anything wrong with using a 32-bit qemu binary.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Andrew Cooper]

On 17/01/17 12:42, Jan Beulich wrote:
>
>>> And I'm not sure we really need to bother considering hypothetical
>>> 128-bit architectures at this point in time.
>> Because considering this case will avoid us painting ourselves into a
>> corner.
> Why would we consider this case here, when all other part of the
> public interface don't do so?
 This is asking why we should continue to shoot ourselves in the foot,
 ABI wise, rather than trying to do something better.

 And the answer is that I'd prefer that we started fixing the problem,
 rather than making it worse.
>>> Okay, so 128 bit handles then. But wait, we should be prepared for
>>> 256-bit environments to, so 256-bit handles then. But wait, ...
>> Precisely. A fixed bit width doesn't work, and cannot work going
>> forwards.  Using a fixed bitsize will force is to burn a hypercall
>> number every time we want to implement this ABI at a larger bit size.
> With wider machine word width the number space of hypercalls
> widens too, so I would not be worried at all using new hypercall
> numbers, or even wholesale new hypercall number ranges.

I will leave this to the other fork of the thread, but our hypercall
space does extend.  It is currently fixed at an absolute maximum of 4k/32.

>
>>> Or maybe I'm simply not getting what you mean to put in place here.
>> The interface should be in terms of void * (and where appropriate,
>> size_t), from the guests point of view, and is what a plain
>> GUEST_HANDLE() gives you.
> As said - that'll further break 32-bit tool stacks on 64-bit kernels.

dmop is not a tool api.  It can only be issued by a kernel, after the
kernel has audited the internals.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Andrew Cooper]

On 17/01/17 12:29, George Dunlap wrote:
> On Tue, Jan 17, 2017 at 11:22 AM, Andrew Cooper
>  wrote:
>> On 16/01/17 16:16, Jan Beulich wrote:
>> On 16.01.17 at 17:05,  wrote:
 On 13/01/17 12:47, Jan Beulich wrote:
> The kernel already has to parse this structure anyway, and will know 
> the
> bitness of its userspace process.  We could easily (at this point)
> require the kernel to turn it into the kernels bitness for forwarding 
> on
> to Xen, which covers the 32bit userspace under a 64bit kernel problem,
> in a way which won't break the hypercall ABI when 128bit comes along.
>>> But that won't cover a 32-bit kernel.
>> Yes it will.
> How that, without a compat translation layer in Xen?
 Why shouldn't there be a compat layer?
>>> Because the compat layer we have is kind of ugly to maintain. Hence
>>> I would expect additions to it to not make the situation any better.
>> This is because our compat handling is particularly ugly (partially
>> because our ABI has varying-size fields at random places in the middle
>> of structures).  Not because a compat layer is the wrong thing to do.
>>
>>> And I'm not sure we really need to bother considering hypothetical
>>> 128-bit architectures at this point in time.
>> Because considering this case will avoid us painting ourselves into a
>> corner.
> Why would we consider this case here, when all other part of the
> public interface don't do so?
 This is asking why we should continue to shoot ourselves in the foot,
 ABI wise, rather than trying to do something better.

 And the answer is that I'd prefer that we started fixing the problem,
 rather than making it worse.
>>> Okay, so 128 bit handles then. But wait, we should be prepared for
>>> 256-bit environments to, so 256-bit handles then. But wait, ...
>> Precisely. A fixed bit width doesn't work, and cannot work going
>> forwards.  Using a fixed bitsize will force is to burn a hypercall
>> number every time we want to implement this ABI at a larger bit size.
> Are we running so low on hypercall numbers that "burning" them when
> the dominant bit width doubles in size is going to be an issue?

There is a fixed ABI of 63 hypercalls.

This can compatibly be extend up to 255 (the amount of extra room in the
hypercall page), but no further, as c/s 2a33551d in 2008 added:

/*
 * Leaf 3 (0x4002)
 * EAX: Number of hypercall transfer pages. This register is always
guaranteed
 *  to specify one hypercall page.

to our public ABI.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Jan Beulich]

>>> On 17.01.17 at 12:22,  wrote:
> On 16/01/17 16:16, Jan Beulich wrote:
> On 16.01.17 at 17:05,  wrote:
>>> On 13/01/17 12:47, Jan Beulich wrote:
 The kernel already has to parse this structure anyway, and will know 
 the
 bitness of its userspace process.  We could easily (at this point)
 require the kernel to turn it into the kernels bitness for forwarding 
 on
 to Xen, which covers the 32bit userspace under a 64bit kernel problem,
 in a way which won't break the hypercall ABI when 128bit comes along.
>> But that won't cover a 32-bit kernel.
> Yes it will.
 How that, without a compat translation layer in Xen?
>>> Why shouldn't there be a compat layer?
>> Because the compat layer we have is kind of ugly to maintain. Hence
>> I would expect additions to it to not make the situation any better.
> 
> This is because our compat handling is particularly ugly (partially
> because our ABI has varying-size fields at random places in the middle
> of structures).  Not because a compat layer is the wrong thing to do.

Well, I disagree. The best possible solution is no translation layers,
as they're only a source of possible extra mistakes.

>> And I'm not sure we really need to bother considering hypothetical
>> 128-bit architectures at this point in time.
> Because considering this case will avoid us painting ourselves into a
> corner.
 Why would we consider this case here, when all other part of the
 public interface don't do so?
>>> This is asking why we should continue to shoot ourselves in the foot,
>>> ABI wise, rather than trying to do something better.
>>>
>>> And the answer is that I'd prefer that we started fixing the problem,
>>> rather than making it worse.
>> Okay, so 128 bit handles then. But wait, we should be prepared for
>> 256-bit environments to, so 256-bit handles then. But wait, ...
> 
> Precisely. A fixed bit width doesn't work, and cannot work going
> forwards.  Using a fixed bitsize will force is to burn a hypercall
> number every time we want to implement this ABI at a larger bit size.

With wider machine word width the number space of hypercalls
widens too, so I would not be worried at all using new hypercall
numbers, or even wholesale new hypercall number ranges.

>> Or maybe I'm simply not getting what you mean to put in place here.
> 
> The interface should be in terms of void * (and where appropriate,
> size_t), from the guests point of view, and is what a plain
> GUEST_HANDLE() gives you.

As said - that'll further break 32-bit tool stacks on 64-bit kernels.
As much as you say we should try to avoid making it harder to
accommodate a hypothetical 128-bit architecture, I think I can
validly demand that the 32-bit tool stack on 64-bit kernel case
should not be broken further than it already is (as you say;
certain things surely work quite okay). Instead such known
brokenness should be fixed, the more that for several years
we've been "successfully" ignoring the x32 ABI with our libraries
and the public interface as a whole.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[George Dunlap]

On Tue, Jan 17, 2017 at 11:22 AM, Andrew Cooper
 wrote:
> On 16/01/17 16:16, Jan Beulich wrote:
> On 16.01.17 at 17:05,  wrote:
>>> On 13/01/17 12:47, Jan Beulich wrote:
 The kernel already has to parse this structure anyway, and will know 
 the
 bitness of its userspace process.  We could easily (at this point)
 require the kernel to turn it into the kernels bitness for forwarding 
 on
 to Xen, which covers the 32bit userspace under a 64bit kernel problem,
 in a way which won't break the hypercall ABI when 128bit comes along.
>> But that won't cover a 32-bit kernel.
> Yes it will.
 How that, without a compat translation layer in Xen?
>>> Why shouldn't there be a compat layer?
>> Because the compat layer we have is kind of ugly to maintain. Hence
>> I would expect additions to it to not make the situation any better.
>
> This is because our compat handling is particularly ugly (partially
> because our ABI has varying-size fields at random places in the middle
> of structures).  Not because a compat layer is the wrong thing to do.
>
>>
>> And I'm not sure we really need to bother considering hypothetical
>> 128-bit architectures at this point in time.
> Because considering this case will avoid us painting ourselves into a
> corner.
 Why would we consider this case here, when all other part of the
 public interface don't do so?
>>> This is asking why we should continue to shoot ourselves in the foot,
>>> ABI wise, rather than trying to do something better.
>>>
>>> And the answer is that I'd prefer that we started fixing the problem,
>>> rather than making it worse.
>> Okay, so 128 bit handles then. But wait, we should be prepared for
>> 256-bit environments to, so 256-bit handles then. But wait, ...
>
> Precisely. A fixed bit width doesn't work, and cannot work going
> forwards.  Using a fixed bitsize will force is to burn a hypercall
> number every time we want to implement this ABI at a larger bit size.

Are we running so low on hypercall numbers that "burning" them when
the dominant bit width doubles in size is going to be an issue?

Actually I don't think that it will be possible to run out of
hypercalls by duplicating them all every time the word size doubles --
as when that happens, the maximum hypercall number will exponentially
increase as well.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Andrew Cooper]

On 16/01/17 16:16, Jan Beulich wrote:
 On 16.01.17 at 17:05,  wrote:
>> On 13/01/17 12:47, Jan Beulich wrote:
>>> The kernel already has to parse this structure anyway, and will know the
>>> bitness of its userspace process.  We could easily (at this point)
>>> require the kernel to turn it into the kernels bitness for forwarding on
>>> to Xen, which covers the 32bit userspace under a 64bit kernel problem,
>>> in a way which won't break the hypercall ABI when 128bit comes along.
> But that won't cover a 32-bit kernel.
 Yes it will.
>>> How that, without a compat translation layer in Xen?
>> Why shouldn't there be a compat layer?
> Because the compat layer we have is kind of ugly to maintain. Hence
> I would expect additions to it to not make the situation any better.

This is because our compat handling is particularly ugly (partially
because our ABI has varying-size fields at random places in the middle
of structures).  Not because a compat layer is the wrong thing to do.

>
> And I'm not sure we really need to bother considering hypothetical
> 128-bit architectures at this point in time.
 Because considering this case will avoid us painting ourselves into a
 corner.
>>> Why would we consider this case here, when all other part of the
>>> public interface don't do so?
>> This is asking why we should continue to shoot ourselves in the foot,
>> ABI wise, rather than trying to do something better.
>>
>> And the answer is that I'd prefer that we started fixing the problem,
>> rather than making it worse.
> Okay, so 128 bit handles then. But wait, we should be prepared for
> 256-bit environments to, so 256-bit handles then. But wait, ...

Precisely. A fixed bit width doesn't work, and cannot work going
forwards.  Using a fixed bitsize will force is to burn a hypercall
number every time we want to implement this ABI at a larger bit size.

> Or maybe I'm simply not getting what you mean to put in place here.

The interface should be in terms of void * (and where appropriate,
size_t), from the guests point of view, and is what a plain
GUEST_HANDLE() gives you.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Jan Beulich]

>>> On 16.01.17 at 18:07,  wrote:
>  If we don't want to bake 64-bit pointers into the ABI then I guess a 
> compat layer is the only way. Guess I'll go and stare at macros until my 
> brain explodes...

Well, before you do I'd really hope for Andrew to provide us (or at
least me, if everyone else is seeing the obvious) with a concrete
example of how he wants the interface to be designed cleanly and
128-bit-ready (yet without the use of pointer types, which we've
always avoided to use in the public interface).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 16 January 2017 16:17
> To: Andrew Cooper 
> Cc: Ian Jackson ; Jennifer Herbert
> ; Paul Durrant ; Wei
> Liu ; xen-de...@lists.xenproject.org; Daniel DeGraaf
> 
> Subject: Re: [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...
> 
> >>> On 16.01.17 at 17:05,  wrote:
> > On 13/01/17 12:47, Jan Beulich wrote:
> >> The kernel already has to parse this structure anyway, and will know
> the
> >> bitness of its userspace process.  We could easily (at this point)
> >> require the kernel to turn it into the kernels bitness for forwarding
> on
> >> to Xen, which covers the 32bit userspace under a 64bit kernel
> problem,
> >> in a way which won't break the hypercall ABI when 128bit comes
> along.
>  But that won't cover a 32-bit kernel.
> >>> Yes it will.
> >> How that, without a compat translation layer in Xen?
> >
> > Why shouldn't there be a compat layer?
> 
> Because the compat layer we have is kind of ugly to maintain. Hence
> I would expect additions to it to not make the situation any better.
> 
>  And I'm not sure we really need to bother considering hypothetical
>  128-bit architectures at this point in time.
> >>> Because considering this case will avoid us painting ourselves into a
> >>> corner.
> >> Why would we consider this case here, when all other part of the
> >> public interface don't do so?
> >
> > This is asking why we should continue to shoot ourselves in the foot,
> > ABI wise, rather than trying to do something better.
> >
> > And the answer is that I'd prefer that we started fixing the problem,
> > rather than making it worse.
> 
> Okay, so 128 bit handles then. But wait, we should be prepared for
> 256-bit environments to, so 256-bit handles then. But wait, ...
> 
> Or maybe I'm simply not getting what you mean to put in place here.
> 

 If we don't want to bake 64-bit pointers into the ABI then I guess a 
compat layer is the only way. Guess I'll go and stare at macros until my brain 
explodes...

  Paul

> Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Jan Beulich]

>>> On 16.01.17 at 17:05,  wrote:
> On 13/01/17 12:47, Jan Beulich wrote:
>> The kernel already has to parse this structure anyway, and will know the
>> bitness of its userspace process.  We could easily (at this point)
>> require the kernel to turn it into the kernels bitness for forwarding on
>> to Xen, which covers the 32bit userspace under a 64bit kernel problem,
>> in a way which won't break the hypercall ABI when 128bit comes along.
 But that won't cover a 32-bit kernel.
>>> Yes it will.
>> How that, without a compat translation layer in Xen?
> 
> Why shouldn't there be a compat layer?

Because the compat layer we have is kind of ugly to maintain. Hence
I would expect additions to it to not make the situation any better.

 And I'm not sure we really need to bother considering hypothetical
 128-bit architectures at this point in time.
>>> Because considering this case will avoid us painting ourselves into a
>>> corner.
>> Why would we consider this case here, when all other part of the
>> public interface don't do so?
> 
> This is asking why we should continue to shoot ourselves in the foot,
> ABI wise, rather than trying to do something better.
> 
> And the answer is that I'd prefer that we started fixing the problem,
> rather than making it worse.

Okay, so 128 bit handles then. But wait, we should be prepared for
256-bit environments to, so 256-bit handles then. But wait, ...

Or maybe I'm simply not getting what you mean to put in place here.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Andrew Cooper]

On 13/01/17 12:47, Jan Beulich wrote:
> The kernel already has to parse this structure anyway, and will know the
> bitness of its userspace process.  We could easily (at this point)
> require the kernel to turn it into the kernels bitness for forwarding on
> to Xen, which covers the 32bit userspace under a 64bit kernel problem,
> in a way which won't break the hypercall ABI when 128bit comes along.
>>> But that won't cover a 32-bit kernel.
>> Yes it will.
> How that, without a compat translation layer in Xen?

Why shouldn't there be a compat layer?

>
>>> And I'm not sure we really need to bother considering hypothetical
>>> 128-bit architectures at this point in time.
>> Because considering this case will avoid us painting ourselves into a
>> corner.
> Why would we consider this case here, when all other part of the
> public interface don't do so?

This is asking why we should continue to shoot ourselves in the foot,
ABI wise, rather than trying to do something better.

And the answer is that I'd prefer that we started fixing the problem,
rather than making it worse.

 As discussed...

 xen_dm_op_buf is currently used directly in the tools code because there 
 is 
 no explicit support for DMOPs in privcmd. When explicit support is added 
 then 
 this can change and we can use a void ptr and size_t as you say.

 For the privcmd -> Xen interface that using XEN_GUEST_HANDLE_64 does 
 indeed 
 limit us and so using XEN_GUEST_HANDLE would indeed seem more sensible 
 (and 
 we won't need a 32-bit compat wrapper for DMOP because it's a tools-only 
 hypercall).
>>> Just like with other tools only interfaces, I think this should remain a
>>> 64-bit handle, to avoid (as you say, but wrongly in conjunction with a
>>> "normal" handle) the need for a compat wrapper.
>> I disagree.  It is a layering violation. (and apologies if this appears
>> to rant, but it seems that all major development I do for Xen is
>> removing layering violations which could have been more easily avoided
>> if v1 had been designed more sensibly.)
> Whether this is a layering violation really depends on the position
> you take: If tools only interfaces are meant to be a direct channel
> from user space to hypervisor, then the kernel has no business
> interposing a translation layer, the more that such a layer would
> need updating every time a new operation got added to that
> tools only interface.

"tools-only" is an illusion.  Hypercalls are a kernel-only operation,
and are interpreted with an ABI determined by the width of the kernel.

It is for this precise reason that a 32bit toolstack under a 64bit
kernel doesn't function correctly.

Despite privcmd's implementation being a straight "throw it over the
wall to Xen", the hypercall ABI is strictly between Xen and the kernel,
not Xen and the toolstack.

>  Even worse, the "tools only" property would
> be partially lost the moment the kernel had a valid reason to look
> at those structures.

This is an ABI which, by its very design, *must* be inspected and
audited by the kernel.  dmop cannot be a "tools interface" like
domctl/sysctl/etc.

The userspace/kernel interaction is going to be in terms of void * and
size_t's, so the kernel can sensibly audit the memory ranges.

>> The 64bit guest handle infrastructure has always been a kludge.
> It has been the better alternative to a compat translation layer
> first of all.

I disagree on calling it better.

In a hypothetical world x86_128 world, how would the current scheme
function?  How would you distinguish a piece of 128bit userspace from a
64 or 32bit userspace under a 128bit kernel?

>
>> Xen and the kernel speak an ABI, which is based on the width of the
>> kernel.  These take a GUEST_HANDLE() as a wrapper around void *, to
>> indicate the size of a pointer used by the kernel.  Under this ABI, Xen
>> is at least the width of the kernel, and must translate a smaller
>>
>> The kernel and its userspace speak an ABI which is based on the width of
>> userspace, and the kernel (for things other than privcmd) translates via
>> its compat layer.
> Which is a fuzzy thing by itself already, when you take into
> consideration processes running both 64- and 32-bit code.

It is only fuzzy because we don't have a rational layer separation to
start with.

If you really don't want the kernel inspecting the internal content of
the call (which, is a property I do agree with), then the correct ABI
would have been to wrap userspace hypercalls (multicall style)
indicating the ABI with which userspace was compiled.  This would at
least allow Xen to interpret the contents in an unambiguous manner.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Jan Beulich]

>>> On 13.01.17 at 13:03,  wrote:
> On 12/01/17 16:29, Jan Beulich wrote:
> On 12.01.17 at 17:10,  wrote:
> +
> +struct xen_dm_op_buf {
> +XEN_GUEST_HANDLE_64(void) h;
> +uint32_t size;
> +};
 Sorry to quibble, but there is a problem here which has only just
 occurred to me.  This ABI isn't futureproof, and has padding at the end
 which affects how the array is layed out.
>> Yes, padding needs to be added.
>>
 The userspace side should be

 struct xen_dm_op_buf {
 void *h;
 size_t size;
 }

 which will work sensibly for 32bit and 64bit userspace, and futureproof
 (for when 128bit turns up).  Its size is also a power of two which
 avoids alignment issues in the array.

 The kernel already has to parse this structure anyway, and will know the
 bitness of its userspace process.  We could easily (at this point)
 require the kernel to turn it into the kernels bitness for forwarding on
 to Xen, which covers the 32bit userspace under a 64bit kernel problem,
 in a way which won't break the hypercall ABI when 128bit comes along.
>> But that won't cover a 32-bit kernel.
> 
> Yes it will.

How that, without a compat translation layer in Xen?

>> And I'm not sure we really need to bother considering hypothetical
>> 128-bit architectures at this point in time.
> 
> Because considering this case will avoid us painting ourselves into a
> corner.

Why would we consider this case here, when all other part of the
public interface don't do so?

>>> As discussed...
>>>
>>> xen_dm_op_buf is currently used directly in the tools code because there is 
>>> no explicit support for DMOPs in privcmd. When explicit support is added 
>>> then 
>>> this can change and we can use a void ptr and size_t as you say.
>>>
>>> For the privcmd -> Xen interface that using XEN_GUEST_HANDLE_64 does indeed 
>>> limit us and so using XEN_GUEST_HANDLE would indeed seem more sensible (and 
>>> we won't need a 32-bit compat wrapper for DMOP because it's a tools-only 
>>> hypercall).
>> Just like with other tools only interfaces, I think this should remain a
>> 64-bit handle, to avoid (as you say, but wrongly in conjunction with a
>> "normal" handle) the need for a compat wrapper.
> 
> I disagree.  It is a layering violation. (and apologies if this appears
> to rant, but it seems that all major development I do for Xen is
> removing layering violations which could have been more easily avoided
> if v1 had been designed more sensibly.)

Whether this is a layering violation really depends on the position
you take: If tools only interfaces are meant to be a direct channel
from user space to hypervisor, then the kernel has no business
interposing a translation layer, the more that such a layer would
need updating every time a new operation got added to that
tools only interface. Even worse, the "tools only" property would
be partially lost the moment the kernel had a valid reason to look
at those structures.

> The 64bit guest handle infrastructure has always been a kludge.

It has been the better alternative to a compat translation layer
first of all.

> Xen and the kernel speak an ABI, which is based on the width of the
> kernel.  These take a GUEST_HANDLE() as a wrapper around void *, to
> indicate the size of a pointer used by the kernel.  Under this ABI, Xen
> is at least the width of the kernel, and must translate a smaller
> 
> The kernel and its userspace speak an ABI which is based on the width of
> userspace, and the kernel (for things other than privcmd) translates via
> its compat layer.

Which is a fuzzy thing by itself already, when you take into
consideration processes running both 64- and 32-bit code.

> In hindsight it is obvious to see why this problem started; at the time
> it was introduced, everything was 32bit, and a short-cut between
> userspace and Xen was taken.  In fact, its such a short-cut that any
> process with a privcmd handle can issue any hypercall, including the fun
> ones such as set_trap_table, update_va_mapping, or iret.
> 
> When 64bit support was added to Xen, this short-cut broke the
> userspace/kernel ABI expectation, because privcmd didn't actually
> translate its ABI (unlike everything else in the kernel).  This results
> in a binary compatibility problem if the toolstack is compiled at a
> width different to the kernel it is running under.

Hmm, I don't think I agree here. For one, the privcmd compat was,
afaict, lost with the transition to pv-ops. But that layer (I dare to
say "of course") was concerned only with translating its own
interface structures (i.e. the ioctl ones). The interface structures
of the actual hypercalls were opaque as you say, and (see my
remark earlier on) validly so imo.

Furthermore I'm not sure we want to make dmop inconsistent
with domctl/sysctl. I'd be curious to know how you would
envision a clean interface to look 

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Wei Liu]

On Fri, Jan 13, 2017 at 09:05:19AM +, Paul Durrant wrote:
> > -Original Message-
> > From: Jan Beulich [mailto:jbeul...@suse.com]
> > Sent: 12 January 2017 16:29
> > To: Andrew Cooper ; Paul Durrant
> > 
> > Cc: Ian Jackson ; Jennifer Herbert
> > ; Wei Liu ; xen-
> > de...@lists.xenproject.org; Daniel De Graaf 
> > Subject: RE: [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...
> > 
> > >>> On 12.01.17 at 17:10,  wrote:
> > >> > +
> > >> > +struct xen_dm_op_buf {
> > >> > +XEN_GUEST_HANDLE_64(void) h;
> > >> > +uint32_t size;
> > >> > +};
> > >>
> > >> Sorry to quibble, but there is a problem here which has only just
> > >> occurred to me.  This ABI isn't futureproof, and has padding at the end
> > >> which affects how the array is layed out.
> > 
> > Yes, padding needs to be added.
> > 
> > >> The userspace side should be
> > >>
> > >> struct xen_dm_op_buf {
> > >> void *h;
> > >> size_t size;
> > >> }
> > >>
> > >> which will work sensibly for 32bit and 64bit userspace, and futureproof
> > >> (for when 128bit turns up).  Its size is also a power of two which
> > >> avoids alignment issues in the array.
> > >>
> > >> The kernel already has to parse this structure anyway, and will know the
> > >> bitness of its userspace process.  We could easily (at this point)
> > >> require the kernel to turn it into the kernels bitness for forwarding on
> > >> to Xen, which covers the 32bit userspace under a 64bit kernel problem,
> > >> in a way which won't break the hypercall ABI when 128bit comes along.
> > 
> > But that won't cover a 32-bit kernel.
> >
> 
> Do we need to care about a 32-bit kernel for a tools-only hypercall? I 
> thought a toolstack already had to be (at least) 64-bit to match Xen.
> 

Toolstack can run on 32bit kernel on 64bit Xen.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Jan Beulich]

>>> On 13.01.17 at 10:05,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 12 January 2017 16:29
>> >>> On 12.01.17 at 17:10,  wrote:
>> >> The userspace side should be
>> >>
>> >> struct xen_dm_op_buf {
>> >> void *h;
>> >> size_t size;
>> >> }
>> >>
>> >> which will work sensibly for 32bit and 64bit userspace, and futureproof
>> >> (for when 128bit turns up).  Its size is also a power of two which
>> >> avoids alignment issues in the array.
>> >>
>> >> The kernel already has to parse this structure anyway, and will know the
>> >> bitness of its userspace process.  We could easily (at this point)
>> >> require the kernel to turn it into the kernels bitness for forwarding on
>> >> to Xen, which covers the 32bit userspace under a 64bit kernel problem,
>> >> in a way which won't break the hypercall ABI when 128bit comes along.
>> 
>> But that won't cover a 32-bit kernel.
>>
> 
> Do we need to care about a 32-bit kernel for a tools-only hypercall? I 
> thought 
> a toolstack already had to be (at least) 64-bit to match Xen.

Why would that be? Didn't XenServer for a long time run with a
32-bit Dom0 on top of a 64-bit Xen? See also domctl/sysctl.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 12 January 2017 16:29
> To: Andrew Cooper ; Paul Durrant
> 
> Cc: Ian Jackson ; Jennifer Herbert
> ; Wei Liu ; xen-
> de...@lists.xenproject.org; Daniel De Graaf 
> Subject: RE: [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...
> 
> >>> On 12.01.17 at 17:10,  wrote:
> >> > +
> >> > +struct xen_dm_op_buf {
> >> > +XEN_GUEST_HANDLE_64(void) h;
> >> > +uint32_t size;
> >> > +};
> >>
> >> Sorry to quibble, but there is a problem here which has only just
> >> occurred to me.  This ABI isn't futureproof, and has padding at the end
> >> which affects how the array is layed out.
> 
> Yes, padding needs to be added.
> 
> >> The userspace side should be
> >>
> >> struct xen_dm_op_buf {
> >> void *h;
> >> size_t size;
> >> }
> >>
> >> which will work sensibly for 32bit and 64bit userspace, and futureproof
> >> (for when 128bit turns up).  Its size is also a power of two which
> >> avoids alignment issues in the array.
> >>
> >> The kernel already has to parse this structure anyway, and will know the
> >> bitness of its userspace process.  We could easily (at this point)
> >> require the kernel to turn it into the kernels bitness for forwarding on
> >> to Xen, which covers the 32bit userspace under a 64bit kernel problem,
> >> in a way which won't break the hypercall ABI when 128bit comes along.
> 
> But that won't cover a 32-bit kernel.
>

Do we need to care about a 32-bit kernel for a tools-only hypercall? I thought 
a toolstack already had to be (at least) 64-bit to match Xen.

  Paul
 
> And I'm not sure we really need to bother considering hypothetical
> 128-bit architectures at this point in time.
> 
> > As discussed...
> >
> > xen_dm_op_buf is currently used directly in the tools code because there
> is
> > no explicit support for DMOPs in privcmd. When explicit support is added
> then
> > this can change and we can use a void ptr and size_t as you say.
> >
> > For the privcmd -> Xen interface that using XEN_GUEST_HANDLE_64 does
> indeed
> > limit us and so using XEN_GUEST_HANDLE would indeed seem more
> sensible (and
> > we won't need a 32-bit compat wrapper for DMOP because it's a tools-only
> > hypercall).
> 
> Just like with other tools only interfaces, I think this should remain a
> 64-bit handle, to avoid (as you say, but wrongly in conjunction with a
> "normal" handle) the need for a compat wrapper.
>
> Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Andrew Cooper]

On 12/01/17 14:58, Paul Durrant wrote:
> ...as a set of hypercalls to be used by a device model.
>
> As stated in the new docs/designs/dm_op.markdown:
>
> "The aim of DMOP is to prevent a compromised device model from
> compromising domains other then the one it is associated with. (And is
> therefore likely already compromised)."
>
> See that file for further information.
>
> This patch simply adds the boilerplate for the hypercall.
>
> Signed-off-by: Paul Durrant 
> Suggested-by: Ian Jackson 
> Suggested-by: Jennifer Herbert 
> ---
> Cc: Ian Jackson 
> Cc: Jennifer Herbert 
> Cc: Daniel De Graaf 
> Cc: Wei Liu 
> Reviewed-by: Jan Beulich 
> Cc: Andrew Cooper 
>
> v3:
> - Re-written large portions of dmop.markdown to remove references to
>   previous proposals and make it a standalone design doc.
>
> v2:
> - Addressed several comments from Jan.
> - Removed modification of __XEN_LATEST_INTERFACE_VERSION__ as it is not
>   needed in this patch.
> ---
>  docs/designs/dmop.markdown| 158 
> ++
>  tools/flask/policy/modules/xen.if |   2 +-
>  tools/libxc/include/xenctrl.h |   1 +
>  tools/libxc/xc_private.c  |  70 +
>  tools/libxc/xc_private.h  |   2 +
>  xen/arch/x86/hvm/Makefile |   1 +
>  xen/arch/x86/hvm/dm.c | 118 
>  xen/arch/x86/hvm/hvm.c|   1 +
>  xen/arch/x86/hypercall.c  |   2 +
>  xen/include/public/hvm/dm_op.h|  71 +
>  xen/include/public/xen.h  |   1 +
>  xen/include/xen/hypercall.h   |   7 ++
>  xen/include/xsm/dummy.h   |   6 ++
>  xen/include/xsm/xsm.h |   6 ++
>  xen/xsm/flask/hooks.c |   7 ++
>  15 files changed, 452 insertions(+), 1 deletion(-)
>  create mode 100644 docs/designs/dmop.markdown
>  create mode 100644 xen/arch/x86/hvm/dm.c
>  create mode 100644 xen/include/public/hvm/dm_op.h
>
> diff --git a/docs/designs/dmop.markdown b/docs/designs/dmop.markdown
> new file mode 100644
> index 000..2a4bd16
> --- /dev/null
> +++ b/docs/designs/dmop.markdown
> @@ -0,0 +1,158 @@
> +DMOP
> +
> +
> +Introduction
> +
> +
> +The aim of DMOP is to prevent a compromised device model from compromising
> +domains other then the one it is associated with. (And is therefore likely
> +already compromised).
> +
> +The problem occurs when you a device model issues an hypercall that
> +includes references to user memory other than the operation structure
> +itself, such as with Track dirty VRAM (as used in VGA emulation).
> +Is this case, the address of this other user memory needs to be vetted,
> +to ensure it is not within restricted address ranges, such as kernel
> +memory. The real problem comes down to how you would vet this address -
> +the idea place to do this is within the privcmd driver, without privcmd
> +having to have specific knowledge of the hypercall's semantics.
> +
> +The Design
> +--
> +
> +The privcmd driver implements a new restriction ioctl, which takes a domid
> +parameter.  After that restriction ioctl is issued, the privcmd driver will
> +permit only DMOP hypercalls, and only with the specified target domid.
> +
> +A DMOP hypercall consists of an array of buffers and lengths, with the
> +first one containing the specific DMOP parameters. These can then reference
> +further buffers from within in the array. Since the only user buffers
> +passed are that found with that array, they can all can be audited by
> +privcmd.
> +
> +The following code illustrates this idea:
> +
> +struct xen_dm_op {
> +uint32_t op;
> +};
> +
> +struct xen_dm_op_buf {
> +XEN_GUEST_HANDLE_64(void) h;
> +uint32_t size;
> +};

Sorry to quibble, but there is a problem here which has only just
occurred to me.  This ABI isn't futureproof, and has padding at the end
which affects how the array is layed out.

The userspace side should be

struct xen_dm_op_buf {
void *h;
size_t size;
}

which will work sensibly for 32bit and 64bit userspace, and futureproof
(for when 128bit turns up).  Its size is also a power of two which
avoids alignment issues in the array.

The kernel already has to parse this structure anyway, and will know the
bitness of its userspace process.  We could easily (at this point)
require the kernel to turn it into the kernels bitness for forwarding on
to Xen, which covers the 32bit userspace under a 64bit kernel problem,
in a way which won't break the hypercall ABI when 128bit comes along.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel