Re: [virtio-dev] Re: [virtio-comment] [PATCH 1/1] ccw: ccw payload description

[James Bottomley <jbottomley@parallels.com>]

On Tue, 2014-01-21 at 14:34 +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 21, 2014 at 01:23:29PM +0100, Cornelia Huck wrote:
> > On Tue, 21 Jan 2014 14:09:46 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Jan 21, 2014 at 12:30:21PM +0100, Cornelia Huck wrote:
> > > > On Tue, 21 Jan 2014 10:53:49 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Tue, Jan 21, 2014 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > > > Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> > > > > > > On Mon, 20 Jan 2014 11:17:17 +0200
> > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > >
> > > > > > >> On Mon, Jan 20, 2014 at 02:28:00PM +1030, Rusty Russell wrote:
> > > > > > >> > We'll just have to live with the lack of padding, and let's try not to
> > > > > > >> > add any more packed-requiring structures.  If we deprecate
> > > > > > >> > virtio-balloon, the rest will be contained in CCW.
> > > > > > >> > 
> > > > > > >> > Cheers,
> > > > > > >> > Rusty.
> > > > > > >> 
> > > > > > >> For CCW, how about my idea of adding optional padding explicitly?
> > > > > > >> In the compatibility section, we can explicitly specify the
> > > > > > >> the length.
> > > > > > >> Drivers can use offsetof(padding) and avoid packed attribute.
> > > > > > >
> > > > > > > Since Rusty does not like the tables, this looks like a good idea.
> > > > > > 
> > > > > > Surely making the padding optional makes it more complex, for no real
> > > > > > gain?  It's not that much simpler in practice, is it?
> > > > > > 
> > > > > > This patch just removes the __attribute__((packed)) where it's useless,
> > > > > > and notes explicitly where it's required:
> > > > 
> > > > (...)
> > > > 
> > > > > I was trying to avoid the non-portable __attribute__((packed))
> > > > > replacing it with portable offsetof(..., padding)
> > > > > everywhere. It only has to be optional for the
> > > > > transitional devices. But if Thomas is fine with
> > > > > __attribute__((packed)) now, that's fine with me too.
> > > > > 
> > > > > I do think we need a general comment explaining the
> > > > > packed directive here.
> > > > > 
> > > > > E.g.
> > > > > 
> > > > > Many device and driver in-memory structure layouts
> > > > > are documented using the C struct syntax.
> > > > > In this case no extra padding is assumed within the structure.
> > > > > To stress this, cases where common C compilers are known to insert extra
> > > > > padding within structures are tagged using the GNU C __attribute__((packed))
> > > > > syntax.
> > > > 
> > > > Do we want to add a general statement regarding our usage of C structs?
> > > > 
> > > > Basically, we can't predict if and where a compiler will insert
> > > > padding, and different compilers on different architectures might pad
> > > > in different ways. So I'd propose to state that all our C structs are
> > > > assumed to be packed with no extra padding appended, even if we take
> > > > care to introduce no new structures where a common C compiler would
> > > > insert padding.
> > > 
> > > What does this add to what I wrote above?
> > 
> > I'd like to be able to drop the __attribute__((packed)), and just make
> > that the default.
> 
> To me __attribute__((packed)) seems helpful, and harmless.
> If someone wants to pack everything in driver and bloat up
> code unnecessarily, one always has the option.

To me as well.  As a driver writer, I start to worry when I *don't* see
an __attribute__((packed)) on any data structure shared between the
driver and the device (whether or not it's a virtual device).
Conversely, relying on the C compiler to do standard padding is
dangerous (and error prone).  For instance, the padding requirements for
64 bit words on x86 is 32 bits, but it's 64 bits on every other
architecture, so 

{
	long a;
	int b;
	long c;
}

packs differently on x86 and on anything else.  Seeing an
__attribute__((packed)) indicates that the author thought about padding.

Furthermore, the standard for all other driver docs I've had to read is
that the padding be explicit, so

{
	u8	a;
	u8	pad[3];
	int	b;
} __attribute__((packed));

Even though that would be "natural" padding.

James