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

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

On Mon, 20 Jan 2014 12:15:43 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 :)

So s/specified/used/ :)

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

Hm, it would be obvious if we used the tables ;)

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

I have no objection to including both the byte count and the reference
to the structure.

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

I think it would be inconsistent: A legacy device that did not
negotiate versions would reject the command, while a transitional
device that (for whatever reasons) accepted version 0 would not.

> 
> > 
> > - 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'?

I don't think we ever specified that the combination of transitional
device and transitional driver MUST operate at version > 0. Either we
should specify this in the section for the version command (and drop
the references to version 0 here and maybe above), or we need to keep
this clause.

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

I thought of it as a "MAY 5 or MAY 8", but "MUST 5 or 8" is probably
better.

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

I've described it in the general overview already.

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

Hm. I'd prefer to spell out explicitely that we added the optional
padding to accomodate compilers adding that padding.

BTW: Should we make sure that all communication blocks use a multiple
of (say) 8 bytes?

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