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

> For the second I think the comparison of two pointers in this as a macro
> type safety check is a common enough idiom that it should be understood.
> But I wouldn't object to a more explicit comment explaining this, or
> explaining that sizeof is necessary to not evaluate hnd a second time in
> the macro.

Will do.

> On the second though, Julien I think it needs to be (&val) since you need
> to compare the pointers to the types to trigger the compiler's "comparing
> distinct pointer types" warning/error.

No, val is already a pointer to the type (see the previous implementation).

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

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.

Regards,

-- 
Julien Grall