All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xen/gntdev,gntalloc: Remove unneeded VM flags
@ 2011-03-07 18:19 Daniel De Graaf
  2011-03-07 19:53 ` Ian Campbell
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel De Graaf @ 2011-03-07 18:19 UTC (permalink / raw)
  To: konrad.wilk; +Cc: Daniel De Graaf, xen-devel

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.

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;
 
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] xen/gntdev,gntalloc: Remove unneeded VM flags
  2011-03-07 18:19 [PATCH] xen/gntdev,gntalloc: Remove unneeded VM flags Daniel De Graaf
@ 2011-03-07 19:53 ` Ian Campbell
  2011-03-07 20:01   ` [PATCH] xen/gntdev, gntalloc: " Daniel De Graaf
  2011-03-07 20:18   ` [PATCH v2] " Daniel De Graaf
  0 siblings, 2 replies; 7+ messages in thread
From: Ian Campbell @ 2011-03-07 19:53 UTC (permalink / raw)
  To: Daniel De Graaf; +Cc: xen-devel, konrad.wilk

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;
>  

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] xen/gntdev, gntalloc: Remove unneeded VM flags
  2011-03-07 19:53 ` Ian Campbell
@ 2011-03-07 20:01   ` Daniel De Graaf
  2011-03-07 20:18   ` [PATCH v2] " Daniel De Graaf
  1 sibling, 0 replies; 7+ messages in thread
From: Daniel De Graaf @ 2011-03-07 20:01 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel, konrad.wilk

On 03/07/2011 02:53 PM, Ian Campbell wrote:
> 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.

PV guests use PTE modification in gntdev, so this does not make any changes
to them. I can clarify that in the commit message if it is helpful.

The reference counting changes are balanced by the already-existing close
functions. The open function was not needed before now because it is only
used when the VM area is being copied on fork, which was not possible. I
can add a note of this if it would clarify the commit.

>>
>> 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;
>>  
> 
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] xen/gntdev, gntalloc: Remove unneeded VM flags
  2011-03-07 19:53 ` Ian Campbell
  2011-03-07 20:01   ` [PATCH] xen/gntdev, gntalloc: " Daniel De Graaf
@ 2011-03-07 20:18   ` Daniel De Graaf
  2011-03-08  9:54     ` Ian Campbell
  1 sibling, 1 reply; 7+ messages in thread
From: Daniel De Graaf @ 2011-03-07 20:18 UTC (permalink / raw)
  To: Ian.Campbell; +Cc: Daniel De Graaf, xen-devel, konrad.wilk

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
(that is, only gntdev on PV guests).  Otherwise, the area does not
require VM_DONTCOPY and VM_PFNMAP, since it can be accessed just like
any other page of RAM.

Since the vm_operations_struct close operations decrement reference
counts, a corresponding open function that increments them is required
now that it is possible to have multiple references to a single area.

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;
 
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] xen/gntdev, gntalloc: Remove unneeded VM flags
  2011-03-07 20:18   ` [PATCH v2] " Daniel De Graaf
@ 2011-03-08  9:54     ` Ian Campbell
  2011-03-09 17:05       ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 7+ messages in thread
From: Ian Campbell @ 2011-03-08  9:54 UTC (permalink / raw)
  To: Daniel De Graaf; +Cc: xen-devel, konrad.wilk

On Mon, 2011-03-07 at 20:18 +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
> (that is, only gntdev on PV guests).  Otherwise, the area does not
> require VM_DONTCOPY and VM_PFNMAP, since it can be accessed just like
> any other page of RAM.
> 
> Since the vm_operations_struct close operations decrement reference
> counts, a corresponding open function that increments them is required
> now that it is possible to have multiple references to a single area.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Looks good, thanks.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  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;
>  

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Re: [PATCH v2] xen/gntdev, gntalloc: Remove unneeded VM flags
  2011-03-08  9:54     ` Ian Campbell
@ 2011-03-09 17:05       ` Konrad Rzeszutek Wilk
  2011-03-09 18:38         ` Daniel De Graaf
  0 siblings, 1 reply; 7+ messages in thread
From: Konrad Rzeszutek Wilk @ 2011-03-09 17:05 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Daniel De Graaf, xen-devel

> > @@ -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;

So the gntalloc driver can be used on PV. You remove the DONTCOPY,PFNMAP, and PFN_AT_MMAP
.. while

> > -	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;
> >  

Here (gntdev) you are more careful. Should we do the same check under PV
for the gntalloc driver?

Have you tested this driver in PV <-> HVM env?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Re: [PATCH v2] xen/gntdev, gntalloc: Remove unneeded VM flags
  2011-03-09 17:05       ` Konrad Rzeszutek Wilk
@ 2011-03-09 18:38         ` Daniel De Graaf
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel De Graaf @ 2011-03-09 18:38 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: Ian Campbell, xen-devel

On 03/09/2011 12:05 PM, Konrad Rzeszutek Wilk wrote:
>>> @@ -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;
> 
> So the gntalloc driver can be used on PV. You remove the DONTCOPY,PFNMAP, and PFN_AT_MMAP
> .. while
> 
>>> -	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;
>>>  
> 
> Here (gntdev) you are more careful. Should we do the same check under PV
> for the gntalloc driver?
> 
> Have you tested this driver in PV <-> HVM env?
> 

I have not run this test case yet; however, I expect that it will work. The
reason that we need to be careful in gntdev on PV guests is because we are
not changing the PFN/MFN mapping on PV; instead, we change the application's
page tables to point to the other domain's memory. This means that the vma
cannot be copied without using another grant mapping hypercall; it also
requires special handling on unmap, which is the reason for gntdev's
dependency on the MMU notifier.

For gntalloc, this is not a concern - the pages are owned by the domain
using the gntalloc device, and can be mapped and unmapped in the same manner
as any other page of memory.

It is possible to use the same PFN/MFN mapping change in PV that we use in
HVM, which would allow PV guests to avoid treating gntdev memory specially.
I found problems with doing this the first time I tried, but I think they
were related to issues with updating the p2m and m2p maps that have since
been addressed. This change would effectively make "use_ptemod" always
false. It's slightly less efficient because the actual map requires two
hypercalls (one to update the p2m, and one to actually set up page tables)
but this doesn't seem like it would be an important issue.

-- 
Daniel De Graaf
National Security Agency

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2011-03-09 18:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-07 18:19 [PATCH] xen/gntdev,gntalloc: Remove unneeded VM flags Daniel De Graaf
2011-03-07 19:53 ` Ian Campbell
2011-03-07 20:01   ` [PATCH] xen/gntdev, gntalloc: " Daniel De Graaf
2011-03-07 20:18   ` [PATCH v2] " Daniel De Graaf
2011-03-08  9:54     ` Ian Campbell
2011-03-09 17:05       ` Konrad Rzeszutek Wilk
2011-03-09 18:38         ` Daniel De Graaf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.