From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38590) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gBtHC-0002Xb-Hv for qemu-devel@nongnu.org; Sun, 14 Oct 2018 23:10:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gBtH7-00046y-K3 for qemu-devel@nongnu.org; Sun, 14 Oct 2018 23:10:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52696) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gBtH7-00046a-Bu for qemu-devel@nongnu.org; Sun, 14 Oct 2018 23:10:33 -0400 References: <1539266915-15216-1-git-send-email-wexu@redhat.com> <1539266915-15216-4-git-send-email-wexu@redhat.com> From: Jason Wang Message-ID: Date: Mon, 15 Oct 2018 11:10:12 +0800 MIME-Version: 1.0 In-Reply-To: <1539266915-15216-4-git-send-email-wexu@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [[RFC v3 03/12] virtio: init memory cache for packed ring List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: wexu@redhat.com, qemu-devel@nongnu.org Cc: tiwei.bie@intel.com, jfreimann@redhat.com, maxime.coquelin@redhat.com On 2018=E5=B9=B410=E6=9C=8811=E6=97=A5 22:08, wexu@redhat.com wrote: > From: Wei Xu > > Expand 1.0 by adding offset calculation accordingly. This is only part of what this patch did and I suggest to another patch=20 to do this. > > Signed-off-by: Wei Xu > --- > hw/virtio/vhost.c | 16 ++++++++-------- > hw/virtio/virtio.c | 35 +++++++++++++++++++++++------------ > include/hw/virtio/virtio.h | 4 ++-- > 3 files changed, 33 insertions(+), 22 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 569c405..9df2da3 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -996,14 +996,14 @@ static int vhost_virtqueue_start(struct vhost_dev= *dev, > r =3D -ENOMEM; > goto fail_alloc_desc; > } > - vq->avail_size =3D s =3D l =3D virtio_queue_get_avail_size(vdev, i= dx); > + vq->avail_size =3D s =3D l =3D virtio_queue_get_driver_size(vdev, = idx); Let's try to use a more consistent name. E.g either use avail/used or=20 driver/device. I prefer to use avail/used, it can save lots of unnecessary changes. > vq->avail_phys =3D a =3D virtio_queue_get_avail_addr(vdev, idx); > vq->avail =3D vhost_memory_map(dev, a, &l, 0); > if (!vq->avail || l !=3D s) { > r =3D -ENOMEM; > goto fail_alloc_avail; > } > - vq->used_size =3D s =3D l =3D virtio_queue_get_used_size(vdev, idx= ); > + vq->used_size =3D s =3D l =3D virtio_queue_get_device_size(vdev, i= dx); > vq->used_phys =3D a =3D virtio_queue_get_used_addr(vdev, idx); > vq->used =3D vhost_memory_map(dev, a, &l, 1); > if (!vq->used || l !=3D s) { > @@ -1051,10 +1051,10 @@ static int vhost_virtqueue_start(struct vhost_d= ev *dev, > fail_vector: > fail_kick: > fail_alloc: > - vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev,= idx), > + vhost_memory_unmap(dev, vq->used, virtio_queue_get_device_size(vde= v, idx), > 0, 0); > fail_alloc_used: > - vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vde= v, idx), > + vhost_memory_unmap(dev, vq->avail, virtio_queue_get_driver_size(vd= ev, idx), > 0, 0); > fail_alloc_avail: > vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev= , idx), > @@ -1101,10 +1101,10 @@ static void vhost_virtqueue_stop(struct vhost_d= ev *dev, > vhost_vq_index); > } > =20 > - vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev,= idx), > - 1, virtio_queue_get_used_size(vdev, idx)); > - vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vde= v, idx), > - 0, virtio_queue_get_avail_size(vdev, idx)); > + vhost_memory_unmap(dev, vq->used, virtio_queue_get_device_size(vde= v, idx), > + 1, virtio_queue_get_device_size(vdev, idx)); > + vhost_memory_unmap(dev, vq->avail, virtio_queue_get_driver_size(vd= ev, idx), > + 0, virtio_queue_get_driver_size(vdev, idx)); > vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev= , idx), > 0, virtio_queue_get_desc_size(vdev, idx)); > } > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 500eecf..bfb3364 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -162,11 +162,8 @@ static void virtio_init_region_cache(VirtIODevice = *vdev, int n) > VRingMemoryRegionCaches *old =3D vq->vring.caches; > VRingMemoryRegionCaches *new =3D NULL; > hwaddr addr, size; > - int event_size; > int64_t len; > =20 > - event_size =3D virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVE= NT_IDX) ? 2 : 0; > - > addr =3D vq->vring.desc; > if (!addr) { > goto out_no_cache; > @@ -174,13 +171,13 @@ static void virtio_init_region_cache(VirtIODevice= *vdev, int n) > new =3D g_new0(VRingMemoryRegionCaches, 1); > size =3D virtio_queue_get_desc_size(vdev, n); > len =3D address_space_cache_init(&new->desc, vdev->dma_as, > - addr, size, false); > + addr, size, true); This looks wrong, for split virtqueue, descriptor ring is read only. > if (len < size) { > virtio_error(vdev, "Cannot map desc"); > goto err_desc; > } > =20 > - size =3D virtio_queue_get_used_size(vdev, n) + event_size; > + size =3D virtio_queue_get_device_size(vdev, n); > len =3D address_space_cache_init(&new->used, vdev->dma_as, > vq->vring.used, size, true); > if (len < size) { > @@ -188,7 +185,7 @@ static void virtio_init_region_cache(VirtIODevice *= vdev, int n) > goto err_used; > } > =20 > - size =3D virtio_queue_get_avail_size(vdev, n) + event_size; > + size =3D virtio_queue_get_driver_size(vdev, n); > len =3D address_space_cache_init(&new->avail, vdev->dma_as, > vq->vring.avail, size, false); > if (len < size) { > @@ -2339,16 +2336,30 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice = *vdev, int n) > return sizeof(VRingDesc) * vdev->vq[n].vring.num; > } > =20 > -hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n) > +hwaddr virtio_queue_get_driver_size(VirtIODevice *vdev, int n) > { > - return offsetof(VRingAvail, ring) + > - sizeof(uint16_t) * vdev->vq[n].vring.num; > + int s; > + > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > + return sizeof(struct VRingPackedDescEvent); > + } else { > + s =3D virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ?= 2 : 0; > + return offsetof(VRingAvail, ring) + > + sizeof(uint16_t) * vdev->vq[n].vring.num + s; I tend to move this to an independent patch. Thanks > + } > } > =20 > -hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n) > +hwaddr virtio_queue_get_device_size(VirtIODevice *vdev, int n) > { > - return offsetof(VRingUsed, ring) + > - sizeof(VRingUsedElem) * vdev->vq[n].vring.num; > + int s; > + > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > + return sizeof(struct VRingPackedDescEvent); > + } else { > + s =3D virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX) ?= 2 : 0; > + return offsetof(VRingUsed, ring) + > + sizeof(VRingUsedElem) * vdev->vq[n].vring.num + s; > + } > } > =20 > uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 9c1fa07..e323e76 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -270,8 +270,8 @@ hwaddr virtio_queue_get_desc_addr(VirtIODevice *vde= v, int n); > hwaddr virtio_queue_get_avail_addr(VirtIODevice *vdev, int n); > hwaddr virtio_queue_get_used_addr(VirtIODevice *vdev, int n); > hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n); > -hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n); > -hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n); > +hwaddr virtio_queue_get_driver_size(VirtIODevice *vdev, int n); > +hwaddr virtio_queue_get_device_size(VirtIODevice *vdev, int n); > uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n); > void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint1= 6_t idx); > void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n);