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

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

On Thu, Oct 10, 2013 at 10:03:58AM +0200, Cornelia Huck wrote:
> On Thu, 10 Oct 2013 08:26:13 +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,
> > 
> > Where's the revision field and how does driver select it?
> > I don't see it anywhere in spec?
> 
> This one is depending on VIRTIO-42; sorry about not being clear on it.
> 
> (If we ditch the revision check, the dependency goes away.)

Okay, so we voted in VIRTIO-30, I'll just commit that as it's
a generic replacement solving same problem.
For now could you rephrase the text to say
"when used through legacy interface" for devices
and "when using legacy interface" for drivers?

That's the language we use elsewhere.

Then VIRTIO-42 would be independent.

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