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

["Michael S. Tsirkin" <mst@redhat.com>]

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.