Re: [Xen-devel] [PATCHv3] xen/gntdev: add ioctl for grant copy

[David Vrabel <david.vrabel@xxxxxxxxxx>]

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(&copy, 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.

Attachment: gntdev-copy.c
Description: Text Data

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