On Mon, 20 Jan 2014 12:15:43 +0200
"Michael S. Tsirkin" <> 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" <> wrote:
> >
> > > On Mon, Jan 20, 2014 at 02:28:00PM +1030, Rusty Russell wrote:
> > > > Cornelia Huck <> 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 <>
> > >
> > >
> > > 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
> > >
>
