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

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

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.

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