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

[Ian Campbell <ian.campbell@citrix.com>]

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.

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.

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

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

> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-
> arm.h
> index 9a96401..e676ffb 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -189,11 +189,12 @@
>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_64_ ## name
>  #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
>  #define XEN_GUEST_HANDLE_PARAM(name)    __guest_handle_ ## name
> +#define barrier()     __asm__ __volatile__("": : :"memory")
>  #define set_xen_guest_handle_raw(hnd, val)                  \
>      do {                                                    \
> -        typeof(&(hnd)) _sxghr_tmp = &(hnd);                 \
> -        _sxghr_tmp->q = 0;                                  \
> -        _sxghr_tmp->p = val;                                \
> +        *((uint64_aligned_t *)&(hnd)) = 0;                  \
> +        barrier();                                          \
> +        (hnd).p = val;                                      \
>      } while ( 0 )
>  #ifdef __XEN_TOOLS__
>  #define get_xen_guest_handle(val, hnd)  do { val = (hnd).p; } while (0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel