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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-11-03 at 14:01 +0000, Julien Grall wrote:
> 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.

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.

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
b) adding something to the non-public Xen side which consumes such
structures. After all any struct in the Xen API ought to be used somewhere
I suppose.

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

Of course, I somehow convinced myself otherwise despite thinking it was
odd...

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

Ah, yes. I was thinking the read was of q not p. Having the Xen side use
type check would indeed be odd (even considering it is indirect via the
access stuff most of the time).

Perhaps the uint could be "bits" or "raw" and the actual pointer ptr, or
tbh "p" probably suffices in this case.

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

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.

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.

Ian.

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