[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw
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@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |