From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44420) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UYCpQ-0005Y5-5T for qemu-devel@nongnu.org; Fri, 03 May 2013 06:03:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UYCpL-00004z-7i for qemu-devel@nongnu.org; Fri, 03 May 2013 06:03:00 -0400 Received: from mail-wi0-x233.google.com ([2a00:1450:400c:c05::233]:46659) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UYCpK-0008W4-Ur for qemu-devel@nongnu.org; Fri, 03 May 2013 06:02:55 -0400 Received: by mail-wi0-f179.google.com with SMTP id l13so431337wie.12 for ; Fri, 03 May 2013 03:02:54 -0700 (PDT) Date: Fri, 3 May 2013 12:02:51 +0200 From: Stefan Hajnoczi Message-ID: <20130503100251.GD24864@stefanha-thinkpad.redhat.com> References: <1367549122-2948-1-git-send-email-qemulist@gmail.com> <1367549122-2948-6-git-send-email-qemulist@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1367549122-2948-6-git-send-email-qemulist@gmail.com> Subject: Re: [Qemu-devel] [PATCH v2 5/6] Vring: use hostmem's RAM safe api List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liu Ping Fan Cc: Peter Maydell , Anthony Liguori , Jan Kiszka , qemu-devel@nongnu.org, Vasilis Liaskovitis , Stefan Hajnoczi , Paolo Bonzini On Fri, May 03, 2013 at 10:45:21AM +0800, Liu Ping Fan wrote: > @@ -109,11 +110,14 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring) > static int get_indirect(Vring *vring, > struct iovec iov[], struct iovec *iov_end, > unsigned int *out_num, unsigned int *in_num, > - struct vring_desc *indirect) > + struct vring_desc *indirect, > + MemoryRegion ***mrs) > { > struct vring_desc desc; > unsigned int i = 0, count, found = 0; > - > + MemoryRegion **cur = *mrs; > + int ret = 0; > + MemoryRegion *tmp; > /* Sanity check */ > if (unlikely(indirect->len % sizeof(desc))) { > error_report("Invalid length in indirect descriptor: " > @@ -132,19 +136,23 @@ static int get_indirect(Vring *vring, > return -EFAULT; > } > > + *mrs[0] = NULL; The goto fail case is broken when we fail with cur > *mrs before calling hostmem_lookup(..., cur, ...) since *cur will be undefined. > do { > struct vring_desc *desc_ptr; > > /* Translate indirect descriptor */ > desc_ptr = hostmem_lookup(indirect->addr + found * sizeof(desc), > - sizeof(desc), NULL, false); > + sizeof(desc), > + &tmp, Please use a more descriptive name, like desc_mr. This function is fairly involved so the variable names should be descriptive. > + 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; > - return -EFAULT; > + ret = -EFAULT; > + goto fail; > } > desc = *desc_ptr; > > @@ -155,31 +163,40 @@ static int get_indirect(Vring *vring, > error_report("Loop detected: last one at %u " > "indirect size %u", i, count); > vring->broken = true; > - return -EFAULT; > + memory_region_unref(tmp); > + ret = -EFAULT; > + goto fail; > } No need to hold onto tmp and handle all these error cases. After the barrier() desc_ptr is no longer used because we've loaded the descriptor into a local struct. Please unref tmp right after barrier(). > @@ -209,15 +240,20 @@ static int get_indirect(Vring *vring, > * never a valid descriptor number) if none was found. A negative code is > * returned on error. > * > + * @mrs should be the same cnt as iov[] > + * > * Stolen from linux/drivers/vhost/vhost.c. > */ > int vring_pop(VirtIODevice *vdev, Vring *vring, > struct iovec iov[], struct iovec *iov_end, > - unsigned int *out_num, unsigned int *in_num) > + unsigned int *out_num, unsigned int *in_num, > + MemoryRegion **mrs) > { > struct vring_desc desc; > unsigned int i, head, found = 0, num = vring->vr.num; > uint16_t avail_idx, last_avail_idx; > + MemoryRegion **indirect, **cur = mrs; > + int ret = 0; > > /* If there was a fatal error then refuse operation */ > if (vring->broken) { > @@ -263,17 +299,20 @@ int vring_pop(VirtIODevice *vdev, Vring *vring, > *out_num = *in_num = 0; > > i = head; > + mrs[0] = NULL; Same goto fail issue here when cur > *mrs before hostmem_lookup(..., cur, ...) has been called. > do { > if (unlikely(i >= num)) { > error_report("Desc index is %u > %u, head = %u", i, num, head); > vring->broken = true; > - return -EFAULT; > + ret = -EFAULT; > + goto fail; > } > if (unlikely(++found > num)) { > error_report("Loop detected: last one at %u vq size %u head %u", > i, num, head); > vring->broken = true; > - return -EFAULT; > + ret = -EFAULT; > + goto fail; > } > desc = vring->vr.desc[i]; > > @@ -281,10 +320,13 @@ int vring_pop(VirtIODevice *vdev, Vring *vring, > barrier(); > > if (desc.flags & VRING_DESC_F_INDIRECT) { > - int ret = get_indirect(vring, iov, iov_end, out_num, in_num, &desc); > + indirect = cur; > + int ret = get_indirect(vring, iov, iov_end, out_num, in_num, &desc, > + &indirect); > if (ret < 0) { > - return ret; > + goto fail; Indentation.