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

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

On Mon, Jan 20, 2014 at 01:39:20PM +0100, Cornelia Huck wrote:
> 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.

But why is this a problem?
Transitional device just has to behave sufficiently
like legacy ones to make legacy drivers work.
It doesn't behave exactly like a legay device though -
for example it has VERSION_1 feature.

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

Well it's not a MUST, but it's a SHOULD:
 driver SHOULD start with trying to set the highest revision it
 supports and continue with lower revisions if it gets a command reject.
seems pretty clear.

Again, if you want to allow for a low-quality
driver which violates the SHOULD requirement,
it will still be simpler to just ask transitional devices
to allow length 8. Less text and more features :)

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

Seems pretty clear from the existing spec - do you
want to strengthen this SHOULD to a MUST?
If yes that's fine with me.

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

Then I don't really see why we should repeat ourselves
here.

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

Why not, but it's not a specific CCW thing.
Something like this in the general section
should address Thomas's comment:

--->
Note on device and driver implementations using C/C++

many C and C++ compilers will pad structures in-memory
based on the size of the structure data fields.  Typically, this padding
is used to align all fields at their natural boundaries.
When using C to generate driver and device code,
one has to take this padding into account,
or disable it using compiler-specific "packing" directives.

structure layout in this specification have been
designed to simplify driver and device code
by reducing the need for such packing.
Specifically each field is at an offset aligned at its natural alignment
boundary.  Further, each structure size is a multiple of
the largest natural alignment boundary of its fields.

For most compilers this is enough to
ensure that structure definitions from this specification
can be used verbatim, and that no extra padding will be added
by the compiler. However, as arbitrary padding within
structures is in theory allowed by the latest ISO/IEC 9899:2011 C
standard (C11), so implementation developers should verify that no extra
padding is inserted by the compiler.

----

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

Most C compilers align structures based on size of data
so this doesn't seem necessary unless we have 8 byte fields.

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