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

[Greg Kurz]

On Wed, 2 Sep 2015 19:57:25 +0200
Greg Kurz <address@hidden> wrote:

> On Wed, 2 Sep 2015 17:50:55 +0200
> Cornelia Huck <address@hidden> wrote:
> 
> > On Wed,  2 Sep 2015 17:23:49 +0200
> > Pierre Morel <address@hidden> 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 <address@hidden>
> > > ---
> > >  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.

--
Greg