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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.01.17 at 13:03, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/01/17 16:29, Jan Beulich wrote:
>>>>> On 12.01.17 at 17:10, <Paul.Durrant@xxxxxxxxxx> 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 like, without requiring a
translation layer anywhere.

Jan

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