From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCHv3] xen/gntdev: add ioctl for grant copy Date: Tue, 1 Dec 2015 10:35:17 +0000 Message-ID: <565D77E5.6030309@citrix.com> References: <1448644625-28449-1-git-send-email-david.vrabel@citrix.com> <20151130213916.GE14317@char.us.oracle.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------010204030707040902070103" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a3iHQ-0004b8-QM for xen-devel@lists.xenproject.org; Tue, 01 Dec 2015 10:35:29 +0000 In-Reply-To: <20151130213916.GE14317@char.us.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk , David Vrabel Cc: xen-devel@lists.xenproject.org, Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org --------------010204030707040902070103 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit On 30/11/15 21:39, Konrad Rzeszutek Wilk wrote: > 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? See attached. >> +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? All the negatives in this sentence are confusing so I'll reword. If we haven't recorded a failure for the previous op in the segment it's because it succeeded. The aim here is to record the first op failure for a segment. From the ioctl documentation: + * 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). >> +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? batch.nr_ops and batch.nr_pages are internal, not supplied by the user and are limited by the batch size. I guess you're really asking about the value of copy.count? This doesn't matter because we process the segments one by one and have a fixed batch size for the ops. There's also a cond_resched() every segment so submitting a single ioctl with a crazy number of segments is really no different from userspace calling the ioctl a crazy number of times. David p.s. Please trim your replies when reviewing. --------------010204030707040902070103 Content-Type: text/x-csrc; name="gntdev-copy.c" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="gntdev-copy.c" #include #include #include #include #include #include #include #include #include struct gntdev_grant_copy_segment { union { void *virt; struct { grant_ref_t ref; uint16_t offset; domid_t domid; } foreign; } source, dest; uint16_t len; uint16_t flags; /* GNTCOPY_* */ int16_t status; /* GNTST_* */ }; #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 *segments; }; #define NR_REFS 15 static void random_fill(char *buf, size_t len) { int fd; int r =3D 0; fd =3D open("/dev/urandom", O_RDONLY); if (fd < 0) { perror("open(/dev/urandom)"); exit(1); } while (r < len) { int ret; ret =3D read(fd, buf + r, len - r); if (ret < 0) { perror("read"); exit(1); } r +=3D ret; } close(fd); } static void do_copy(struct ioctl_gntdev_grant_copy *copy, char *a, char *= b) { int fd; unsigned int i; int ret; random_fill(a, NR_REFS * 4096); fd =3D open("/dev/xen/gntdev", O_RDWR); if (fd < 0) { perror("open(/dev/xen/gntdev)"); exit(1); } ret =3D ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, copy); if (ret < 0) { perror("ioctl(IOCTL_GNTDEV_GRANT_COPY)"); exit(1); } close(fd); for (i =3D 0; i < NR_REFS; i++) { if (copy->segments[i].status !=3D GNTST_okay) { fprintf(stderr, "[%u]: bad copy status: %d\n", i, copy->segme= nts[i].status); exit(1); } } if (memcmp(a, b, NR_REFS * 4096) !=3D 0) { fprintf(stderr, "a !=3D b\n"); exit(1); } } int main(void) { xc_gntshr *gs; xc_gnttab *gt; uint32_t refs[NR_REFS]; struct gntdev_grant_copy_segment seg[NR_REFS]; struct ioctl_gntdev_grant_copy copy; char *shared; char local[4096 * NR_REFS]; unsigned int i; gs =3D xc_gntshr_open(NULL, 0); if (!gs) { perror("xc_gntshr_open"); exit(1); } gt =3D xc_gnttab_open(NULL, 0); if (!gt) { perror("xc_gnttab_open"); exit(1); } shared =3D xc_gntshr_share_pages(gs, 0, NR_REFS, refs, 1); if (shared =3D=3D NULL) { perror("xc_gntshr_share_pages"); exit(1); } /* * 1. ref -> local */ printf("ref -> local\n"); for (i =3D 0; i < NR_REFS; i++) { seg[i].source.foreign.ref =3D refs[i]; seg[i].source.foreign.offset =3D 0; seg[i].source.foreign.domid =3D 0; seg[i].dest.virt =3D local + i * 4096; seg[i].len =3D 4096; seg[i].flags =3D GNTCOPY_source_gref; } copy.count =3D NR_REFS; copy.segments =3D seg; do_copy(©, shared, local); printf(" ok\n"); /* * 2. local -> ref */ printf("local -> ref\n"); for (i =3D 0; i < NR_REFS; i++) { seg[i].source.virt =3D local + i * 4096; seg[i].dest.foreign.ref =3D refs[i]; seg[i].dest.foreign.offset =3D 0; seg[i].dest.foreign.domid =3D 0; seg[i].len =3D 4096; seg[i].flags =3D GNTCOPY_dest_gref; } copy.count =3D NR_REFS; copy.segments =3D seg; do_copy(©, local, shared); printf(" ok\n"); return 0; =20 } --------------010204030707040902070103 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------010204030707040902070103--