On 03/12/13 12:05, Ian Campbell wrote: > On Tue, 2013-12-03 at 11:57 +0100, Roger Pau Monne wrote: >> Using __packed__ on the public interface is not correct, this >> structures should be compiled using the native ABI, and __packed__ >> should only be used in the backend counterpart of those structures >> (which needs to handle different ABIs). >> >> This was even worse in the ARM case, where the Linux kernel was >> incorrectly using the X86_32 protocol ABI. This patch fixes it, but >> also breaks compatibility, so an ARM DomU kernel compiled with >> this patch will fail to communicate with PV disk devices unless the >> Dom0 also has this patch. > > This is acceptable IMHO, the ARM ABI is clearly defined and previous > kernels were simply buggy. The fact that front and backend were > equivalently buggy and so it happened to work is not an excuse. > >> Signed-off-by: Roger Pau Monné <[email protected]> >> Reported-by: Julien Grall <[email protected]> >> Cc: Julien Grall <[email protected]> >> Cc: Konrad Rzeszutek Wilk <[email protected]> >> Cc: David Vrabel <[email protected]> >> Cc: Boris Ostrovsky <[email protected]> >> Cc: Stefano Stabellini <[email protected]> >> --- >> include/xen/interface/io/blkif.h | 28 +++++++--------------------- >> 1 files changed, 7 insertions(+), 21 deletions(-) >> >> diff --git a/include/xen/interface/io/blkif.h >> b/include/xen/interface/io/blkif.h >> index 65e1209..002ea22 100644 >> --- a/include/xen/interface/io/blkif.h >> +++ b/include/xen/interface/io/blkif.h >> @@ -141,14 +141,11 @@ struct blkif_request_segment_aligned { >> /* @last_sect: last sector in frame to transfer (inclusive). */ >> uint8_t first_sect, last_sect; >> uint16_t _pad; /* padding to make it 8 bytes, so it's cache-aligned >> */ >> -} __attribute__((__packed__)); >> +}; >> >> struct blkif_request_rw { >> uint8_t nr_segments; /* number of segments */ >> blkif_vdev_t handle; /* only for read/write requests */ >> -#ifdef CONFIG_X86_64 >> - uint32_t _pad1; /* offsetof(blkif_request,u.rw.id) == 8 */ >> -#endif > > These padding fields would still serve a purpose even after removing the > packing, which is to document/clarify where there are holes for various > architectures. They could either be retained or perhaps replaced by a > comment?
Those paddings are already present in drivers/block/xen-blkback/common.h for each of the different ABIs, which I think is enough, but if I had to, I would rather replace them with comments. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
