Re: [PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

[David Vrabel <david.vrabel@citrix.com>]

On 09/08/13 15:11, Andres Lagar-Cavilla wrote:
> From: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> When a foreign mapper attempts to map guest frames that are paged out,
> the mapper receives an ENOENT response and will have to try again
> while a helper process pages the target frame back in.
> 
> Gating checks on PRIVCMD_MMAPBATCH* ioctl args were preventing retries
> of mapping calls.
> 
> V2: Fixed autotranslated physmap mode breakage introduced by V1.
> V3: Enforce full range mapping for first ioctl call on a given VMA.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> ---
>  drivers/xen/privcmd.c |   58 +++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index f8e5dd7..938e6e6 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -43,9 +43,12 @@ MODULE_LICENSE("GPL");
>  
>  #define PRIV_VMA_LOCKED ((void *)1)
>  
> -#ifndef HAVE_ARCH_PRIVCMD_MMAP
>  static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma);
> -#endif
> +
> +static int privcmd_enforce_singleshot_mapping_granular(
> +               struct vm_area_struct *vma,
> +               unsigned long addr,
> +               unsigned long nr_pages);
>  
>  static long privcmd_ioctl_hypercall(void __user *udata)
>  {
> @@ -422,19 +425,26 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>  	vma = find_vma(mm, m.addr);
>  	if (!vma ||
>  	    vma->vm_ops != &privcmd_vm_ops ||
> -	    (m.addr != vma->vm_start) ||
> -	    ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) ||
> -	    !privcmd_enforce_singleshot_mapping(vma)) {
> +	    (m.addr < vma->vm_start) ||
> +	    ((m.addr + (nr_pages << PAGE_SHIFT)) > vma->vm_end) ||
> +	    !privcmd_enforce_singleshot_mapping_granular(vma, m.addr, nr_pages)) {
>  		up_write(&mm->mmap_sem);
>  		ret = -EINVAL;
>  		goto out;

I think this unnecessarily calls
privcmd_enforce_singleshot_mapping_granular() on the first call.

If you change the tests to the following, does it work for you?

/* Suitable VMA? */
if (!vma || vma->vm_ops != &privcmd_vm_ops)
	// error

if (!privcmd_enforce_singleshot_mapping(..)) {
        /* Already populated into this VMA once, are we populating a
           subset that is not yet populated? */
	if (is_subset_of_vma(vma, m,addr, nr_pages)
            && privcmd_enforce_single_shot_mapping_granular())
        	// ok
	else
		// error
} else {
	/* Nothing populated in this VMA, must try to fully populate
	   it. */
	if (is_all_of_vma(vma, m.addr, nr_pages))
		// ok
	else
		// error
}


Thanks.

David