[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_s
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size |
Date: |
Thu, 3 Sep 2015 10:23:44 +0200 |
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