[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, 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: > > diff --git a/content.tex b/content.tex > index 803615d..539bf68 100644 > --- a/content.tex > +++ b/content.tex > @@ -1992,7 +1992,7 @@ about a queue. It uses the following structure for communicating: > struct vq_config_block { > be16 index; > be16 max_num; > - } __attribute__ ((packed)); > + }; > \end{lstlisting} > > The requested number of buffers for queue index is returned in > @@ -2010,7 +2010,7 @@ structure is > be16 num; > be64 avail; > be64 used; > - } __attribute__ ((packed)); > + }; > \end{lstlisting} > > desc, avail and used contain the guest addresses for the descriptor table, > @@ -2029,7 +2029,7 @@ CCW_CMD_SET_VQ uses the following communication block: > be32 align; > be16 index; > be16 num; > - } __attribute__ ((packed)); > + }; > \end{lstlisting} > > queue contains the guest address for queue index, num the number of buffers > @@ -2071,7 +2071,8 @@ for a total of 8192 feature bits. Feature bits are in > little-endian byte order. > > The CCW commands dealing with features use the following > -communication block: > +communication block. Note that this command is exactly > +5 bytes long, violating the normal C structure padding rules: > > \begin{lstlisting} > struct virtio_feature_desc { > @@ -2159,7 +2160,8 @@ two stages: > > To communicate the location of the summary and queue indicator bits, > the driver uses the CCW_CMD_SET_IND_ADAPTER command with the following > -payload: > +payload. Note that this command is exactly 25 bytes long, violating > +the normal C structure padding rules: > > \begin{lstlisting} > struct virtio_thinint_area { 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.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]