Re: [Xen-devel] [PATCH v3 4/6] xen: introduce XEN_GUEST_HANDLE_PARAM

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.08.12 at 16:50, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> 
>>> wrote:
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -51,18 +51,34 @@
>  
>  #define XEN_HYPERCALL_TAG   0XEA1
>  
> +#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
>  
>  #ifndef __ASSEMBLY__
> -#define ___DEFINE_XEN_GUEST_HANDLE(name, type) \
> -    typedef struct { type *p; } __guest_handle_ ## name
> +#define ___DEFINE_XEN_GUEST_HANDLE(name, type)                  \
> +    typedef struct { type *p; }                                 \
> +        __guest_handle_ ## name;                                \
> +    typedef struct { union { type *p; uint64_aligned_t q; }; }  \
> +        __guest_handle_64_ ## name;

Why struct { union { ... } } instead of just union { ... }?

>  
> +/*
> + * XEN_GUEST_HANDLE represents a guest pointer, when passed as a field
> + * in a struct in memory. On ARM is always 8 bytes sizes and 8 bytes
> + * aligned.
> + * XEN_GUEST_HANDLE_PARAM represent a guest pointer, when passed as an
> + * hypercall argument. It is 4 bytes on aarch and 8 bytes on aarch64.
> + */
>  #define __DEFINE_XEN_GUEST_HANDLE(name, type) \
>      ___DEFINE_XEN_GUEST_HANDLE(name, type);   \
>      ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type)
>  #define DEFINE_XEN_GUEST_HANDLE(name)   __DEFINE_XEN_GUEST_HANDLE(name, name)
> -#define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
> +#define __XEN_GUEST_HANDLE(name)        __guest_handle_64_ ## name
>  #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
> -#define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = val; } while (0)
> +/* this is going to be changes on 64 bit */
> +#define XEN_GUEST_HANDLE_PARAM(name)    __guest_handle_ ## name
> +#define set_xen_guest_handle_raw(hnd, val)                  \
> +    do { if ( sizeof(hnd) == 8 ) *(uint64_t *)&(hnd) = 0;   \

If you made the "normal" handle a union too, you could avoid
the explicit cast (which e.g. gcc, when not passed
-fno-strict-aliasing, will choke on) and instead use (hnd).q (and
at once avoid the double initialization of the low half).

Also, the condition to do this could be "sizeof(hnd) > sizeof((hnd).p)",
usable at once for 64-bit avoiding a full double initialization there.

> +         (hnd).p = val;                                     \

In a public header you certainly want to avoid evaluating a
macro argument twice.

> +    } while ( 0 )
>  #ifdef __XEN_TOOLS__
>  #define get_xen_guest_handle(val, hnd)  do { val = (hnd).p; } while (0)
>  #endif

Seeing the patch I btw realized that there's no easy way to
avoid having the type as a second argument in the conversion
macros. Nevertheless I still don't like the explicitly specified type
there.

Jan


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