Re: [PATCH] xen/gntdev,gntalloc: Remove unneeded VM flags

[Ian Campbell <Ian.Campbell@citrix.com>]

On Mon, 2011-03-07 at 18:19 +0000, Daniel De Graaf wrote:
> The only time when granted pages need to be treated specially is when
> using Xen's PTE modification for grant mappings owned by another domain.
> Otherwise, the area does not require VM_DONTCOPY and VM_PFNMAP, since it
> can be accessed just like any other page of RAM.

This needs clarifying that it only applies to gntalloc and/or HVM
guests, I don't think it applies to gntdev in PV guests, right?

The actual patch seems to make some reference counting changes which
don't seem to be covered by the above description, and they don't seem
to be balanced (I see increments but no decrements).

Ian.

> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/gntalloc.c |   14 ++++++++++++--
>  drivers/xen/gntdev.c   |   16 ++++++++++++++--
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> index a7ffdfe..f6832f4 100644
> --- a/drivers/xen/gntalloc.c
> +++ b/drivers/xen/gntalloc.c
> @@ -427,6 +427,17 @@ static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
>  	return 0;
>  }
>  
> +static void gntalloc_vma_open(struct vm_area_struct *vma)
> +{
> +	struct gntalloc_gref *gref = vma->vm_private_data;
> +	if (!gref)
> +		return;
> +
> +	spin_lock(&gref_lock);
> +	gref->users++;
> +	spin_unlock(&gref_lock);
> +}
> +
>  static void gntalloc_vma_close(struct vm_area_struct *vma)
>  {
>  	struct gntalloc_gref *gref = vma->vm_private_data;
> @@ -441,6 +452,7 @@ static void gntalloc_vma_close(struct vm_area_struct *vma)
>  }
>  
>  static struct vm_operations_struct gntalloc_vmops = {
> +	.open = gntalloc_vma_open,
>  	.close = gntalloc_vma_close,
>  };
>  
> @@ -471,8 +483,6 @@ static int gntalloc_mmap(struct file *filp, struct vm_area_struct *vma)
>  	vma->vm_private_data = gref;
>  
>  	vma->vm_flags |= VM_RESERVED;
> -	vma->vm_flags |= VM_DONTCOPY;
> -	vma->vm_flags |= VM_PFNMAP | VM_PFN_AT_MMAP;
>  
>  	vma->vm_ops = &gntalloc_vmops;
>  
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 2faf797..69c787a 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -334,17 +334,26 @@ static int unmap_grant_pages(struct grant_map *map, int offset, int pages)
>  
>  /* ------------------------------------------------------------------ */
>  
> +static void gntdev_vma_open(struct vm_area_struct *vma)
> +{
> +	struct grant_map *map = vma->vm_private_data;
> +
> +	pr_debug("gntdev_vma_open %p\n", vma);
> +	atomic_inc(&map->users);
> +}
> +
>  static void gntdev_vma_close(struct vm_area_struct *vma)
>  {
>  	struct grant_map *map = vma->vm_private_data;
>  
> -	pr_debug("close %p\n", vma);
> +	pr_debug("gntdev_vma_close %p\n", vma);
>  	map->vma = NULL;
>  	vma->vm_private_data = NULL;
>  	gntdev_put_map(map);
>  }
>  
>  static struct vm_operations_struct gntdev_vmops = {
> +	.open = gntdev_vma_open,
>  	.close = gntdev_vma_close,
>  };
>  
> @@ -656,7 +665,10 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma)
>  
>  	vma->vm_ops = &gntdev_vmops;
>  
> -	vma->vm_flags |= VM_RESERVED|VM_DONTCOPY|VM_DONTEXPAND|VM_PFNMAP;
> +	vma->vm_flags |= VM_RESERVED|VM_DONTEXPAND;
> +
> +	if (use_ptemod)
> +		vma->vm_flags |= VM_DONTCOPY|VM_PFNMAP;
>  
>  	vma->vm_private_data = map;
>