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

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

On Wed, 2015-11-04 at 11:17 +0000, Julien Grall wrote:
> 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.

Up to you. Note that "the user" here would potentially include
xen.git/tools, and I would expect them to want to define it.

Also...

>  There is
> no harm to open-code them as I did today and avoid possible issue in the
> code later.

... there is always a downside to open coding. If you don't want to expose
the ability to define a BUG_ON to the application then just drop that #elif
from the chain.

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

Not sure I follow. If hnd isn't a suitable struct xen guest handle then
other things will fail. If a user is passing a non struct xen_guest_handle
to this which happens to contain the same fields then more fool them, and
if it happens to be 8 bytes anyway your check won't catch that.

> > 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 };

Right, that's a shame.

> 
> > 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);

I meant on the reading side.

Ian.

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