On Thu, 3 Sep 2015 10:23:44 +0200
Greg Kurz <gk...@linux.vnet.ibm.com> wrote:
> On Wed, 2 Sep 2015 19:57:25 +0200
> Greg Kurz <gk...@linux.vnet.ibm.com> wrote:
>
> > On Wed, 2 Sep 2015 17:50:55 +0200
> > Cornelia Huck <cornelia.h...@de.ibm.com> wrote:
> >
> > > On Wed, 2 Sep 2015 17:23:49 +0200
> > > Pierre Morel <pmo...@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 <pmo...@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.
