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

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

On Thu, Oct 10, 2013 at 11:04:39AM +0200, Cornelia Huck wrote:
> On Thu, 10 Oct 2013 11:46:05 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 don't think this should be a MUST; but a SHOULD would be reasonable.
> 
> I can put in language as well that drivers SHOULD specify the correct
> length; the virtio-ccw commands do not lend themselves to the scenario
> I described above, and suppressing a length check would be more of a
> crutch for not-so-good drivers.

Hmm if it's not a MUST then drivers can't rely on it.
So why is it useful?

I guess I'm kind of confused as to why this is useful - on the one hand
you prefer failing on easy to handle errors such as reserved field
!= 0 (device could simply ignore it).
I kind of see the point - this makes sure drivers initialize everything.
On the other hand you want this flexibility to pass large
lengths. I thought the point is to make drivers simpler:
they can always use large length and not worry that device
will be confused. But if it's a SHOULD then drivers can't rely
on it being there, so I guess that's not the prupose?

> > 
> > 
> > > > 
> > > > 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).
> 
> I'd probably need to the s390 architecture there; it's all in there :)
> 
> > But I don't see why it's a MUST - help debugging broken drivers
> > does not seem to merit more than a MAY.
> 
> It's not really a case of "help debugging broken drivers". If I were
> writing a real s390 attachment specification, I'd be expected to write
> down what the device _does_ in case of checking. I'd really like to put
> down what we do as a MUST where it makes sense, so driver authors know
> what they can rely on.
> 
> > 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?
> 
> I'm more and more inclinded to just drop the command reject in that
> case and use the normal channel program check instead.
> 
> > 
> > > > 
> > > > 
> > > > > +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
> > > > 
> >