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

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

On Thu, 10 Oct 2013 14:10:39 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Oct 10, 2013 at 12:56:56PM +0200, Cornelia Huck wrote:
> > On Thu, 10 Oct 2013 12:29:59 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > 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?
> > 
> > It obviously must be either two MUSTs or two SHOULDs. It probably
> > should not be MUST, as I don't remember another device failing on a too
> > large buffer. SHOULD is more like a 'best practice' to me.
> > 
> > Remember that an incorrect length without length check disabled will
> > always yield a check; this is mandated by the architecture.
> > 
> > > 
> > > 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).
> > 
> > Well, we _can_ ignore it, if we specify it that way :) A
> > reserved/ignored or reserved/must be zero field are both fine for this
> > case.
> > 
> > > 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?
> > 
> > No, the purpose is not to be too different from other devices
> > implementing channel architecture. A driver will usually only suppress
> > length checking in special cases (like the variable data length case);
> > the normal mode of operation is to specify the correct length and leave
> > the length check on.
> 
> I don't know too much about this. I was going by your text:
> 
> 	Otherwise, the normal conditions for handling incorrect data lenghts apply.
> 
> which makes one think we are making some kind of exception here.

That was referring to the type of error status posted, but I'll ditch
this.

> 
> In any case, I think if it's a SHOULD that's fine, but it seems
> a separate issue from ring layout.
> Maybe a separate chapter explaining the virtio-ccw specific
> length handling rules will make sense?

Maybe in the general remarks for virtio-ccw?

---

For the virtio-ccw specific channel commands, the general mechanism for
channel command data length checking applies, as detailed in the
z/Architecture Principles of Operation: I/O Interruptions -> Subchannel
Status Word.

If a driver did not suppress length checks for a channel command, the
device MUST present a subchannel status as detailed in the architecture
when the actual length did not match the expected length.

If a driver did suppress length checks for a channel command, the
device MUST present a check condition if the transmitted data does not
contain enough data to process the command. If the driver submitted a
buffer that was too long, it SHOULD accept the command. The driver
SHOULD attempt to provide the correct length even if it suppresses
length checks.