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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

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

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

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.

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.

The GUEST_HANDLE_64() is a hack around the problem to try and make the
ABI from different width userspace appear the same, without fixing the
underlying problem of privcmd.

~Andrew

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