Re: [Xen-devel] [PATCH 3/3] xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 23/08/12 20:40, Konrad Rzeszutek Wilk wrote:
> On Thu, Aug 23, 2012 at 06:13:46PM +0100, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@xxxxxxxxxx>
>>
>> PRIVCMD_MMAPBATCH_V2 extends PRIVCMD_MMAPBATCH with an additional
>> field for reporting the error code for every frame that could not be
>> mapped.  libxc prefers PRIVCMD_MMAPBATCH_V2 over PRIVCMD_MMAPBATCH.
[...]
>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
>> index f8c1b6d..4f97160 100644
>> --- a/drivers/xen/privcmd.c
>> +++ b/drivers/xen/privcmd.c
>> @@ -275,34 +276,58 @@ static int mmap_return_errors(void *data, void *state)
>>  {
>>      xen_pfn_t *mfnp = data;
>>      struct mmap_batch_state *st = state;
>> +    int ret = 0;
>>  
>> -    return put_user(*mfnp, st->user++);
>> +    if (st->user_err) {
>> +            if ((*mfnp & 0xf0000000U) == 0xf0000000U)
>> +                    ret = -ENOENT;
>> +            else if ((*mfnp & 0xf0000000U) == 0x80000000U)
>> +                    ret = -EINVAL;
> 
> Yikes. Any way those 0xf000.. and 0x8000 can at least be #defined

Agreed.

>> +            else
>> +                    ret = 0;
>> +            return __put_user(ret, st->user_err);
>> +    } else
>> +            return __put_user(*mfnp, st->user_mfn++);
>>  }
>>  
>>  static struct vm_operations_struct privcmd_vm_ops;
>>  
>> -static long privcmd_ioctl_mmap_batch(void __user *udata)
>> +static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>>  {
>>      int ret;
>> -    struct privcmd_mmapbatch m;
>> +    struct privcmd_mmapbatch_v2 m;
>>      struct mm_struct *mm = current->mm;
>>      struct vm_area_struct *vma;
>>      unsigned long nr_pages;
>>      LIST_HEAD(pagelist);
>>      struct mmap_batch_state state;
>>  
>> +    printk("%s(%d)\n", __func__, version);
>> +
> 
> Hehe.

Heh.  I didn't polish up these patches as I wasn't sure this new
interface was useful.

>>      if (!xen_initial_domain())
>>              return -EPERM;
>>  
>> -    if (copy_from_user(&m, udata, sizeof(m)))
>> -            return -EFAULT;
>> +    if (version == 1) {
>> +            if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch)))
>> +                    return -EFAULT;
>> +            if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
>> +                    return -EFAULT;
> 
> That is new..

We use access_ok() here so we can use the less expensive __get_user()
and __put_user() later on.

>> +            if (copy_from_user(&m, udata, sizeof(struct 
>> privcmd_mmapbatch_v2)))
>> +                    return -EFAULT;
>> +            if (!access_ok(VERIFY_READ, m.arr, m.num * sizeof(*m.arr)))
>> +                    return -EFAULT;
>> +            if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
>> +                    return -EFAULT;
> 
> I think the VERIFY_WRITE can cover both versions?

Yes, VERIFY_WRITE is a superset of VERIFY_READ.  In v1, m.arr is
read/write but in v2, m.arr is const so we use VERIFY_READ so the
userspace buffer can reside in a read-only section.

>> --- a/include/xen/privcmd.h
>> +++ b/include/xen/privcmd.h
>> @@ -62,6 +62,14 @@ struct privcmd_mmapbatch {
>>      xen_pfn_t __user *arr; /* array of mfns - top nibble set on err */
>>  };
>>  
>> +struct privcmd_mmapbatch_v2 {
>> +    unsigned int num; /* number of pages to populate */
> 
> unsigend int? Not 'u32'?
>> +    domid_t dom;      /* target domain */
>> +    __u64 addr;       /* virtual address */
>> +    const xen_pfn_t __user *arr; /* array of mfns */
>> +    int __user *err;  /* array of error codes */
> 
> int? Not a specific type?

It's an existing interface supported by classic Xen kernels and
currently being used by libxc.  So while I agree that it's not the best
interface, I don't think it can be changed.

David

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