From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56941) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXCHx-00057R-Qh for qemu-devel@nongnu.org; Wed, 02 Sep 2015 13:57:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZXCHu-0003YW-Jj for qemu-devel@nongnu.org; Wed, 02 Sep 2015 13:57:37 -0400 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:51058) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXCHu-0003YD-Ac for qemu-devel@nongnu.org; Wed, 02 Sep 2015 13:57:34 -0400 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 2 Sep 2015 18:57:31 +0100 Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t82HvSdR30081034 for ; Wed, 2 Sep 2015 17:57:29 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t82HvS4g022287 for ; Wed, 2 Sep 2015 11:57:28 -0600 Date: Wed, 2 Sep 2015 19:57:25 +0200 From: Greg Kurz Message-ID: <20150902195725.0363617c@bahia.local> In-Reply-To: <20150902175055.78b06a5b.cornelia.huck@de.ibm.com> References: <1441207429-23221-1-git-send-email-pmorel@linux.vnet.ibm.com> <20150902175055.78b06a5b.cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: Pierre Morel , qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com On Wed, 2 Sep 2015 17:50:55 +0200 Cornelia Huck wrote: > On Wed, 2 Sep 2015 17:23:49 +0200 > Pierre Morel 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 > > --- > > 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: 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...