On Fri, May 3, 2013 at 6:02 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> 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.
>
Will fix,
>> 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.
>
Ok,
>> + 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().
>
Ok,
>> @@ -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.
>
Will fix
>> 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.
Will fix.
Thanks,
Pingfan
