[PATCH] virtio_ring: make structure defines packed

[PATCH] virtio_ring: make structure defines packed

[Christian Borntraeger]

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-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
---
 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;

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

Re: [PATCH] virtio_ring: make structure defines packed

[Arnd Bergmann]

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

Re: [PATCH] virtio_ring: make structure defines packed

[Arnd Bergmann]

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

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

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   

Re: [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   


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

Re: [PATCH] virtio_ring: make structure defines packed

[Arnd Bergmann]

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

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

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Arnd Bergmann]

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

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Anthony Liguori]

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
>   

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[ron minnich]

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

Re: [PATCH] virtio_ring: make structure defines packed

[ron minnich]

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

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

Re: [kvm-devel] [PATCH] virtio_ring: make structure defines packed

[Christian Borntraeger]

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

Re: [PATCH] virtio_ring: make structure defines packed

[Christian Borntraeger]

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

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

[PATCH] virtio_ring: make structure defines packed

[Christian Borntraeger]

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;