OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


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


On Tue, 21 Jan 2014 14:34:33 +0200
"Michael S. Tsirkin" <mst@redhat.com> 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.

Hm... so you just want to make it more explicit? I don't have a strong
opinion on this.

> 
> > > 
> > > 
> > > > (We may want to specify our le32, ... usage in the same paragraph.)
> > > 
> > > u8 etc too - these are not standard types.
> > > I would make it a separate paragraph though.
> > > 
> > 
> > I'll try to write something up.
> 



[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]