[PATCH v3 0/2] xen: modify kernel mappings corresponding to granted pages

[PATCH v3 0/2] xen: modify kernel mappings corresponding to granted pages

[Stefano Stabellini]

Hi all,
this is the third version of the patch "xen: modify kernel mappings
corresponding to granted pages".
Compared to the latest version, following Jeremy's suggestion, I dropped
support for highmem pages. That made the code much simpler, therefore I
folded the multicall patch in the main patch because it didn't make much
sense to keep them separate anymore.
I modified gntdev to use lowmem pages only, so that we are always going
to be able to successfully modify the kernel mappings directly in the m2p
override.
In order to do that I extended the alloc_xenballooned_pages interface
with an highmem parameter that allows the caller to explicitly request
for highmem or lowmem pages.


Shortlog and diffstat follow:

Stefano Stabellini (2):
      xen: add an "highmem" parameter to alloc_xenballooned_pages
      xen: modify kernel mappings corresponding to granted pages

 arch/x86/include/asm/xen/page.h |    5 ++-
 arch/x86/xen/p2m.c              |   67 ++++++++++++++++++++++++++++++++------
 drivers/xen/balloon.c           |   12 +++++--
 drivers/xen/gntdev.c            |   29 +++++++++++++++-
 drivers/xen/grant-table.c       |    4 +-
 include/xen/balloon.h           |    3 +-
 include/xen/grant_table.h       |    1 +
 7 files changed, 100 insertions(+), 21 deletions(-)


A git branch with the two patches on top of Linux 3.1 rc4 is available
here:

git://xenbits.xen.org/people/sstabellini/linux-pvhvm.git 3.1-rc4-kernel_mappings_3

Cheers,

Stefano

[PATCH v3 1/2] xen: add an "highmem" parameter to alloc_xenballooned_pages

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Add an highmem parameter to alloc_xenballooned_pages, to allow callers to
request lowmem or highmem pages.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/balloon.c |   12 ++++++++----
 drivers/xen/gntdev.c  |    2 +-
 include/xen/balloon.h |    3 ++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 5dfd8f8..7f7d463 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -501,20 +501,24 @@ EXPORT_SYMBOL_GPL(balloon_set_new_target);
  * alloc_xenballooned_pages - get pages that have been ballooned out
  * @nr_pages: Number of pages to get
  * @pages: pages returned
+ * @highmem: highmem or lowmem pages
  * @return 0 on success, error otherwise
  */
-int alloc_xenballooned_pages(int nr_pages, struct page** pages)
+int alloc_xenballooned_pages(int nr_pages, struct page** pages, bool highmem)
 {
 	int pgno = 0;
 	struct page* page;
 	mutex_lock(&balloon_mutex);
 	while (pgno < nr_pages) {
-		page = balloon_retrieve(true);
-		if (page) {
+		page = balloon_retrieve(highmem);
+		if (page && PageHighMem(page) == highmem) {
 			pages[pgno++] = page;
 		} else {
 			enum bp_state st;
-			st = decrease_reservation(nr_pages - pgno, GFP_HIGHUSER);
+			if (page)
+				balloon_append(page);
+			st = decrease_reservation(nr_pages - pgno,
+					highmem ? GFP_HIGHUSER : GFP_USER);
 			if (st != BP_DONE)
 				goto out_undo;
 		}
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index f914b26..07a56c2 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -123,7 +123,7 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
 	    NULL == add->pages)
 		goto err;
 
-	if (alloc_xenballooned_pages(count, add->pages))
+	if (alloc_xenballooned_pages(count, add->pages, 0 /* lowmem */))
 		goto err;
 
 	for (i = 0; i < count; i++) {
diff --git a/include/xen/balloon.h b/include/xen/balloon.h
index 76f7538..b1a8233 100644
--- a/include/xen/balloon.h
+++ b/include/xen/balloon.h
@@ -25,7 +25,8 @@ extern struct balloon_stats balloon_stats;
 
 void balloon_set_new_target(unsigned long target);
 
-int alloc_xenballooned_pages(int nr_pages, struct page** pages);
+int alloc_xenballooned_pages(int nr_pages, struct page** pages,
+		bool highmem);
 void free_xenballooned_pages(int nr_pages, struct page** pages);
 
 struct sys_device;
-- 
1.7.2.3

Re: [PATCH v3 1/2] xen: add an "highmem" parameter to alloc_xenballooned_pages

[Jeremy Fitzhardinge]

On 09/07/2011 09:39 AM, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> Add an highmem parameter to alloc_xenballooned_pages, to allow callers to
> request lowmem or highmem pages.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  drivers/xen/balloon.c |   12 ++++++++----
>  drivers/xen/gntdev.c  |    2 +-
>  include/xen/balloon.h |    3 ++-
>  3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 5dfd8f8..7f7d463 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -501,20 +501,24 @@ EXPORT_SYMBOL_GPL(balloon_set_new_target);
>   * alloc_xenballooned_pages - get pages that have been ballooned out
>   * @nr_pages: Number of pages to get
>   * @pages: pages returned
> + * @highmem: highmem or lowmem pages
>   * @return 0 on success, error otherwise
>   */
> -int alloc_xenballooned_pages(int nr_pages, struct page** pages)
> +int alloc_xenballooned_pages(int nr_pages, struct page** pages, bool highmem)
>  {
>  	int pgno = 0;
>  	struct page* page;
>  	mutex_lock(&balloon_mutex);
>  	while (pgno < nr_pages) {
> -		page = balloon_retrieve(true);
> -		if (page) {
> +		page = balloon_retrieve(highmem);
> +		if (page && PageHighMem(page) == highmem) {
>  			pages[pgno++] = page;
>  		} else {
>  			enum bp_state st;
> -			st = decrease_reservation(nr_pages - pgno, GFP_HIGHUSER);
> +			if (page)
> +				balloon_append(page);
> +			st = decrease_reservation(nr_pages - pgno,
> +					highmem ? GFP_HIGHUSER : GFP_USER);
>  			if (st != BP_DONE)
>  				goto out_undo;
>  		}
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index f914b26..07a56c2 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -123,7 +123,7 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
>  	    NULL == add->pages)
>  		goto err;
>  
> -	if (alloc_xenballooned_pages(count, add->pages))
> +	if (alloc_xenballooned_pages(count, add->pages, 0 /* lowmem */))

If the parameter is "bool" you should pass true/false.  But it might be
better to just make it take a GFP_ constant directly.

    J

Re: [PATCH v3 1/2] xen: add an "highmem" parameter to alloc_xenballooned_pages

[Stefano Stabellini]

On Wed, 7 Sep 2011, Jeremy Fitzhardinge wrote:
> > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> > index f914b26..07a56c2 100644
> > --- a/drivers/xen/gntdev.c
> > +++ b/drivers/xen/gntdev.c
> > @@ -123,7 +123,7 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
> >  	    NULL == add->pages)
> >  		goto err;
> >  
> > -	if (alloc_xenballooned_pages(count, add->pages))
> > +	if (alloc_xenballooned_pages(count, add->pages, 0 /* lowmem */))
> 
> If the parameter is "bool" you should pass true/false.  But it might be
> better to just make it take a GFP_ constant directly.

I would rather have a bool to be consistent with the other balloon
interfaces.

[PATCH v3 2/2] xen: modify kernel mappings corresponding to granted pages

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

If we want to use granted pages for AIO, changing the mappings of a user
vma and the corresponding p2m is not enough, we also need to update the
kernel mappings accordingly.
In order to avoid the complexity of dealing with highmem, we allocated
the pages lowmem.
We issue a HYPERVISOR_grant_table_op right away in
m2p_add_override and we remove the mappings using another
HYPERVISOR_grant_table_op in m2p_remove_override.
Considering that m2p_add_override and m2p_remove_override are called
once per page we use multicalls and hypercall batching.

Use the kmap_op pointer directly as argument to do the mapping as it is
guaranteed to be present up until the unmapping is done.
Before issuing any unmapping multicalls, we need to make sure that the
mapping has already being done, because we need the kmap->handle to be
set correctly.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/x86/include/asm/xen/page.h |    5 ++-
 arch/x86/xen/p2m.c              |   67 ++++++++++++++++++++++++++++++++------
 drivers/xen/gntdev.c            |   27 +++++++++++++++-
 drivers/xen/grant-table.c       |    4 +-
 include/xen/grant_table.h       |    1 +
 5 files changed, 89 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
index 7ff4669..0ce1884 100644
--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -12,6 +12,7 @@
 #include <asm/pgtable.h>
 
 #include <xen/interface/xen.h>
+#include <xen/grant_table.h>
 #include <xen/features.h>
 
 /* Xen machine address */
@@ -31,8 +32,10 @@ typedef struct xpaddr {
 #define INVALID_P2M_ENTRY	(~0UL)
 #define FOREIGN_FRAME_BIT	(1UL<<(BITS_PER_LONG-1))
 #define IDENTITY_FRAME_BIT	(1UL<<(BITS_PER_LONG-2))
+#define GRANT_FRAME_BIT	(1UL<<(BITS_PER_LONG-3))
 #define FOREIGN_FRAME(m)	((m) | FOREIGN_FRAME_BIT)
 #define IDENTITY_FRAME(m)	((m) | IDENTITY_FRAME_BIT)
+#define GRANT_FRAME(m)	((m) | GRANT_FRAME_BIT)
 
 /* Maximum amount of memory we can handle in a domain in pages */
 #define MAX_DOMAIN_PAGES						\
@@ -48,7 +51,7 @@ extern unsigned long set_phys_range_identity(unsigned long pfn_s,
 					     unsigned long pfn_e);
 
 extern int m2p_add_override(unsigned long mfn, struct page *page,
-			    bool clear_pte);
+			    struct gnttab_map_grant_ref *kmap_op);
 extern int m2p_remove_override(struct page *page, bool clear_pte);
 extern struct page *m2p_find_override(unsigned long mfn);
 extern unsigned long m2p_find_override_pfn(unsigned long mfn, unsigned long pfn);
diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 58efeb9..6c26ac80 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -161,7 +161,9 @@
 #include <asm/xen/page.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
+#include <xen/grant_table.h>
 
+#include "multicalls.h"
 #include "xen-ops.h"
 
 static void __init m2p_override_init(void);
@@ -676,7 +678,8 @@ static unsigned long mfn_hash(unsigned long mfn)
 }
 
 /* Add an MFN override for a particular page */
-int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
+int m2p_add_override(unsigned long mfn, struct page *page,
+		struct gnttab_map_grant_ref *kmap_op)
 {
 	unsigned long flags;
 	unsigned long pfn;
@@ -699,9 +702,20 @@ int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
 	if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
 		return -ENOMEM;
 
-	if (clear_pte && !PageHighMem(page))
-		/* Just zap old mapping for now */
-		pte_clear(&init_mm, address, ptep);
+	if (kmap_op != NULL) {
+		if (!PageHighMem(page)) {
+			struct multicall_space mcs = xen_mc_entry(sizeof(*kmap_op));
+
+			MULTI_grant_table_op(mcs.mc,
+					GNTTABOP_map_grant_ref, kmap_op, 1);
+
+			xen_mc_issue(PARAVIRT_LAZY_MMU);
+		}
+		page->private |= GRANT_FRAME_BIT;
+		/* let's use dev_bus_addr to record the old mfn instead */
+		kmap_op->dev_bus_addr = page->index;
+		page->index = (unsigned long) kmap_op;
+	}
 	spin_lock_irqsave(&m2p_override_lock, flags);
 	list_add(&page->lru,  &m2p_overrides[mfn_hash(mfn)]);
 	spin_unlock_irqrestore(&m2p_override_lock, flags);
@@ -735,13 +749,44 @@ int m2p_remove_override(struct page *page, bool clear_pte)
 	spin_lock_irqsave(&m2p_override_lock, flags);
 	list_del(&page->lru);
 	spin_unlock_irqrestore(&m2p_override_lock, flags);
-	set_phys_to_machine(pfn, page->index);
 
-	if (clear_pte && !PageHighMem(page))
-		set_pte_at(&init_mm, address, ptep,
-				pfn_pte(pfn, PAGE_KERNEL));
-		/* No tlb flush necessary because the caller already
-		 * left the pte unmapped. */
+	if (clear_pte) {
+		struct gnttab_map_grant_ref *map_op =
+			(struct gnttab_map_grant_ref *) page->index;
+		set_phys_to_machine(pfn, map_op->dev_bus_addr);
+		if (!PageHighMem(page)) {
+			struct multicall_space mcs =
+				xen_mc_entry(sizeof(struct gnttab_unmap_grant_ref));
+			struct gnttab_unmap_grant_ref *unmap_op = mcs.args;
+
+			/* 
+			 * Has the grant_op mapping multicall being issued? If not,
+			 * make sure it is called now.
+			 */
+			if (map_op->handle == -1)
+				xen_mc_flush();
+			if (map_op->handle == -1) {
+				printk(KERN_WARNING "m2p_remove_override: pfn %lx mfn %lx, "
+						"failed to modify kernel mappings", pfn, mfn);
+				return -1;
+			}
+
+			unmap_op->host_addr = map_op->host_addr;
+			unmap_op->handle = map_op->handle;
+			unmap_op->dev_bus_addr = 0;
+
+			MULTI_grant_table_op(mcs.mc,
+					GNTTABOP_unmap_grant_ref, unmap_op, 1);
+
+			xen_mc_issue(PARAVIRT_LAZY_MMU);
+
+			set_pte_at(&init_mm, address, ptep,
+					pfn_pte(pfn, PAGE_KERNEL));
+			__flush_tlb_single(address);
+			map_op->host_addr = 0;
+		}
+	} else
+		set_phys_to_machine(pfn, page->index);
 
 	return 0;
 }
@@ -758,7 +803,7 @@ struct page *m2p_find_override(unsigned long mfn)
 	spin_lock_irqsave(&m2p_override_lock, flags);
 
 	list_for_each_entry(p, bucket, lru) {
-		if (p->private == mfn) {
+		if ((p->private & (~GRANT_FRAME_BIT)) == mfn) {
 			ret = p;
 			break;
 		}
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 07a56c2..783aaad 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -83,6 +83,7 @@ struct grant_map {
 	struct ioctl_gntdev_grant_ref *grants;
 	struct gnttab_map_grant_ref   *map_ops;
 	struct gnttab_unmap_grant_ref *unmap_ops;
+	struct gnttab_map_grant_ref   *kmap_ops;
 	struct page **pages;
 };
 
@@ -116,10 +117,12 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
 	add->grants    = kzalloc(sizeof(add->grants[0])    * count, GFP_KERNEL);
 	add->map_ops   = kzalloc(sizeof(add->map_ops[0])   * count, GFP_KERNEL);
 	add->unmap_ops = kzalloc(sizeof(add->unmap_ops[0]) * count, GFP_KERNEL);
+	add->kmap_ops  = kzalloc(sizeof(add->kmap_ops[0])  * count, GFP_KERNEL);
 	add->pages     = kzalloc(sizeof(add->pages[0])     * count, GFP_KERNEL);
 	if (NULL == add->grants    ||
 	    NULL == add->map_ops   ||
 	    NULL == add->unmap_ops ||
+	    NULL == add->kmap_ops  ||
 	    NULL == add->pages)
 		goto err;
 
@@ -129,6 +132,7 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
 	for (i = 0; i < count; i++) {
 		add->map_ops[i].handle = -1;
 		add->unmap_ops[i].handle = -1;
+		add->kmap_ops[i].handle = -1;
 	}
 
 	add->index = 0;
@@ -142,6 +146,7 @@ err:
 	kfree(add->grants);
 	kfree(add->map_ops);
 	kfree(add->unmap_ops);
+	kfree(add->kmap_ops);
 	kfree(add);
 	return NULL;
 }
@@ -243,10 +248,30 @@ static int map_grant_pages(struct grant_map *map)
 			gnttab_set_unmap_op(&map->unmap_ops[i], addr,
 				map->flags, -1 /* handle */);
 		}
+	} else {
+		for (i = 0; i < map->count; i++) {
+			unsigned level;
+			unsigned long address = (unsigned long)
+				pfn_to_kaddr(page_to_pfn(map->pages[i]));
+			pte_t *ptep;
+			u64 pte_maddr = 0;
+			if (!PageHighMem(map->pages[i])) {
+				ptep = lookup_address(address, &level);
+				pte_maddr =
+					arbitrary_virt_to_machine(ptep).maddr;
+			}
+			gnttab_set_map_op(&map->kmap_ops[i], pte_maddr,
+				map->flags |
+				GNTMAP_host_map |
+				GNTMAP_contains_pte,
+				map->grants[i].ref,
+				map->grants[i].domid);
+		}
 	}
 
 	pr_debug("map %d+%d\n", map->index, map->count);
-	err = gnttab_map_refs(map->map_ops, map->pages, map->count);
+	err = gnttab_map_refs(map->map_ops, use_ptemod ? map->kmap_ops : NULL,
+			map->pages, map->count);
 	if (err)
 		return err;
 
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 4f44b34..ed6622f 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -448,6 +448,7 @@ unsigned int gnttab_max_grant_frames(void)
 EXPORT_SYMBOL_GPL(gnttab_max_grant_frames);
 
 int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
+			struct gnttab_map_grant_ref *kmap_ops,
 		    struct page **pages, unsigned int count)
 {
 	int i, ret;
@@ -488,8 +489,7 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
 			 */
 			return -EOPNOTSUPP;
 		}
-		ret = m2p_add_override(mfn, pages[i],
-				       map_ops[i].flags & GNTMAP_contains_pte);
+		ret = m2p_add_override(mfn, pages[i], &kmap_ops[i]);
 		if (ret)
 			return ret;
 	}
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index b1fab6b..6b99bfb 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -156,6 +156,7 @@ unsigned int gnttab_max_grant_frames(void);
 #define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))
 
 int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
+			struct gnttab_map_grant_ref *kmap_ops,
 		    struct page **pages, unsigned int count);
 int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
 		      struct page **pages, unsigned int count);
-- 
1.7.2.3

Re: [PATCH v3 2/2] xen: modify kernel mappings corresponding to granted pages

[Jeremy Fitzhardinge]

On 09/07/2011 09:39 AM, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> If we want to use granted pages for AIO, changing the mappings of a user
> vma and the corresponding p2m is not enough, we also need to update the
> kernel mappings accordingly.
> In order to avoid the complexity of dealing with highmem, we allocated
> the pages lowmem.
> We issue a HYPERVISOR_grant_table_op right away in
> m2p_add_override and we remove the mappings using another
> HYPERVISOR_grant_table_op in m2p_remove_override.
> Considering that m2p_add_override and m2p_remove_override are called
> once per page we use multicalls and hypercall batching.
>
> Use the kmap_op pointer directly as argument to do the mapping as it is
> guaranteed to be present up until the unmapping is done.
> Before issuing any unmapping multicalls, we need to make sure that the
> mapping has already being done, because we need the kmap->handle to be
> set correctly.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/x86/include/asm/xen/page.h |    5 ++-
>  arch/x86/xen/p2m.c              |   67 ++++++++++++++++++++++++++++++++------
>  drivers/xen/gntdev.c            |   27 +++++++++++++++-
>  drivers/xen/grant-table.c       |    4 +-
>  include/xen/grant_table.h       |    1 +
>  5 files changed, 89 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index 7ff4669..0ce1884 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -12,6 +12,7 @@
>  #include <asm/pgtable.h>
>  
>  #include <xen/interface/xen.h>
> +#include <xen/grant_table.h>
>  #include <xen/features.h>
>  
>  /* Xen machine address */
> @@ -31,8 +32,10 @@ typedef struct xpaddr {
>  #define INVALID_P2M_ENTRY	(~0UL)
>  #define FOREIGN_FRAME_BIT	(1UL<<(BITS_PER_LONG-1))
>  #define IDENTITY_FRAME_BIT	(1UL<<(BITS_PER_LONG-2))
> +#define GRANT_FRAME_BIT	(1UL<<(BITS_PER_LONG-3))
>  #define FOREIGN_FRAME(m)	((m) | FOREIGN_FRAME_BIT)
>  #define IDENTITY_FRAME(m)	((m) | IDENTITY_FRAME_BIT)
> +#define GRANT_FRAME(m)	((m) | GRANT_FRAME_BIT)
>  
>  /* Maximum amount of memory we can handle in a domain in pages */
>  #define MAX_DOMAIN_PAGES						\
> @@ -48,7 +51,7 @@ extern unsigned long set_phys_range_identity(unsigned long pfn_s,
>  					     unsigned long pfn_e);
>  
>  extern int m2p_add_override(unsigned long mfn, struct page *page,
> -			    bool clear_pte);
> +			    struct gnttab_map_grant_ref *kmap_op);
>  extern int m2p_remove_override(struct page *page, bool clear_pte);
>  extern struct page *m2p_find_override(unsigned long mfn);
>  extern unsigned long m2p_find_override_pfn(unsigned long mfn, unsigned long pfn);
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 58efeb9..6c26ac80 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -161,7 +161,9 @@
>  #include <asm/xen/page.h>
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/hypervisor.h>
> +#include <xen/grant_table.h>
>  
> +#include "multicalls.h"
>  #include "xen-ops.h"
>  
>  static void __init m2p_override_init(void);
> @@ -676,7 +678,8 @@ static unsigned long mfn_hash(unsigned long mfn)
>  }
>  
>  /* Add an MFN override for a particular page */
> -int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
> +int m2p_add_override(unsigned long mfn, struct page *page,
> +		struct gnttab_map_grant_ref *kmap_op)
>  {
>  	unsigned long flags;
>  	unsigned long pfn;
> @@ -699,9 +702,20 @@ int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
>  	if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
>  		return -ENOMEM;
>  
> -	if (clear_pte && !PageHighMem(page))
> -		/* Just zap old mapping for now */
> -		pte_clear(&init_mm, address, ptep);
> +	if (kmap_op != NULL) {
> +		if (!PageHighMem(page)) {
> +			struct multicall_space mcs = xen_mc_entry(sizeof(*kmap_op));
> +
> +			MULTI_grant_table_op(mcs.mc,
> +					GNTTABOP_map_grant_ref, kmap_op, 1);
> +
> +			xen_mc_issue(PARAVIRT_LAZY_MMU);
> +		}
> +		page->private |= GRANT_FRAME_BIT;
> +		/* let's use dev_bus_addr to record the old mfn instead */
> +		kmap_op->dev_bus_addr = page->index;
> +		page->index = (unsigned long) kmap_op;
> +	}
>  	spin_lock_irqsave(&m2p_override_lock, flags);
>  	list_add(&page->lru,  &m2p_overrides[mfn_hash(mfn)]);
>  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> @@ -735,13 +749,44 @@ int m2p_remove_override(struct page *page, bool clear_pte)
>  	spin_lock_irqsave(&m2p_override_lock, flags);
>  	list_del(&page->lru);
>  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> -	set_phys_to_machine(pfn, page->index);
>  
> -	if (clear_pte && !PageHighMem(page))
> -		set_pte_at(&init_mm, address, ptep,
> -				pfn_pte(pfn, PAGE_KERNEL));
> -		/* No tlb flush necessary because the caller already
> -		 * left the pte unmapped. */
> +	if (clear_pte) {
> +		struct gnttab_map_grant_ref *map_op =
> +			(struct gnttab_map_grant_ref *) page->index;
> +		set_phys_to_machine(pfn, map_op->dev_bus_addr);
> +		if (!PageHighMem(page)) {
> +			struct multicall_space mcs =
> +				xen_mc_entry(sizeof(struct gnttab_unmap_grant_ref));
> +			struct gnttab_unmap_grant_ref *unmap_op = mcs.args;
> +
> +			/* 
> +			 * Has the grant_op mapping multicall being issued? If not,
> +			 * make sure it is called now.
> +			 */
> +			if (map_op->handle == -1)
> +				xen_mc_flush();

Hm, this will end up flushing the empty uninitialized entry you just
added, which also implicitly frees the space, so all the stuff below is
- at best - a noop.

But also, this pattern of getting results back from batched calls is
unusual - actually, I think this is unique.  If you have batched up a
map operation which has its map_op args allocated from the multicall
buffer, then the flush will implicitly free them as well, so it isn't
valid to read back from the structure later on.  If you want to have an
args structure you can use once the hypercall has been issued, you need
to manually manage its lifetime.

> +			if (map_op->handle == -1) {
> +				printk(KERN_WARNING "m2p_remove_override: pfn %lx mfn %lx, "
> +						"failed to modify kernel mappings", pfn, mfn);
> +				return -1;
> +			}
> +
> +			unmap_op->host_addr = map_op->host_addr;
> +			unmap_op->handle = map_op->handle;
> +			unmap_op->dev_bus_addr = 0;
> +
> +			MULTI_grant_table_op(mcs.mc,
> +					GNTTABOP_unmap_grant_ref, unmap_op, 1);
> +
> +			xen_mc_issue(PARAVIRT_LAZY_MMU);
> +
> +			set_pte_at(&init_mm, address, ptep,
> +					pfn_pte(pfn, PAGE_KERNEL));
> +			__flush_tlb_single(address);
> +			map_op->host_addr = 0;
> +		}
> +	} else
> +		set_phys_to_machine(pfn, page->index);
>  
>  	return 0;
>  }
> @@ -758,7 +803,7 @@ struct page *m2p_find_override(unsigned long mfn)
>  	spin_lock_irqsave(&m2p_override_lock, flags);
>  
>  	list_for_each_entry(p, bucket, lru) {
> -		if (p->private == mfn) {
> +		if ((p->private & (~GRANT_FRAME_BIT)) == mfn) {
>  			ret = p;
>  			break;
>  		}
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 07a56c2..783aaad 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -83,6 +83,7 @@ struct grant_map {
>  	struct ioctl_gntdev_grant_ref *grants;
>  	struct gnttab_map_grant_ref   *map_ops;
>  	struct gnttab_unmap_grant_ref *unmap_ops;
> +	struct gnttab_map_grant_ref   *kmap_ops;
>  	struct page **pages;
>  };
>  
> @@ -116,10 +117,12 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
>  	add->grants    = kzalloc(sizeof(add->grants[0])    * count, GFP_KERNEL);
>  	add->map_ops   = kzalloc(sizeof(add->map_ops[0])   * count, GFP_KERNEL);
>  	add->unmap_ops = kzalloc(sizeof(add->unmap_ops[0]) * count, GFP_KERNEL);
> +	add->kmap_ops  = kzalloc(sizeof(add->kmap_ops[0])  * count, GFP_KERNEL);
>  	add->pages     = kzalloc(sizeof(add->pages[0])     * count, GFP_KERNEL);
>  	if (NULL == add->grants    ||
>  	    NULL == add->map_ops   ||
>  	    NULL == add->unmap_ops ||
> +	    NULL == add->kmap_ops  ||
>  	    NULL == add->pages)
>  		goto err;
>  
> @@ -129,6 +132,7 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
>  	for (i = 0; i < count; i++) {
>  		add->map_ops[i].handle = -1;
>  		add->unmap_ops[i].handle = -1;
> +		add->kmap_ops[i].handle = -1;
>  	}
>  
>  	add->index = 0;
> @@ -142,6 +146,7 @@ err:
>  	kfree(add->grants);
>  	kfree(add->map_ops);
>  	kfree(add->unmap_ops);
> +	kfree(add->kmap_ops);
>  	kfree(add);
>  	return NULL;
>  }
> @@ -243,10 +248,30 @@ static int map_grant_pages(struct grant_map *map)
>  			gnttab_set_unmap_op(&map->unmap_ops[i], addr,
>  				map->flags, -1 /* handle */);
>  		}
> +	} else {
> +		for (i = 0; i < map->count; i++) {
> +			unsigned level;
> +			unsigned long address = (unsigned long)
> +				pfn_to_kaddr(page_to_pfn(map->pages[i]));
> +			pte_t *ptep;
> +			u64 pte_maddr = 0;
> +			if (!PageHighMem(map->pages[i])) {
> +				ptep = lookup_address(address, &level);
> +				pte_maddr =
> +					arbitrary_virt_to_machine(ptep).maddr;
> +			}
> +			gnttab_set_map_op(&map->kmap_ops[i], pte_maddr,
> +				map->flags |
> +				GNTMAP_host_map |
> +				GNTMAP_contains_pte,
> +				map->grants[i].ref,
> +				map->grants[i].domid);
> +		}
>  	}
>  
>  	pr_debug("map %d+%d\n", map->index, map->count);
> -	err = gnttab_map_refs(map->map_ops, map->pages, map->count);
> +	err = gnttab_map_refs(map->map_ops, use_ptemod ? map->kmap_ops : NULL,
> +			map->pages, map->count);
>  	if (err)
>  		return err;
>  
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 4f44b34..ed6622f 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -448,6 +448,7 @@ unsigned int gnttab_max_grant_frames(void)
>  EXPORT_SYMBOL_GPL(gnttab_max_grant_frames);
>  
>  int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
> +			struct gnttab_map_grant_ref *kmap_ops,
>  		    struct page **pages, unsigned int count)

Formatting looks wonky here.

>  {
>  	int i, ret;
> @@ -488,8 +489,7 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>  			 */
>  			return -EOPNOTSUPP;
>  		}
> -		ret = m2p_add_override(mfn, pages[i],
> -				       map_ops[i].flags & GNTMAP_contains_pte);
> +		ret = m2p_add_override(mfn, pages[i], &kmap_ops[i]);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index b1fab6b..6b99bfb 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -156,6 +156,7 @@ unsigned int gnttab_max_grant_frames(void);
>  #define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))
>  
>  int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
> +			struct gnttab_map_grant_ref *kmap_ops,
>  		    struct page **pages, unsigned int count);
>  int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>  		      struct page **pages, unsigned int count);

    J

Re: [PATCH v3 2/2] xen: modify kernel mappings corresponding to granted pages

[Stefano Stabellini]

On Wed, 7 Sep 2011, Jeremy Fitzhardinge wrote:
> On 09/07/2011 09:39 AM, stefano.stabellini@eu.citrix.com wrote:
> > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >
> > If we want to use granted pages for AIO, changing the mappings of a user
> > vma and the corresponding p2m is not enough, we also need to update the
> > kernel mappings accordingly.
> > In order to avoid the complexity of dealing with highmem, we allocated
> > the pages lowmem.
> > We issue a HYPERVISOR_grant_table_op right away in
> > m2p_add_override and we remove the mappings using another
> > HYPERVISOR_grant_table_op in m2p_remove_override.
> > Considering that m2p_add_override and m2p_remove_override are called
> > once per page we use multicalls and hypercall batching.
> >
> > Use the kmap_op pointer directly as argument to do the mapping as it is
> > guaranteed to be present up until the unmapping is done.
> > Before issuing any unmapping multicalls, we need to make sure that the
> > mapping has already being done, because we need the kmap->handle to be
> > set correctly.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  arch/x86/include/asm/xen/page.h |    5 ++-
> >  arch/x86/xen/p2m.c              |   67 ++++++++++++++++++++++++++++++++------
> >  drivers/xen/gntdev.c            |   27 +++++++++++++++-
> >  drivers/xen/grant-table.c       |    4 +-
> >  include/xen/grant_table.h       |    1 +
> >  5 files changed, 89 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> > index 7ff4669..0ce1884 100644
> > --- a/arch/x86/include/asm/xen/page.h
> > +++ b/arch/x86/include/asm/xen/page.h
> > @@ -12,6 +12,7 @@
> >  #include <asm/pgtable.h>
> >  
> >  #include <xen/interface/xen.h>
> > +#include <xen/grant_table.h>
> >  #include <xen/features.h>
> >  
> >  /* Xen machine address */
> > @@ -31,8 +32,10 @@ typedef struct xpaddr {
> >  #define INVALID_P2M_ENTRY	(~0UL)
> >  #define FOREIGN_FRAME_BIT	(1UL<<(BITS_PER_LONG-1))
> >  #define IDENTITY_FRAME_BIT	(1UL<<(BITS_PER_LONG-2))
> > +#define GRANT_FRAME_BIT	(1UL<<(BITS_PER_LONG-3))
> >  #define FOREIGN_FRAME(m)	((m) | FOREIGN_FRAME_BIT)
> >  #define IDENTITY_FRAME(m)	((m) | IDENTITY_FRAME_BIT)
> > +#define GRANT_FRAME(m)	((m) | GRANT_FRAME_BIT)
> >  
> >  /* Maximum amount of memory we can handle in a domain in pages */
> >  #define MAX_DOMAIN_PAGES						\
> > @@ -48,7 +51,7 @@ extern unsigned long set_phys_range_identity(unsigned long pfn_s,
> >  					     unsigned long pfn_e);
> >  
> >  extern int m2p_add_override(unsigned long mfn, struct page *page,
> > -			    bool clear_pte);
> > +			    struct gnttab_map_grant_ref *kmap_op);
> >  extern int m2p_remove_override(struct page *page, bool clear_pte);
> >  extern struct page *m2p_find_override(unsigned long mfn);
> >  extern unsigned long m2p_find_override_pfn(unsigned long mfn, unsigned long pfn);
> > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> > index 58efeb9..6c26ac80 100644
> > --- a/arch/x86/xen/p2m.c
> > +++ b/arch/x86/xen/p2m.c
> > @@ -161,7 +161,9 @@
> >  #include <asm/xen/page.h>
> >  #include <asm/xen/hypercall.h>
> >  #include <asm/xen/hypervisor.h>
> > +#include <xen/grant_table.h>
> >  
> > +#include "multicalls.h"
> >  #include "xen-ops.h"
> >  
> >  static void __init m2p_override_init(void);
> > @@ -676,7 +678,8 @@ static unsigned long mfn_hash(unsigned long mfn)
> >  }
> >  
> >  /* Add an MFN override for a particular page */
> > -int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
> > +int m2p_add_override(unsigned long mfn, struct page *page,
> > +		struct gnttab_map_grant_ref *kmap_op)
> >  {
> >  	unsigned long flags;
> >  	unsigned long pfn;
> > @@ -699,9 +702,20 @@ int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
> >  	if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
> >  		return -ENOMEM;
> >  
> > -	if (clear_pte && !PageHighMem(page))
> > -		/* Just zap old mapping for now */
> > -		pte_clear(&init_mm, address, ptep);
> > +	if (kmap_op != NULL) {
> > +		if (!PageHighMem(page)) {
> > +			struct multicall_space mcs = xen_mc_entry(sizeof(*kmap_op));
> > +
> > +			MULTI_grant_table_op(mcs.mc,
> > +					GNTTABOP_map_grant_ref, kmap_op, 1);
> > +
> > +			xen_mc_issue(PARAVIRT_LAZY_MMU);
> > +		}
> > +		page->private |= GRANT_FRAME_BIT;
> > +		/* let's use dev_bus_addr to record the old mfn instead */
> > +		kmap_op->dev_bus_addr = page->index;
> > +		page->index = (unsigned long) kmap_op;
> > +	}
> >  	spin_lock_irqsave(&m2p_override_lock, flags);
> >  	list_add(&page->lru,  &m2p_overrides[mfn_hash(mfn)]);
> >  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> > @@ -735,13 +749,44 @@ int m2p_remove_override(struct page *page, bool clear_pte)
> >  	spin_lock_irqsave(&m2p_override_lock, flags);
> >  	list_del(&page->lru);
> >  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> > -	set_phys_to_machine(pfn, page->index);
> >  
> > -	if (clear_pte && !PageHighMem(page))
> > -		set_pte_at(&init_mm, address, ptep,
> > -				pfn_pte(pfn, PAGE_KERNEL));
> > -		/* No tlb flush necessary because the caller already
> > -		 * left the pte unmapped. */
> > +	if (clear_pte) {
> > +		struct gnttab_map_grant_ref *map_op =
> > +			(struct gnttab_map_grant_ref *) page->index;
> > +		set_phys_to_machine(pfn, map_op->dev_bus_addr);
> > +		if (!PageHighMem(page)) {
> > +			struct multicall_space mcs =
> > +				xen_mc_entry(sizeof(struct gnttab_unmap_grant_ref));
> > +			struct gnttab_unmap_grant_ref *unmap_op = mcs.args;
> > +
> > +			/* 
> > +			 * Has the grant_op mapping multicall being issued? If not,
> > +			 * make sure it is called now.
> > +			 */
> > +			if (map_op->handle == -1)
> > +				xen_mc_flush();
> 
> Hm, this will end up flushing the empty uninitialized entry you just
> added, which also implicitly frees the space, so all the stuff below is
> - at best - a noop.

I'll have to move it earlier.


> But also, this pattern of getting results back from batched calls is
> unusual - actually, I think this is unique.  If you have batched up a
> map operation which has its map_op args allocated from the multicall
> buffer, then the flush will implicitly free them as well, so it isn't
> valid to read back from the structure later on.  If you want to have an
> args structure you can use once the hypercall has been issued, you need
> to manually manage its lifetime.

But I am not using the multicall buffer as argument, I am using the
kmap_op passed to m2p_add_override.

Re: [PATCH v3 2/2] xen: modify kernel mappings corresponding to granted pages

[Jeremy Fitzhardinge]

On 09/07/2011 10:38 AM, Stefano Stabellini wrote:
>
>> But also, this pattern of getting results back from batched calls is
>> unusual - actually, I think this is unique.  If you have batched up a
>> map operation which has its map_op args allocated from the multicall
>> buffer, then the flush will implicitly free them as well, so it isn't
>> valid to read back from the structure later on.  If you want to have an
>> args structure you can use once the hypercall has been issued, you need
>> to manually manage its lifetime.
> But I am not using the multicall buffer as argument, I am using the
> kmap_op passed to m2p_add_override.

Yep, I overlooked that.

    J

Re: [PATCH v3 2/2] xen: modify kernel mappings corresponding to granted pages

[Konrad Rzeszutek Wilk]

On Wed, Sep 07, 2011 at 05:39:31PM +0100, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> If we want to use granted pages for AIO, changing the mappings of a user
> vma and the corresponding p2m is not enough, we also need to update the
> kernel mappings accordingly.
> In order to avoid the complexity of dealing with highmem, we allocated
> the pages lowmem.
> We issue a HYPERVISOR_grant_table_op right away in
> m2p_add_override and we remove the mappings using another
> HYPERVISOR_grant_table_op in m2p_remove_override.
> Considering that m2p_add_override and m2p_remove_override are called
> once per page we use multicalls and hypercall batching.
> 
> Use the kmap_op pointer directly as argument to do the mapping as it is
> guaranteed to be present up until the unmapping is done.
> Before issuing any unmapping multicalls, we need to make sure that the
> mapping has already being done, because we need the kmap->handle to be
> set correctly.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/x86/include/asm/xen/page.h |    5 ++-
>  arch/x86/xen/p2m.c              |   67 ++++++++++++++++++++++++++++++++------
>  drivers/xen/gntdev.c            |   27 +++++++++++++++-
>  drivers/xen/grant-table.c       |    4 +-
>  include/xen/grant_table.h       |    1 +
>  5 files changed, 89 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index 7ff4669..0ce1884 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -12,6 +12,7 @@
>  #include <asm/pgtable.h>
>  
>  #include <xen/interface/xen.h>
> +#include <xen/grant_table.h>
>  #include <xen/features.h>
>  
>  /* Xen machine address */
> @@ -31,8 +32,10 @@ typedef struct xpaddr {
>  #define INVALID_P2M_ENTRY	(~0UL)
>  #define FOREIGN_FRAME_BIT	(1UL<<(BITS_PER_LONG-1))
>  #define IDENTITY_FRAME_BIT	(1UL<<(BITS_PER_LONG-2))
> +#define GRANT_FRAME_BIT	(1UL<<(BITS_PER_LONG-3))
>  #define FOREIGN_FRAME(m)	((m) | FOREIGN_FRAME_BIT)
>  #define IDENTITY_FRAME(m)	((m) | IDENTITY_FRAME_BIT)
> +#define GRANT_FRAME(m)	((m) | GRANT_FRAME_BIT)
>  
>  /* Maximum amount of memory we can handle in a domain in pages */
>  #define MAX_DOMAIN_PAGES						\
> @@ -48,7 +51,7 @@ extern unsigned long set_phys_range_identity(unsigned long pfn_s,
>  					     unsigned long pfn_e);
>  
>  extern int m2p_add_override(unsigned long mfn, struct page *page,
> -			    bool clear_pte);
> +			    struct gnttab_map_grant_ref *kmap_op);
>  extern int m2p_remove_override(struct page *page, bool clear_pte);
>  extern struct page *m2p_find_override(unsigned long mfn);
>  extern unsigned long m2p_find_override_pfn(unsigned long mfn, unsigned long pfn);
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 58efeb9..6c26ac80 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -161,7 +161,9 @@
>  #include <asm/xen/page.h>
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/hypervisor.h>
> +#include <xen/grant_table.h>
>  
> +#include "multicalls.h"
>  #include "xen-ops.h"
>  
>  static void __init m2p_override_init(void);
> @@ -676,7 +678,8 @@ static unsigned long mfn_hash(unsigned long mfn)
>  }
>  
>  /* Add an MFN override for a particular page */
> -int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
> +int m2p_add_override(unsigned long mfn, struct page *page,
> +		struct gnttab_map_grant_ref *kmap_op)

So how come you ripped out 'clear_pte' here but left it in
m2p_remove_override? Is it b/c you aren't doing the pte_clear in
the first place?

Also there are other users of 'm2p_add_override' that you hadn't modified
(blkback.c) so it will cause an compile failure.

Please also test this patchset with the blkback in the picture to make
sure this: cf8d91633ddef9e816ccbf3da833c79ce508988d
does not happen.

>  {
>  	unsigned long flags;
>  	unsigned long pfn;
> @@ -699,9 +702,20 @@ int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
>  	if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
>  		return -ENOMEM;
>  
> -	if (clear_pte && !PageHighMem(page))
> -		/* Just zap old mapping for now */
> -		pte_clear(&init_mm, address, ptep);
> +	if (kmap_op != NULL) {
> +		if (!PageHighMem(page)) {
> +			struct multicall_space mcs = xen_mc_entry(sizeof(*kmap_op));
> +
> +			MULTI_grant_table_op(mcs.mc,
> +					GNTTABOP_map_grant_ref, kmap_op, 1);
> +
> +			xen_mc_issue(PARAVIRT_LAZY_MMU);
> +		}
> +		page->private |= GRANT_FRAME_BIT;
> +		/* let's use dev_bus_addr to record the old mfn instead */
> +		kmap_op->dev_bus_addr = page->index;
> +		page->index = (unsigned long) kmap_op;
> +	}
>  	spin_lock_irqsave(&m2p_override_lock, flags);
>  	list_add(&page->lru,  &m2p_overrides[mfn_hash(mfn)]);
>  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> @@ -735,13 +749,44 @@ int m2p_remove_override(struct page *page, bool clear_pte)
>  	spin_lock_irqsave(&m2p_override_lock, flags);
>  	list_del(&page->lru);
>  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> -	set_phys_to_machine(pfn, page->index);
>  
> -	if (clear_pte && !PageHighMem(page))
> -		set_pte_at(&init_mm, address, ptep,
> -				pfn_pte(pfn, PAGE_KERNEL));
> -		/* No tlb flush necessary because the caller already
> -		 * left the pte unmapped. */
> +	if (clear_pte) {
> +		struct gnttab_map_grant_ref *map_op =
> +			(struct gnttab_map_grant_ref *) page->index;
> +		set_phys_to_machine(pfn, map_op->dev_bus_addr);
> +		if (!PageHighMem(page)) {
> +			struct multicall_space mcs =
> +				xen_mc_entry(sizeof(struct gnttab_unmap_grant_ref));
> +			struct gnttab_unmap_grant_ref *unmap_op = mcs.args;
> +
> +			/* 

scripts/checkpath.pl will throw a fit at the above.

> +			 * Has the grant_op mapping multicall being issued? If not,
> +			 * make sure it is called now.
> +			 */
> +			if (map_op->handle == -1)
> +				xen_mc_flush();
> +			if (map_op->handle == -1) {
> +				printk(KERN_WARNING "m2p_remove_override: pfn %lx mfn %lx, "
> +						"failed to modify kernel mappings", pfn, mfn);
> +				return -1;
> +			}
> +
> +			unmap_op->host_addr = map_op->host_addr;
> +			unmap_op->handle = map_op->handle;
> +			unmap_op->dev_bus_addr = 0;
> +
> +			MULTI_grant_table_op(mcs.mc,
> +					GNTTABOP_unmap_grant_ref, unmap_op, 1);
> +
> +			xen_mc_issue(PARAVIRT_LAZY_MMU);
> +
> +			set_pte_at(&init_mm, address, ptep,
> +					pfn_pte(pfn, PAGE_KERNEL));
> +			__flush_tlb_single(address);
> +			map_op->host_addr = 0;
> +		}
> +	} else
> +		set_phys_to_machine(pfn, page->index);
>  
>  	return 0;
>  }
> @@ -758,7 +803,7 @@ struct page *m2p_find_override(unsigned long mfn)
>  	spin_lock_irqsave(&m2p_override_lock, flags);
>  
>  	list_for_each_entry(p, bucket, lru) {
> -		if (p->private == mfn) {
> +		if ((p->private & (~GRANT_FRAME_BIT)) == mfn) {
>  			ret = p;
>  			break;
>  		}
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 07a56c2..783aaad 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -83,6 +83,7 @@ struct grant_map {
>  	struct ioctl_gntdev_grant_ref *grants;
>  	struct gnttab_map_grant_ref   *map_ops;
>  	struct gnttab_unmap_grant_ref *unmap_ops;
> +	struct gnttab_map_grant_ref   *kmap_ops;
>  	struct page **pages;
>  };
>  
> @@ -116,10 +117,12 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
>  	add->grants    = kzalloc(sizeof(add->grants[0])    * count, GFP_KERNEL);
>  	add->map_ops   = kzalloc(sizeof(add->map_ops[0])   * count, GFP_KERNEL);
>  	add->unmap_ops = kzalloc(sizeof(add->unmap_ops[0]) * count, GFP_KERNEL);
> +	add->kmap_ops  = kzalloc(sizeof(add->kmap_ops[0])  * count, GFP_KERNEL);
>  	add->pages     = kzalloc(sizeof(add->pages[0])     * count, GFP_KERNEL);
>  	if (NULL == add->grants    ||
>  	    NULL == add->map_ops   ||
>  	    NULL == add->unmap_ops ||
> +	    NULL == add->kmap_ops  ||
>  	    NULL == add->pages)
>  		goto err;
>  
> @@ -129,6 +132,7 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
>  	for (i = 0; i < count; i++) {
>  		add->map_ops[i].handle = -1;
>  		add->unmap_ops[i].handle = -1;
> +		add->kmap_ops[i].handle = -1;
>  	}
>  
>  	add->index = 0;
> @@ -142,6 +146,7 @@ err:
>  	kfree(add->grants);
>  	kfree(add->map_ops);
>  	kfree(add->unmap_ops);
> +	kfree(add->kmap_ops);
>  	kfree(add);
>  	return NULL;
>  }
> @@ -243,10 +248,30 @@ static int map_grant_pages(struct grant_map *map)
>  			gnttab_set_unmap_op(&map->unmap_ops[i], addr,
>  				map->flags, -1 /* handle */);
>  		}
> +	} else {
> +		for (i = 0; i < map->count; i++) {
> +			unsigned level;
> +			unsigned long address = (unsigned long)
> +				pfn_to_kaddr(page_to_pfn(map->pages[i]));
> +			pte_t *ptep;
> +			u64 pte_maddr = 0;
> +			if (!PageHighMem(map->pages[i])) {
> +				ptep = lookup_address(address, &level);
> +				pte_maddr =
> +					arbitrary_virt_to_machine(ptep).maddr;
> +			}
> +			gnttab_set_map_op(&map->kmap_ops[i], pte_maddr,
> +				map->flags |
> +				GNTMAP_host_map |
> +				GNTMAP_contains_pte,
> +				map->grants[i].ref,
> +				map->grants[i].domid);
> +		}
>  	}
>  
>  	pr_debug("map %d+%d\n", map->index, map->count);
> -	err = gnttab_map_refs(map->map_ops, map->pages, map->count);
> +	err = gnttab_map_refs(map->map_ops, use_ptemod ? map->kmap_ops : NULL,
> +			map->pages, map->count);
>  	if (err)
>  		return err;
>  
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 4f44b34..ed6622f 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -448,6 +448,7 @@ unsigned int gnttab_max_grant_frames(void)
>  EXPORT_SYMBOL_GPL(gnttab_max_grant_frames);
>  
>  int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
> +			struct gnttab_map_grant_ref *kmap_ops,
>  		    struct page **pages, unsigned int count)
>  {
>  	int i, ret;
> @@ -488,8 +489,7 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>  			 */
>  			return -EOPNOTSUPP;
>  		}
> -		ret = m2p_add_override(mfn, pages[i],
> -				       map_ops[i].flags & GNTMAP_contains_pte);
> +		ret = m2p_add_override(mfn, pages[i], &kmap_ops[i]);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index b1fab6b..6b99bfb 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -156,6 +156,7 @@ unsigned int gnttab_max_grant_frames(void);
>  #define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))
>  
>  int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
> +			struct gnttab_map_grant_ref *kmap_ops,
>  		    struct page **pages, unsigned int count);
>  int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>  		      struct page **pages, unsigned int count);
> -- 
> 1.7.2.3

Re: [PATCH v3 2/2] xen: modify kernel mappings corresponding to granted pages

[Stefano Stabellini]

On Wed, 7 Sep 2011, Konrad Rzeszutek Wilk wrote:
> > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> > index 58efeb9..6c26ac80 100644
> > --- a/arch/x86/xen/p2m.c
> > +++ b/arch/x86/xen/p2m.c
> > @@ -161,7 +161,9 @@
> >  #include <asm/xen/page.h>
> >  #include <asm/xen/hypercall.h>
> >  #include <asm/xen/hypervisor.h>
> > +#include <xen/grant_table.h>
> >  
> > +#include "multicalls.h"
> >  #include "xen-ops.h"
> >  
> >  static void __init m2p_override_init(void);
> > @@ -676,7 +678,8 @@ static unsigned long mfn_hash(unsigned long mfn)
> >  }
> >  
> >  /* Add an MFN override for a particular page */
> > -int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
> > +int m2p_add_override(unsigned long mfn, struct page *page,
> > +		struct gnttab_map_grant_ref *kmap_op)
> 
> So how come you ripped out 'clear_pte' here but left it in
> m2p_remove_override? Is it b/c you aren't doing the pte_clear in
> the first place?

Because the "clear_pte" has been replaced with a proper update of the
kernel mappings for these pages. If the caller doesn't need that, it can
just pass NULL.


> Also there are other users of 'm2p_add_override' that you hadn't modified
> (blkback.c) so it will cause an compile failure.
> 
> Please also test this patchset with the blkback in the picture to make
> sure this: cf8d91633ddef9e816ccbf3da833c79ce508988d
> does not happen.

I think I didn't find a compilation problem because false becomes NULL
with my compiler. Fixed now.


> >  {
> >  	unsigned long flags;
> >  	unsigned long pfn;
> > @@ -699,9 +702,20 @@ int m2p_add_override(unsigned long mfn, struct page *page, bool clear_pte)
> >  	if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
> >  		return -ENOMEM;
> >  
> > -	if (clear_pte && !PageHighMem(page))
> > -		/* Just zap old mapping for now */
> > -		pte_clear(&init_mm, address, ptep);
> > +	if (kmap_op != NULL) {
> > +		if (!PageHighMem(page)) {
> > +			struct multicall_space mcs = xen_mc_entry(sizeof(*kmap_op));
> > +
> > +			MULTI_grant_table_op(mcs.mc,
> > +					GNTTABOP_map_grant_ref, kmap_op, 1);
> > +
> > +			xen_mc_issue(PARAVIRT_LAZY_MMU);
> > +		}
> > +		page->private |= GRANT_FRAME_BIT;
> > +		/* let's use dev_bus_addr to record the old mfn instead */
> > +		kmap_op->dev_bus_addr = page->index;
> > +		page->index = (unsigned long) kmap_op;
> > +	}
> >  	spin_lock_irqsave(&m2p_override_lock, flags);
> >  	list_add(&page->lru,  &m2p_overrides[mfn_hash(mfn)]);
> >  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> > @@ -735,13 +749,44 @@ int m2p_remove_override(struct page *page, bool clear_pte)
> >  	spin_lock_irqsave(&m2p_override_lock, flags);
> >  	list_del(&page->lru);
> >  	spin_unlock_irqrestore(&m2p_override_lock, flags);
> > -	set_phys_to_machine(pfn, page->index);
> >  
> > -	if (clear_pte && !PageHighMem(page))
> > -		set_pte_at(&init_mm, address, ptep,
> > -				pfn_pte(pfn, PAGE_KERNEL));
> > -		/* No tlb flush necessary because the caller already
> > -		 * left the pte unmapped. */
> > +	if (clear_pte) {
> > +		struct gnttab_map_grant_ref *map_op =
> > +			(struct gnttab_map_grant_ref *) page->index;
> > +		set_phys_to_machine(pfn, map_op->dev_bus_addr);
> > +		if (!PageHighMem(page)) {
> > +			struct multicall_space mcs =
> > +				xen_mc_entry(sizeof(struct gnttab_unmap_grant_ref));
> > +			struct gnttab_unmap_grant_ref *unmap_op = mcs.args;
> > +
> > +			/* 
> 
> scripts/checkpath.pl will throw a fit at the above.
> 

right, fixed.