Daniel De Graaf
2011-Mar-07 18:19 UTC
[Xen-devel] [PATCH] xen/gntdev,gntalloc: Remove unneeded VM flags
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-07 19:53 UTC
Re: [Xen-devel] [PATCH] xen/gntdev,gntalloc: Remove unneeded VM flags
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; >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2011-Mar-07 20:01 UTC
Re: [Xen-devel] [PATCH] xen/gntdev, gntalloc: Remove unneeded VM flags
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; >> > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2011-Mar-07 20:18 UTC
[Xen-devel] [PATCH v2] xen/gntdev, gntalloc: Remove unneeded VM flags
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-08 09:54 UTC
[Xen-devel] Re: [PATCH v2] xen/gntdev, gntalloc: Remove unneeded VM flags
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; >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Mar-09 17:05 UTC
Re: [Xen-devel] Re: [PATCH v2] xen/gntdev, gntalloc: Remove unneeded VM flags
> > @@ -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? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2011-Mar-09 18:38 UTC
Re: [Xen-devel] Re: [PATCH v2] xen/gntdev, gntalloc: Remove unneeded VM flags
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Seemingly Similar Threads
- [PATCH] xen/gntdev: do not set VM_PFNMAP
- [patch 1/2] xen-gntalloc: integer overflow in gntalloc_ioctl_alloc()
- [patch 1/2] xen-gntalloc: integer overflow in gntalloc_ioctl_alloc()
- [patch 1/2] xen-gntalloc: integer overflow in gntalloc_ioctl_alloc()
- Error in running libvchan.