|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCHv3] xen/gntdev: add ioctl for grant copy
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@xxxxxxxxxx>
> ---
> 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(©, 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, ©.segments[i], sizeof(seg))) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = gntdev_grant_copy_seg(&batch, &seg,
> ©.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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |