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

[Thanos Makatos <thanos.makatos@xxxxxxxxxx>]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@xxxxxxxxxx]
> Sent: 05 December 2014 6:06 PM
> To: Thanos Makatos; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: David Vrabel
> Subject: Re: [Xen-devel] [PATCH v2] introduce grant copy for user land
> 
> 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@xxxxxxxxxx>
> > ---
> >   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.

Correct.

> > +           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.

If an error has occurred, "err" will be returned so the value of either 
"nr_segs2cp" or "nr_segments" is irrelevant. If no error occurs, then we have 
copied "nr_segments", therefore this is what we should return. I think I got 
something wrong and I thought there was some corner case so that why we needed 
an extra variable, but it doesn't seem to be the case. I'll replace 
"nr_segments" with "nr_segments".

> -boris
> 


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