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

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

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:
> > Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> > > Move the ccw payload description to tables, instead of using
> > > C structures with gcc extensions or textual descriptions.
> > 
> > I think it's horrible.
> > 
> > Almost everyone reading this will understand C.  And they can cut and
> > paste the code from the spec into their implementation, which helps
> > everyone.

I don't think that all other virtio-ccw implementations would be in C;
but if we want to stay with code snippets, C is probably the best
choice.

> > 
> > 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.

> 
> For example:
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> diff --git a/content.tex b/content.tex
> index 803615d..f912090 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2077,7 +2077,8 @@ communication block:
>  	struct virtio_feature_desc {
>  		le32 features;
>  		u8 index;
> -	} __attribute__ ((packed));
> +		u8 padding[3]; /* Optional padding */
> +	};
>  \end{lstlisting}
> 
>  features are the 32 bits of features currently accessed, while
> @@ -2092,6 +2093,18 @@ For communicating its supported features to the device, the driver
>  uses the CCW_CMD_WRITE_FEAT command, denoting a features/index
>  combination.
> 
> +padding field is optional. If present, it is ignored by the
> +device.
> +
> +\paragraph{Legacy Interface: A Note on Handling Device Features}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features / Legacy Interface: A Note on Handling Device Features}
> +
> +Legacy devices MAY not support the optional padding field;
> +they MAY reject CCW_CMD_READ_FEAT and CCW_CMD_WRITE_FEAT
> +commands unless the communication block byte count is exactly 5.
> +
> +Transitional drivers MUST make the communication block byte count
> +be exactly 5, up to and excluding the optional padding field.
> +

What we should keep in mind here is that we want to reject short
buffers that don't contain enough information to process the command,
but always want to accept long buffers if the driver suppressed length
checking.

So what about the following:

The legacy interface specified the communication block without the
optional padding. This means:

- A legacy device will reject CCW_CMD_READ_FEAT and CCW_CMD_WRITE_FEAT
if the length of the communication block exceeds 5, unless the driver
suppressed length checks for the command. Similarily, a transitional
device operating with revision 0 MUST reject any such commands.

- A transitional or non-transitional device MUST accept payload lengths
of 5 or 8. If the driver suppressed length checks for the command, it
MUST accept any payload length of at least 5.

- A transitional driver MUST supply a communication block of a length
of exactly 5 if it is driving a legacy device or a device operating
with revision 0, unless it suppresses length checks for the command.

- A transitional driver operating a device with revision 1 or a
non-transitional driver MAY supply a communication block of a length of
either 5 or 8 if it doesn't suppress length checks for the command, and
at least 5 if it does.

(Too verbose, perhaps?)

>  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
> 
>  The device's configuration space is located in host memory. It is
>