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

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

On Aug 23, 2012, at 1:13 PM, 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.
> 
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
> drivers/xen/privcmd.c |   54 +++++++++++++++++++++++++++++++++++++++----------
> include/xen/privcmd.h |   10 +++++++++
> 2 files changed, 53 insertions(+), 11 deletions(-)
> 
> 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
> @@ -248,7 +248,8 @@ struct mmap_batch_state {
>       struct vm_area_struct *vma;
>       int err;
> 
> -     xen_pfn_t __user *user;
> +     xen_pfn_t __user *user_mfn;
> +     int __user *user_err;
> };
> 
> static int mmap_batch_fn(void *data, void *state)
> @@ -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;
Wires crossed. 0x80..0 is ENOENT, 0xf0..0 is EINVAL. Really in need of 
constants.

> +             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);
Surely this and other unconditional printk's to go away in next round…

Thanks
Andres

> +
>       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;
> +             m.err = NULL;
> +     } else {
> +             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;
> +     }
> 
>       nr_pages = m.num;
>       if ((m.num <= 0) || (nr_pages > (LONG_MAX >> PAGE_SHIFT)))
>               return -EINVAL;
> 
>       ret = gather_array(&pagelist, m.num, sizeof(xen_pfn_t),
> -                        m.arr);
> +                        (xen_pfn_t *)m.arr);
> 
>       if (ret || list_empty(&pagelist))
>               goto out;
> @@ -331,10 +356,11 @@ static long privcmd_ioctl_mmap_batch(void __user *udata)
>       up_write(&mm->mmap_sem);
> 
>       if (state.err > 0) {
> -             state.user = m.arr;
> +             state.user_mfn = (xen_pfn_t *)m.arr;
> +             state.user_err = m.err;
>               ret = traverse_pages(m.num, sizeof(xen_pfn_t),
> -                            &pagelist,
> -                            mmap_return_errors, &state);
> +                                  &pagelist,
> +                                  mmap_return_errors, &state);
>       }
> 
> out:
> @@ -359,7 +385,13 @@ static long privcmd_ioctl(struct file *file,
>               break;
> 
>       case IOCTL_PRIVCMD_MMAPBATCH:
> -             ret = privcmd_ioctl_mmap_batch(udata);
> +             ret = privcmd_ioctl_mmap_batch(udata, 1);
> +             printk("%s() batch ret = %d\n", __func__, ret);
> +             break;
> +
> +     case IOCTL_PRIVCMD_MMAPBATCH_V2:
> +             ret = privcmd_ioctl_mmap_batch(udata, 2);
> +             printk("%s() batch ret = %d\n", __func__, ret);
>               break;
> 
>       default:
> diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
> index 17857fb..9fa27c4 100644
> --- 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 */
> +     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 */
> +};
> +
> /*
>  * @cmd: IOCTL_PRIVCMD_HYPERCALL
>  * @arg: &privcmd_hypercall_t
> @@ -73,5 +81,7 @@ struct privcmd_mmapbatch {
>       _IOC(_IOC_NONE, 'P', 2, sizeof(struct privcmd_mmap))
> #define IOCTL_PRIVCMD_MMAPBATCH                                       \
>       _IOC(_IOC_NONE, 'P', 3, sizeof(struct privcmd_mmapbatch))
> +#define IOCTL_PRIVCMD_MMAPBATCH_V2                           \
> +     _IOC(_IOC_NONE, 'P', 4, sizeof(struct privcmd_mmapbatch_v2))
> 
> #endif /* __LINUX_PUBLIC_PRIVCMD_H__ */
> -- 
> 1.7.2.5
> 


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