[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(&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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.