Re: [PATCH] virtio-ccw: split descriptor/available/used rings (alternate)

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

On Thu, Oct 10, 2013 at 10:16:35AM +0200, Cornelia Huck wrote:
> On Thu, 10 Oct 2013 08:43:44 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Oct 09, 2013 at 05:59:36PM +0200, Cornelia Huck wrote:
> > > Extend vq_info_block so that the addresses for descriptor table,
> > > available ring and used ring may be transmitted independently.
> > > 
> > > Depending upon the selected revision, post a command reject instead
> > > of a channel program check if the driver uses the legacy format
> > > and length checks are suppressed.
> > > 
> > > VIRTIO-23
> > > 
> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > 
> > > ---
> > > 
> > > This is an alternate approach, extending the exiting structure instead
> > > of creating a different layout. I'm not 100% sure whether doing a
> > > command reject instead of a channel program check in case of a short
> > > buffer is the right approach, though. Doing a channel program check
> > > would probably cover that error just as well, and we could resolve
> > > VIRTIO-23 independently of VIRTIO-42.
> > > ---
> > >  virtio-v1.0-wd01-part1-specification.txt |   32 +++++++++++++++++++++++++++---
> > >  1 file changed, 29 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/virtio-v1.0-wd01-part1-specification.txt b/virtio-v1.0-wd01-part1-specification.txt
> > > index ae646db..baff12f 100644
> > > --- a/virtio-v1.0-wd01-part1-specification.txt
> > > +++ b/virtio-v1.0-wd01-part1-specification.txt
> > > @@ -1642,15 +1642,41 @@ host about the location used for its queue. The transmitted
> > >  structure is
> > >  
> > >  struct vq_info_block {
> > > +	__u64 desc;
> > > +	__u32 res0;
> > > +	__u16 index;
> > > +	__u16 num;
> > > +	__u64 avail;
> > > +	__u64 used;
> > > +} __attribute__ ((packed));
> > > +
> > > +desc, avail and used contain the guest addresses for the descriptor table,
> > > +available ring and used ring for queue index, respectively. The actual
> > > +virtqueue size (number of allocated buffers) is transmitted in num.
> > > +res0 is reserved and must contain 0; otherwise, the device MUST post a
> > > +unit check with command reject.
> > > +
> > > +If the revision selected by the driver is at least 1, the device MUST
> > > +post a unit check with command reject if the transmitted data is between
> > > +16 and 31 bytes if the driver suppressed incorrect length indication
> > > +for the channel command. Otherwise, the normal conditions for handling
> > > +incorrect data lenghts apply.
> > 
> > Also I don't understand the following: is there any
> > flexibility for drivers wrt the transmitted data length?
> > Above structure is 32 bytes in size.
> > So any other length is a driver bug.
> 
> Not really. The driver may transmit a larger buffer then is needed, and
> suppress length checking via a ccw flag. The device can then process
> the data it needs, and disregard the rest. This is used sometimes for
> variable-length responses where a driver can just supply the largest
> possible buffer and check afterwards how much data it got. Depending on
> the command, this may work with short buffers as well.
> 
> (In the virtio-ccw code so far, I required a minimum length and allowed
> a larger length when length checks have been turned off.)

If drivers rely on this, this probably should be documented in the spec.
Specifically if I read the spec today it says command legth is X,
it seems quite reasonable to just stick
assert(length == X) in code, and people will interpret it
like this - was saw it with message framing.

If you think devices should assept longer lengths,
please put a MUST in text saying this.


> > 
> > I'm guessing there's any
> > number of other possibly invalid values that drivers can supply
> > in some fields.
> > E.g. stick a wrong PA outside RAM in one of the fields - seems
> > more likely to happen actually.
> > Why worry what happens then?
> 
> There are really two different cases there:
> 
> - The driver puts in values that are obviously incorrect, like a reserved
>   field that is != 0 - this should be answered with a check, most likely
>   a channel program check. (We may want to do this as well if an address
>   can be immediately verified to be incorrect; many commands for other
>   devices do.)
> - The driver puts in junk that looks valid. The command will succeed,
>   problems will happen later.

Fine, so let's add text in the CCW section to
explain that device can validate commands and suggest
good ways to handle broken drivers (e.g. reject).
But I don't see why it's a MUST - help debugging broken drivers
does not seem to merit more than a MAY.
Also in this specific case, it seems to be more
trouble than it's worth:
sticking specific length requirements in the spec will
just add maintainance overhead as we'll
have to remember to update it if/when structure changes.
finally, the wording looks very strange to me:
"if the transmitted data is between 16 and 31 bytes"
what if it's less than 16?

> > 
> > 
> > > +2.3.3.2.2.1. Legacy Interface: A Note on Configuring a Virtqueue
> > > +----------------------------------------------------------------
> > > +
> > > +For a legacy driver or for a driver that selected revision 0,
> > > +CCW_CMD_SET_VQ uses the following communication block:
> > > +
> > > +struct vq_info_block_legacy {
> > >  	__u64 queue;
> > >  	__u32 align;
> > >  	__u16 index;
> > >  	__u16 num;
> > >  } __attribute__ ((packed));
> > >  
> > > -queue contains the guest address for queue index. The actual
> > > -number of allocated buffers is transmitted in num and their
> > > -alignment in align.
> > > +queue contains the guest address for queue index, num the number of buffers
> > > +and align the alignment.
> > >  
> > >  100.3.3.2.2.  Virtqueue Layout
> > >  ------------------------------
> > > -- 
> > > 1.7.9.5
> >