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

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

On Mon, Jan 20, 2014 at 10:45:04AM +0100, Cornelia Huck wrote:
> 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.

As I see it interface is what we use for communication. It's not the spec
so it can not specify :)

I think saying it's "up to and exlcluding padding field" is also nice since
it implies people can use offsetof instead of
hard-coding 5.

Also, you are saying "5" and "8" but it's not obvious
it's bytes (and not e.g. pages).

How about "For legacy interfaces, the communication block
MUST be 5 bytes in length, up to and excluding
the optional padding field".

Same applies everywhere.

> This means:
> 
> - A legacy device will reject CCW_CMD_READ_FEAT and CCW_CMD_WRITE_FEAT

Let's put this in past or present tense: past tense used below.

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

I don't see why we should require this last bit.
Let's drop this line and make all new devices allow padding.

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

Confused. transitional driver + transitional device must never
operate with revision 0. Drop text after 'or'?

> 
> - A transitional driver operating a device with revision 1 or a
> non-transitional driver MAY supply a communication block of a length of

why is this a MAY and not a MUST?

> either 5 or 8 if it doesn't suppress length checks for the command, and
> at least 5 if it does.
> 
> (Too verbose, perhaps?)

I'm afraid it is. Also this mixes legacy and non legacy description.
I think this paragraph should be split.

A. Describe behaviour of new devices/drivers in the main section.
We can simply say drivers can either supply full buffer or exclude the
optional padding, devices must accept both.

B. Describe behaviour with legacy devices in a separate
paragraph.
We can simply say when operating the legacy interface, drivers MUST
exclude the optional padding.


C. If you want to describe what happens when length checks are
suppressed, this is not command specific and should
go into some other place imho.

To avoid confusion since compilers include the
oadding automatically, it might make sense to stress -
when we describe excluding padding - that length will be
"5 bytes up to and excluding the optional padding field"
but I see no need to do this for normal case
when padding is included.

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