[PATCH] introduce grant copy for user land

[PATCH] introduce grant copy for user land

[Thanos Makatos]

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. The number of grant-copy segments is currently limited to
16 in order to simplify the implementation, however the ABI allows an
arbitrary number of operations.

Signed-off-by: Thanos Makatos <thanos.makatos@citrix.com>
---
 drivers/xen/gntdev.c      |  115 +++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/xen/gntdev.h |   38 +++++++++++++++
 2 files changed, 153 insertions(+)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 073b4a1..77d5b14 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -707,6 +707,118 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
 	return rc;
 }
 
+/*
+ * Limit number of operations to simplify implementation. NB the API
+ * allows for an arbitrary number of operations.
+ */
+#define GNTDEV_GRANT_COPY_MAX_OPS 16
+
+static long gntdev_ioctl_grant_copy(struct gntdev_priv *priv, void __user *u)
+{
+	struct ioctl_gntdev_grant_copy op;
+	int err = 0, i;
+	unsigned int nr_pinned = 0;
+	struct gcopy_cb {
+		struct page *pages[GNTDEV_GRANT_COPY_MAX_OPS];
+		struct gnttab_copy batch[GNTDEV_GRANT_COPY_MAX_OPS];
+		struct gntdev_grant_copy_segment
+			segments[GNTDEV_GRANT_COPY_MAX_OPS];
+	} *gcopy_cb = NULL;
+	struct gntdev_grant_copy_segment *segments;
+
+	if (copy_from_user(&op, u, sizeof(op))) {
+		err = -EFAULT;
+		goto out;
+	}
+
+	if (!op.count) {
+		err = 0;
+		goto out;
+	}
+
+	if (op.count > GNTDEV_GRANT_COPY_MAX_OPS) {
+		pr_warn("copying more than %d segments not yet implemented\n",
+			GNTDEV_GRANT_COPY_MAX_OPS);
+		err = -ENOSYS;
+		goto out;
+	}
+
+	gcopy_cb = kmalloc(sizeof(*gcopy_cb), GFP_KERNEL);
+	if (!gcopy_cb) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (copy_from_user(gcopy_cb->segments, op.segments,
+			   sizeof(*op.segments) * op.count)) {
+		err = -EFAULT;
+		goto out;
+	}
+
+	for (i = 0; i < op.count; i++) {
+
+		unsigned long start, offset;
+		struct gntdev_grant_copy_segment *seg = &gcopy_cb->segments[i];
+		xen_pfn_t pgaddr;
+
+		start = (unsigned long)seg->iov.iov_base & PAGE_MASK;
+		offset = (unsigned long)seg->iov.iov_base & ~PAGE_MASK;
+		if (offset + seg->iov.iov_len > PAGE_SIZE) {
+			pr_warn("segments crossing page boundarys not yet implemented\n");
+			err = -ENOSYS;
+			goto out;
+		}
+
+		err = get_user_pages_fast(start, 1, op.dir,
+					  &gcopy_cb->pages[i]);
+		if (err != 1) {
+			err = -EFAULT;
+			goto out;
+		}
+
+		nr_pinned++;
+
+		pgaddr = pfn_to_mfn(page_to_pfn(gcopy_cb->pages[i]));
+
+		gcopy_cb->batch[i].len = seg->iov.iov_len;
+		if (op.dir) {
+			/* copy from guest */
+			gcopy_cb->batch[i].source.u.ref = seg->ref;
+			gcopy_cb->batch[i].source.domid = op.domid;
+			gcopy_cb->batch[i].source.offset = seg->offset;
+			gcopy_cb->batch[i].dest.u.gmfn = pgaddr;
+			gcopy_cb->batch[i].dest.domid = DOMID_SELF;
+			gcopy_cb->batch[i].dest.offset = offset;
+			gcopy_cb->batch[i].flags = GNTCOPY_source_gref;
+		} else {
+			/* copy to guest */
+			gcopy_cb->batch[i].source.u.gmfn = pgaddr;
+			gcopy_cb->batch[i].source.domid = DOMID_SELF;
+			gcopy_cb->batch[i].source.offset = offset;
+			gcopy_cb->batch[i].dest.u.ref = seg->ref;
+			gcopy_cb->batch[i].dest.domid = op.domid;
+			gcopy_cb->batch[i].dest.offset = seg->offset;
+			gcopy_cb->batch[i].flags = GNTCOPY_dest_gref;
+		}
+	}
+
+	gnttab_batch_copy(gcopy_cb->batch, op.count);
+	segments = op.segments;
+	for (i = 0; i < op.count; i++) {
+		err = put_user(gcopy_cb->batch[i].status, &segments[i].status);
+		if (err)
+			goto out;
+	}
+
+out:
+	if (gcopy_cb) {
+		for (i = 0; i < nr_pinned; i++)
+			put_page(gcopy_cb->pages[i]);
+		kfree(gcopy_cb);
+	}
+	return err;
+}
+
 static long gntdev_ioctl(struct file *flip,
 			 unsigned int cmd, unsigned long arg)
 {
@@ -726,6 +838,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 5304bd3..2db3186 100644
--- a/include/uapi/xen/gntdev.h
+++ b/include/uapi/xen/gntdev.h
@@ -33,6 +33,12 @@
 #ifndef __LINUX_PUBLIC_GNTDEV_H__
 #define __LINUX_PUBLIC_GNTDEV_H__
 
+#ifdef __KERNEL__
+#include <linux/uio.h>
+#else
+#include <sys/uio.h>
+#endif
+
 struct ioctl_gntdev_grant_ref {
 	/* The domain ID of the grant to be mapped. */
 	uint32_t domid;
@@ -142,6 +148,38 @@ struct ioctl_gntdev_unmap_notify {
 	uint32_t event_channel_port;
 };
 
+struct gntdev_grant_copy_segment {
+	/*
+	 * source address and length
+	 */
+	struct iovec iov;
+
+	/* the granted page */
+	uint32_t ref;
+
+	/* offset in the granted page */
+	uint16_t offset;
+
+	/* grant copy result (GNTST_XXX) */
+	int16_t status;
+};
+
+#define IOCTL_GNTDEV_GRANT_COPY \
+_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
+struct ioctl_gntdev_grant_copy {
+	/*
+	 * copy direction: 0 to copy to guest, 1 to copy from guest
+	 */
+	int dir;
+
+	/* domain ID */
+	uint32_t domid;
+
+	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 */
-- 
1.7.9.5

Re: [PATCH] introduce grant copy for user land

[David Vrabel]

On 02/10/14 16:15, 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. The number of grant-copy segments is currently limited to
> 16 in order to simplify the implementation, however the ABI allows an
> arbitrary number of operations.

Sorry for not responding earlier.  If I haven't responded to a patch in
a week, a reminder ping is appreciated.

The arbitrary limitations in number of ops and page alignment should be
removed.  I think both can be removed relatively easily by consuming
page aligned chunks of segments and doign the hypercall when a batch of
ops is filled.

> +struct gntdev_grant_copy_segment {
> +	/*
> +	 * source address and length
> +	 */
> +	struct iovec iov;
> +
> +	/* the granted page */
> +	uint32_t ref;
> +
> +	/* offset in the granted page */
> +	uint16_t offset;
> +
> +	/* grant copy result (GNTST_XXX) */
> +	int16_t status;
> +};
> +
> +#define IOCTL_GNTDEV_GRANT_COPY \
> +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
> +struct ioctl_gntdev_grant_copy {
> +	/*
> +	 * copy direction: 0 to copy to guest, 1 to copy from guest
> +	 */
> +	int dir;

I think this dir should be per-segment and use the GNTCPY_source_gref
and GNTCOPY_dest_gref flags, since per-op direction is what the
hypercall provides.

> +
> +	/* domain ID */
> +	uint32_t domid;
> +
> +	unsigned int count;
> +
> +	struct gntdev_grant_copy_segment __user *segments;
> +};

David

Re: [PATCH] introduce grant copy for user land

[Thanos Makatos]

> The arbitrary limitations in number of ops and page alignment should be
> removed.  I think both can be removed relatively easily by consuming page
> aligned chunks of segments and doign the hypercall when a batch of ops is
> filled.

Wouldn't this lead to multiple calls to gnttab_batch_copy(), potentially hurting performance?

> > +#define IOCTL_GNTDEV_GRANT_COPY \
> > +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
> > +struct ioctl_gntdev_grant_copy {
> > +	/*
> > +	 * copy direction: 0 to copy to guest, 1 to copy from guest
> > +	 */
> > +	int dir;
> 
> I think this dir should be per-segment and use the GNTCPY_source_gref and
> GNTCOPY_dest_gref flags, since per-op direction is what the hypercall
> provides.

OK.

Re: [PATCH] introduce grant copy for user land

[David Vrabel]

On 11/11/14 12:27, Thanos Makatos wrote:
>> The arbitrary limitations in number of ops and page alignment should be
>> removed.  I think both can be removed relatively easily by consuming page
>> aligned chunks of segments and doign the hypercall when a batch of ops is
>> filled.
> 
> Wouldn't this lead to multiple calls to gnttab_batch_copy(), potentially hurting performance?

The incremental benefits of batching diminishes as the batch size increase.

We also don't want multi-page allocations in this driver, so the batch
size should be set accordingly.

>>> +#define IOCTL_GNTDEV_GRANT_COPY \
>>> +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
>>> +struct ioctl_gntdev_grant_copy {
>>> +	/*
>>> +	 * copy direction: 0 to copy to guest, 1 to copy from guest
>>> +	 */
>>> +	int dir;
>>
>> I think this dir should be per-segment and use the GNTCPY_source_gref and
>> GNTCOPY_dest_gref flags, since per-op direction is what the hypercall
>> provides.
> 
> OK.

The interface should also support grant to grant copies.

David

Re: [PATCH] introduce grant copy for user land

[Thanos Makatos]

> On 11/11/14 12:27, Thanos Makatos wrote:
> >> The arbitrary limitations in number of ops and page alignment should
> >> be removed.  I think both can be removed relatively easily by
> >> consuming page aligned chunks of segments and doign the hypercall
> >> when a batch of ops is filled.
> >
> > Wouldn't this lead to multiple calls to gnttab_batch_copy(), potentially
> hurting performance?
> 
> The incremental benefits of batching diminishes as the batch size increase.

To overcome the page alignment limitation, struct gntdev_grant_copy_segment
will have to contain a set of grant refs instead of just one grant ref,
correct?
 
> We also don't want multi-page allocations in this driver, so the batch size
> should be set accordingly.

Why not?

> The interface should also support grant to grant copies.

To support grant to grant copies, struct gntdev_grant_copy_segment will have
to contain an additional set of grant refs, correct?

Re: [PATCH] introduce grant copy for user land

[David Vrabel]

On 11/11/14 14:42, Thanos Makatos wrote:
>> On 11/11/14 12:27, Thanos Makatos wrote:
>>>> The arbitrary limitations in number of ops and page alignment should
>>>> be removed.  I think both can be removed relatively easily by
>>>> consuming page aligned chunks of segments and doign the hypercall
>>>> when a batch of ops is filled.
>>>
>>> Wouldn't this lead to multiple calls to gnttab_batch_copy(), potentially
>> hurting performance?
>>
>> The incremental benefits of batching diminishes as the batch size increase.
> 
> To overcome the page alignment limitation, struct gntdev_grant_copy_segment
> will have to contain a set of grant refs instead of just one grant ref,
> correct?

Oh, that would be awkward.  Keeping the alignment requirement would be best.

>> We also don't want multi-page allocations in this driver, so the batch size
>> should be set accordingly.
> 
> Why not?

Multi-page allocations are more likely to fail because of memory
fragmentation.

>> The interface should also support grant to grant copies.
> 
> To support grant to grant copies, struct gntdev_grant_copy_segment will have
> to contain an additional set of grant refs, correct?

The hypervisor ABI uses a union of handle and grant ref.  Perhaps
something similar for the kernel interface?

David

Re: [PATCH] introduce grant copy for user land

[Thanos Makatos]

> >> We also don't want multi-page allocations in this driver, so the
> >> batch size should be set accordingly.
> >
> > Why not?
> 
> Multi-page allocations are more likely to fail because of memory
> fragmentation.

Since we're keeping the alignment restriction, the code stays pretty much the same. Therefore what we need is to
pick the correct value for GNTDEV_GRANT_COPY_MAX_OPS such that sizeof(struct gcopy_cb) < PAGE_SIZE, correct?

> >> The interface should also support grant to grant copies.
> >
> > To support grant to grant copies, struct gntdev_grant_copy_segment
> > will have to contain an additional set of grant refs, correct?
> 
> The hypervisor ABI uses a union of handle and grant ref.  Perhaps something
> similar for the kernel interface?

Indeed, this is what I had in mind.