Re: [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size

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

On Thu, 3 Sep 2015 10:23:44 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Wed, 2 Sep 2015 19:57:25 +0200
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > On Wed, 2 Sep 2015 17:50:55 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > 
> > > On Wed,  2 Sep 2015 17:23:49 +0200
> > > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> > > 
> > > > Being working on dataplane I notice something strange:
> > > > 
> > > > virtio_queue_get_avail_size() used a 64bit size index
> > > > for the calculation of the available ring size.
> > > > 
> > > > It is quite strange but it did work with the old calculation
> > > > of the avail ring, at most with performance penalty,
> > > > and I wonder where I missed something.
> > > > 
> > > > This patch let use a 16bit size as defined in virtio_ring.h
> > > > 
> > > > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index 788b556..5c856eb 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -1460,7 +1460,7 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
> > > >  hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
> > > >  {
> > > >      return offsetof(VRingAvail, ring) +
> > > > -        sizeof(uint64_t) * vdev->vq[n].vring.num;
> > > > +        sizeof(uint16_t) * vdev->vq[n].vring.num;
> > > >  }
> > > > 
> > > >  hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
> > > 
> > > I'm wondering about the semantics of the _size() functions. Naively I
> > > would expect (size of buffer) * (number of buffers). I think at least
> > 
> > Looking at where these functions are called, it really looks like they are
> > expected to return the size of the memory region to be mapped. Since we have:
> > 
> 
> Acutally no... they are also used to compute the address of used_event_idx
> and avail_event_idx.
> 
> > typedef struct VRingAvail
> > {
> >     uint16_t flags;
> >     uint16_t idx;
> >     uint16_t ring[0];
> > } VRingAvail;
> > 
> > Pierre's patch looks valid. But while we're here, why not introducing
> > something like:
> > 
> > #define member_size(type, member) sizeof(((type *)0)->member)
> > 
> > It would consolidate the _size functions and the types they are referring to:
> > 
> > -        sizeof(uint64_t) * vdev->vq[n].vring.num;
> > +        member_size(VRingAvail, vring[0]) * vdev->vq[n].vring.num;
> > 
> > > vhost expects the {used,avail} indices in there as well? The
> > > s390-virtio code seems not to expect the indices to be contained in the
> > > size, though...
> > 
> > 
> 
> Sorry I missed the real question... should these _size functions return
> the actual size + sizeof(uint16_t) ? 
> 
> Indeed, I could verify the the s390-virtio code uses the _size functions
> to compute the address of used_event_idx and avail_event_idx...
> The vhost code only uses the _size functions to map memory... and
> doesn't add sizeof(uint16_t)... which looks like a bug.

Yes, this probably worked by chance because (a) the avail size is too
big anyway and (b) the used size added the offset value... and probably
nobody cares much about s390-virtio reset, but that might explain some
headscratchers we were seeing very occasionally.