Re: [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw

[Julien Grall <julien.grall@citrix.com>]

Hi,


On 03/11/15 14:34, Ian Campbell wrote:
> On Tue, 2015-11-03 at 14:01 +0000, Julien Grall wrote:
>> On 03/11/15 12:35, Ian Campbell wrote:
>>> On Mon, 2015-11-02 at 15:55 +0000, Stefano Stabellini wrote:
>>>>
>>>>> +/*
>>>>> + * Macro to set a guest pointer in the handle.
>>>>> + *
>>>>> + * Note that it's not possible to implement safely a macro to
>>>>> retrieve the
>>>>> + * handle unless the guest is built with strict aliasing
>>>>> disabling.
>>>>> + * Hence, we don't provide a such macro in the public headers.
>>>>> + */
>>>>> +#define set_xen_guest_handle_raw(hnd,
>>>>> val)                              \
>>>>> +    do
>>>>> {                                                                \
>>>>> +        /* Check if the handle is 64-bit (i.e 8-byte)
>>>>> */                \
>>>>> +        (void) sizeof(struct { int : -!!(sizeof (hnd) != 8);
>>>>> });        \
>>>>> +        /* Check if the type of val is compatible with the handle
>>>>> */    \
>>>>> +        (void) sizeof((val) !=
>>>>> (hnd).p);                                \
>>>>> +        (hnd).q =
>>>>> (uint64_t)(uintptr_t)(val);                           \
>>>>>      } while ( 0 )
>>>>
>>>> Honestly I would be OK with having a typeof in the public headers to
>>>> avoid this code, which is much harder to follow.
>>>
>>> I suppose your objection is to two things:
>>>
>>> +        /* Check if the handle is 64-bit (i.e 8-byte)
>>> */                \
>>> +        (void) sizeof(struct { int : -!!(sizeof (hnd) != 8);
>>> });        \
>>>
>>> and
>>>
>>> +        /* Check if the type of val is compatible with the handle
>>> */    \
>>> +        (void) sizeof((val) !=
>>> (hnd).p);                                \
>>>
>>> The first is really just an open coding of BUILD_BUG_ON, I suppose for
>>> some
>>> reason BUILD_BUG_ON cannot just be used here (I assume because this is
>>> itself a macro).
>>>
>>> Personally I think a comment referring back to BUILD_BUG_ON e.g.:
>>>     /* BUILD_BUG_ON(sizeof(hnd) != 8); Cannot use real B_B_O in a macro
>>> */
>>> would be sufficient.
>>
>> You could use BUILD_BUG_ON in a macro, but this is part of the public
>> interface and I don't think we should require the guest/toolstack to
>> provide a BUILD_BUG_ON macro.
> 
> Ah, yes, that makes more sense.
> 
> we could:
> #ifdef(__XEN__)
> #define XEN_BUILD_BUG_ON(x) BUILD_BUG_ON(x)
> #elif !defined(XEN_BUILD_BUG_ON)
> #define XEN_BUILD_BUG_ON(x)
> #endif
> and using XEN_BUILD_BUG_ON in the macro
> 
> So, __XEN__ builds get the check and users can opt in by providing
> XEN_BUILD_BUG_ON if they want. If they don't then they don't get the check.

I wouldn't let the user the choice to disable build-time check. There is
no harm to open-code them as I did today and avoid possible issue in the
code later.

> Or maybe we could just omit this from the public API by one or both of a)
> adding an explicit 8 byte type to the union purely to force the size and/or

This is already done in the current version:

    typedef union { type *p; uint64_aligned_t q; }              \
        __guest_handle_64_ ## name;

However I don't see how this ensure that the caller of
set_xen_raw_guest_handle will effectively hnd as a pointer to an 8-byte
placeholder.


> b) adding something to the non-public Xen side which consumes such
> structures. After all any struct in the Xen API ought to be used somewhere
> I suppose.
> 

[...]

>>> Also given this new usage I think it
>>> would be worth renaming p and q to something less opaque, value and
>>> type_check or something would be fine IMHO.
>>
>> I guess you mean replacing "p" by "type_check" and "q" by value. If so,
>> I disagree with that because we have to use the actual "p" within Xen in
>> order to get the guest pointer and have type safety. It would be odd to
>> return "type_check".
> 
> Ah, yes. I was thinking the read was of q not p. Having the Xen side use
> type check would indeed be odd (even considering it is indirect via the
> access stuff most of the time).
> 
> Perhaps the uint could be "bits" or "raw" and the actual pointer ptr, or
> tbh "p" probably suffices in this case.
> 
>> I didn't change the names because I wasn't able to find better ones that
>> could fit for the 2 usages.
>>
>>>
>>>>  Why don't we do something like the following:
>>>
>>> Apart from Jan's comment about __asm__ and a question I have about
>>> whether
>>> it isn't even needed, how certain are you that this doesn't violate any
>>> of
>>> the C aliasing rules etc?
>>>
>>> BTW, Julien, I think it would be fine to also make this macro differ
>>> for
>>> arm32 and arm64, since the arm64 variant would then surely be simpler
>>> and
>>> the arm32 one might (or might not) be.
>>
>> I agree that the macro could be simpler (only a single line). However I
>> didn't want to differ because there is no advantage other than have a
>> good looking code for the arm64 bits. It's just add extra code to take
>> care.
> 
> Would the arm32 case be simplified since it would be a regular struct with
> 4 bytes padding and 4 bytes pointer, which can both be easily set in the
> macro. If that works then the simplicity would seem to justify having the
> two subarches use different cases here.

One of my first idea was to use a structure with 2 fields (4 bytes
padding + 4 bytes pointer) on arm32. But it's not possible to assign in
a single expression all the fields as we lack of the type within the macro:

test.c:14:9: error: expected expression before ‘{’ token
     b = { .a = 0, .b = 0 };

> This looses out on the arm32 hypervisor sanity checking that the padding
> bytes are 0 (as required by the ABI) but TBH I haven't checked that the
> current version has that property either.

It's done during the assignation by the compiler:

(hnd).q = (uint64_t)(uintptr_t)(val);

Regards,

-- 
Julien Grall

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