From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51187) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu2Fv-0002RY-Pc for qemu-devel@nongnu.org; Thu, 27 Nov 2014 11:49:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xu2Fq-0005bQ-Jg for qemu-devel@nongnu.org; Thu, 27 Nov 2014 11:49:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50323) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu2Fq-0005bJ-CM for qemu-devel@nongnu.org; Thu, 27 Nov 2014 11:49:18 -0500 Message-ID: <547755FE.10707@redhat.com> Date: Thu, 27 Nov 2014 17:49:02 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1417083310-12063-1-git-send-email-pl@kamp.de> In-Reply-To: <1417083310-12063-1-git-send-email-pl@kamp.de> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH] vring: calculate descriptor address directly List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, benoit@irqsave.net, ming.lei@canonical.com, armbru@redhat.com, mreitz@redhat.com, stefanha@redhat.com On 27/11/2014 11:15, Peter Lieven wrote: > vring_map causes a huge overhead by calling memory_region_find everytime. > the vring_map is executed already on vring_setup and there is also the memory > region referenced. > > Signed-off-by: Peter Lieven vring_map/unmap is going to disappear and be replaced by address_space_map/unmap, so for now I'd rather keep it... Paolo > --- > hw/virtio/dataplane/vring.c | 51 +++++-------------------------------------- > 1 file changed, 5 insertions(+), 46 deletions(-) > > diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c > index 61f6d83..cfd484f 100644 > --- a/hw/virtio/dataplane/vring.c > +++ b/hw/virtio/dataplane/vring.c > @@ -53,15 +53,6 @@ out: > return NULL; > } > > -static void vring_unmap(void *buffer, bool is_write) > -{ > - ram_addr_t addr; > - MemoryRegion *mr; > - > - mr = qemu_ram_addr_from_host(buffer, &addr); > - memory_region_unref(mr); > -} > - > /* Map the guest's vring to host memory */ > bool vring_setup(Vring *vring, VirtIODevice *vdev, int n) > { > @@ -160,7 +151,6 @@ static int get_desc(Vring *vring, VirtQueueElement *elem, > unsigned *num; > struct iovec *iov; > hwaddr *addr; > - MemoryRegion *mr; > > if (desc->flags & VRING_DESC_F_WRITE) { > num = &elem->in_num; > @@ -186,13 +176,10 @@ static int get_desc(Vring *vring, VirtQueueElement *elem, > } > > /* TODO handle non-contiguous memory across region boundaries */ > - iov->iov_base = vring_map(&mr, desc->addr, desc->len, > - desc->flags & VRING_DESC_F_WRITE); > - if (!iov->iov_base) { > - error_report("Failed to map descriptor addr %#" PRIx64 " len %u", > - (uint64_t)desc->addr, desc->len); > + if (desc->addr + desc->len > memory_region_size(vring->mr)) { > return -EFAULT; > } > + iov->iov_base = (void*) memory_region_get_ram_ptr(vring->mr) + desc->addr; > > /* The MemoryRegion is looked up again and unref'ed later, leave the > * ref in place. */ > @@ -230,22 +217,14 @@ static int get_indirect(Vring *vring, VirtQueueElement *elem, > > do { > struct vring_desc *desc_ptr; > - MemoryRegion *mr; > > /* Translate indirect descriptor */ > - desc_ptr = vring_map(&mr, > - indirect->addr + found * sizeof(desc), > - sizeof(desc), false); > - if (!desc_ptr) { > - error_report("Failed to map indirect descriptor " > - "addr %#" PRIx64 " len %zu", > - (uint64_t)indirect->addr + found * sizeof(desc), > - sizeof(desc)); > - vring->broken = true; > + if (indirect->addr + (found + 1) * sizeof(desc) > memory_region_size(vring->mr)) { > return -EFAULT; > } > + desc_ptr = (void*) memory_region_get_ram_ptr(vring->mr) + > + indirect->addr + found * sizeof(desc); > desc = *desc_ptr; > - memory_region_unref(mr); > > /* Ensure descriptor has been loaded before accessing fields */ > barrier(); /* read_barrier_depends(); */ > @@ -273,23 +252,6 @@ static int get_indirect(Vring *vring, VirtQueueElement *elem, > return 0; > } > > -static void vring_unmap_element(VirtQueueElement *elem) > -{ > - int i; > - > - /* This assumes that the iovecs, if changed, are never moved past > - * the end of the valid area. This is true if iovec manipulations > - * are done with iov_discard_front and iov_discard_back. > - */ > - for (i = 0; i < elem->out_num; i++) { > - vring_unmap(elem->out_sg[i].iov_base, false); > - } > - > - for (i = 0; i < elem->in_num; i++) { > - vring_unmap(elem->in_sg[i].iov_base, true); > - } > -} > - > /* This looks in the virtqueue and for the first available buffer, and converts > * it to an iovec for convenient access. Since descriptors consist of some > * number of output then some number of input descriptors, it's actually two > @@ -399,7 +361,6 @@ out: > if (ret == -EFAULT) { > vring->broken = true; > } > - vring_unmap_element(elem); > return ret; > } > > @@ -413,8 +374,6 @@ void vring_push(Vring *vring, VirtQueueElement *elem, int len) > unsigned int head = elem->index; > uint16_t new; > > - vring_unmap_element(elem); > - > /* Don't touch vring if a fatal error occurred */ > if (vring->broken) { > return; >