[PATCHv1 0/2] xen/grant-table: fix PV guests and remove V2 support

[PATCHv1 0/2] xen/grant-table: fix PV guests and remove V2 support

[David Vrabel]

The first patch fixes a logn standing issue where the map/unmap of
grant table frames by PV guests was unsafe and could BUG() or have
other bad behaviour.  This was caused by calling function that were
not permitted from atomic contexts.

The second patch removes all V2 table code since it is unused.

David

[PATCH 1/2] x86/xen: safely map and unmap grant frames when in atomic context

[David Vrabel]

arch_gnttab_map_frames() and arch_gnttab_unmap_frames() are called in
atomic context but were calling alloc_vm_area() which might sleep.

Also, if a driver attempts to allocate a grant ref from an interrupt
and the table needs expanding, then the CPU may already by in lazy MMU
mode and apply_to_page_range() will BUG when it tries to re-enable
lazy MMU mode.

These two functions are only used in PV guests.

Introduce arch_gnttab_init() to allocates the virtual address space in
advance.

Avoid the use of apply_to_page_range() by using saving and using the
array of PTE addresses from the alloc_vm_area() call.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/arm/xen/grant-table.c |    5 ++
 arch/x86/xen/grant-table.c |  147 +++++++++++++++++++++++++++-----------------
 drivers/xen/grant-table.c  |    9 ++-
 include/xen/grant_table.h  |    1 +
 4 files changed, 105 insertions(+), 57 deletions(-)

diff --git a/arch/arm/xen/grant-table.c b/arch/arm/xen/grant-table.c
index 859a9bb..91cf08b 100644
--- a/arch/arm/xen/grant-table.c
+++ b/arch/arm/xen/grant-table.c
@@ -51,3 +51,8 @@ int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
 {
 	return -ENOSYS;
 }
+
+int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
+{
+	return 0;
+}
diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
index c985835..27301df 100644
--- a/arch/x86/xen/grant-table.c
+++ b/arch/x86/xen/grant-table.c
@@ -36,6 +36,7 @@
 
 #include <linux/sched.h>
 #include <linux/mm.h>
+#include <linux/slab.h>
 #include <linux/vmalloc.h>
 
 #include <xen/interface/xen.h>
@@ -44,87 +45,121 @@
 
 #include <asm/pgtable.h>
 
-static int map_pte_fn(pte_t *pte, struct page *pmd_page,
-		      unsigned long addr, void *data)
+static struct gnttab_vm_area {
+	struct vm_struct *area;
+	pte_t **ptes;
+} gnttab_shared_vm_area, gnttab_status_vm_area;
+
+int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
+			   unsigned long max_nr_gframes,
+			   void **__shared)
 {
-	unsigned long **frames = (unsigned long **)data;
+	void *shared = *__shared;
+	unsigned long addr;
+	unsigned long i;
 
-	set_pte_at(&init_mm, addr, pte, mfn_pte((*frames)[0], PAGE_KERNEL));
-	(*frames)++;
-	return 0;
-}
+	if (shared == NULL)
+		*__shared = shared = gnttab_shared_vm_area.area->addr;
 
-/*
- * This function is used to map shared frames to store grant status. It is
- * different from map_pte_fn above, the frames type here is uint64_t.
- */
-static int map_pte_fn_status(pte_t *pte, struct page *pmd_page,
-			     unsigned long addr, void *data)
-{
-	uint64_t **frames = (uint64_t **)data;
+	addr = (unsigned long)shared;
+
+	for (i = 0; i < nr_gframes; i++) {
+		set_pte_at(&init_mm, addr, gnttab_shared_vm_area.ptes[i],
+			   mfn_pte(frames[i], PAGE_KERNEL));
+		addr += PAGE_SIZE;
+	}
 
-	set_pte_at(&init_mm, addr, pte, mfn_pte((*frames)[0], PAGE_KERNEL));
-	(*frames)++;
 	return 0;
 }
 
-static int unmap_pte_fn(pte_t *pte, struct page *pmd_page,
-			unsigned long addr, void *data)
+int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
+			   unsigned long max_nr_gframes,
+			   grant_status_t **__shared)
 {
+	grant_status_t *shared = *__shared;
+	unsigned long addr;
+	unsigned long i;
+
+	if (shared == NULL)
+		*__shared = shared = gnttab_status_vm_area.area->addr;
+
+	addr = (unsigned long)shared;
+
+	for (i = 0; i < nr_gframes; i++) {
+		set_pte_at(&init_mm, addr, gnttab_status_vm_area.ptes[i],
+			   mfn_pte(frames[i], PAGE_KERNEL));
+		addr += PAGE_SIZE;
+	}
 
-	set_pte_at(&init_mm, addr, pte, __pte(0));
 	return 0;
 }
 
-int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
-			   unsigned long max_nr_gframes,
-			   void **__shared)
+void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
 {
-	int rc;
-	void *shared = *__shared;
+	pte_t **ptes;
+	unsigned long addr;
+	unsigned long i;
 
-	if (shared == NULL) {
-		struct vm_struct *area =
-			alloc_vm_area(PAGE_SIZE * max_nr_gframes, NULL);
-		BUG_ON(area == NULL);
-		shared = area->addr;
-		*__shared = shared;
-	}
+	if (shared == gnttab_status_vm_area.area->addr)
+		ptes = gnttab_status_vm_area.ptes;
+	else
+		ptes = gnttab_shared_vm_area.ptes;
 
-	rc = apply_to_page_range(&init_mm, (unsigned long)shared,
-				 PAGE_SIZE * nr_gframes,
-				 map_pte_fn, &frames);
-	return rc;
+	addr = (unsigned long)shared;
+
+	for (i = 0; i < nr_gframes; i++) {
+		set_pte_at(&init_mm, addr, ptes[i], __pte(0));
+		addr += PAGE_SIZE;
+	}
 }
 
-int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
-			   unsigned long max_nr_gframes,
-			   grant_status_t **__shared)
+static int __init arch_gnttab_valloc(struct gnttab_vm_area *area,
+					 unsigned nr_frames)
 {
-	int rc;
-	grant_status_t *shared = *__shared;
+	area->ptes = kmalloc(sizeof(pte_t *) * nr_frames, GFP_KERNEL);
+	if (area->ptes == NULL)
+		return -ENOMEM;
 
-	if (shared == NULL) {
-		/* No need to pass in PTE as we are going to do it
-		 * in apply_to_page_range anyhow. */
-		struct vm_struct *area =
-			alloc_vm_area(PAGE_SIZE * max_nr_gframes, NULL);
-		BUG_ON(area == NULL);
-		shared = area->addr;
-		*__shared = shared;
+	area->area = alloc_vm_area(PAGE_SIZE * nr_frames, area->ptes);
+	if (area->area == NULL) {
+		kfree(area->ptes);
+		return -ENOMEM;
 	}
 
-	rc = apply_to_page_range(&init_mm, (unsigned long)shared,
-				 PAGE_SIZE * nr_gframes,
-				 map_pte_fn_status, &frames);
-	return rc;
+	return 0;
 }
 
-void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
+static void __init arch_gnttab_vfree(struct gnttab_vm_area *area)
 {
-	apply_to_page_range(&init_mm, (unsigned long)shared,
-			    PAGE_SIZE * nr_gframes, unmap_pte_fn, NULL);
+	free_vm_area(area->area);
+	kfree(area->ptes);
 }
+
+int __init arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
+{
+	int ret;
+
+	if (!xen_pv_domain())
+		return 0;
+
+	ret = arch_gnttab_valloc(&gnttab_shared_vm_area, nr_shared);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Always allocate the space for the status frames in case
+	 * we're migrated to a host with V2 support.
+	 */
+	ret = arch_gnttab_valloc(&gnttab_status_vm_area, nr_status);
+	if (ret < 0)
+		goto err;
+
+	return 0;
+  err:
+	arch_gnttab_vfree(&gnttab_shared_vm_area);
+	return -ENOMEM;
+}
+
 #ifdef CONFIG_XEN_PVH
 #include <xen/balloon.h>
 #include <xen/events.h>
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 5d4de88..eeba754 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -1195,18 +1195,20 @@ static int gnttab_expand(unsigned int req_entries)
 int gnttab_init(void)
 {
 	int i;
+	unsigned long max_nr_grant_frames;
 	unsigned int max_nr_glist_frames, nr_glist_frames;
 	unsigned int nr_init_grefs;
 	int ret;
 
 	gnttab_request_version();
+	max_nr_grant_frames = gnttab_max_grant_frames();
 	nr_grant_frames = 1;
 
 	/* Determine the maximum number of frames required for the
 	 * grant reference free list on the current hypervisor.
 	 */
 	BUG_ON(grefs_per_grant_frame == 0);
-	max_nr_glist_frames = (gnttab_max_grant_frames() *
+	max_nr_glist_frames = (max_nr_grant_frames *
 			       grefs_per_grant_frame / RPP);
 
 	gnttab_list = kmalloc(max_nr_glist_frames * sizeof(grant_ref_t *),
@@ -1223,6 +1225,11 @@ int gnttab_init(void)
 		}
 	}
 
+	ret = arch_gnttab_init(max_nr_grant_frames,
+			       nr_status_frames(max_nr_grant_frames));
+	if (ret < 0)
+		goto ini_nomem;
+
 	if (gnttab_setup() < 0) {
 		ret = -ENODEV;
 		goto ini_nomem;
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index a5af2a2..5c1aba1 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -170,6 +170,7 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap, phys_addr_t addr,
 	unmap->dev_bus_addr = 0;
 }
 
+int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status);
 int arch_gnttab_map_shared(xen_pfn_t *frames, unsigned long nr_gframes,
 			   unsigned long max_nr_gframes,
 			   void **__shared);
-- 
1.7.10.4

Re: [PATCH 1/2] x86/xen: safely map and unmap grant frames when in atomic context

[Konrad Rzeszutek Wilk]

On Wed, Jul 02, 2014 at 11:25:28AM +0100, David Vrabel wrote:
> arch_gnttab_map_frames() and arch_gnttab_unmap_frames() are called in
> atomic context but were calling alloc_vm_area() which might sleep.
> 
> Also, if a driver attempts to allocate a grant ref from an interrupt
> and the table needs expanding, then the CPU may already by in lazy MMU
> mode and apply_to_page_range() will BUG when it tries to re-enable
> lazy MMU mode.
> 
> These two functions are only used in PV guests.
> 
> Introduce arch_gnttab_init() to allocates the virtual address space in
> advance.
> 
> Avoid the use of apply_to_page_range() by using saving and using the
> array of PTE addresses from the alloc_vm_area() call.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

With:
 a) I believe all the __init has to go as the code will be
    called during suspend/resume cycle. But you already know that :-)

 b) If you could also add in the description: "alloc_vm_area'
    pre-allocates the pagetable so there is no need to worry about
    having to do a PGD/PUD/PMD walk (like apply_to_page_range does) and
    we can instead do set_pte. 

that you can stick on it:

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!
> ---
>  arch/arm/xen/grant-table.c |    5 ++
>  arch/x86/xen/grant-table.c |  147 +++++++++++++++++++++++++++-----------------
>  drivers/xen/grant-table.c  |    9 ++-
>  include/xen/grant_table.h  |    1 +
>  4 files changed, 105 insertions(+), 57 deletions(-)
> 
> diff --git a/arch/arm/xen/grant-table.c b/arch/arm/xen/grant-table.c
> index 859a9bb..91cf08b 100644
> --- a/arch/arm/xen/grant-table.c
> +++ b/arch/arm/xen/grant-table.c
> @@ -51,3 +51,8 @@ int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
>  {
>  	return -ENOSYS;
>  }
> +
> +int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
> +{
> +	return 0;
> +}
> diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
> index c985835..27301df 100644
> --- a/arch/x86/xen/grant-table.c
> +++ b/arch/x86/xen/grant-table.c
> @@ -36,6 +36,7 @@
>  
>  #include <linux/sched.h>
>  #include <linux/mm.h>
> +#include <linux/slab.h>
>  #include <linux/vmalloc.h>
>  
>  #include <xen/interface/xen.h>
> @@ -44,87 +45,121 @@
>  
>  #include <asm/pgtable.h>
>  
> -static int map_pte_fn(pte_t *pte, struct page *pmd_page,
> -		      unsigned long addr, void *data)
> +static struct gnttab_vm_area {
> +	struct vm_struct *area;
> +	pte_t **ptes;
> +} gnttab_shared_vm_area, gnttab_status_vm_area;
> +
> +int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
> +			   unsigned long max_nr_gframes,
> +			   void **__shared)
>  {
> -	unsigned long **frames = (unsigned long **)data;
> +	void *shared = *__shared;
> +	unsigned long addr;
> +	unsigned long i;
>  
> -	set_pte_at(&init_mm, addr, pte, mfn_pte((*frames)[0], PAGE_KERNEL));
> -	(*frames)++;
> -	return 0;
> -}
> +	if (shared == NULL)
> +		*__shared = shared = gnttab_shared_vm_area.area->addr;
>  
> -/*
> - * This function is used to map shared frames to store grant status. It is
> - * different from map_pte_fn above, the frames type here is uint64_t.
> - */
> -static int map_pte_fn_status(pte_t *pte, struct page *pmd_page,
> -			     unsigned long addr, void *data)
> -{
> -	uint64_t **frames = (uint64_t **)data;
> +	addr = (unsigned long)shared;
> +
> +	for (i = 0; i < nr_gframes; i++) {
> +		set_pte_at(&init_mm, addr, gnttab_shared_vm_area.ptes[i],
> +			   mfn_pte(frames[i], PAGE_KERNEL));
> +		addr += PAGE_SIZE;
> +	}
>  
> -	set_pte_at(&init_mm, addr, pte, mfn_pte((*frames)[0], PAGE_KERNEL));
> -	(*frames)++;
>  	return 0;
>  }
>  
> -static int unmap_pte_fn(pte_t *pte, struct page *pmd_page,
> -			unsigned long addr, void *data)
> +int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> +			   unsigned long max_nr_gframes,
> +			   grant_status_t **__shared)
>  {
> +	grant_status_t *shared = *__shared;
> +	unsigned long addr;
> +	unsigned long i;
> +
> +	if (shared == NULL)
> +		*__shared = shared = gnttab_status_vm_area.area->addr;
> +
> +	addr = (unsigned long)shared;
> +
> +	for (i = 0; i < nr_gframes; i++) {
> +		set_pte_at(&init_mm, addr, gnttab_status_vm_area.ptes[i],
> +			   mfn_pte(frames[i], PAGE_KERNEL));
> +		addr += PAGE_SIZE;
> +	}
>  
> -	set_pte_at(&init_mm, addr, pte, __pte(0));
>  	return 0;
>  }
>  
> -int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
> -			   unsigned long max_nr_gframes,
> -			   void **__shared)
> +void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
>  {
> -	int rc;
> -	void *shared = *__shared;
> +	pte_t **ptes;
> +	unsigned long addr;
> +	unsigned long i;
>  
> -	if (shared == NULL) {
> -		struct vm_struct *area =
> -			alloc_vm_area(PAGE_SIZE * max_nr_gframes, NULL);
> -		BUG_ON(area == NULL);
> -		shared = area->addr;
> -		*__shared = shared;
> -	}
> +	if (shared == gnttab_status_vm_area.area->addr)
> +		ptes = gnttab_status_vm_area.ptes;
> +	else
> +		ptes = gnttab_shared_vm_area.ptes;
>  
> -	rc = apply_to_page_range(&init_mm, (unsigned long)shared,
> -				 PAGE_SIZE * nr_gframes,
> -				 map_pte_fn, &frames);
> -	return rc;
> +	addr = (unsigned long)shared;
> +
> +	for (i = 0; i < nr_gframes; i++) {
> +		set_pte_at(&init_mm, addr, ptes[i], __pte(0));
> +		addr += PAGE_SIZE;
> +	}
>  }
>  
> -int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> -			   unsigned long max_nr_gframes,
> -			   grant_status_t **__shared)
> +static int __init arch_gnttab_valloc(struct gnttab_vm_area *area,
> +					 unsigned nr_frames)
>  {
> -	int rc;
> -	grant_status_t *shared = *__shared;
> +	area->ptes = kmalloc(sizeof(pte_t *) * nr_frames, GFP_KERNEL);
> +	if (area->ptes == NULL)
> +		return -ENOMEM;
>  
> -	if (shared == NULL) {
> -		/* No need to pass in PTE as we are going to do it
> -		 * in apply_to_page_range anyhow. */
> -		struct vm_struct *area =
> -			alloc_vm_area(PAGE_SIZE * max_nr_gframes, NULL);
> -		BUG_ON(area == NULL);
> -		shared = area->addr;
> -		*__shared = shared;
> +	area->area = alloc_vm_area(PAGE_SIZE * nr_frames, area->ptes);
> +	if (area->area == NULL) {
> +		kfree(area->ptes);
> +		return -ENOMEM;
>  	}
>  
> -	rc = apply_to_page_range(&init_mm, (unsigned long)shared,
> -				 PAGE_SIZE * nr_gframes,
> -				 map_pte_fn_status, &frames);
> -	return rc;
> +	return 0;
>  }
>  
> -void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
> +static void __init arch_gnttab_vfree(struct gnttab_vm_area *area)
>  {
> -	apply_to_page_range(&init_mm, (unsigned long)shared,
> -			    PAGE_SIZE * nr_gframes, unmap_pte_fn, NULL);
> +	free_vm_area(area->area);
> +	kfree(area->ptes);
>  }
> +
> +int __init arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
> +{
> +	int ret;
> +
> +	if (!xen_pv_domain())
> +		return 0;
> +
> +	ret = arch_gnttab_valloc(&gnttab_shared_vm_area, nr_shared);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Always allocate the space for the status frames in case
> +	 * we're migrated to a host with V2 support.
> +	 */
> +	ret = arch_gnttab_valloc(&gnttab_status_vm_area, nr_status);
> +	if (ret < 0)
> +		goto err;
> +
> +	return 0;
> +  err:
> +	arch_gnttab_vfree(&gnttab_shared_vm_area);
> +	return -ENOMEM;
> +}
> +
>  #ifdef CONFIG_XEN_PVH
>  #include <xen/balloon.h>
>  #include <xen/events.h>
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 5d4de88..eeba754 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -1195,18 +1195,20 @@ static int gnttab_expand(unsigned int req_entries)
>  int gnttab_init(void)
>  {
>  	int i;
> +	unsigned long max_nr_grant_frames;
>  	unsigned int max_nr_glist_frames, nr_glist_frames;
>  	unsigned int nr_init_grefs;
>  	int ret;
>  
>  	gnttab_request_version();
> +	max_nr_grant_frames = gnttab_max_grant_frames();
>  	nr_grant_frames = 1;
>  
>  	/* Determine the maximum number of frames required for the
>  	 * grant reference free list on the current hypervisor.
>  	 */
>  	BUG_ON(grefs_per_grant_frame == 0);
> -	max_nr_glist_frames = (gnttab_max_grant_frames() *
> +	max_nr_glist_frames = (max_nr_grant_frames *
>  			       grefs_per_grant_frame / RPP);
>  
>  	gnttab_list = kmalloc(max_nr_glist_frames * sizeof(grant_ref_t *),
> @@ -1223,6 +1225,11 @@ int gnttab_init(void)
>  		}
>  	}
>  
> +	ret = arch_gnttab_init(max_nr_grant_frames,
> +			       nr_status_frames(max_nr_grant_frames));
> +	if (ret < 0)
> +		goto ini_nomem;
> +
>  	if (gnttab_setup() < 0) {
>  		ret = -ENODEV;
>  		goto ini_nomem;
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index a5af2a2..5c1aba1 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -170,6 +170,7 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap, phys_addr_t addr,
>  	unmap->dev_bus_addr = 0;
>  }
>  
> +int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status);
>  int arch_gnttab_map_shared(xen_pfn_t *frames, unsigned long nr_gframes,
>  			   unsigned long max_nr_gframes,
>  			   void **__shared);
> -- 
> 1.7.10.4
> 

[PATCH 2/2] xen/grant-table: remove support for V2 tables

[David Vrabel]

Since 11c7ff17c9b6dbf3a4e4f36be30ad531a6cf0ec9 (xen/grant-table: Force
to use v1 of grants.) the code for V2 grant tables is not used.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/arm/xen/grant-table.c |    9 +-
 arch/x86/xen/grant-table.c |   60 +--------
 drivers/xen/grant-table.c  |  309 +-------------------------------------------
 include/xen/grant_table.h  |   30 +----
 4 files changed, 13 insertions(+), 395 deletions(-)

diff --git a/arch/arm/xen/grant-table.c b/arch/arm/xen/grant-table.c
index 91cf08b..e437918 100644
--- a/arch/arm/xen/grant-table.c
+++ b/arch/arm/xen/grant-table.c
@@ -45,14 +45,7 @@ void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
 	return;
 }
 
-int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
-			   unsigned long max_nr_gframes,
-			   grant_status_t **__shared)
-{
-	return -ENOSYS;
-}
-
-int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
+int arch_gnttab_init(unsigned long nr_shared)
 {
 	return 0;
 }
diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
index 27301df..c1ef6b2 100644
--- a/arch/x86/xen/grant-table.c
+++ b/arch/x86/xen/grant-table.c
@@ -48,7 +48,7 @@
 static struct gnttab_vm_area {
 	struct vm_struct *area;
 	pte_t **ptes;
-} gnttab_shared_vm_area, gnttab_status_vm_area;
+} gnttab_shared_vm_area;
 
 int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
 			   unsigned long max_nr_gframes,
@@ -72,43 +72,16 @@ int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
 	return 0;
 }
 
-int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
-			   unsigned long max_nr_gframes,
-			   grant_status_t **__shared)
-{
-	grant_status_t *shared = *__shared;
-	unsigned long addr;
-	unsigned long i;
-
-	if (shared == NULL)
-		*__shared = shared = gnttab_status_vm_area.area->addr;
-
-	addr = (unsigned long)shared;
-
-	for (i = 0; i < nr_gframes; i++) {
-		set_pte_at(&init_mm, addr, gnttab_status_vm_area.ptes[i],
-			   mfn_pte(frames[i], PAGE_KERNEL));
-		addr += PAGE_SIZE;
-	}
-
-	return 0;
-}
-
 void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
 {
-	pte_t **ptes;
 	unsigned long addr;
 	unsigned long i;
 
-	if (shared == gnttab_status_vm_area.area->addr)
-		ptes = gnttab_status_vm_area.ptes;
-	else
-		ptes = gnttab_shared_vm_area.ptes;
-
 	addr = (unsigned long)shared;
 
 	for (i = 0; i < nr_gframes; i++) {
-		set_pte_at(&init_mm, addr, ptes[i], __pte(0));
+		set_pte_at(&init_mm, addr, gnttab_shared_vm_area.ptes[i],
+			   __pte(0));
 		addr += PAGE_SIZE;
 	}
 }
@@ -129,35 +102,12 @@ static int __init arch_gnttab_valloc(struct gnttab_vm_area *area,
 	return 0;
 }
 
-static void __init arch_gnttab_vfree(struct gnttab_vm_area *area)
+int __init arch_gnttab_init(unsigned long nr_shared)
 {
-	free_vm_area(area->area);
-	kfree(area->ptes);
-}
-
-int __init arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
-{
-	int ret;
-
 	if (!xen_pv_domain())
 		return 0;
 
-	ret = arch_gnttab_valloc(&gnttab_shared_vm_area, nr_shared);
-	if (ret < 0)
-		return ret;
-
-	/*
-	 * Always allocate the space for the status frames in case
-	 * we're migrated to a host with V2 support.
-	 */
-	ret = arch_gnttab_valloc(&gnttab_status_vm_area, nr_status);
-	if (ret < 0)
-		goto err;
-
-	return 0;
-  err:
-	arch_gnttab_vfree(&gnttab_shared_vm_area);
-	return -ENOMEM;
+	return arch_gnttab_valloc(&gnttab_shared_vm_area, nr_shared);
 }
 
 #ifdef CONFIG_XEN_PVH
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index eeba754..c254ae0 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -69,7 +69,6 @@ struct grant_frames xen_auto_xlat_grant_frames;
 
 static union {
 	struct grant_entry_v1 *v1;
-	union grant_entry_v2 *v2;
 	void *addr;
 } gnttab_shared;
 
@@ -120,36 +119,10 @@ struct gnttab_ops {
 	 * by bit operations.
 	 */
 	int (*query_foreign_access)(grant_ref_t ref);
-	/*
-	 * Grant a domain to access a range of bytes within the page referred by
-	 * an available grant entry. Ref parameter is reference of a grant entry
-	 * which will be sub-page accessed, domid is id of grantee domain, frame
-	 * is frame address of subpage grant, flags is grant type and flag
-	 * information, page_off is offset of the range of bytes, and length is
-	 * length of bytes to be accessed.
-	 */
-	void (*update_subpage_entry)(grant_ref_t ref, domid_t domid,
-				     unsigned long frame, int flags,
-				     unsigned page_off, unsigned length);
-	/*
-	 * Redirect an available grant entry on domain A to another grant
-	 * reference of domain B, then allow domain C to use grant reference
-	 * of domain B transitively. Ref parameter is an available grant entry
-	 * reference on domain A, domid is id of domain C which accesses grant
-	 * entry transitively, flags is grant type and flag information,
-	 * trans_domid is id of domain B whose grant entry is finally accessed
-	 * transitively, trans_gref is grant entry transitive reference of
-	 * domain B.
-	 */
-	void (*update_trans_entry)(grant_ref_t ref, domid_t domid, int flags,
-				   domid_t trans_domid, grant_ref_t trans_gref);
 };
 
 static struct gnttab_ops *gnttab_interface;
 
-/*This reflects status of grant entries, so act as a global value*/
-static grant_status_t *grstatus;
-
 static int grant_table_version;
 static int grefs_per_grant_frame;
 
@@ -231,7 +204,7 @@ static void put_free_entry(grant_ref_t ref)
 }
 
 /*
- * Following applies to gnttab_update_entry_v1 and gnttab_update_entry_v2.
+ * Following applies to gnttab_update_entry_v1.
  * Introducing a valid entry into the grant table:
  *  1. Write ent->domid.
  *  2. Write ent->frame:
@@ -250,15 +223,6 @@ static void gnttab_update_entry_v1(grant_ref_t ref, domid_t domid,
 	gnttab_shared.v1[ref].flags = flags;
 }
 
-static void gnttab_update_entry_v2(grant_ref_t ref, domid_t domid,
-				   unsigned long frame, unsigned flags)
-{
-	gnttab_shared.v2[ref].hdr.domid = domid;
-	gnttab_shared.v2[ref].full_page.frame = frame;
-	wmb();
-	gnttab_shared.v2[ref].hdr.flags = GTF_permit_access | flags;
-}
-
 /*
  * Public grant-issuing interface functions
  */
@@ -285,132 +249,11 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);
 
-static void gnttab_update_subpage_entry_v2(grant_ref_t ref, domid_t domid,
-					   unsigned long frame, int flags,
-					   unsigned page_off, unsigned length)
-{
-	gnttab_shared.v2[ref].sub_page.frame = frame;
-	gnttab_shared.v2[ref].sub_page.page_off = page_off;
-	gnttab_shared.v2[ref].sub_page.length = length;
-	gnttab_shared.v2[ref].hdr.domid = domid;
-	wmb();
-	gnttab_shared.v2[ref].hdr.flags =
-				GTF_permit_access | GTF_sub_page | flags;
-}
-
-int gnttab_grant_foreign_access_subpage_ref(grant_ref_t ref, domid_t domid,
-					    unsigned long frame, int flags,
-					    unsigned page_off,
-					    unsigned length)
-{
-	if (flags & (GTF_accept_transfer | GTF_reading |
-		     GTF_writing | GTF_transitive))
-		return -EPERM;
-
-	if (gnttab_interface->update_subpage_entry == NULL)
-		return -ENOSYS;
-
-	gnttab_interface->update_subpage_entry(ref, domid, frame, flags,
-					       page_off, length);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_subpage_ref);
-
-int gnttab_grant_foreign_access_subpage(domid_t domid, unsigned long frame,
-					int flags, unsigned page_off,
-					unsigned length)
-{
-	int ref, rc;
-
-	ref = get_free_entries(1);
-	if (unlikely(ref < 0))
-		return -ENOSPC;
-
-	rc = gnttab_grant_foreign_access_subpage_ref(ref, domid, frame, flags,
-						     page_off, length);
-	if (rc < 0) {
-		put_free_entry(ref);
-		return rc;
-	}
-
-	return ref;
-}
-EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_subpage);
-
-bool gnttab_subpage_grants_available(void)
-{
-	return gnttab_interface->update_subpage_entry != NULL;
-}
-EXPORT_SYMBOL_GPL(gnttab_subpage_grants_available);
-
-static void gnttab_update_trans_entry_v2(grant_ref_t ref, domid_t domid,
-					 int flags, domid_t trans_domid,
-					 grant_ref_t trans_gref)
-{
-	gnttab_shared.v2[ref].transitive.trans_domid = trans_domid;
-	gnttab_shared.v2[ref].transitive.gref = trans_gref;
-	gnttab_shared.v2[ref].hdr.domid = domid;
-	wmb();
-	gnttab_shared.v2[ref].hdr.flags =
-				GTF_permit_access | GTF_transitive | flags;
-}
-
-int gnttab_grant_foreign_access_trans_ref(grant_ref_t ref, domid_t domid,
-					  int flags, domid_t trans_domid,
-					  grant_ref_t trans_gref)
-{
-	if (flags & (GTF_accept_transfer | GTF_reading |
-		     GTF_writing | GTF_sub_page))
-		return -EPERM;
-
-	if (gnttab_interface->update_trans_entry == NULL)
-		return -ENOSYS;
-
-	gnttab_interface->update_trans_entry(ref, domid, flags, trans_domid,
-					     trans_gref);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_trans_ref);
-
-int gnttab_grant_foreign_access_trans(domid_t domid, int flags,
-				      domid_t trans_domid,
-				      grant_ref_t trans_gref)
-{
-	int ref, rc;
-
-	ref = get_free_entries(1);
-	if (unlikely(ref < 0))
-		return -ENOSPC;
-
-	rc = gnttab_grant_foreign_access_trans_ref(ref, domid, flags,
-						   trans_domid, trans_gref);
-	if (rc < 0) {
-		put_free_entry(ref);
-		return rc;
-	}
-
-	return ref;
-}
-EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_trans);
-
-bool gnttab_trans_grants_available(void)
-{
-	return gnttab_interface->update_trans_entry != NULL;
-}
-EXPORT_SYMBOL_GPL(gnttab_trans_grants_available);
-
 static int gnttab_query_foreign_access_v1(grant_ref_t ref)
 {
 	return gnttab_shared.v1[ref].flags & (GTF_reading|GTF_writing);
 }
 
-static int gnttab_query_foreign_access_v2(grant_ref_t ref)
-{
-	return grstatus[ref] & (GTF_reading|GTF_writing);
-}
-
 int gnttab_query_foreign_access(grant_ref_t ref)
 {
 	return gnttab_interface->query_foreign_access(ref);
@@ -433,29 +276,6 @@ static int gnttab_end_foreign_access_ref_v1(grant_ref_t ref, int readonly)
 	return 1;
 }
 
-static int gnttab_end_foreign_access_ref_v2(grant_ref_t ref, int readonly)
-{
-	gnttab_shared.v2[ref].hdr.flags = 0;
-	mb();
-	if (grstatus[ref] & (GTF_reading|GTF_writing)) {
-		return 0;
-	} else {
-		/* The read of grstatus needs to have acquire
-		semantics.  On x86, reads already have
-		that, and we just need to protect against
-		compiler reorderings.  On other
-		architectures we may need a full
-		barrier. */
-#ifdef CONFIG_X86
-		barrier();
-#else
-		mb();
-#endif
-	}
-
-	return 1;
-}
-
 static inline int _gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
 {
 	return gnttab_interface->end_foreign_access_ref(ref, readonly);
@@ -616,37 +436,6 @@ static unsigned long gnttab_end_foreign_transfer_ref_v1(grant_ref_t ref)
 	return frame;
 }
 
-static unsigned long gnttab_end_foreign_transfer_ref_v2(grant_ref_t ref)
-{
-	unsigned long frame;
-	u16           flags;
-	u16          *pflags;
-
-	pflags = &gnttab_shared.v2[ref].hdr.flags;
-
-	/*
-	 * If a transfer is not even yet started, try to reclaim the grant
-	 * reference and return failure (== 0).
-	 */
-	while (!((flags = *pflags) & GTF_transfer_committed)) {
-		if (sync_cmpxchg(pflags, flags, 0) == flags)
-			return 0;
-		cpu_relax();
-	}
-
-	/* If a transfer is in progress then wait until it is completed. */
-	while (!(flags & GTF_transfer_completed)) {
-		flags = *pflags;
-		cpu_relax();
-	}
-
-	rmb();  /* Read the frame number /after/ reading completion status. */
-	frame = gnttab_shared.v2[ref].full_page.frame;
-	BUG_ON(frame == 0);
-
-	return frame;
-}
-
 unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
 {
 	return gnttab_interface->end_foreign_transfer_ref(ref);
@@ -962,12 +751,6 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
 }
 EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
 
-static unsigned nr_status_frames(unsigned nr_grant_frames)
-{
-	BUG_ON(grefs_per_grant_frame == 0);
-	return (nr_grant_frames * grefs_per_grant_frame + SPP - 1) / SPP;
-}
-
 static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
 {
 	int rc;
@@ -985,55 +768,6 @@ static void gnttab_unmap_frames_v1(void)
 	arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
 }
 
-static int gnttab_map_frames_v2(xen_pfn_t *frames, unsigned int nr_gframes)
-{
-	uint64_t *sframes;
-	unsigned int nr_sframes;
-	struct gnttab_get_status_frames getframes;
-	int rc;
-
-	nr_sframes = nr_status_frames(nr_gframes);
-
-	/* No need for kzalloc as it is initialized in following hypercall
-	 * GNTTABOP_get_status_frames.
-	 */
-	sframes = kmalloc(nr_sframes  * sizeof(uint64_t), GFP_ATOMIC);
-	if (!sframes)
-		return -ENOMEM;
-
-	getframes.dom        = DOMID_SELF;
-	getframes.nr_frames  = nr_sframes;
-	set_xen_guest_handle(getframes.frame_list, sframes);
-
-	rc = HYPERVISOR_grant_table_op(GNTTABOP_get_status_frames,
-				       &getframes, 1);
-	if (rc == -ENOSYS) {
-		kfree(sframes);
-		return -ENOSYS;
-	}
-
-	BUG_ON(rc || getframes.status);
-
-	rc = arch_gnttab_map_status(sframes, nr_sframes,
-				    nr_status_frames(gnttab_max_grant_frames()),
-				    &grstatus);
-	BUG_ON(rc);
-	kfree(sframes);
-
-	rc = arch_gnttab_map_shared(frames, nr_gframes,
-				    gnttab_max_grant_frames(),
-				    &gnttab_shared.addr);
-	BUG_ON(rc);
-
-	return 0;
-}
-
-static void gnttab_unmap_frames_v2(void)
-{
-	arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
-	arch_gnttab_unmap(grstatus, nr_status_frames(nr_grant_frames));
-}
-
 static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
 {
 	struct gnttab_setup_table setup;
@@ -1101,43 +835,13 @@ static struct gnttab_ops gnttab_v1_ops = {
 	.query_foreign_access		= gnttab_query_foreign_access_v1,
 };
 
-static struct gnttab_ops gnttab_v2_ops = {
-	.map_frames			= gnttab_map_frames_v2,
-	.unmap_frames			= gnttab_unmap_frames_v2,
-	.update_entry			= gnttab_update_entry_v2,
-	.end_foreign_access_ref		= gnttab_end_foreign_access_ref_v2,
-	.end_foreign_transfer_ref	= gnttab_end_foreign_transfer_ref_v2,
-	.query_foreign_access		= gnttab_query_foreign_access_v2,
-	.update_subpage_entry		= gnttab_update_subpage_entry_v2,
-	.update_trans_entry		= gnttab_update_trans_entry_v2,
-};
-
 static void gnttab_request_version(void)
 {
-	int rc;
-	struct gnttab_set_version gsv;
+	/* Only version 1 is used, which will always be available. */
+	grant_table_version = 1;
+	grefs_per_grant_frame = PAGE_SIZE / sizeof(struct grant_entry_v1);
+	gnttab_interface = &gnttab_v1_ops;
 
-	gsv.version = 1;
-
-	rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
-	if (rc == 0 && gsv.version == 2) {
-		grant_table_version = 2;
-		grefs_per_grant_frame = PAGE_SIZE / sizeof(union grant_entry_v2);
-		gnttab_interface = &gnttab_v2_ops;
-	} else if (grant_table_version == 2) {
-		/*
-		 * If we've already used version 2 features,
-		 * but then suddenly discover that they're not
-		 * available (e.g. migrating to an older
-		 * version of Xen), almost unbounded badness
-		 * can happen.
-		 */
-		panic("we need grant tables version 2, but only version 1 is available");
-	} else {
-		grant_table_version = 1;
-		grefs_per_grant_frame = PAGE_SIZE / sizeof(struct grant_entry_v1);
-		gnttab_interface = &gnttab_v1_ops;
-	}
 	pr_info("Grant tables using version %d layout\n", grant_table_version);
 }
 
@@ -1225,8 +929,7 @@ int gnttab_init(void)
 		}
 	}
 
-	ret = arch_gnttab_init(max_nr_grant_frames,
-			       nr_status_frames(max_nr_grant_frames));
+	ret = arch_gnttab_init(max_nr_grant_frames);
 	if (ret < 0)
 		goto ini_nomem;
 
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 5c1aba1..3387465 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -64,24 +64,6 @@ int gnttab_resume(void);
 
 int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
 				int readonly);
-int gnttab_grant_foreign_access_subpage(domid_t domid, unsigned long frame,
-					int flags, unsigned page_off,
-					unsigned length);
-int gnttab_grant_foreign_access_trans(domid_t domid, int flags,
-				      domid_t trans_domid,
-				      grant_ref_t trans_gref);
-
-/*
- * Are sub-page grants available on this version of Xen?  Returns true if they
- * are, and false if they're not.
- */
-bool gnttab_subpage_grants_available(void);
-
-/*
- * Are transitive grants available on this version of Xen?  Returns true if they
- * are, and false if they're not.
- */
-bool gnttab_trans_grants_available(void);
 
 /*
  * End access through the given grant reference, iff the grant entry is no
@@ -128,13 +110,6 @@ void gnttab_cancel_free_callback(struct gnttab_free_callback *callback);
 
 void gnttab_grant_foreign_access_ref(grant_ref_t ref, domid_t domid,
 				     unsigned long frame, int readonly);
-int gnttab_grant_foreign_access_subpage_ref(grant_ref_t ref, domid_t domid,
-					    unsigned long frame, int flags,
-					    unsigned page_off,
-					    unsigned length);
-int gnttab_grant_foreign_access_trans_ref(grant_ref_t ref, domid_t domid,
-					  int flags, domid_t trans_domid,
-					  grant_ref_t trans_gref);
 
 void gnttab_grant_foreign_transfer_ref(grant_ref_t, domid_t domid,
 				       unsigned long pfn);
@@ -170,13 +145,10 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap, phys_addr_t addr,
 	unmap->dev_bus_addr = 0;
 }
 
-int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status);
+int arch_gnttab_init(unsigned long nr_shared);
 int arch_gnttab_map_shared(xen_pfn_t *frames, unsigned long nr_gframes,
 			   unsigned long max_nr_gframes,
 			   void **__shared);
-int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
-			   unsigned long max_nr_gframes,
-			   grant_status_t **__shared);
 void arch_gnttab_unmap(void *shared, unsigned long nr_gframes);
 
 struct grant_frames {
-- 
1.7.10.4

Re: [PATCH 2/2] xen/grant-table: remove support for V2 tables

[Konrad Rzeszutek Wilk]

On Wed, Jul 02, 2014 at 11:25:29AM +0100, David Vrabel wrote:
> Since 11c7ff17c9b6dbf3a4e4f36be30ad531a6cf0ec9 (xen/grant-table: Force
> to use v1 of grants.) the code for V2 grant tables is not used.

I get compile warnings with this patch:
WARNING: vmlinux.o(.text+0x40268a): Section mismatch in reference from the function gnttab_init() to the function .init.text:arch_gnttab
_init()
The function gnttab_init() references
the function __init arch_gnttab_init().
This is often because gnttab_init lacks a __init
annotation or the annotation of arch_gnttab_init is wrong.


> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  arch/arm/xen/grant-table.c |    9 +-
>  arch/x86/xen/grant-table.c |   60 +--------
>  drivers/xen/grant-table.c  |  309 +-------------------------------------------
>  include/xen/grant_table.h  |   30 +----
>  4 files changed, 13 insertions(+), 395 deletions(-)
> 
> diff --git a/arch/arm/xen/grant-table.c b/arch/arm/xen/grant-table.c
> index 91cf08b..e437918 100644
> --- a/arch/arm/xen/grant-table.c
> +++ b/arch/arm/xen/grant-table.c
> @@ -45,14 +45,7 @@ void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
>  	return;
>  }
>  
> -int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> -			   unsigned long max_nr_gframes,
> -			   grant_status_t **__shared)
> -{
> -	return -ENOSYS;
> -}
> -
> -int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
> +int arch_gnttab_init(unsigned long nr_shared)
>  {
>  	return 0;
>  }
> diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
> index 27301df..c1ef6b2 100644
> --- a/arch/x86/xen/grant-table.c
> +++ b/arch/x86/xen/grant-table.c
> @@ -48,7 +48,7 @@
>  static struct gnttab_vm_area {
>  	struct vm_struct *area;
>  	pte_t **ptes;
> -} gnttab_shared_vm_area, gnttab_status_vm_area;
> +} gnttab_shared_vm_area;
>  
>  int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
>  			   unsigned long max_nr_gframes,
> @@ -72,43 +72,16 @@ int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
>  	return 0;
>  }
>  
> -int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> -			   unsigned long max_nr_gframes,
> -			   grant_status_t **__shared)
> -{
> -	grant_status_t *shared = *__shared;
> -	unsigned long addr;
> -	unsigned long i;
> -
> -	if (shared == NULL)
> -		*__shared = shared = gnttab_status_vm_area.area->addr;
> -
> -	addr = (unsigned long)shared;
> -
> -	for (i = 0; i < nr_gframes; i++) {
> -		set_pte_at(&init_mm, addr, gnttab_status_vm_area.ptes[i],
> -			   mfn_pte(frames[i], PAGE_KERNEL));
> -		addr += PAGE_SIZE;
> -	}
> -
> -	return 0;
> -}
> -
>  void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
>  {
> -	pte_t **ptes;
>  	unsigned long addr;
>  	unsigned long i;
>  
> -	if (shared == gnttab_status_vm_area.area->addr)
> -		ptes = gnttab_status_vm_area.ptes;
> -	else
> -		ptes = gnttab_shared_vm_area.ptes;
> -
>  	addr = (unsigned long)shared;
>  
>  	for (i = 0; i < nr_gframes; i++) {
> -		set_pte_at(&init_mm, addr, ptes[i], __pte(0));
> +		set_pte_at(&init_mm, addr, gnttab_shared_vm_area.ptes[i],
> +			   __pte(0));
>  		addr += PAGE_SIZE;
>  	}
>  }
> @@ -129,35 +102,12 @@ static int __init arch_gnttab_valloc(struct gnttab_vm_area *area,
>  	return 0;
>  }
>  
> -static void __init arch_gnttab_vfree(struct gnttab_vm_area *area)
> +int __init arch_gnttab_init(unsigned long nr_shared)
>  {
> -	free_vm_area(area->area);
> -	kfree(area->ptes);
> -}
> -
> -int __init arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
> -{
> -	int ret;
> -
>  	if (!xen_pv_domain())
>  		return 0;
>  
> -	ret = arch_gnttab_valloc(&gnttab_shared_vm_area, nr_shared);
> -	if (ret < 0)
> -		return ret;
> -
> -	/*
> -	 * Always allocate the space for the status frames in case
> -	 * we're migrated to a host with V2 support.
> -	 */
> -	ret = arch_gnttab_valloc(&gnttab_status_vm_area, nr_status);
> -	if (ret < 0)
> -		goto err;
> -
> -	return 0;
> -  err:
> -	arch_gnttab_vfree(&gnttab_shared_vm_area);
> -	return -ENOMEM;
> +	return arch_gnttab_valloc(&gnttab_shared_vm_area, nr_shared);
>  }
>  
>  #ifdef CONFIG_XEN_PVH
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index eeba754..c254ae0 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -69,7 +69,6 @@ struct grant_frames xen_auto_xlat_grant_frames;
>  
>  static union {
>  	struct grant_entry_v1 *v1;
> -	union grant_entry_v2 *v2;
>  	void *addr;
>  } gnttab_shared;
>  
> @@ -120,36 +119,10 @@ struct gnttab_ops {
>  	 * by bit operations.
>  	 */
>  	int (*query_foreign_access)(grant_ref_t ref);
> -	/*
> -	 * Grant a domain to access a range of bytes within the page referred by
> -	 * an available grant entry. Ref parameter is reference of a grant entry
> -	 * which will be sub-page accessed, domid is id of grantee domain, frame
> -	 * is frame address of subpage grant, flags is grant type and flag
> -	 * information, page_off is offset of the range of bytes, and length is
> -	 * length of bytes to be accessed.
> -	 */
> -	void (*update_subpage_entry)(grant_ref_t ref, domid_t domid,
> -				     unsigned long frame, int flags,
> -				     unsigned page_off, unsigned length);
> -	/*
> -	 * Redirect an available grant entry on domain A to another grant
> -	 * reference of domain B, then allow domain C to use grant reference
> -	 * of domain B transitively. Ref parameter is an available grant entry
> -	 * reference on domain A, domid is id of domain C which accesses grant
> -	 * entry transitively, flags is grant type and flag information,
> -	 * trans_domid is id of domain B whose grant entry is finally accessed
> -	 * transitively, trans_gref is grant entry transitive reference of
> -	 * domain B.
> -	 */
> -	void (*update_trans_entry)(grant_ref_t ref, domid_t domid, int flags,
> -				   domid_t trans_domid, grant_ref_t trans_gref);
>  };
>  
>  static struct gnttab_ops *gnttab_interface;
>  
> -/*This reflects status of grant entries, so act as a global value*/
> -static grant_status_t *grstatus;
> -
>  static int grant_table_version;
>  static int grefs_per_grant_frame;
>  
> @@ -231,7 +204,7 @@ static void put_free_entry(grant_ref_t ref)
>  }
>  
>  /*
> - * Following applies to gnttab_update_entry_v1 and gnttab_update_entry_v2.
> + * Following applies to gnttab_update_entry_v1.
>   * Introducing a valid entry into the grant table:
>   *  1. Write ent->domid.
>   *  2. Write ent->frame:
> @@ -250,15 +223,6 @@ static void gnttab_update_entry_v1(grant_ref_t ref, domid_t domid,
>  	gnttab_shared.v1[ref].flags = flags;
>  }
>  
> -static void gnttab_update_entry_v2(grant_ref_t ref, domid_t domid,
> -				   unsigned long frame, unsigned flags)
> -{
> -	gnttab_shared.v2[ref].hdr.domid = domid;
> -	gnttab_shared.v2[ref].full_page.frame = frame;
> -	wmb();
> -	gnttab_shared.v2[ref].hdr.flags = GTF_permit_access | flags;
> -}
> -
>  /*
>   * Public grant-issuing interface functions
>   */
> @@ -285,132 +249,11 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
>  }
>  EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);
>  
> -static void gnttab_update_subpage_entry_v2(grant_ref_t ref, domid_t domid,
> -					   unsigned long frame, int flags,
> -					   unsigned page_off, unsigned length)
> -{
> -	gnttab_shared.v2[ref].sub_page.frame = frame;
> -	gnttab_shared.v2[ref].sub_page.page_off = page_off;
> -	gnttab_shared.v2[ref].sub_page.length = length;
> -	gnttab_shared.v2[ref].hdr.domid = domid;
> -	wmb();
> -	gnttab_shared.v2[ref].hdr.flags =
> -				GTF_permit_access | GTF_sub_page | flags;
> -}
> -
> -int gnttab_grant_foreign_access_subpage_ref(grant_ref_t ref, domid_t domid,
> -					    unsigned long frame, int flags,
> -					    unsigned page_off,
> -					    unsigned length)
> -{
> -	if (flags & (GTF_accept_transfer | GTF_reading |
> -		     GTF_writing | GTF_transitive))
> -		return -EPERM;
> -
> -	if (gnttab_interface->update_subpage_entry == NULL)
> -		return -ENOSYS;
> -
> -	gnttab_interface->update_subpage_entry(ref, domid, frame, flags,
> -					       page_off, length);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_subpage_ref);
> -
> -int gnttab_grant_foreign_access_subpage(domid_t domid, unsigned long frame,
> -					int flags, unsigned page_off,
> -					unsigned length)
> -{
> -	int ref, rc;
> -
> -	ref = get_free_entries(1);
> -	if (unlikely(ref < 0))
> -		return -ENOSPC;
> -
> -	rc = gnttab_grant_foreign_access_subpage_ref(ref, domid, frame, flags,
> -						     page_off, length);
> -	if (rc < 0) {
> -		put_free_entry(ref);
> -		return rc;
> -	}
> -
> -	return ref;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_subpage);
> -
> -bool gnttab_subpage_grants_available(void)
> -{
> -	return gnttab_interface->update_subpage_entry != NULL;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_subpage_grants_available);
> -
> -static void gnttab_update_trans_entry_v2(grant_ref_t ref, domid_t domid,
> -					 int flags, domid_t trans_domid,
> -					 grant_ref_t trans_gref)
> -{
> -	gnttab_shared.v2[ref].transitive.trans_domid = trans_domid;
> -	gnttab_shared.v2[ref].transitive.gref = trans_gref;
> -	gnttab_shared.v2[ref].hdr.domid = domid;
> -	wmb();
> -	gnttab_shared.v2[ref].hdr.flags =
> -				GTF_permit_access | GTF_transitive | flags;
> -}
> -
> -int gnttab_grant_foreign_access_trans_ref(grant_ref_t ref, domid_t domid,
> -					  int flags, domid_t trans_domid,
> -					  grant_ref_t trans_gref)
> -{
> -	if (flags & (GTF_accept_transfer | GTF_reading |
> -		     GTF_writing | GTF_sub_page))
> -		return -EPERM;
> -
> -	if (gnttab_interface->update_trans_entry == NULL)
> -		return -ENOSYS;
> -
> -	gnttab_interface->update_trans_entry(ref, domid, flags, trans_domid,
> -					     trans_gref);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_trans_ref);
> -
> -int gnttab_grant_foreign_access_trans(domid_t domid, int flags,
> -				      domid_t trans_domid,
> -				      grant_ref_t trans_gref)
> -{
> -	int ref, rc;
> -
> -	ref = get_free_entries(1);
> -	if (unlikely(ref < 0))
> -		return -ENOSPC;
> -
> -	rc = gnttab_grant_foreign_access_trans_ref(ref, domid, flags,
> -						   trans_domid, trans_gref);
> -	if (rc < 0) {
> -		put_free_entry(ref);
> -		return rc;
> -	}
> -
> -	return ref;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_trans);
> -
> -bool gnttab_trans_grants_available(void)
> -{
> -	return gnttab_interface->update_trans_entry != NULL;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_trans_grants_available);
> -
>  static int gnttab_query_foreign_access_v1(grant_ref_t ref)
>  {
>  	return gnttab_shared.v1[ref].flags & (GTF_reading|GTF_writing);
>  }
>  
> -static int gnttab_query_foreign_access_v2(grant_ref_t ref)
> -{
> -	return grstatus[ref] & (GTF_reading|GTF_writing);
> -}
> -
>  int gnttab_query_foreign_access(grant_ref_t ref)
>  {
>  	return gnttab_interface->query_foreign_access(ref);
> @@ -433,29 +276,6 @@ static int gnttab_end_foreign_access_ref_v1(grant_ref_t ref, int readonly)
>  	return 1;
>  }
>  
> -static int gnttab_end_foreign_access_ref_v2(grant_ref_t ref, int readonly)
> -{
> -	gnttab_shared.v2[ref].hdr.flags = 0;
> -	mb();
> -	if (grstatus[ref] & (GTF_reading|GTF_writing)) {
> -		return 0;
> -	} else {
> -		/* The read of grstatus needs to have acquire
> -		semantics.  On x86, reads already have
> -		that, and we just need to protect against
> -		compiler reorderings.  On other
> -		architectures we may need a full
> -		barrier. */
> -#ifdef CONFIG_X86
> -		barrier();
> -#else
> -		mb();
> -#endif
> -	}
> -
> -	return 1;
> -}
> -
>  static inline int _gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
>  {
>  	return gnttab_interface->end_foreign_access_ref(ref, readonly);
> @@ -616,37 +436,6 @@ static unsigned long gnttab_end_foreign_transfer_ref_v1(grant_ref_t ref)
>  	return frame;
>  }
>  
> -static unsigned long gnttab_end_foreign_transfer_ref_v2(grant_ref_t ref)
> -{
> -	unsigned long frame;
> -	u16           flags;
> -	u16          *pflags;
> -
> -	pflags = &gnttab_shared.v2[ref].hdr.flags;
> -
> -	/*
> -	 * If a transfer is not even yet started, try to reclaim the grant
> -	 * reference and return failure (== 0).
> -	 */
> -	while (!((flags = *pflags) & GTF_transfer_committed)) {
> -		if (sync_cmpxchg(pflags, flags, 0) == flags)
> -			return 0;
> -		cpu_relax();
> -	}
> -
> -	/* If a transfer is in progress then wait until it is completed. */
> -	while (!(flags & GTF_transfer_completed)) {
> -		flags = *pflags;
> -		cpu_relax();
> -	}
> -
> -	rmb();  /* Read the frame number /after/ reading completion status. */
> -	frame = gnttab_shared.v2[ref].full_page.frame;
> -	BUG_ON(frame == 0);
> -
> -	return frame;
> -}
> -
>  unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
>  {
>  	return gnttab_interface->end_foreign_transfer_ref(ref);
> @@ -962,12 +751,6 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>  }
>  EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
>  
> -static unsigned nr_status_frames(unsigned nr_grant_frames)
> -{
> -	BUG_ON(grefs_per_grant_frame == 0);
> -	return (nr_grant_frames * grefs_per_grant_frame + SPP - 1) / SPP;
> -}
> -
>  static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
>  {
>  	int rc;
> @@ -985,55 +768,6 @@ static void gnttab_unmap_frames_v1(void)
>  	arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
>  }
>  
> -static int gnttab_map_frames_v2(xen_pfn_t *frames, unsigned int nr_gframes)
> -{
> -	uint64_t *sframes;
> -	unsigned int nr_sframes;
> -	struct gnttab_get_status_frames getframes;
> -	int rc;
> -
> -	nr_sframes = nr_status_frames(nr_gframes);
> -
> -	/* No need for kzalloc as it is initialized in following hypercall
> -	 * GNTTABOP_get_status_frames.
> -	 */
> -	sframes = kmalloc(nr_sframes  * sizeof(uint64_t), GFP_ATOMIC);
> -	if (!sframes)
> -		return -ENOMEM;
> -
> -	getframes.dom        = DOMID_SELF;
> -	getframes.nr_frames  = nr_sframes;
> -	set_xen_guest_handle(getframes.frame_list, sframes);
> -
> -	rc = HYPERVISOR_grant_table_op(GNTTABOP_get_status_frames,
> -				       &getframes, 1);
> -	if (rc == -ENOSYS) {
> -		kfree(sframes);
> -		return -ENOSYS;
> -	}
> -
> -	BUG_ON(rc || getframes.status);
> -
> -	rc = arch_gnttab_map_status(sframes, nr_sframes,
> -				    nr_status_frames(gnttab_max_grant_frames()),
> -				    &grstatus);
> -	BUG_ON(rc);
> -	kfree(sframes);
> -
> -	rc = arch_gnttab_map_shared(frames, nr_gframes,
> -				    gnttab_max_grant_frames(),
> -				    &gnttab_shared.addr);
> -	BUG_ON(rc);
> -
> -	return 0;
> -}
> -
> -static void gnttab_unmap_frames_v2(void)
> -{
> -	arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
> -	arch_gnttab_unmap(grstatus, nr_status_frames(nr_grant_frames));
> -}
> -
>  static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
>  {
>  	struct gnttab_setup_table setup;
> @@ -1101,43 +835,13 @@ static struct gnttab_ops gnttab_v1_ops = {
>  	.query_foreign_access		= gnttab_query_foreign_access_v1,
>  };
>  
> -static struct gnttab_ops gnttab_v2_ops = {
> -	.map_frames			= gnttab_map_frames_v2,
> -	.unmap_frames			= gnttab_unmap_frames_v2,
> -	.update_entry			= gnttab_update_entry_v2,
> -	.end_foreign_access_ref		= gnttab_end_foreign_access_ref_v2,
> -	.end_foreign_transfer_ref	= gnttab_end_foreign_transfer_ref_v2,
> -	.query_foreign_access		= gnttab_query_foreign_access_v2,
> -	.update_subpage_entry		= gnttab_update_subpage_entry_v2,
> -	.update_trans_entry		= gnttab_update_trans_entry_v2,
> -};
> -
>  static void gnttab_request_version(void)
>  {
> -	int rc;
> -	struct gnttab_set_version gsv;
> +	/* Only version 1 is used, which will always be available. */
> +	grant_table_version = 1;
> +	grefs_per_grant_frame = PAGE_SIZE / sizeof(struct grant_entry_v1);
> +	gnttab_interface = &gnttab_v1_ops;
>  
> -	gsv.version = 1;
> -
> -	rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
> -	if (rc == 0 && gsv.version == 2) {
> -		grant_table_version = 2;
> -		grefs_per_grant_frame = PAGE_SIZE / sizeof(union grant_entry_v2);
> -		gnttab_interface = &gnttab_v2_ops;
> -	} else if (grant_table_version == 2) {
> -		/*
> -		 * If we've already used version 2 features,
> -		 * but then suddenly discover that they're not
> -		 * available (e.g. migrating to an older
> -		 * version of Xen), almost unbounded badness
> -		 * can happen.
> -		 */
> -		panic("we need grant tables version 2, but only version 1 is available");
> -	} else {
> -		grant_table_version = 1;
> -		grefs_per_grant_frame = PAGE_SIZE / sizeof(struct grant_entry_v1);
> -		gnttab_interface = &gnttab_v1_ops;
> -	}
>  	pr_info("Grant tables using version %d layout\n", grant_table_version);
>  }
>  
> @@ -1225,8 +929,7 @@ int gnttab_init(void)
>  		}
>  	}
>  
> -	ret = arch_gnttab_init(max_nr_grant_frames,
> -			       nr_status_frames(max_nr_grant_frames));
> +	ret = arch_gnttab_init(max_nr_grant_frames);
>  	if (ret < 0)
>  		goto ini_nomem;
>  
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 5c1aba1..3387465 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -64,24 +64,6 @@ int gnttab_resume(void);
>  
>  int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
>  				int readonly);
> -int gnttab_grant_foreign_access_subpage(domid_t domid, unsigned long frame,
> -					int flags, unsigned page_off,
> -					unsigned length);
> -int gnttab_grant_foreign_access_trans(domid_t domid, int flags,
> -				      domid_t trans_domid,
> -				      grant_ref_t trans_gref);
> -
> -/*
> - * Are sub-page grants available on this version of Xen?  Returns true if they
> - * are, and false if they're not.
> - */
> -bool gnttab_subpage_grants_available(void);
> -
> -/*
> - * Are transitive grants available on this version of Xen?  Returns true if they
> - * are, and false if they're not.
> - */
> -bool gnttab_trans_grants_available(void);
>  
>  /*
>   * End access through the given grant reference, iff the grant entry is no
> @@ -128,13 +110,6 @@ void gnttab_cancel_free_callback(struct gnttab_free_callback *callback);
>  
>  void gnttab_grant_foreign_access_ref(grant_ref_t ref, domid_t domid,
>  				     unsigned long frame, int readonly);
> -int gnttab_grant_foreign_access_subpage_ref(grant_ref_t ref, domid_t domid,
> -					    unsigned long frame, int flags,
> -					    unsigned page_off,
> -					    unsigned length);
> -int gnttab_grant_foreign_access_trans_ref(grant_ref_t ref, domid_t domid,
> -					  int flags, domid_t trans_domid,
> -					  grant_ref_t trans_gref);
>  
>  void gnttab_grant_foreign_transfer_ref(grant_ref_t, domid_t domid,
>  				       unsigned long pfn);
> @@ -170,13 +145,10 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap, phys_addr_t addr,
>  	unmap->dev_bus_addr = 0;
>  }
>  
> -int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status);
> +int arch_gnttab_init(unsigned long nr_shared);
>  int arch_gnttab_map_shared(xen_pfn_t *frames, unsigned long nr_gframes,
>  			   unsigned long max_nr_gframes,
>  			   void **__shared);
> -int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> -			   unsigned long max_nr_gframes,
> -			   grant_status_t **__shared);
>  void arch_gnttab_unmap(void *shared, unsigned long nr_gframes);
>  
>  struct grant_frames {
> -- 
> 1.7.10.4
> 

Re: [PATCH 2/2] xen/grant-table: remove support for V2 tables

[David Vrabel]

On 08/07/14 18:10, Konrad Rzeszutek Wilk wrote:
> On Wed, Jul 02, 2014 at 11:25:29AM +0100, David Vrabel wrote:
>> Since 11c7ff17c9b6dbf3a4e4f36be30ad531a6cf0ec9 (xen/grant-table: Force
>> to use v1 of grants.) the code for V2 grant tables is not used.
> 
> I get compile warnings with this patch:
> WARNING: vmlinux.o(.text+0x40268a): Section mismatch in reference from the function gnttab_init() to the function .init.text:arch_gnttab
> _init()
> The function gnttab_init() references
> the function __init arch_gnttab_init().
> This is often because gnttab_init lacks a __init
> annotation or the annotation of arch_gnttab_init is wrong.

Thanks.  I'd missed this warning (despite having the appropriate option
turned on).  But it's patch #1 that broke this.

David

Re: [PATCH 2/2] xen/grant-table: remove support for V2 tables

[Boris Ostrovsky]

On 07/02/2014 06:25 AM, David Vrabel wrote:
> Since 11c7ff17c9b6dbf3a4e4f36be30ad531a6cf0ec9 (xen/grant-table: Force
> to use v1 of grants.) the code for V2 grant tables is not used.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---


>   }
>   
>   #ifdef CONFIG_XEN_PVH
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index eeba754..c254ae0 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -69,7 +69,6 @@ struct grant_frames xen_auto_xlat_grant_frames;
>   
>   static union {
>   	struct grant_entry_v1 *v1;
> -	union grant_entry_v2 *v2;
>   	void *addr;
>   } gnttab_shared;

Since we are getting rid of v2, do we need to keep version-specific data 
structures?

-boris

Re: [PATCH 2/2] xen/grant-table: remove support for V2 tables

[Konrad Rzeszutek Wilk]

On Wed, Jul 02, 2014 at 11:25:29AM +0100, David Vrabel wrote:
> Since 11c7ff17c9b6dbf3a4e4f36be30ad531a6cf0ec9 (xen/grant-table: Force
> to use v1 of grants.) the code for V2 grant tables is not used.

.. and if we ever need it we can resurrect it.

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  arch/arm/xen/grant-table.c |    9 +-
>  arch/x86/xen/grant-table.c |   60 +--------
>  drivers/xen/grant-table.c  |  309 +-------------------------------------------
>  include/xen/grant_table.h  |   30 +----
>  4 files changed, 13 insertions(+), 395 deletions(-)
> 
> diff --git a/arch/arm/xen/grant-table.c b/arch/arm/xen/grant-table.c
> index 91cf08b..e437918 100644
> --- a/arch/arm/xen/grant-table.c
> +++ b/arch/arm/xen/grant-table.c
> @@ -45,14 +45,7 @@ void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
>  	return;
>  }
>  
> -int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> -			   unsigned long max_nr_gframes,
> -			   grant_status_t **__shared)
> -{
> -	return -ENOSYS;
> -}
> -
> -int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
> +int arch_gnttab_init(unsigned long nr_shared)
>  {
>  	return 0;
>  }
> diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
> index 27301df..c1ef6b2 100644
> --- a/arch/x86/xen/grant-table.c
> +++ b/arch/x86/xen/grant-table.c
> @@ -48,7 +48,7 @@
>  static struct gnttab_vm_area {
>  	struct vm_struct *area;
>  	pte_t **ptes;
> -} gnttab_shared_vm_area, gnttab_status_vm_area;
> +} gnttab_shared_vm_area;
>  
>  int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
>  			   unsigned long max_nr_gframes,
> @@ -72,43 +72,16 @@ int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
>  	return 0;
>  }
>  
> -int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> -			   unsigned long max_nr_gframes,
> -			   grant_status_t **__shared)
> -{
> -	grant_status_t *shared = *__shared;
> -	unsigned long addr;
> -	unsigned long i;
> -
> -	if (shared == NULL)
> -		*__shared = shared = gnttab_status_vm_area.area->addr;
> -
> -	addr = (unsigned long)shared;
> -
> -	for (i = 0; i < nr_gframes; i++) {
> -		set_pte_at(&init_mm, addr, gnttab_status_vm_area.ptes[i],
> -			   mfn_pte(frames[i], PAGE_KERNEL));
> -		addr += PAGE_SIZE;
> -	}
> -
> -	return 0;
> -}
> -
>  void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
>  {
> -	pte_t **ptes;
>  	unsigned long addr;
>  	unsigned long i;
>  
> -	if (shared == gnttab_status_vm_area.area->addr)
> -		ptes = gnttab_status_vm_area.ptes;
> -	else
> -		ptes = gnttab_shared_vm_area.ptes;
> -
>  	addr = (unsigned long)shared;
>  
>  	for (i = 0; i < nr_gframes; i++) {
> -		set_pte_at(&init_mm, addr, ptes[i], __pte(0));
> +		set_pte_at(&init_mm, addr, gnttab_shared_vm_area.ptes[i],
> +			   __pte(0));
>  		addr += PAGE_SIZE;
>  	}
>  }
> @@ -129,35 +102,12 @@ static int __init arch_gnttab_valloc(struct gnttab_vm_area *area,
>  	return 0;
>  }
>  
> -static void __init arch_gnttab_vfree(struct gnttab_vm_area *area)
> +int __init arch_gnttab_init(unsigned long nr_shared)
>  {
> -	free_vm_area(area->area);
> -	kfree(area->ptes);
> -}
> -
> -int __init arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status)
> -{
> -	int ret;
> -
>  	if (!xen_pv_domain())
>  		return 0;
>  
> -	ret = arch_gnttab_valloc(&gnttab_shared_vm_area, nr_shared);
> -	if (ret < 0)
> -		return ret;
> -
> -	/*
> -	 * Always allocate the space for the status frames in case
> -	 * we're migrated to a host with V2 support.
> -	 */
> -	ret = arch_gnttab_valloc(&gnttab_status_vm_area, nr_status);
> -	if (ret < 0)
> -		goto err;
> -
> -	return 0;
> -  err:
> -	arch_gnttab_vfree(&gnttab_shared_vm_area);
> -	return -ENOMEM;
> +	return arch_gnttab_valloc(&gnttab_shared_vm_area, nr_shared);
>  }
>  
>  #ifdef CONFIG_XEN_PVH
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index eeba754..c254ae0 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -69,7 +69,6 @@ struct grant_frames xen_auto_xlat_grant_frames;
>  
>  static union {
>  	struct grant_entry_v1 *v1;
> -	union grant_entry_v2 *v2;
>  	void *addr;
>  } gnttab_shared;
>  
> @@ -120,36 +119,10 @@ struct gnttab_ops {
>  	 * by bit operations.
>  	 */
>  	int (*query_foreign_access)(grant_ref_t ref);
> -	/*
> -	 * Grant a domain to access a range of bytes within the page referred by
> -	 * an available grant entry. Ref parameter is reference of a grant entry
> -	 * which will be sub-page accessed, domid is id of grantee domain, frame
> -	 * is frame address of subpage grant, flags is grant type and flag
> -	 * information, page_off is offset of the range of bytes, and length is
> -	 * length of bytes to be accessed.
> -	 */
> -	void (*update_subpage_entry)(grant_ref_t ref, domid_t domid,
> -				     unsigned long frame, int flags,
> -				     unsigned page_off, unsigned length);
> -	/*
> -	 * Redirect an available grant entry on domain A to another grant
> -	 * reference of domain B, then allow domain C to use grant reference
> -	 * of domain B transitively. Ref parameter is an available grant entry
> -	 * reference on domain A, domid is id of domain C which accesses grant
> -	 * entry transitively, flags is grant type and flag information,
> -	 * trans_domid is id of domain B whose grant entry is finally accessed
> -	 * transitively, trans_gref is grant entry transitive reference of
> -	 * domain B.
> -	 */
> -	void (*update_trans_entry)(grant_ref_t ref, domid_t domid, int flags,
> -				   domid_t trans_domid, grant_ref_t trans_gref);
>  };
>  
>  static struct gnttab_ops *gnttab_interface;
>  
> -/*This reflects status of grant entries, so act as a global value*/
> -static grant_status_t *grstatus;
> -
>  static int grant_table_version;
>  static int grefs_per_grant_frame;
>  
> @@ -231,7 +204,7 @@ static void put_free_entry(grant_ref_t ref)
>  }
>  
>  /*
> - * Following applies to gnttab_update_entry_v1 and gnttab_update_entry_v2.
> + * Following applies to gnttab_update_entry_v1.
>   * Introducing a valid entry into the grant table:
>   *  1. Write ent->domid.
>   *  2. Write ent->frame:
> @@ -250,15 +223,6 @@ static void gnttab_update_entry_v1(grant_ref_t ref, domid_t domid,
>  	gnttab_shared.v1[ref].flags = flags;
>  }
>  
> -static void gnttab_update_entry_v2(grant_ref_t ref, domid_t domid,
> -				   unsigned long frame, unsigned flags)
> -{
> -	gnttab_shared.v2[ref].hdr.domid = domid;
> -	gnttab_shared.v2[ref].full_page.frame = frame;
> -	wmb();
> -	gnttab_shared.v2[ref].hdr.flags = GTF_permit_access | flags;
> -}
> -
>  /*
>   * Public grant-issuing interface functions
>   */
> @@ -285,132 +249,11 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
>  }
>  EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);
>  
> -static void gnttab_update_subpage_entry_v2(grant_ref_t ref, domid_t domid,
> -					   unsigned long frame, int flags,
> -					   unsigned page_off, unsigned length)
> -{
> -	gnttab_shared.v2[ref].sub_page.frame = frame;
> -	gnttab_shared.v2[ref].sub_page.page_off = page_off;
> -	gnttab_shared.v2[ref].sub_page.length = length;
> -	gnttab_shared.v2[ref].hdr.domid = domid;
> -	wmb();
> -	gnttab_shared.v2[ref].hdr.flags =
> -				GTF_permit_access | GTF_sub_page | flags;
> -}
> -
> -int gnttab_grant_foreign_access_subpage_ref(grant_ref_t ref, domid_t domid,
> -					    unsigned long frame, int flags,
> -					    unsigned page_off,
> -					    unsigned length)
> -{
> -	if (flags & (GTF_accept_transfer | GTF_reading |
> -		     GTF_writing | GTF_transitive))
> -		return -EPERM;
> -
> -	if (gnttab_interface->update_subpage_entry == NULL)
> -		return -ENOSYS;
> -
> -	gnttab_interface->update_subpage_entry(ref, domid, frame, flags,
> -					       page_off, length);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_subpage_ref);
> -
> -int gnttab_grant_foreign_access_subpage(domid_t domid, unsigned long frame,
> -					int flags, unsigned page_off,
> -					unsigned length)
> -{
> -	int ref, rc;
> -
> -	ref = get_free_entries(1);
> -	if (unlikely(ref < 0))
> -		return -ENOSPC;
> -
> -	rc = gnttab_grant_foreign_access_subpage_ref(ref, domid, frame, flags,
> -						     page_off, length);
> -	if (rc < 0) {
> -		put_free_entry(ref);
> -		return rc;
> -	}
> -
> -	return ref;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_subpage);
> -
> -bool gnttab_subpage_grants_available(void)
> -{
> -	return gnttab_interface->update_subpage_entry != NULL;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_subpage_grants_available);
> -
> -static void gnttab_update_trans_entry_v2(grant_ref_t ref, domid_t domid,
> -					 int flags, domid_t trans_domid,
> -					 grant_ref_t trans_gref)
> -{
> -	gnttab_shared.v2[ref].transitive.trans_domid = trans_domid;
> -	gnttab_shared.v2[ref].transitive.gref = trans_gref;
> -	gnttab_shared.v2[ref].hdr.domid = domid;
> -	wmb();
> -	gnttab_shared.v2[ref].hdr.flags =
> -				GTF_permit_access | GTF_transitive | flags;
> -}
> -
> -int gnttab_grant_foreign_access_trans_ref(grant_ref_t ref, domid_t domid,
> -					  int flags, domid_t trans_domid,
> -					  grant_ref_t trans_gref)
> -{
> -	if (flags & (GTF_accept_transfer | GTF_reading |
> -		     GTF_writing | GTF_sub_page))
> -		return -EPERM;
> -
> -	if (gnttab_interface->update_trans_entry == NULL)
> -		return -ENOSYS;
> -
> -	gnttab_interface->update_trans_entry(ref, domid, flags, trans_domid,
> -					     trans_gref);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_trans_ref);
> -
> -int gnttab_grant_foreign_access_trans(domid_t domid, int flags,
> -				      domid_t trans_domid,
> -				      grant_ref_t trans_gref)
> -{
> -	int ref, rc;
> -
> -	ref = get_free_entries(1);
> -	if (unlikely(ref < 0))
> -		return -ENOSPC;
> -
> -	rc = gnttab_grant_foreign_access_trans_ref(ref, domid, flags,
> -						   trans_domid, trans_gref);
> -	if (rc < 0) {
> -		put_free_entry(ref);
> -		return rc;
> -	}
> -
> -	return ref;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_trans);
> -
> -bool gnttab_trans_grants_available(void)
> -{
> -	return gnttab_interface->update_trans_entry != NULL;
> -}
> -EXPORT_SYMBOL_GPL(gnttab_trans_grants_available);
> -
>  static int gnttab_query_foreign_access_v1(grant_ref_t ref)
>  {
>  	return gnttab_shared.v1[ref].flags & (GTF_reading|GTF_writing);
>  }
>  
> -static int gnttab_query_foreign_access_v2(grant_ref_t ref)
> -{
> -	return grstatus[ref] & (GTF_reading|GTF_writing);
> -}
> -
>  int gnttab_query_foreign_access(grant_ref_t ref)
>  {
>  	return gnttab_interface->query_foreign_access(ref);
> @@ -433,29 +276,6 @@ static int gnttab_end_foreign_access_ref_v1(grant_ref_t ref, int readonly)
>  	return 1;
>  }
>  
> -static int gnttab_end_foreign_access_ref_v2(grant_ref_t ref, int readonly)
> -{
> -	gnttab_shared.v2[ref].hdr.flags = 0;
> -	mb();
> -	if (grstatus[ref] & (GTF_reading|GTF_writing)) {
> -		return 0;
> -	} else {
> -		/* The read of grstatus needs to have acquire
> -		semantics.  On x86, reads already have
> -		that, and we just need to protect against
> -		compiler reorderings.  On other
> -		architectures we may need a full
> -		barrier. */
> -#ifdef CONFIG_X86
> -		barrier();
> -#else
> -		mb();
> -#endif
> -	}
> -
> -	return 1;
> -}
> -
>  static inline int _gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
>  {
>  	return gnttab_interface->end_foreign_access_ref(ref, readonly);
> @@ -616,37 +436,6 @@ static unsigned long gnttab_end_foreign_transfer_ref_v1(grant_ref_t ref)
>  	return frame;
>  }
>  
> -static unsigned long gnttab_end_foreign_transfer_ref_v2(grant_ref_t ref)
> -{
> -	unsigned long frame;
> -	u16           flags;
> -	u16          *pflags;
> -
> -	pflags = &gnttab_shared.v2[ref].hdr.flags;
> -
> -	/*
> -	 * If a transfer is not even yet started, try to reclaim the grant
> -	 * reference and return failure (== 0).
> -	 */
> -	while (!((flags = *pflags) & GTF_transfer_committed)) {
> -		if (sync_cmpxchg(pflags, flags, 0) == flags)
> -			return 0;
> -		cpu_relax();
> -	}
> -
> -	/* If a transfer is in progress then wait until it is completed. */
> -	while (!(flags & GTF_transfer_completed)) {
> -		flags = *pflags;
> -		cpu_relax();
> -	}
> -
> -	rmb();  /* Read the frame number /after/ reading completion status. */
> -	frame = gnttab_shared.v2[ref].full_page.frame;
> -	BUG_ON(frame == 0);
> -
> -	return frame;
> -}
> -
>  unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
>  {
>  	return gnttab_interface->end_foreign_transfer_ref(ref);
> @@ -962,12 +751,6 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>  }
>  EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
>  
> -static unsigned nr_status_frames(unsigned nr_grant_frames)
> -{
> -	BUG_ON(grefs_per_grant_frame == 0);
> -	return (nr_grant_frames * grefs_per_grant_frame + SPP - 1) / SPP;
> -}
> -
>  static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
>  {
>  	int rc;
> @@ -985,55 +768,6 @@ static void gnttab_unmap_frames_v1(void)
>  	arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
>  }
>  
> -static int gnttab_map_frames_v2(xen_pfn_t *frames, unsigned int nr_gframes)
> -{
> -	uint64_t *sframes;
> -	unsigned int nr_sframes;
> -	struct gnttab_get_status_frames getframes;
> -	int rc;
> -
> -	nr_sframes = nr_status_frames(nr_gframes);
> -
> -	/* No need for kzalloc as it is initialized in following hypercall
> -	 * GNTTABOP_get_status_frames.
> -	 */
> -	sframes = kmalloc(nr_sframes  * sizeof(uint64_t), GFP_ATOMIC);
> -	if (!sframes)
> -		return -ENOMEM;
> -
> -	getframes.dom        = DOMID_SELF;
> -	getframes.nr_frames  = nr_sframes;
> -	set_xen_guest_handle(getframes.frame_list, sframes);
> -
> -	rc = HYPERVISOR_grant_table_op(GNTTABOP_get_status_frames,
> -				       &getframes, 1);
> -	if (rc == -ENOSYS) {
> -		kfree(sframes);
> -		return -ENOSYS;
> -	}
> -
> -	BUG_ON(rc || getframes.status);
> -
> -	rc = arch_gnttab_map_status(sframes, nr_sframes,
> -				    nr_status_frames(gnttab_max_grant_frames()),
> -				    &grstatus);
> -	BUG_ON(rc);
> -	kfree(sframes);
> -
> -	rc = arch_gnttab_map_shared(frames, nr_gframes,
> -				    gnttab_max_grant_frames(),
> -				    &gnttab_shared.addr);
> -	BUG_ON(rc);
> -
> -	return 0;
> -}
> -
> -static void gnttab_unmap_frames_v2(void)
> -{
> -	arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
> -	arch_gnttab_unmap(grstatus, nr_status_frames(nr_grant_frames));
> -}
> -
>  static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
>  {
>  	struct gnttab_setup_table setup;
> @@ -1101,43 +835,13 @@ static struct gnttab_ops gnttab_v1_ops = {
>  	.query_foreign_access		= gnttab_query_foreign_access_v1,
>  };
>  
> -static struct gnttab_ops gnttab_v2_ops = {
> -	.map_frames			= gnttab_map_frames_v2,
> -	.unmap_frames			= gnttab_unmap_frames_v2,
> -	.update_entry			= gnttab_update_entry_v2,
> -	.end_foreign_access_ref		= gnttab_end_foreign_access_ref_v2,
> -	.end_foreign_transfer_ref	= gnttab_end_foreign_transfer_ref_v2,
> -	.query_foreign_access		= gnttab_query_foreign_access_v2,
> -	.update_subpage_entry		= gnttab_update_subpage_entry_v2,
> -	.update_trans_entry		= gnttab_update_trans_entry_v2,
> -};
> -
>  static void gnttab_request_version(void)
>  {
> -	int rc;
> -	struct gnttab_set_version gsv;
> +	/* Only version 1 is used, which will always be available. */
> +	grant_table_version = 1;
> +	grefs_per_grant_frame = PAGE_SIZE / sizeof(struct grant_entry_v1);
> +	gnttab_interface = &gnttab_v1_ops;
>  
> -	gsv.version = 1;
> -
> -	rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1);
> -	if (rc == 0 && gsv.version == 2) {
> -		grant_table_version = 2;
> -		grefs_per_grant_frame = PAGE_SIZE / sizeof(union grant_entry_v2);
> -		gnttab_interface = &gnttab_v2_ops;
> -	} else if (grant_table_version == 2) {
> -		/*
> -		 * If we've already used version 2 features,
> -		 * but then suddenly discover that they're not
> -		 * available (e.g. migrating to an older
> -		 * version of Xen), almost unbounded badness
> -		 * can happen.
> -		 */
> -		panic("we need grant tables version 2, but only version 1 is available");
> -	} else {
> -		grant_table_version = 1;
> -		grefs_per_grant_frame = PAGE_SIZE / sizeof(struct grant_entry_v1);
> -		gnttab_interface = &gnttab_v1_ops;
> -	}
>  	pr_info("Grant tables using version %d layout\n", grant_table_version);
>  }
>  
> @@ -1225,8 +929,7 @@ int gnttab_init(void)
>  		}
>  	}
>  
> -	ret = arch_gnttab_init(max_nr_grant_frames,
> -			       nr_status_frames(max_nr_grant_frames));
> +	ret = arch_gnttab_init(max_nr_grant_frames);
>  	if (ret < 0)
>  		goto ini_nomem;
>  
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 5c1aba1..3387465 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -64,24 +64,6 @@ int gnttab_resume(void);
>  
>  int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
>  				int readonly);
> -int gnttab_grant_foreign_access_subpage(domid_t domid, unsigned long frame,
> -					int flags, unsigned page_off,
> -					unsigned length);
> -int gnttab_grant_foreign_access_trans(domid_t domid, int flags,
> -				      domid_t trans_domid,
> -				      grant_ref_t trans_gref);
> -
> -/*
> - * Are sub-page grants available on this version of Xen?  Returns true if they
> - * are, and false if they're not.
> - */
> -bool gnttab_subpage_grants_available(void);
> -
> -/*
> - * Are transitive grants available on this version of Xen?  Returns true if they
> - * are, and false if they're not.
> - */
> -bool gnttab_trans_grants_available(void);
>  
>  /*
>   * End access through the given grant reference, iff the grant entry is no
> @@ -128,13 +110,6 @@ void gnttab_cancel_free_callback(struct gnttab_free_callback *callback);
>  
>  void gnttab_grant_foreign_access_ref(grant_ref_t ref, domid_t domid,
>  				     unsigned long frame, int readonly);
> -int gnttab_grant_foreign_access_subpage_ref(grant_ref_t ref, domid_t domid,
> -					    unsigned long frame, int flags,
> -					    unsigned page_off,
> -					    unsigned length);
> -int gnttab_grant_foreign_access_trans_ref(grant_ref_t ref, domid_t domid,
> -					  int flags, domid_t trans_domid,
> -					  grant_ref_t trans_gref);
>  
>  void gnttab_grant_foreign_transfer_ref(grant_ref_t, domid_t domid,
>  				       unsigned long pfn);
> @@ -170,13 +145,10 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap, phys_addr_t addr,
>  	unmap->dev_bus_addr = 0;
>  }
>  
> -int arch_gnttab_init(unsigned long nr_shared, unsigned long nr_status);
> +int arch_gnttab_init(unsigned long nr_shared);
>  int arch_gnttab_map_shared(xen_pfn_t *frames, unsigned long nr_gframes,
>  			   unsigned long max_nr_gframes,
>  			   void **__shared);
> -int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> -			   unsigned long max_nr_gframes,
> -			   grant_status_t **__shared);
>  void arch_gnttab_unmap(void *shared, unsigned long nr_gframes);
>  
>  struct grant_frames {
> -- 
> 1.7.10.4
> 

Re: [PATCHv1 0/2] xen/grant-table: fix PV guests and remove V2 support

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of David Vrabel
> Sent: 02 July 2014 11:25
> To: xen-devel@lists.xenproject.org
> Cc: Boris Ostrovsky; David Vrabel
> Subject: [Xen-devel] [PATCHv1 0/2] xen/grant-table: fix PV guests and
> remove V2 support
> 
> The first patch fixes a logn standing issue where the map/unmap of
> grant table frames by PV guests was unsafe and could BUG() or have
> other bad behaviour.  This was caused by calling function that were
> not permitted from atomic contexts.
> 
> The second patch removes all V2 table code since it is unused.
> 

How confident are you of that assertion?

  Paul

> David
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCHv1 0/2] xen/grant-table: fix PV guests and remove V2 support

[David Vrabel]

On 04/07/14 11:59, Paul Durrant wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of David Vrabel
>> Sent: 02 July 2014 11:25
>> To: xen-devel@lists.xenproject.org
>> Cc: Boris Ostrovsky; David Vrabel
>> Subject: [Xen-devel] [PATCHv1 0/2] xen/grant-table: fix PV guests and
>> remove V2 support
>>
>> The first patch fixes a logn standing issue where the map/unmap of
>> grant table frames by PV guests was unsafe and could BUG() or have
>> other bad behaviour.  This was caused by calling function that were
>> not permitted from atomic contexts.
>>
>> The second patch removes all V2 table code since it is unused.
>>
> 
> How confident are you of that assertion?

This is removing the /Linux/ code which has been using V1 only for a
while now.

David

Re: [PATCHv1 0/2] xen/grant-table: fix PV guests and remove V2 support

[Paul Durrant]

> -----Original Message-----
> From: David Vrabel
> Sent: 04 July 2014 12:55
> To: Paul Durrant; xen-devel@lists.xenproject.org
> Cc: Boris Ostrovsky
> Subject: Re: [Xen-devel] [PATCHv1 0/2] xen/grant-table: fix PV guests and
> remove V2 support
> 
> On 04/07/14 11:59, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> >> bounces@lists.xen.org] On Behalf Of David Vrabel
> >> Sent: 02 July 2014 11:25
> >> To: xen-devel@lists.xenproject.org
> >> Cc: Boris Ostrovsky; David Vrabel
> >> Subject: [Xen-devel] [PATCHv1 0/2] xen/grant-table: fix PV guests and
> >> remove V2 support
> >>
> >> The first patch fixes a logn standing issue where the map/unmap of
> >> grant table frames by PV guests was unsafe and could BUG() or have
> >> other bad behaviour.  This was caused by calling function that were
> >> not permitted from atomic contexts.
> >>
> >> The second patch removes all V2 table code since it is unused.
> >>
> >
> > How confident are you of that assertion?
> 
> This is removing the /Linux/ code which has been using V1 only for a
> while now.
> 

If you believe that the API is likely to wither then that seems reasonable. I would have thought the code was unlikely to see much change though and it seems a shame to remove an interface to something that it still there in Xen... I guess *someone* may want to use it in future; particularly the copy-only grants could be useful for something like IDC (if the backend was unprivileged).

> David

Re: [PATCHv1 0/2] xen/grant-table: fix PV guests and remove V2 support

[David Vrabel]

On 04/07/14 13:01, Paul Durrant wrote:
>> -----Original Message-----
>> From: David Vrabel
>> Sent: 04 July 2014 12:55
>> To: Paul Durrant; xen-devel@lists.xenproject.org
>> Cc: Boris Ostrovsky
>> Subject: Re: [Xen-devel] [PATCHv1 0/2] xen/grant-table: fix PV guests and
>> remove V2 support
>>
>> On 04/07/14 11:59, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>>>> bounces@lists.xen.org] On Behalf Of David Vrabel
>>>> Sent: 02 July 2014 11:25
>>>> To: xen-devel@lists.xenproject.org
>>>> Cc: Boris Ostrovsky; David Vrabel
>>>> Subject: [Xen-devel] [PATCHv1 0/2] xen/grant-table: fix PV guests and
>>>> remove V2 support
>>>>
>>>> The first patch fixes a logn standing issue where the map/unmap of
>>>> grant table frames by PV guests was unsafe and could BUG() or have
>>>> other bad behaviour.  This was caused by calling function that were
>>>> not permitted from atomic contexts.
>>>>
>>>> The second patch removes all V2 table code since it is unused.
>>>>
>>>
>>> How confident are you of that assertion?
>>
>> This is removing the /Linux/ code which has been using V1 only for a
>> while now.
>>
> 
> If you believe that the API is likely to wither then that seems
> reasonable. I would have thought the code was unlikely to see much
> change though and it seems a shame to remove an interface to something
> that it still there in Xen... I guess *someone* may want to use it in
> future; particularly the copy-only grants could be useful for something
> like IDC (if the backend was unprivileged).

It's already withering. For example PVH doesn't support V2 as it doesn't
handle mapping/populating the status frames.  And fixing a long standing
bug (see patch 1/2) was made more complicated by having to fix up the
(unused) code for dealing with the status frames.

And as an aside, I'm not convinced that copy-only grants are what's
needed for IDC with untrusted backend.  I think revocable grants are
required for efficient usage of shared rings.

David

Re: [PATCHv1 0/2] xen/grant-table: fix PV guests and remove V2 support

[Paul Durrant]

> -----Original Message-----
> From: David Vrabel
> Sent: 04 July 2014 13:21
> To: Paul Durrant; David Vrabel; xen-devel@lists.xenproject.org
> Cc: Boris Ostrovsky
> Subject: Re: [Xen-devel] [PATCHv1 0/2] xen/grant-table: fix PV guests and
> remove V2 support
> 
> On 04/07/14 13:01, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: David Vrabel
> >> Sent: 04 July 2014 12:55
> >> To: Paul Durrant; xen-devel@lists.xenproject.org
> >> Cc: Boris Ostrovsky
> >> Subject: Re: [Xen-devel] [PATCHv1 0/2] xen/grant-table: fix PV guests and
> >> remove V2 support
> >>
> >> On 04/07/14 11:59, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> >>>> bounces@lists.xen.org] On Behalf Of David Vrabel
> >>>> Sent: 02 July 2014 11:25
> >>>> To: xen-devel@lists.xenproject.org
> >>>> Cc: Boris Ostrovsky; David Vrabel
> >>>> Subject: [Xen-devel] [PATCHv1 0/2] xen/grant-table: fix PV guests and
> >>>> remove V2 support
> >>>>
> >>>> The first patch fixes a logn standing issue where the map/unmap of
> >>>> grant table frames by PV guests was unsafe and could BUG() or have
> >>>> other bad behaviour.  This was caused by calling function that were
> >>>> not permitted from atomic contexts.
> >>>>
> >>>> The second patch removes all V2 table code since it is unused.
> >>>>
> >>>
> >>> How confident are you of that assertion?
> >>
> >> This is removing the /Linux/ code which has been using V1 only for a
> >> while now.
> >>
> >
> > If you believe that the API is likely to wither then that seems
> > reasonable. I would have thought the code was unlikely to see much
> > change though and it seems a shame to remove an interface to something
> > that it still there in Xen... I guess *someone* may want to use it in
> > future; particularly the copy-only grants could be useful for something
> > like IDC (if the backend was unprivileged).
> 
> It's already withering. For example PVH doesn't support V2 as it doesn't
> handle mapping/populating the status frames.  And fixing a long standing
> bug (see patch 1/2) was made more complicated by having to fix up the
> (unused) code for dealing with the status frames.
> 
> And as an aside, I'm not convinced that copy-only grants are what's
> needed for IDC with untrusted backend.  I think revocable grants are
> required for efficient usage of shared rings.
> 

Ok. Fair enough,

  Paul

> David