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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 16 Aug 2012, Jan Beulich wrote:
> >>> 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 { ... }?

good point :)


> > +/*
> > + * 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.

That's a really good suggestion.
I am going to make both handles unions and therefore
set_xen_guest_handle_raw becomes:

#define set_xen_guest_handle_raw(hnd, val)                  \
    do { (hnd).q = 0;                                       \
         (hnd).p = val;                                     \
    } while ( 0 )


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

I agree, I don't like it either but I couldn't find anything better.

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