Re: [PATCHv3] xen/gntdev: add ioctl for grant copy

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Nov 27, 2015 at 05:17:05PM +0000, David Vrabel wrote:
> Add IOCTL_GNTDEV_GRANT_COPY to allow applications to copy between user
> space buffers and grant references.
> 
> This interface is similar to the GNTTABOP_copy hypercall ABI except
> the local buffers are provided using a virtual address (instead of a
> GFN and offset).  To avoid userspace from having to page align its
> buffers the driver will use two or more ops if required.
> 
> If the ioctl returns 0, the application must check the status of each
> segment with the segments status field.  If the ioctl returns a -ve
> error code (EINVAL or EFAULT), the status of individual ops is
> undefined.

Are there any test tools that could be used for this? To make sure that
regression wise this does not get broken?

> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> v3:
> - Rewrite with different API that matches the capabilities of the
>   hypervisor ABI and eliminates some of the size/alignment
>   restrictions.
> ---
>  drivers/xen/gntdev.c      | 193 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/xen/gntdev.h |  47 +++++++++++
>  2 files changed, 240 insertions(+)
> 
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 2ea0b3b..5d78c79 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -748,6 +748,196 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
>  	return rc;
>  }
>  
> +#define GNTDEV_COPY_BATCH 16
> +
> +struct gntdev_copy_batch {
> +	struct gnttab_copy ops[GNTDEV_COPY_BATCH];
> +	struct page *pages[2 * GNTDEV_COPY_BATCH];
> +	s16 __user *status[GNTDEV_COPY_BATCH];
> +	unsigned int nr_ops;
> +	unsigned int nr_pages;
> +};
> +
> +static int gntdev_get_page(struct gntdev_copy_batch *batch, void __user *virt,
> +			   unsigned long *gfn)
> +{
> +	unsigned long addr = (unsigned long)virt;
> +	struct page *page;
> +	unsigned long xen_pfn;
> +	int ret;
> +
> +	ret = get_user_pages_fast(addr, 1, 1, &page);

Could the '1,1' have a comment please?

> +	if (ret < 0)
> +		return ret;
> +
> +	batch->pages[batch->nr_pages++] = page;
> +
> +	xen_pfn = page_to_xen_pfn(page) + XEN_PFN_DOWN(addr & ~PAGE_MASK);
> +	*gfn = pfn_to_gfn(xen_pfn);
> +
> +	return 0;
> +}
> +
> +static void gntdev_put_pages(struct gntdev_copy_batch *batch)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < batch->nr_pages; i++)
> +		put_page(batch->pages[i]);
> +	batch->nr_pages = 0;
> +}
> +
> +static int gntdev_copy(struct gntdev_copy_batch *batch)
> +{
> +	unsigned int i;
> +
> +	gnttab_batch_copy(batch->ops, batch->nr_ops);
> +	gntdev_put_pages(batch);
> +
> +	/*
> +	 * For each completed op, update the status if the op failed
> +	 * and a previous failure for the segment hasn't been
> +	 * recorded.

How could an previous failure not be recorded? Could you mention that
in this nice comment please?

> +	 */
> +	for (i = 0; i < batch->nr_ops; i++) {
> +		s16 status = batch->ops[i].status;
> +		s16 old_status;
> +
> +		if (status == GNTST_okay)
> +			continue;
> +
> +		if (__get_user(old_status, batch->status[i]))
> +			return -EFAULT;
> +
> +		if (old_status != GNTST_okay)
> +			continue;
> +
> +		if (__put_user(status, batch->status[i]))
> +			return -EFAULT;
> +	}
> +
> +	batch->nr_ops = 0;
> +	return 0;
> +}
> +
> +static int gntdev_grant_copy_seg(struct gntdev_copy_batch *batch,
> +				 struct gntdev_grant_copy_segment *seg,
> +				 s16 __user *status)
> +{
> +	uint16_t copied = 0;
> +
> +	/* Can't cross page if source/dest is a grant ref. */
> +	if (seg->flags & GNTCOPY_source_gref) {
> +		if (seg->source.foreign.offset + seg->len > XEN_PAGE_SIZE)
> +			return -EINVAL;
> +	}
> +	if (seg->flags & GNTCOPY_dest_gref) {
> +		if (seg->dest.foreign.offset + seg->len > XEN_PAGE_SIZE)
> +			return -EINVAL;
> +	}
> +
> +	if (put_user(GNTST_okay, status))
> +		return -EFAULT;
> +
> +	while (copied < seg->len) {
> +		struct gnttab_copy *op;
> +		void __user *virt;
> +		size_t len, off;
> +		unsigned long gfn;
> +		int ret;
> +
> +		if (batch->nr_ops >= GNTDEV_COPY_BATCH) {
> +			ret = gntdev_copy(batch);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		len = seg->len - copied;
> +
> +		op = &batch->ops[batch->nr_ops];
> +		op->flags = 0;
> +
> +		if (seg->flags & GNTCOPY_source_gref) {
> +			op->source.u.ref = seg->source.foreign.ref;
> +			op->source.domid = seg->source.foreign.domid;
> +			op->source.offset = seg->source.foreign.offset + copied;
> +			op->flags |= GNTCOPY_source_gref;
> +		} else {
> +			virt = seg->source.virt + copied;
> +			off = (unsigned long)virt & ~XEN_PAGE_MASK;
> +			len = min(len, XEN_PAGE_SIZE - off);
> +
> +			ret = gntdev_get_page(batch, virt, &gfn);
> +			if (ret < 0)
> +				return ret;
> +
> +			op->source.u.gmfn = gfn;
> +			op->source.domid = DOMID_SELF;
> +			op->source.offset = off;
> +		}
> +
> +		if (seg->flags & GNTCOPY_dest_gref) {
> +			op->dest.u.ref = seg->dest.foreign.ref;
> +			op->dest.domid = seg->dest.foreign.domid;
> +			op->dest.offset = seg->dest.foreign.offset + copied;
> +			op->flags |= GNTCOPY_dest_gref;
> +		} else {
> +			virt = seg->dest.virt + copied;
> +			off = (unsigned long)virt & ~XEN_PAGE_MASK;
> +			len = min(len, XEN_PAGE_SIZE - off);
> +
> +			ret = gntdev_get_page(batch, virt, &gfn);
> +			if (ret < 0)
> +				return ret;
> +
> +			op->dest.u.gmfn = gfn;
> +			op->dest.domid = DOMID_SELF;
> +			op->dest.offset = off;
> +		}
> +
> +		op->len = len;
> +		copied += len;
> +
> +		batch->status[batch->nr_ops] = status;
> +		batch->nr_ops++;
> +	}
> +
> +	return 0;
> +}
> +
> +static long gntdev_ioctl_grant_copy(struct gntdev_priv *priv, void __user *u)
> +{
> +	struct ioctl_gntdev_grant_copy copy;
> +	struct gntdev_copy_batch batch = { .nr_ops = 0, .nr_pages = 0, };
> +	unsigned int i;
> +	int ret = 0;
> +
> +	if (copy_from_user(&copy, u, sizeof(copy)))
> +		return -EFAULT;
> 
+

No check on the .nr_pages' ? What if it is 0xfffffffffffffffffffffffff?

Ditto for .nr_ops?

> +	for (i = 0; i < copy.count; i++) {
> +		struct gntdev_grant_copy_segment seg;
> +
> +		if (copy_from_user(&seg, &copy.segments[i], sizeof(seg))) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		ret = gntdev_grant_copy_seg(&batch, &seg, &copy.segments[i].status);
> +		if (ret < 0)
> +			goto out;
> +
> +		cond_resched();
> +	}
> +	if (batch.nr_ops)
> +		ret = gntdev_copy(&batch);
> +	return ret;
> +
> +  out:
> +	gntdev_put_pages(&batch);
> +	return ret;
> +}
> +
>  static long gntdev_ioctl(struct file *flip,
>  			 unsigned int cmd, unsigned long arg)
>  {
> @@ -767,6 +957,9 @@ static long gntdev_ioctl(struct file *flip,
>  	case IOCTL_GNTDEV_SET_UNMAP_NOTIFY:
>  		return gntdev_ioctl_notify(priv, ptr);
>  
> +	case IOCTL_GNTDEV_GRANT_COPY:
> +		return gntdev_ioctl_grant_copy(priv, ptr);
> +
>  	default:
>  		pr_debug("priv %p, unknown cmd %x\n", priv, cmd);
>  		return -ENOIOCTLCMD;
> diff --git a/include/uapi/xen/gntdev.h b/include/uapi/xen/gntdev.h
> index aa7610a..4b02343 100644
> --- a/include/uapi/xen/gntdev.h
> +++ b/include/uapi/xen/gntdev.h
> @@ -144,6 +144,53 @@ struct ioctl_gntdev_unmap_notify {
>  	__u32 event_channel_port;
>  };
>  
> +struct gntdev_grant_copy_segment {
> +	union {
> +		void __user *virt;
> +		struct {
> +			grant_ref_t ref;
> +			__u16 offset;
> +			domid_t domid;
> +		} foreign;
> +	} source, dest;
> +	__u16 len;
> +
> +	__u16 flags;  /* GNTCOPY_* */
> +	__s16 status; /* GNTST_* */
> +};
> +
> +/*
> + * Copy between grant references and local buffers.
> + *
> + * The copy is split into @count @segments, each of which can copy
> + * to/from one grant reference.
> + *
> + * Each segment is similar to struct gnttab_copy in the hypervisor ABI
> + * except the local buffer is specified using a virtual address
> + * (instead of a GFN and offset).
> + *
> + * The local buffer may cross a Xen page boundary -- the driver will
> + * split segments into multiple ops if required.
> + *
> + * Returns 0 if all segments have been processed and @status in each
> + * segment is valid.  Note that one or more segments may have failed
> + * (status != GNTST_okay).
> + *
> + * If the driver had to split a segment into two or more ops, @status
> + * includes the status of the first failed op for that segment (or
> + * GNTST_okay if all ops were successful).
> + *
> + * If -1 is returned, the status of all segments is undefined.
> + *
> + * - EINVAL: a segment crosses the boundary of a foreign page.
> + */
> +#define IOCTL_GNTDEV_GRANT_COPY \
> +	_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
> +struct ioctl_gntdev_grant_copy {
> +	unsigned int count;
> +	struct gntdev_grant_copy_segment __user *segments;
> +};
> +
>  /* Clear (set to zero) the byte specified by index */
>  #define UNMAP_NOTIFY_CLEAR_BYTE 0x1
>  /* Send an interrupt on the indicated event channel */
> -- 
> 2.1.4
>