| Message ID | 20200728101529.13753-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | public/domctl: Fix the struct xen_domctl ABI in 32bit builds | expand |
Hi Andrew, On 28/07/2020 11:15, Andrew Cooper wrote: > The Xen domctl ABI currently relies on the union containing a field with > alignment of 8. > > 32bit projects which only copy the used subset of functionality end up with an > ABI breakage if they don't have at least one uint64_aligned_t field copied. > > Insert explicit padding, and some build assertions to ensure it never changes > moving forwards. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
On 28.07.2020 12:15, Andrew Cooper wrote: > The Xen domctl ABI currently relies on the union containing a field with > alignment of 8. > > 32bit projects which only copy the used subset of functionality end up with an > ABI breakage if they don't have at least one uint64_aligned_t field copied. I'm entirely find with the change, but I'm struggling with this justification: How can _any_ change to the header in our tree help people using custom derivations (i.e. including those defining only some of the union members)? > Further proof that C isn't an appropriate way to desribe an ABI... Further proof that it requires more careful writing in C to serve as an ABI description, I would say. > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -959,6 +959,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > return ret; > } > > +static void __init __maybe_unused build_assertions(void) > +{ > + struct xen_domctl d; Doesn't this also need __maybe_unused? Afaik ... > + BUILD_BUG_ON(sizeof(d) != 16 /* header */ + 128 /* union */); > + BUILD_BUG_ON(offsetof(typeof(d), u) != 16); > +} ... neither sizeof() nor typeof() count as actual uses. Jan
diff --git a/xen/common/domctl.c b/xen/common/domctl.c index a69b3b59a8..20ef8399bd 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -959,6 +959,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) return ret; } +static void __init __maybe_unused build_assertions(void) +{ + struct xen_domctl d; + + BUILD_BUG_ON(sizeof(d) != 16 /* header */ + 128 /* union */); + BUILD_BUG_ON(offsetof(typeof(d), u) != 16); +} + /* * Local variables: * mode: C diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 59bdc28c89..9464a9058a 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1222,6 +1222,7 @@ struct xen_domctl { #define XEN_DOMCTL_gdbsx_domstatus 1003 uint32_t interface_version; /* XEN_DOMCTL_INTERFACE_VERSION */ domid_t domain; + uint16_t _pad[3]; union { struct xen_domctl_createdomain createdomain; struct xen_domctl_getdomaininfo getdomaininfo;
The Xen domctl ABI currently relies on the union containing a field with alignment of 8. 32bit projects which only copy the used subset of functionality end up with an ABI breakage if they don't have at least one uint64_aligned_t field copied. Insert explicit padding, and some build assertions to ensure it never changes moving forwards. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <ian.jackson@citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> Further proof that C isn't an appropriate way to desribe an ABI... --- xen/common/domctl.c | 8 ++++++++ xen/include/public/domctl.h | 1 + 2 files changed, 9 insertions(+)