Re: [PATCH v2] introduce grant copy for user land

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 12/02/2014 11:13 AM, Thanos Makatos wrote:
> This patch introduces the interface to allow user-space applications
> execute grant-copy operations. This is done by sending an ioctl to the
> grant device.
>
> Signed-off-by: Thanos Makatos <thanos.makatos@citrix.com>
> ---
>   drivers/xen/gntdev.c      |  171 +++++++++++++++++++++++++++++++++++++++++++++
>   include/uapi/xen/gntdev.h |   69 ++++++++++++++++++
>   2 files changed, 240 insertions(+)
>
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 51f4c95..7b4a8e0 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -705,6 +705,174 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
>   	return rc;
>   }
>   
> +static int gntdev_gcopy_batch(int nr_segments, unsigned long gcopy_cb,
> +		struct gntdev_grant_copy_segment __user *__segments, int dir,
> +		int src, int dst) {
> +
> +	static const int batch_size = PAGE_SIZE / (sizeof(struct page*) +
> +		sizeof(struct gnttab_copy) + sizeof(struct gntdev_grant_copy_segment));
> +	struct page **pages = (struct page **)gcopy_cb;
> +	struct gnttab_copy *batch = (struct gnttab_copy *)((unsigned long)pages +
> +			sizeof(struct page*) * batch_size);
> +	struct gntdev_grant_copy_segment *segments =
> +		(struct gntdev_grant_copy_segment *)((unsigned long)batch +
> +				sizeof(struct gnttab_copy) * batch_size);
> +	unsigned int nr_pinned = 0, nr_segs2cp = 0;
> +	int err = 0, i;
> +	const int write = dir == GNTCOPY_IOCTL_g2s;
> +
> +	nr_segments = min(nr_segments, batch_size);
> +
> +	if (unlikely(copy_from_user(segments, __segments,
> +			   sizeof(struct gntdev_grant_copy_segment) * nr_segments))) {
> +		pr_debug("failed to copy %d segments from user", nr_segments);
> +		err = -EFAULT;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < nr_segments; i++) {
> +
> +		xen_pfn_t pgaddr;
> +		unsigned long start, offset;
> +		struct gntdev_grant_copy_segment *seg = &segments[i];
> +
> +		if (dir == GNTCOPY_IOCTL_s2g || dir == GNTCOPY_IOCTL_g2s) {
> +
> +			start = (unsigned long)seg->self.iov.iov_base & PAGE_MASK;
> +			offset = (unsigned long)seg->self.iov.iov_base & ~PAGE_MASK;
> +			if (unlikely(offset + seg->self.iov.iov_len > PAGE_SIZE)) {
> +				pr_warn("segments crossing page boundaries not yet "
> +						"implemented\n");
> +				err = -ENOSYS;
> +				goto out;
> +			}
> +
> +			err = get_user_pages_fast(start, 1, write, &pages[i]);
> +			if (unlikely(err != 1)) {
> +				pr_debug("failed to get user page %lu", start);
> +				err = -EFAULT;
> +				goto out;
> +			}
> +
> +			nr_pinned++;
> +
> +			pgaddr = pfn_to_mfn(page_to_pfn(pages[i]));
> +		}
> +
> +		nr_segs2cp++;
> +
> +		switch (dir) {
> +		case GNTCOPY_IOCTL_g2s: /* copy from guest */
> +			batch[i].len = seg->self.iov.iov_len;
> +			batch[i].source.u.ref = seg->self.ref;
> +			batch[i].source.domid = src;
> +			batch[i].source.offset = seg->self.offset;
> +			batch[i].dest.u.gmfn = pgaddr;
> +			batch[i].dest.domid = DOMID_SELF;
> +			batch[i].dest.offset = offset;
> +			batch[i].flags = GNTCOPY_source_gref;
> +			break;
> +		case GNTCOPY_IOCTL_s2g: /* copy to guest */
> +			batch[i].len = seg->self.iov.iov_len;
> +			batch[i].source.u.gmfn = pgaddr;
> +			batch[i].source.domid = DOMID_SELF;
> +			batch[i].source.offset = offset;
> +			batch[i].dest.u.ref = seg->self.ref;
> +			batch[i].dest.domid = dst;
> +			batch[i].dest.offset = seg->self.offset;
> +			batch[i].flags = GNTCOPY_dest_gref;
> +			break;
> +		case GNTCOPY_IOCTL_g2g: /* copy guest to guest */
> +			batch[i].len = seg->g2g.len;
> +			batch[i].source.u.ref = seg->g2g.src.ref;
> +			batch[i].source.domid = src;
> +			batch[i].source.offset = seg->g2g.src.offset;
> +			batch[i].dest.u.ref = seg->g2g.dst.ref;
> +			batch[i].dest.domid = dst;
> +			batch[i].dest.offset = seg->g2g.dst.offset;
> +			batch[i].flags = GNTCOPY_source_gref | GNTCOPY_dest_gref;
> +			break;
> +		default:
> +			pr_debug("invalid grant-copy direction %d\n", dir);
> +			err = -EINVAL;
> +			goto out;
> +		}
> +	}
> +
> +	gnttab_batch_copy(batch, nr_segs2cp);
> +	for (i = 0; i < nr_segs2cp; i++) {

Can nr_segs2cp be not equal to nr_segments here? If you got to this 
point you have gone through the full loop.

> +		err = put_user(batch[i].status, &__segments[i].status);
> +		if (unlikely(err)) {
> +			pr_debug("failed to copy error code %d to user: %d\n",
> +					batch[i].status, err);
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	for (i = 0; i < nr_pinned; i++)
> +		put_page(pages[i]);
> +
> +	if (unlikely(err))
> +		return err;
> +
> +	return nr_segs2cp;

And I think here it can be either 0 (which is the case of an error) or 
nr_segments. If you error out of the 'for' loop you haven't actually 
copied anything, even though nr_segs2cp might be non-zero.


-boris