Re: [virtio-comment] Feedback for chapter 4

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Sun, 12 Jan 2014 13:24:09 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jan 10, 2014 at 01:49:49PM +0100, Thomas Huth wrote:

> > Chapter 4.3.2.*:
> > 
> > - In this chapter, the C-structs are marked with "__attribute__ ((packed));"
> >   which is just a GNU-C extension, as far as I know. In the other chapters,
> >   the structs are not marked with this string. So for consistency, I'd remove
> >   them here, too (and maybe state somewhere at the beginning of the spec
> >   that structs are considered to be without compiler padding inbetween)
> 
> I agree since C spec seems to allow compilers to add
> arbitrary padding anywhere in theory, we should mention all structures
> assume there's no padding.
> 
> And in most places, we don't really need the packing as
> fields are aligned properly so any sane compiler will just do the right
> thing.
> 
> Unfortunately CCW has several instances where any reasonable
> compiler would instroduce padding, I think just a note won't be enough
> since compiler-introduced issues are so hard to debug.
> So I think that instead of asking people to use compiler extensions to
> suppress padding, it's best just to add padding explicitly.
> 
> Cornelia, can we do this?
> For example
> 
> struct virtio_feature_desc {
> 	be32 features;
> 	u8 index;
> 	u8 padding[3]; /* Optional padding */
> };
> 
> Unfortunately this won't work with existing devices,
> but we can note in the legacy section that
> transitional drivers must suppress padding, and
> transitional devices must support commands without padding.

Indeed, we need to have different handling depending on which virtio
version we're driving, and unfortunately suppressing incorrect length
checks for the channel command won't help here (short read).

> 
> Similarly:
> 
> struct virtio_thinint_area {
> 	be64 summary_indicator;
> 	be64 indicator;
> 	be64 bit_nr;
> 	u8 isc;
> 	u8 padding[7]; /* Optional padding */
> };
> 

I'll go through the ccw structures and see what I come up with.