Christian Borntraeger
2008-Feb-08 05:01 UTC
[PATCH] virtio_ring: make structure defines packed
Currently the virtio_ring structure are not declared packed, but they describe an hardware like interface. We should not allow compilers to make alignments and optimizations that can be different between the guest and host compiler. I propose to declare all structures that are in shared memory as packed. Does anybody see a problem with packed? Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- include/linux/virtio_ring.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) Index: kvm/include/linux/virtio_ring.h ==================================================================--- kvm.orig/include/linux/virtio_ring.h +++ kvm/include/linux/virtio_ring.h @@ -35,14 +35,14 @@ struct vring_desc __u16 flags; /* We chain unused descriptors via this, too */ __u16 next; -}; +} __attribute__ ((packed)); struct vring_avail { __u16 flags; __u16 idx; __u16 ring[]; -}; +} __attribute__ ((packed)); /* u32 is used here for ids for padding reasons. */ struct vring_used_elem @@ -51,14 +51,15 @@ struct vring_used_elem __u32 id; /* Total length of the descriptor chain which was used (written to) */ __u32 len; -}; +} __attribute__ ((packed)); struct vring_used { __u16 flags; __u16 idx; + __u32 padding; struct vring_used_elem ring[]; -}; +} __attribute__ ((packed)); struct vring { unsigned int num;
On Friday 08 February 2008, Christian Borntraeger wrote:> Currently the virtio_ring structure are not declared packed, but they > describe an hardware like interface. We should not allow compilers to make > alignments and optimizations that can be different between the guest and > host compiler. > > I propose to declare all structures that are in shared memory as packed. > > Does anybody see a problem with packed?Packed structures can lead to unoptimized object code when the compiler cannot assume natural alingment and does all accesses in single bytes. It may even produce incorrect code if the host interface requires larger-than-byte accesses. I'm not aware of any version of gcc that introduces padding when all struct members are naturally aligned as they are in the structures you propose to change, so I'd think we're better off not having them declared as packed. Also, if someone does find a compelling reason to make them packed indeed, the way to express that now should be '__packed', not '__attribute__((packed))'. Arnd <><
Arnd Bergmann
2008-Feb-09 18:20 UTC
[kvm-devel] [PATCH] virtio_ring: make structure defines packed
On Sunday 10 February 2008, you wrote:> Christian Borntraeger wrote: > > ?struct vring_used > > ?{ > > ??????__u16 flags; > > ??????__u16 idx; > > +?????__u32 padding; > > ??????struct vring_used_elem ring[]; > > -}; > > +} __attribute__ ((packed)); > > ? > > This padding that you've put in is not something that would normally > occur on x86. ?I've checked with GCC on 32-bit and 64-bit and neither > would include that padding. ?Is that padding included on s390?No, it is not included on any architecture that is supported by Linux. The only tricky case to watch out is if vring_used_elem contained a __u64 member. In that case, the alignment would be 32 bits on x86 and 64 bits on s390, powerpc, mips and parisc. Arnd <><
ron minnich
2008-Feb-09 22:33 UTC
[kvm-devel] [PATCH] virtio_ring: make structure defines packed
On Feb 8, 2008 5:01 AM, Christian Borntraeger <borntraeger@de.ibm.com> wrote:> Currently the virtio_ring structure are not declared packed, but they > describe an hardware like interface. We should not allow compilers to make > alignments and optimizations that can be different between the guest and > host compiler. > > I propose to declare all structures that are in shared memory as packed. > > Does anybody see a problem with packed?yes, I see many problems with packed. We went through this discussion already on another hypervisor several years ago. They started out with packed and later dropped it. Packed doesn't really solve any problems -- it just buries them somewhere. The problem for me is that gcc packed is not compatible with the plan 9 C compiler. So, for a truly heterogeneous setup, you are going to have to have code to marshall the structures when transferring between domains, and you are better off not trying to fake it with packed -- it can actually make the marshalling less efficient. Thanks ron
Anthony Liguori
2008-Feb-10 00:17 UTC
[kvm-devel] [PATCH] virtio_ring: make structure defines packed
Hi Christian, Christian Borntraeger wrote:> struct vring_used > { > __u16 flags; > __u16 idx; > + __u32 padding; > struct vring_used_elem ring[]; > -}; > +} __attribute__ ((packed)); >This padding that you've put in is not something that would normally occur on x86. I've checked with GCC on 32-bit and 64-bit and neither would include that padding. Is that padding included on s390? I don't think we can do this on x86 at this point as it changes the ABI. FWIW, I agree with Arnd also that we should not explicitly pad structures that are naturally aligned anyway. Regards, Anthony Liguori> struct vring { > unsigned int num; > > ------------------------------------------------------------------------- > This SF.net email is sponsored by: Microsoft > Defy all challenges. Microsoft(R) Visual Studio 2008. > http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ > _______________________________________________ > kvm-devel mailing list > kvm-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/kvm-devel >
Christian Borntraeger
2008-Feb-11 04:56 UTC
[kvm-devel] [PATCH] virtio_ring: make structure defines packed
Am Sonntag, 10. Februar 2008 schrieb ron minnich:> The problem for me is that gcc packed is not compatible with the plan > 9 C compiler. So, for a truly heterogeneous setup, you are going to > have > to have code to marshall the structures when transferring between > domains, and you are better off not trying to fake it with packed -- > it can actually make the > marshalling less efficient.Ok, I am conviced. The packed idea came up for mixed platform setups, but you are right - this reuqires much more. Christian
Reasonably Related Threads
- [PATCH] virtio_ring: make structure defines packed
- [PATCH] virtio: fix size computation according to the definition of struct vring_used in vring_size
- [PATCH] virtio: fix size computation according to the definition of struct vring_used in vring_size
- [PATCH v3] virtio: force spec specified alignment on types
- [PATCH v3] virtio: force spec specified alignment on types