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.
