[PATCHv6 0/3] xen: improve migration performance

[PATCHv6 0/3] xen: improve migration performance

[David Vrabel]

This series signficantly improves the performance of migration by
speeding up privcmd's MMAPBATCH_V2 ioctl (for PV toolstack domains).

Changes in v6:
- unify PVH and ARM foreign mapping.
- more fixes and refactoring than I can count.

David

[PATCHv6 1/3] arm: make __get_user() work for 8 byte values

[David Vrabel]

get_user(), __put_user(), and put_user() all worked with 8-byte values
but __get_user() did not.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-arm-kernel at lists.infradead.org
---
 arch/arm/include/asm/uaccess.h |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index ce0786e..d8f535b 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -306,6 +306,7 @@ do {									\
 	case 1:	__get_user_asm_byte(__gu_val, __gu_addr, err);	break;	\
 	case 2:	__get_user_asm_half(__gu_val, __gu_addr, err);	break;	\
 	case 4:	__get_user_asm_word(__gu_val, __gu_addr, err);	break;	\
+	case 8:	__get_user_asm_dword(__gu_val, __gu_addr, err);	break;	\
 	default: (__gu_val) = __get_user_bad();				\
 	}								\
 	(x) = (__typeof__(*(ptr)))__gu_val;				\
@@ -365,6 +366,37 @@ do {									\
 	: "r" (addr), "i" (-EFAULT)				\
 	: "cc")
 
+#ifndef __ARMEB__
+#define	__reg_get0	"%R1"
+#define	__reg_get1	"%Q1"
+#else
+#define	__reg_get0	"%Q1"
+#define	__reg_get1	"%R1"
+#endif
+
+#define __get_user_asm_dword(x, addr, err)			\
+	__asm__ __volatile__(					\
+ ARM(	"1:	" TUSER(ldr) "	" __reg_get1 ", [%2], #4\n"	) \
+ ARM(	"2:	" TUSER(ldr) "	" __reg_get0 ", [%2]\n"	) \
+ THUMB(	"1:	" TUSER(ldr) "	" __reg_get1 ", [%2]\n"	) \
+ THUMB(	"2:	" TUSER(ldr) "	" __reg_get0 ", [%2, #4]\n"	) \
+	"3:\n"							\
+	"	.pushsection .fixup,\"ax\"\n"			\
+	"	.align	2\n"					\
+	"4:	mov	%0, %3\n"				\
+	"	mov	" __reg_get1 ", #0\n"			\
+	"	mov	" __reg_get0 ", #0\n"			\
+	"	b	3b\n"					\
+	"	.popsection\n"					\
+	"	.pushsection __ex_table,\"a\"\n"		\
+	"	.align	3\n"					\
+	"	.long	1b, 4b\n"				\
+	"	.long	2b, 4b\n"				\
+	"	.popsection"					\
+	: "+r" (err), "=&r" (x)					\
+	: "r" (addr), "i" (-EFAULT)				\
+	: "cc")
+
 #define __put_user(x, ptr)						\
 ({									\
 	long __pu_err = 0;						\
-- 
1.7.10.4

[PATCHv6 1/3] arm: make __get_user() work for 8 byte values

[David Vrabel]

get_user(), __put_user(), and put_user() all worked with 8-byte values
but __get_user() did not.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
---
 arch/arm/include/asm/uaccess.h |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index ce0786e..d8f535b 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -306,6 +306,7 @@ do {									\
 	case 1:	__get_user_asm_byte(__gu_val, __gu_addr, err);	break;	\
 	case 2:	__get_user_asm_half(__gu_val, __gu_addr, err);	break;	\
 	case 4:	__get_user_asm_word(__gu_val, __gu_addr, err);	break;	\
+	case 8:	__get_user_asm_dword(__gu_val, __gu_addr, err);	break;	\
 	default: (__gu_val) = __get_user_bad();				\
 	}								\
 	(x) = (__typeof__(*(ptr)))__gu_val;				\
@@ -365,6 +366,37 @@ do {									\
 	: "r" (addr), "i" (-EFAULT)				\
 	: "cc")
 
+#ifndef __ARMEB__
+#define	__reg_get0	"%R1"
+#define	__reg_get1	"%Q1"
+#else
+#define	__reg_get0	"%Q1"
+#define	__reg_get1	"%R1"
+#endif
+
+#define __get_user_asm_dword(x, addr, err)			\
+	__asm__ __volatile__(					\
+ ARM(	"1:	" TUSER(ldr) "	" __reg_get1 ", [%2], #4\n"	) \
+ ARM(	"2:	" TUSER(ldr) "	" __reg_get0 ", [%2]\n"	) \
+ THUMB(	"1:	" TUSER(ldr) "	" __reg_get1 ", [%2]\n"	) \
+ THUMB(	"2:	" TUSER(ldr) "	" __reg_get0 ", [%2, #4]\n"	) \
+	"3:\n"							\
+	"	.pushsection .fixup,\"ax\"\n"			\
+	"	.align	2\n"					\
+	"4:	mov	%0, %3\n"				\
+	"	mov	" __reg_get1 ", #0\n"			\
+	"	mov	" __reg_get0 ", #0\n"			\
+	"	b	3b\n"					\
+	"	.popsection\n"					\
+	"	.pushsection __ex_table,\"a\"\n"		\
+	"	.align	3\n"					\
+	"	.long	1b, 4b\n"				\
+	"	.long	2b, 4b\n"				\
+	"	.popsection"					\
+	: "+r" (err), "=&r" (x)					\
+	: "r" (addr), "i" (-EFAULT)				\
+	: "cc")
+
 #define __put_user(x, ptr)						\
 ({									\
 	long __pu_err = 0;						\
-- 
1.7.10.4

Re: [PATCHv6 1/3] arm: make __get_user() work for 8 byte values

[Russell King - ARM Linux]

On Fri, Mar 06, 2015 at 12:58:33PM +0000, David Vrabel wrote:
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index ce0786e..d8f535b 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -306,6 +306,7 @@ do {									\
>  	case 1:	__get_user_asm_byte(__gu_val, __gu_addr, err);	break;	\
>  	case 2:	__get_user_asm_half(__gu_val, __gu_addr, err);	break;	\
>  	case 4:	__get_user_asm_word(__gu_val, __gu_addr, err);	break;	\
> +	case 8:	__get_user_asm_dword(__gu_val, __gu_addr, err);	break;	\

Obviously not tested.  __gu_val is an unsigned long - it's 32-bit.  This
will truncate the 64-bit read into 32-bits.

It's going to take some work to sort out something that works, and
right now I don't have time for that (catching up post-op, sorry).

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCHv6 1/3] arm: make __get_user() work for 8 byte values

[Russell King - ARM Linux]

On Fri, Mar 06, 2015 at 12:58:33PM +0000, David Vrabel wrote:
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index ce0786e..d8f535b 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -306,6 +306,7 @@ do {									\
>  	case 1:	__get_user_asm_byte(__gu_val, __gu_addr, err);	break;	\
>  	case 2:	__get_user_asm_half(__gu_val, __gu_addr, err);	break;	\
>  	case 4:	__get_user_asm_word(__gu_val, __gu_addr, err);	break;	\
> +	case 8:	__get_user_asm_dword(__gu_val, __gu_addr, err);	break;	\

Obviously not tested.  __gu_val is an unsigned long - it's 32-bit.  This
will truncate the 64-bit read into 32-bits.

It's going to take some work to sort out something that works, and
right now I don't have time for that (catching up post-op, sorry).

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCHv6 2/3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

[David Vrabel]

Auto-translated physmap guests (arm, arm64 and x86 PVHVM/PVH) map and
unmap foreign GFNs using the same method (updating the physmap).
Unify the two arm and x86 implementations into one commont one.

Note that on arm and arm64, the correct error code will be returned
(instead of always -EFAULT) and map/unmap failure warnings are no
longer printed.  These changes are required if the foreign domain is
paging (-ENOENT failures are expected and must be propagated up to the
caller).

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/arm/xen/enlighten.c |   90 ++-----------------------------
 arch/x86/xen/mmu.c       |  110 ++------------------------------------
 drivers/xen/Kconfig      |    6 +++
 drivers/xen/Makefile     |    1 +
 drivers/xen/xlate_mmu.c  |  133 ++++++++++++++++++++++++++++++++++++++++++++++
 include/xen/xen-ops.h    |   10 ++++
 6 files changed, 156 insertions(+), 194 deletions(-)
 create mode 100644 drivers/xen/xlate_mmu.c

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 263a204..5c04389 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -53,105 +53,21 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
 
 static __read_mostly int xen_events_irq = -1;
 
-/* map fgmfn of domid to lpfn in the current domain */
-static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
-			    unsigned int domid)
-{
-	int rc;
-	struct xen_add_to_physmap_range xatp = {
-		.domid = DOMID_SELF,
-		.foreign_domid = domid,
-		.size = 1,
-		.space = XENMAPSPACE_gmfn_foreign,
-	};
-	xen_ulong_t idx = fgmfn;
-	xen_pfn_t gpfn = lpfn;
-	int err = 0;
-
-	set_xen_guest_handle(xatp.idxs, &idx);
-	set_xen_guest_handle(xatp.gpfns, &gpfn);
-	set_xen_guest_handle(xatp.errs, &err);
-
-	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
-	if (rc || err) {
-		pr_warn("Failed to map pfn to mfn rc:%d:%d pfn:%lx mfn:%lx\n",
-			rc, err, lpfn, fgmfn);
-		return 1;
-	}
-	return 0;
-}
-
-struct remap_data {
-	xen_pfn_t fgmfn; /* foreign domain's gmfn */
-	pgprot_t prot;
-	domid_t  domid;
-	struct vm_area_struct *vma;
-	int index;
-	struct page **pages;
-	struct xen_remap_mfn_info *info;
-};
-
-static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
-			void *data)
-{
-	struct remap_data *info = data;
-	struct page *page = info->pages[info->index++];
-	unsigned long pfn = page_to_pfn(page);
-	pte_t pte = pte_mkspecial(pfn_pte(pfn, info->prot));
-
-	if (map_foreign_page(pfn, info->fgmfn, info->domid))
-		return -EFAULT;
-	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
-
-	return 0;
-}
-
 int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 			       unsigned long addr,
 			       xen_pfn_t mfn, int nr,
 			       pgprot_t prot, unsigned domid,
 			       struct page **pages)
 {
-	int err;
-	struct remap_data data;
-
-	/* TBD: Batching, current sole caller only does page at a time */
-	if (nr > 1)
-		return -EINVAL;
-
-	data.fgmfn = mfn;
-	data.prot = prot;
-	data.domid = domid;
-	data.vma = vma;
-	data.index = 0;
-	data.pages = pages;
-	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
-				  remap_pte_fn, &data);
-	return err;
+	return xen_xlate_remap_gfn_range(vma, addr, mfn, nr,
+					 prot, domid, pages);
 }
 EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
 
 int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
 			       int nr, struct page **pages)
 {
-	int i;
-
-	for (i = 0; i < nr; i++) {
-		struct xen_remove_from_physmap xrp;
-		unsigned long rc, pfn;
-
-		pfn = page_to_pfn(pages[i]);
-
-		xrp.domid = DOMID_SELF;
-		xrp.gpfn = pfn;
-		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
-		if (rc) {
-			pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
-				pfn, rc);
-			return rc;
-		}
-	}
-	return 0;
+	return xen_xlate_unmap_gfn_range(vma, nr, pages);
 }
 EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
 
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index adca9e2..3d536a5 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -2436,95 +2436,6 @@ void __init xen_hvm_init_mmu_ops(void)
 }
 #endif
 
-#ifdef CONFIG_XEN_PVH
-/*
- * Map foreign gfn (fgfn), to local pfn (lpfn). This for the user
- * space creating new guest on pvh dom0 and needing to map domU pages.
- */
-static int xlate_add_to_p2m(unsigned long lpfn, unsigned long fgfn,
-			    unsigned int domid)
-{
-	int rc, err = 0;
-	xen_pfn_t gpfn = lpfn;
-	xen_ulong_t idx = fgfn;
-
-	struct xen_add_to_physmap_range xatp = {
-		.domid = DOMID_SELF,
-		.foreign_domid = domid,
-		.size = 1,
-		.space = XENMAPSPACE_gmfn_foreign,
-	};
-	set_xen_guest_handle(xatp.idxs, &idx);
-	set_xen_guest_handle(xatp.gpfns, &gpfn);
-	set_xen_guest_handle(xatp.errs, &err);
-
-	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
-	if (rc < 0)
-		return rc;
-	return err;
-}
-
-static int xlate_remove_from_p2m(unsigned long spfn, int count)
-{
-	struct xen_remove_from_physmap xrp;
-	int i, rc;
-
-	for (i = 0; i < count; i++) {
-		xrp.domid = DOMID_SELF;
-		xrp.gpfn = spfn+i;
-		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
-		if (rc)
-			break;
-	}
-	return rc;
-}
-
-struct xlate_remap_data {
-	unsigned long fgfn; /* foreign domain's gfn */
-	pgprot_t prot;
-	domid_t  domid;
-	int index;
-	struct page **pages;
-};
-
-static int xlate_map_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
-			    void *data)
-{
-	int rc;
-	struct xlate_remap_data *remap = data;
-	unsigned long pfn = page_to_pfn(remap->pages[remap->index++]);
-	pte_t pteval = pte_mkspecial(pfn_pte(pfn, remap->prot));
-
-	rc = xlate_add_to_p2m(pfn, remap->fgfn, remap->domid);
-	if (rc)
-		return rc;
-	native_set_pte(ptep, pteval);
-
-	return 0;
-}
-
-static int xlate_remap_gfn_range(struct vm_area_struct *vma,
-				 unsigned long addr, unsigned long mfn,
-				 int nr, pgprot_t prot, unsigned domid,
-				 struct page **pages)
-{
-	int err;
-	struct xlate_remap_data pvhdata;
-
-	BUG_ON(!pages);
-
-	pvhdata.fgfn = mfn;
-	pvhdata.prot = prot;
-	pvhdata.domid = domid;
-	pvhdata.index = 0;
-	pvhdata.pages = pages;
-	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
-				  xlate_map_pte_fn, &pvhdata);
-	flush_tlb_all();
-	return err;
-}
-#endif
-
 #define REMAP_BATCH_SIZE 16
 
 struct remap_data {
@@ -2564,8 +2475,8 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 	if (xen_feature(XENFEAT_auto_translated_physmap)) {
 #ifdef CONFIG_XEN_PVH
 		/* We need to update the local page tables and the xen HAP */
-		return xlate_remap_gfn_range(vma, addr, mfn, nr, prot,
-					     domid, pages);
+		return xen_xlate_remap_gfn_range(vma, addr, mfn, nr, prot,
+						 domid, pages);
 #else
 		return -EINVAL;
 #endif
@@ -2609,22 +2520,7 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
 		return 0;
 
 #ifdef CONFIG_XEN_PVH
-	while (numpgs--) {
-		/*
-		 * The mmu has already cleaned up the process mmu
-		 * resources at this point (lookup_address will return
-		 * NULL).
-		 */
-		unsigned long pfn = page_to_pfn(pages[numpgs]);
-
-		xlate_remove_from_p2m(pfn, 1);
-	}
-	/*
-	 * We don't need to flush tlbs because as part of
-	 * xlate_remove_from_p2m, the hypervisor will do tlb flushes
-	 * after removing the p2m entries from the EPT/NPT
-	 */
-	return 0;
+	return xen_xlate_unmap_gfn_range(vma, numpgs, pages);
 #else
 	return -EINVAL;
 #endif
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index b812462..afc39ca 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -253,4 +253,10 @@ config XEN_EFI
 	def_bool y
 	depends on X86_64 && EFI
 
+config XEN_AUTO_XLATE
+	def_bool y
+	depends on ARM || ARM64 || XEN_PVHVM
+	help
+	  Support for auto-translated physmap guests.
+
 endmenu
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 2ccd359..40edd1c 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU)	+= xen-acpi-cpuhotplug.o
 obj-$(CONFIG_XEN_ACPI_PROCESSOR)	+= xen-acpi-processor.o
 obj-$(CONFIG_XEN_EFI)			+= efi.o
 obj-$(CONFIG_XEN_SCSI_BACKEND)		+= xen-scsiback.o
+obj-$(CONFIG_XEN_AUTO_XLATE)		+= xlate_mmu.o
 xen-evtchn-y				:= evtchn.o
 xen-gntdev-y				:= gntdev.o
 xen-gntalloc-y				:= gntalloc.o
diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
new file mode 100644
index 0000000..7724d90
--- /dev/null
+++ b/drivers/xen/xlate_mmu.c
@@ -0,0 +1,133 @@
+/*
+ * MMU operations common to all auto-translated physmap guests.
+ *
+ * Copyright (C) 2015 Citrix Systems R&D Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+#include <linux/kernel.h>
+#include <linux/mm.h>
+
+#include <asm/xen/hypercall.h>
+#include <asm/xen/hypervisor.h>
+
+#include <xen/xen.h>
+#include <xen/page.h>
+#include <xen/interface/xen.h>
+#include <xen/interface/memory.h>
+
+/* map fgmfn of domid to lpfn in the current domain */
+static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
+			    unsigned int domid)
+{
+	int rc;
+	struct xen_add_to_physmap_range xatp = {
+		.domid = DOMID_SELF,
+		.foreign_domid = domid,
+		.size = 1,
+		.space = XENMAPSPACE_gmfn_foreign,
+	};
+	xen_ulong_t idx = fgmfn;
+	xen_pfn_t gpfn = lpfn;
+	int err = 0;
+
+	set_xen_guest_handle(xatp.idxs, &idx);
+	set_xen_guest_handle(xatp.gpfns, &gpfn);
+	set_xen_guest_handle(xatp.errs, &err);
+
+	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
+	return rc < 0 ? rc : err;
+}
+
+struct remap_data {
+	xen_pfn_t fgmfn; /* foreign domain's gmfn */
+	pgprot_t prot;
+	domid_t  domid;
+	struct vm_area_struct *vma;
+	int index;
+	struct page **pages;
+	struct xen_remap_mfn_info *info;
+};
+
+static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
+			void *data)
+{
+	struct remap_data *info = data;
+	struct page *page = info->pages[info->index++];
+	unsigned long pfn = page_to_pfn(page);
+	pte_t pte = pte_mkspecial(pfn_pte(pfn, info->prot));
+	int rc;
+
+	rc = map_foreign_page(pfn, info->fgmfn, info->domid);
+	if (rc < 0)
+		return rc;
+	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
+
+	return 0;
+}
+
+int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
+			      unsigned long addr,
+			      xen_pfn_t gfn, int nr,
+			      pgprot_t prot, unsigned domid,
+			      struct page **pages)
+{
+	int err;
+	struct remap_data data;
+
+	/* TBD: Batching, current sole caller only does page at a time */
+	if (nr > 1)
+		return -EINVAL;
+
+	data.fgmfn = gfn;
+	data.prot = prot;
+	data.domid = domid;
+	data.vma = vma;
+	data.index = 0;
+	data.pages = pages;
+	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
+				  remap_pte_fn, &data);
+	return err;
+}
+EXPORT_SYMBOL_GPL(xen_xlate_remap_gfn_range);
+
+int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
+			      int nr, struct page **pages)
+{
+	int i;
+
+	for (i = 0; i < nr; i++) {
+		struct xen_remove_from_physmap xrp;
+		unsigned long pfn;
+
+		pfn = page_to_pfn(pages[i]);
+
+		xrp.domid = DOMID_SELF;
+		xrp.gpfn = pfn;
+		(void)HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xen_xlate_unmap_gfn_range);
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 8333821..ba7d232 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -34,6 +34,16 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 			       struct page **pages);
 int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
 			       int numpgs, struct page **pages);
+#ifdef CONFIG_XEN_AUTO_XLATE
+int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
+			      unsigned long addr,
+			      xen_pfn_t gfn, int nr,
+			      pgprot_t prot,
+			      unsigned domid, 
+			      struct page **pages);
+int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
+			      int nr, struct page **pages);
+#endif
 
 bool xen_running_on_version_or_later(unsigned int major, unsigned int minor);
 
-- 
1.7.10.4

Re: [PATCHv6 2/3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

[Konrad Rzeszutek Wilk]

On Fri, Mar 06, 2015 at 12:58:34PM +0000, David Vrabel wrote:
> Auto-translated physmap guests (arm, arm64 and x86 PVHVM/PVH) map and
> unmap foreign GFNs using the same method (updating the physmap).
> Unify the two arm and x86 implementations into one commont one.
> 
> Note that on arm and arm64, the correct error code will be returned
> (instead of always -EFAULT) and map/unmap failure warnings are no
> longer printed.  These changes are required if the foreign domain is
> paging (-ENOENT failures are expected and must be propagated up to the
> caller).
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  arch/arm/xen/enlighten.c |   90 ++-----------------------------
>  arch/x86/xen/mmu.c       |  110 ++------------------------------------
>  drivers/xen/Kconfig      |    6 +++
>  drivers/xen/Makefile     |    1 +
>  drivers/xen/xlate_mmu.c  |  133 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/xen/xen-ops.h    |   10 ++++
>  6 files changed, 156 insertions(+), 194 deletions(-)
>  create mode 100644 drivers/xen/xlate_mmu.c
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 263a204..5c04389 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -53,105 +53,21 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
>  
>  static __read_mostly int xen_events_irq = -1;
>  
> -/* map fgmfn of domid to lpfn in the current domain */
> -static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> -			    unsigned int domid)
> -{
> -	int rc;
> -	struct xen_add_to_physmap_range xatp = {
> -		.domid = DOMID_SELF,
> -		.foreign_domid = domid,
> -		.size = 1,
> -		.space = XENMAPSPACE_gmfn_foreign,
> -	};
> -	xen_ulong_t idx = fgmfn;
> -	xen_pfn_t gpfn = lpfn;
> -	int err = 0;
> -
> -	set_xen_guest_handle(xatp.idxs, &idx);
> -	set_xen_guest_handle(xatp.gpfns, &gpfn);
> -	set_xen_guest_handle(xatp.errs, &err);
> -
> -	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
> -	if (rc || err) {
> -		pr_warn("Failed to map pfn to mfn rc:%d:%d pfn:%lx mfn:%lx\n",
> -			rc, err, lpfn, fgmfn);
> -		return 1;
> -	}
> -	return 0;
> -}
> -
> -struct remap_data {
> -	xen_pfn_t fgmfn; /* foreign domain's gmfn */
> -	pgprot_t prot;
> -	domid_t  domid;
> -	struct vm_area_struct *vma;
> -	int index;
> -	struct page **pages;
> -	struct xen_remap_mfn_info *info;
> -};
> -
> -static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> -			void *data)
> -{
> -	struct remap_data *info = data;
> -	struct page *page = info->pages[info->index++];
> -	unsigned long pfn = page_to_pfn(page);
> -	pte_t pte = pte_mkspecial(pfn_pte(pfn, info->prot));
> -
> -	if (map_foreign_page(pfn, info->fgmfn, info->domid))
> -		return -EFAULT;
> -	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> -
> -	return 0;
> -}
> -
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
>  			       xen_pfn_t mfn, int nr,
>  			       pgprot_t prot, unsigned domid,
>  			       struct page **pages)
>  {
> -	int err;
> -	struct remap_data data;
> -
> -	/* TBD: Batching, current sole caller only does page at a time */
> -	if (nr > 1)
> -		return -EINVAL;
> -
> -	data.fgmfn = mfn;
> -	data.prot = prot;
> -	data.domid = domid;
> -	data.vma = vma;
> -	data.index = 0;
> -	data.pages = pages;
> -	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> -				  remap_pte_fn, &data);
> -	return err;
> +	return xen_xlate_remap_gfn_range(vma, addr, mfn, nr,
> +					 prot, domid, pages);
>  }
>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>  
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  			       int nr, struct page **pages)
>  {
> -	int i;
> -
> -	for (i = 0; i < nr; i++) {
> -		struct xen_remove_from_physmap xrp;
> -		unsigned long rc, pfn;
> -
> -		pfn = page_to_pfn(pages[i]);
> -
> -		xrp.domid = DOMID_SELF;
> -		xrp.gpfn = pfn;
> -		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> -		if (rc) {
> -			pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> -				pfn, rc);
> -			return rc;
> -		}
> -	}
> -	return 0;
> +	return xen_xlate_unmap_gfn_range(vma, nr, pages);
>  }
>  EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
>  
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index adca9e2..3d536a5 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -2436,95 +2436,6 @@ void __init xen_hvm_init_mmu_ops(void)
>  }
>  #endif
>  
> -#ifdef CONFIG_XEN_PVH
> -/*
> - * Map foreign gfn (fgfn), to local pfn (lpfn). This for the user
> - * space creating new guest on pvh dom0 and needing to map domU pages.
> - */
> -static int xlate_add_to_p2m(unsigned long lpfn, unsigned long fgfn,
> -			    unsigned int domid)
> -{
> -	int rc, err = 0;
> -	xen_pfn_t gpfn = lpfn;
> -	xen_ulong_t idx = fgfn;
> -
> -	struct xen_add_to_physmap_range xatp = {
> -		.domid = DOMID_SELF,
> -		.foreign_domid = domid,
> -		.size = 1,
> -		.space = XENMAPSPACE_gmfn_foreign,
> -	};
> -	set_xen_guest_handle(xatp.idxs, &idx);
> -	set_xen_guest_handle(xatp.gpfns, &gpfn);
> -	set_xen_guest_handle(xatp.errs, &err);
> -
> -	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
> -	if (rc < 0)
> -		return rc;
> -	return err;
> -}
> -
> -static int xlate_remove_from_p2m(unsigned long spfn, int count)
> -{
> -	struct xen_remove_from_physmap xrp;
> -	int i, rc;
> -
> -	for (i = 0; i < count; i++) {
> -		xrp.domid = DOMID_SELF;
> -		xrp.gpfn = spfn+i;
> -		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> -		if (rc)
> -			break;
> -	}
> -	return rc;
> -}
> -
> -struct xlate_remap_data {
> -	unsigned long fgfn; /* foreign domain's gfn */
> -	pgprot_t prot;
> -	domid_t  domid;
> -	int index;
> -	struct page **pages;
> -};
> -
> -static int xlate_map_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> -			    void *data)
> -{
> -	int rc;
> -	struct xlate_remap_data *remap = data;
> -	unsigned long pfn = page_to_pfn(remap->pages[remap->index++]);
> -	pte_t pteval = pte_mkspecial(pfn_pte(pfn, remap->prot));
> -
> -	rc = xlate_add_to_p2m(pfn, remap->fgfn, remap->domid);
> -	if (rc)
> -		return rc;
> -	native_set_pte(ptep, pteval);
> -
> -	return 0;
> -}
> -
> -static int xlate_remap_gfn_range(struct vm_area_struct *vma,
> -				 unsigned long addr, unsigned long mfn,
> -				 int nr, pgprot_t prot, unsigned domid,
> -				 struct page **pages)
> -{
> -	int err;
> -	struct xlate_remap_data pvhdata;
> -
> -	BUG_ON(!pages);
> -
> -	pvhdata.fgfn = mfn;
> -	pvhdata.prot = prot;
> -	pvhdata.domid = domid;
> -	pvhdata.index = 0;
> -	pvhdata.pages = pages;
> -	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> -				  xlate_map_pte_fn, &pvhdata);
> -	flush_tlb_all();
> -	return err;
> -}
> -#endif
> -
>  #define REMAP_BATCH_SIZE 16
>  
>  struct remap_data {
> @@ -2564,8 +2475,8 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  	if (xen_feature(XENFEAT_auto_translated_physmap)) {
>  #ifdef CONFIG_XEN_PVH
>  		/* We need to update the local page tables and the xen HAP */
> -		return xlate_remap_gfn_range(vma, addr, mfn, nr, prot,
> -					     domid, pages);
> +		return xen_xlate_remap_gfn_range(vma, addr, mfn, nr, prot,
> +						 domid, pages);
>  #else
>  		return -EINVAL;
>  #endif
> @@ -2609,22 +2520,7 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  		return 0;
>  
>  #ifdef CONFIG_XEN_PVH
> -	while (numpgs--) {
> -		/*
> -		 * The mmu has already cleaned up the process mmu
> -		 * resources at this point (lookup_address will return
> -		 * NULL).
> -		 */
> -		unsigned long pfn = page_to_pfn(pages[numpgs]);
> -
> -		xlate_remove_from_p2m(pfn, 1);
> -	}
> -	/*
> -	 * We don't need to flush tlbs because as part of
> -	 * xlate_remove_from_p2m, the hypervisor will do tlb flushes
> -	 * after removing the p2m entries from the EPT/NPT
> -	 */
> -	return 0;
> +	return xen_xlate_unmap_gfn_range(vma, numpgs, pages);
>  #else
>  	return -EINVAL;
>  #endif
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index b812462..afc39ca 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -253,4 +253,10 @@ config XEN_EFI
>  	def_bool y
>  	depends on X86_64 && EFI
>  
> +config XEN_AUTO_XLATE
> +	def_bool y
> +	depends on ARM || ARM64 || XEN_PVHVM
> +	help
> +	  Support for auto-translated physmap guests.
> +
>  endmenu
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 2ccd359..40edd1c 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU)	+= xen-acpi-cpuhotplug.o
>  obj-$(CONFIG_XEN_ACPI_PROCESSOR)	+= xen-acpi-processor.o
>  obj-$(CONFIG_XEN_EFI)			+= efi.o
>  obj-$(CONFIG_XEN_SCSI_BACKEND)		+= xen-scsiback.o
> +obj-$(CONFIG_XEN_AUTO_XLATE)		+= xlate_mmu.o
>  xen-evtchn-y				:= evtchn.o
>  xen-gntdev-y				:= gntdev.o
>  xen-gntalloc-y				:= gntalloc.o
> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
> new file mode 100644
> index 0000000..7724d90
> --- /dev/null
> +++ b/drivers/xen/xlate_mmu.c
> @@ -0,0 +1,133 @@
> +/*
> + * MMU operations common to all auto-translated physmap guests.
> + *
> + * Copyright (C) 2015 Citrix Systems R&D Ltd.


And others. You are moving some of the code from existing code to this
one.

> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation; or, when distributed
> + * separately from the Linux kernel or incorporated into other
> + * software packages, subject to the following license:
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this source file (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use, copy, modify,
> + * merge, publish, distribute, sublicense, and/or sell copies of the Software,
> + * and to permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +
> +#include <asm/xen/hypercall.h>
> +#include <asm/xen/hypervisor.h>
> +
> +#include <xen/xen.h>
> +#include <xen/page.h>
> +#include <xen/interface/xen.h>
> +#include <xen/interface/memory.h>
> +
> +/* map fgmfn of domid to lpfn in the current domain */
> +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> +			    unsigned int domid)
> +{
> +	int rc;
> +	struct xen_add_to_physmap_range xatp = {
> +		.domid = DOMID_SELF,
> +		.foreign_domid = domid,
> +		.size = 1,
> +		.space = XENMAPSPACE_gmfn_foreign,
> +	};
> +	xen_ulong_t idx = fgmfn;
> +	xen_pfn_t gpfn = lpfn;
> +	int err = 0;
> +
> +	set_xen_guest_handle(xatp.idxs, &idx);
> +	set_xen_guest_handle(xatp.gpfns, &gpfn);
> +	set_xen_guest_handle(xatp.errs, &err);
> +
> +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
> +	return rc < 0 ? rc : err;
> +}
> +
> +struct remap_data {
> +	xen_pfn_t fgmfn; /* foreign domain's gmfn */
> +	pgprot_t prot;
> +	domid_t  domid;
> +	struct vm_area_struct *vma;
> +	int index;
> +	struct page **pages;
> +	struct xen_remap_mfn_info *info;
> +};
> +
> +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> +			void *data)
> +{
> +	struct remap_data *info = data;
> +	struct page *page = info->pages[info->index++];
> +	unsigned long pfn = page_to_pfn(page);
> +	pte_t pte = pte_mkspecial(pfn_pte(pfn, info->prot));
> +	int rc;
> +
> +	rc = map_foreign_page(pfn, info->fgmfn, info->domid);
> +	if (rc < 0)
> +		return rc;
> +	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> +
> +	return 0;
> +}
> +
> +int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
> +			      unsigned long addr,
> +			      xen_pfn_t gfn, int nr,
> +			      pgprot_t prot, unsigned domid,
> +			      struct page **pages)
> +{
> +	int err;
> +	struct remap_data data;
> +
> +	/* TBD: Batching, current sole caller only does page at a time */
> +	if (nr > 1)
> +		return -EINVAL;
> +
> +	data.fgmfn = gfn;
> +	data.prot = prot;
> +	data.domid = domid;
> +	data.vma = vma;
> +	data.index = 0;
> +	data.pages = pages;
> +	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> +				  remap_pte_fn, &data);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(xen_xlate_remap_gfn_range);
> +
> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
> +			      int nr, struct page **pages)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr; i++) {
> +		struct xen_remove_from_physmap xrp;
> +		unsigned long pfn;
> +
> +		pfn = page_to_pfn(pages[i]);
> +
> +		xrp.domid = DOMID_SELF;
> +		xrp.gpfn = pfn;
> +		(void)HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xen_xlate_unmap_gfn_range);
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index 8333821..ba7d232 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -34,6 +34,16 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       struct page **pages);
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  			       int numpgs, struct page **pages);
> +#ifdef CONFIG_XEN_AUTO_XLATE
> +int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
> +			      unsigned long addr,
> +			      xen_pfn_t gfn, int nr,
> +			      pgprot_t prot,
> +			      unsigned domid, 
> +			      struct page **pages);
> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
> +			      int nr, struct page **pages);
> +#endif
>  
>  bool xen_running_on_version_or_later(unsigned int major, unsigned int minor);
>  
> -- 
> 1.7.10.4
> 

Re: [PATCHv6 2/3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

[David Vrabel]

On 06/03/15 15:51, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 06, 2015 at 12:58:34PM +0000, David Vrabel wrote:
>> 
>> --- /dev/null
>> +++ b/drivers/xen/xlate_mmu.c
>> @@ -0,0 +1,133 @@
>> +/*
>> + * MMU operations common to all auto-translated physmap guests.
>> + *
>> + * Copyright (C) 2015 Citrix Systems R&D Ltd.
> 
> 
> And others. You are moving some of the code from existing code to this
> one.

It was copied from the ARM implementation, arch/arm/xen/enlighten.c,
which contains no copyright information.

David

Re: [PATCHv6 2/3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

[Stefano Stabellini]

On Fri, 6 Mar 2015, David Vrabel wrote:
> Auto-translated physmap guests (arm, arm64 and x86 PVHVM/PVH) map and
> unmap foreign GFNs using the same method (updating the physmap).
> Unify the two arm and x86 implementations into one commont one.
> 
> Note that on arm and arm64, the correct error code will be returned
> (instead of always -EFAULT) and map/unmap failure warnings are no
> longer printed.  These changes are required if the foreign domain is
> paging (-ENOENT failures are expected and must be propagated up to the
> caller).
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  arch/arm/xen/enlighten.c |   90 ++-----------------------------
>  arch/x86/xen/mmu.c       |  110 ++------------------------------------
>  drivers/xen/Kconfig      |    6 +++
>  drivers/xen/Makefile     |    1 +
>  drivers/xen/xlate_mmu.c  |  133 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/xen/xen-ops.h    |   10 ++++
>  6 files changed, 156 insertions(+), 194 deletions(-)
>  create mode 100644 drivers/xen/xlate_mmu.c
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 263a204..5c04389 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -53,105 +53,21 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
>  
>  static __read_mostly int xen_events_irq = -1;
>  
> -/* map fgmfn of domid to lpfn in the current domain */
> -static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> -			    unsigned int domid)
> -{
> -	int rc;
> -	struct xen_add_to_physmap_range xatp = {
> -		.domid = DOMID_SELF,
> -		.foreign_domid = domid,
> -		.size = 1,
> -		.space = XENMAPSPACE_gmfn_foreign,
> -	};
> -	xen_ulong_t idx = fgmfn;
> -	xen_pfn_t gpfn = lpfn;
> -	int err = 0;
> -
> -	set_xen_guest_handle(xatp.idxs, &idx);
> -	set_xen_guest_handle(xatp.gpfns, &gpfn);
> -	set_xen_guest_handle(xatp.errs, &err);
> -
> -	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
> -	if (rc || err) {
> -		pr_warn("Failed to map pfn to mfn rc:%d:%d pfn:%lx mfn:%lx\n",
> -			rc, err, lpfn, fgmfn);
> -		return 1;
> -	}
> -	return 0;
> -}
> -
> -struct remap_data {
> -	xen_pfn_t fgmfn; /* foreign domain's gmfn */
> -	pgprot_t prot;
> -	domid_t  domid;
> -	struct vm_area_struct *vma;
> -	int index;
> -	struct page **pages;
> -	struct xen_remap_mfn_info *info;
> -};
> -
> -static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> -			void *data)
> -{
> -	struct remap_data *info = data;
> -	struct page *page = info->pages[info->index++];
> -	unsigned long pfn = page_to_pfn(page);
> -	pte_t pte = pte_mkspecial(pfn_pte(pfn, info->prot));
> -
> -	if (map_foreign_page(pfn, info->fgmfn, info->domid))
> -		return -EFAULT;
> -	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> -
> -	return 0;
> -}
> -
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
>  			       xen_pfn_t mfn, int nr,
>  			       pgprot_t prot, unsigned domid,
>  			       struct page **pages)
>  {
> -	int err;
> -	struct remap_data data;
> -
> -	/* TBD: Batching, current sole caller only does page at a time */
> -	if (nr > 1)
> -		return -EINVAL;
> -
> -	data.fgmfn = mfn;
> -	data.prot = prot;
> -	data.domid = domid;
> -	data.vma = vma;
> -	data.index = 0;
> -	data.pages = pages;
> -	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> -				  remap_pte_fn, &data);
> -	return err;
> +	return xen_xlate_remap_gfn_range(vma, addr, mfn, nr,
> +					 prot, domid, pages);
>  }
>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>  
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  			       int nr, struct page **pages)
>  {
> -	int i;
> -
> -	for (i = 0; i < nr; i++) {
> -		struct xen_remove_from_physmap xrp;
> -		unsigned long rc, pfn;
> -
> -		pfn = page_to_pfn(pages[i]);
> -
> -		xrp.domid = DOMID_SELF;
> -		xrp.gpfn = pfn;
> -		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> -		if (rc) {
> -			pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> -				pfn, rc);
> -			return rc;
> -		}
> -	}
> -	return 0;
> +	return xen_xlate_unmap_gfn_range(vma, nr, pages);
>  }
>  EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
>  
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index adca9e2..3d536a5 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -2436,95 +2436,6 @@ void __init xen_hvm_init_mmu_ops(void)
>  }
>  #endif
>  
> -#ifdef CONFIG_XEN_PVH
> -/*
> - * Map foreign gfn (fgfn), to local pfn (lpfn). This for the user
> - * space creating new guest on pvh dom0 and needing to map domU pages.
> - */
> -static int xlate_add_to_p2m(unsigned long lpfn, unsigned long fgfn,
> -			    unsigned int domid)
> -{
> -	int rc, err = 0;
> -	xen_pfn_t gpfn = lpfn;
> -	xen_ulong_t idx = fgfn;
> -
> -	struct xen_add_to_physmap_range xatp = {
> -		.domid = DOMID_SELF,
> -		.foreign_domid = domid,
> -		.size = 1,
> -		.space = XENMAPSPACE_gmfn_foreign,
> -	};
> -	set_xen_guest_handle(xatp.idxs, &idx);
> -	set_xen_guest_handle(xatp.gpfns, &gpfn);
> -	set_xen_guest_handle(xatp.errs, &err);
> -
> -	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
> -	if (rc < 0)
> -		return rc;
> -	return err;
> -}
> -
> -static int xlate_remove_from_p2m(unsigned long spfn, int count)
> -{
> -	struct xen_remove_from_physmap xrp;
> -	int i, rc;
> -
> -	for (i = 0; i < count; i++) {
> -		xrp.domid = DOMID_SELF;
> -		xrp.gpfn = spfn+i;
> -		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> -		if (rc)
> -			break;
> -	}
> -	return rc;
> -}
> -
> -struct xlate_remap_data {
> -	unsigned long fgfn; /* foreign domain's gfn */
> -	pgprot_t prot;
> -	domid_t  domid;
> -	int index;
> -	struct page **pages;
> -};
> -
> -static int xlate_map_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> -			    void *data)
> -{
> -	int rc;
> -	struct xlate_remap_data *remap = data;
> -	unsigned long pfn = page_to_pfn(remap->pages[remap->index++]);
> -	pte_t pteval = pte_mkspecial(pfn_pte(pfn, remap->prot));
> -
> -	rc = xlate_add_to_p2m(pfn, remap->fgfn, remap->domid);
> -	if (rc)
> -		return rc;
> -	native_set_pte(ptep, pteval);
> -
> -	return 0;
> -}
> -
> -static int xlate_remap_gfn_range(struct vm_area_struct *vma,
> -				 unsigned long addr, unsigned long mfn,
> -				 int nr, pgprot_t prot, unsigned domid,
> -				 struct page **pages)
> -{
> -	int err;
> -	struct xlate_remap_data pvhdata;
> -
> -	BUG_ON(!pages);
> -
> -	pvhdata.fgfn = mfn;
> -	pvhdata.prot = prot;
> -	pvhdata.domid = domid;
> -	pvhdata.index = 0;
> -	pvhdata.pages = pages;
> -	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> -				  xlate_map_pte_fn, &pvhdata);
> -	flush_tlb_all();
> -	return err;
> -}
> -#endif
> -
>  #define REMAP_BATCH_SIZE 16
>  
>  struct remap_data {
> @@ -2564,8 +2475,8 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  	if (xen_feature(XENFEAT_auto_translated_physmap)) {
>  #ifdef CONFIG_XEN_PVH
>  		/* We need to update the local page tables and the xen HAP */
> -		return xlate_remap_gfn_range(vma, addr, mfn, nr, prot,
> -					     domid, pages);
> +		return xen_xlate_remap_gfn_range(vma, addr, mfn, nr, prot,
> +						 domid, pages);
>  #else
>  		return -EINVAL;
>  #endif
> @@ -2609,22 +2520,7 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  		return 0;
>  
>  #ifdef CONFIG_XEN_PVH
> -	while (numpgs--) {
> -		/*
> -		 * The mmu has already cleaned up the process mmu
> -		 * resources at this point (lookup_address will return
> -		 * NULL).
> -		 */
> -		unsigned long pfn = page_to_pfn(pages[numpgs]);
> -
> -		xlate_remove_from_p2m(pfn, 1);
> -	}
> -	/*
> -	 * We don't need to flush tlbs because as part of
> -	 * xlate_remove_from_p2m, the hypervisor will do tlb flushes
> -	 * after removing the p2m entries from the EPT/NPT
> -	 */
> -	return 0;
> +	return xen_xlate_unmap_gfn_range(vma, numpgs, pages);
>  #else
>  	return -EINVAL;
>  #endif
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index b812462..afc39ca 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -253,4 +253,10 @@ config XEN_EFI
>  	def_bool y
>  	depends on X86_64 && EFI
>  
> +config XEN_AUTO_XLATE
> +	def_bool y
> +	depends on ARM || ARM64 || XEN_PVHVM
> +	help
> +	  Support for auto-translated physmap guests.
> +
>  endmenu

I think the dependency chain is inverted: in order to enable XEN on ARM
and ARM64 or to enable XEN_PVHVM, one needs XEN_AUTO_XLATE.
I think that config XEN should select XEN_AUTO_XLATE on ARM and ARM64
and config XEN_PVHVM should select XEN_AUTO_XLATE on x86.


> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 2ccd359..40edd1c 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU)	+= xen-acpi-cpuhotplug.o
>  obj-$(CONFIG_XEN_ACPI_PROCESSOR)	+= xen-acpi-processor.o
>  obj-$(CONFIG_XEN_EFI)			+= efi.o
>  obj-$(CONFIG_XEN_SCSI_BACKEND)		+= xen-scsiback.o
> +obj-$(CONFIG_XEN_AUTO_XLATE)		+= xlate_mmu.o
>  xen-evtchn-y				:= evtchn.o
>  xen-gntdev-y				:= gntdev.o
>  xen-gntalloc-y				:= gntalloc.o
> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
> new file mode 100644
> index 0000000..7724d90
> --- /dev/null
> +++ b/drivers/xen/xlate_mmu.c
> @@ -0,0 +1,133 @@
> +/*
> + * MMU operations common to all auto-translated physmap guests.
> + *
> + * Copyright (C) 2015 Citrix Systems R&D Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation; or, when distributed
> + * separately from the Linux kernel or incorporated into other
> + * software packages, subject to the following license:
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this source file (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use, copy, modify,
> + * merge, publish, distribute, sublicense, and/or sell copies of the Software,
> + * and to permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +
> +#include <asm/xen/hypercall.h>
> +#include <asm/xen/hypervisor.h>
> +
> +#include <xen/xen.h>
> +#include <xen/page.h>
> +#include <xen/interface/xen.h>
> +#include <xen/interface/memory.h>
> +
> +/* map fgmfn of domid to lpfn in the current domain */
> +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> +			    unsigned int domid)
> +{
> +	int rc;
> +	struct xen_add_to_physmap_range xatp = {
> +		.domid = DOMID_SELF,
> +		.foreign_domid = domid,
> +		.size = 1,
> +		.space = XENMAPSPACE_gmfn_foreign,
> +	};
> +	xen_ulong_t idx = fgmfn;
> +	xen_pfn_t gpfn = lpfn;
> +	int err = 0;
> +
> +	set_xen_guest_handle(xatp.idxs, &idx);
> +	set_xen_guest_handle(xatp.gpfns, &gpfn);
> +	set_xen_guest_handle(xatp.errs, &err);
> +
> +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
> +	return rc < 0 ? rc : err;
> +}
> +
> +struct remap_data {
> +	xen_pfn_t fgmfn; /* foreign domain's gmfn */
> +	pgprot_t prot;
> +	domid_t  domid;
> +	struct vm_area_struct *vma;
> +	int index;
> +	struct page **pages;
> +	struct xen_remap_mfn_info *info;
> +};
> +
> +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> +			void *data)
> +{
> +	struct remap_data *info = data;
> +	struct page *page = info->pages[info->index++];
> +	unsigned long pfn = page_to_pfn(page);
> +	pte_t pte = pte_mkspecial(pfn_pte(pfn, info->prot));
> +	int rc;
> +
> +	rc = map_foreign_page(pfn, info->fgmfn, info->domid);
> +	if (rc < 0)
> +		return rc;
> +	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> +
> +	return 0;
> +}
> +
> +int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
> +			      unsigned long addr,
> +			      xen_pfn_t gfn, int nr,
> +			      pgprot_t prot, unsigned domid,
> +			      struct page **pages)
> +{
> +	int err;
> +	struct remap_data data;
> +
> +	/* TBD: Batching, current sole caller only does page at a time */
> +	if (nr > 1)
> +		return -EINVAL;
> +
> +	data.fgmfn = gfn;
> +	data.prot = prot;
> +	data.domid = domid;
> +	data.vma = vma;
> +	data.index = 0;
> +	data.pages = pages;
> +	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> +				  remap_pte_fn, &data);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(xen_xlate_remap_gfn_range);
> +
> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
> +			      int nr, struct page **pages)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr; i++) {
> +		struct xen_remove_from_physmap xrp;
> +		unsigned long pfn;
> +
> +		pfn = page_to_pfn(pages[i]);
> +
> +		xrp.domid = DOMID_SELF;
> +		xrp.gpfn = pfn;
> +		(void)HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);

Why drop the warning we had before?


> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xen_xlate_unmap_gfn_range);
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index 8333821..ba7d232 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -34,6 +34,16 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       struct page **pages);
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  			       int numpgs, struct page **pages);
> +#ifdef CONFIG_XEN_AUTO_XLATE
> +int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
> +			      unsigned long addr,
> +			      xen_pfn_t gfn, int nr,
> +			      pgprot_t prot,
> +			      unsigned domid, 
> +			      struct page **pages);
> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
> +			      int nr, struct page **pages);
> +#endif

I don't think we actually need the #ifdef in the header file?


>  bool xen_running_on_version_or_later(unsigned int major, unsigned int minor);
>  
> -- 
> 1.7.10.4
> 

Re: [PATCHv6 2/3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

[David Vrabel]

On 06/03/15 17:51, Stefano Stabellini wrote:
> On Fri, 6 Mar 2015, David Vrabel wrote:
>> Auto-translated physmap guests (arm, arm64 and x86 PVHVM/PVH) map and
>> unmap foreign GFNs using the same method (updating the physmap).
>> Unify the two arm and x86 implementations into one commont one.
>>
>> Note that on arm and arm64, the correct error code will be returned
>> (instead of always -EFAULT) and map/unmap failure warnings are no
>> longer printed.  These changes are required if the foreign domain is
>> paging (-ENOENT failures are expected and must be propagated up to the
>> caller).
>>
[...]
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -253,4 +253,10 @@ config XEN_EFI
>>  	def_bool y
>>  	depends on X86_64 && EFI
>>  
>> +config XEN_AUTO_XLATE
>> +	def_bool y
>> +	depends on ARM || ARM64 || XEN_PVHVM
>> +	help
>> +	  Support for auto-translated physmap guests.
>> +
>>  endmenu
> 
> I think the dependency chain is inverted: in order to enable XEN on ARM
> and ARM64 or to enable XEN_PVHVM, one needs XEN_AUTO_XLATE.
> I think that config XEN should select XEN_AUTO_XLATE on ARM and ARM64
> and config XEN_PVHVM should select XEN_AUTO_XLATE on x86.

This is a non-visible option that is automatically enabled if required,
so I'm not sure what you mean here.

>> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
>> +			      int nr, struct page **pages)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < nr; i++) {
>> +		struct xen_remove_from_physmap xrp;
>> +		unsigned long pfn;
>> +
>> +		pfn = page_to_pfn(pages[i]);
>> +
>> +		xrp.domid = DOMID_SELF;
>> +		xrp.gpfn = pfn;
>> +		(void)HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> 
> Why drop the warning we had before?

Because map failures (and hence) unmap failures are expected and would
result in log spam (particularly with an x86 HVM guest).

>> --- a/include/xen/xen-ops.h
>> +++ b/include/xen/xen-ops.h
>> @@ -34,6 +34,16 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>>  			       struct page **pages);
>>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>>  			       int numpgs, struct page **pages);
>> +#ifdef CONFIG_XEN_AUTO_XLATE
>> +int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
>> +			      unsigned long addr,
>> +			      xen_pfn_t gfn, int nr,
>> +			      pgprot_t prot,
>> +			      unsigned domid, 
>> +			      struct page **pages);
>> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
>> +			      int nr, struct page **pages);
>> +#endif
> 
> I don't think we actually need the #ifdef in the header file?

It serves as useful documentation if nothing else.

David

Re: [PATCHv6 2/3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

[Stefano Stabellini]

On Fri, 6 Mar 2015, David Vrabel wrote:
> On 06/03/15 17:51, Stefano Stabellini wrote:
> > On Fri, 6 Mar 2015, David Vrabel wrote:
> >> Auto-translated physmap guests (arm, arm64 and x86 PVHVM/PVH) map and
> >> unmap foreign GFNs using the same method (updating the physmap).
> >> Unify the two arm and x86 implementations into one commont one.
> >>
> >> Note that on arm and arm64, the correct error code will be returned
> >> (instead of always -EFAULT) and map/unmap failure warnings are no
> >> longer printed.  These changes are required if the foreign domain is
> >> paging (-ENOENT failures are expected and must be propagated up to the
> >> caller).
> >>
> [...]
> >> --- a/drivers/xen/Kconfig
> >> +++ b/drivers/xen/Kconfig
> >> @@ -253,4 +253,10 @@ config XEN_EFI
> >>  	def_bool y
> >>  	depends on X86_64 && EFI
> >>  
> >> +config XEN_AUTO_XLATE
> >> +	def_bool y
> >> +	depends on ARM || ARM64 || XEN_PVHVM
> >> +	help
> >> +	  Support for auto-translated physmap guests.
> >> +
> >>  endmenu
> > 
> > I think the dependency chain is inverted: in order to enable XEN on ARM
> > and ARM64 or to enable XEN_PVHVM, one needs XEN_AUTO_XLATE.
> > I think that config XEN should select XEN_AUTO_XLATE on ARM and ARM64
> > and config XEN_PVHVM should select XEN_AUTO_XLATE on x86.
> 
> This is a non-visible option that is automatically enabled if required,
> so I'm not sure what you mean here.

OK


> >> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
> >> +			      int nr, struct page **pages)
> >> +{
> >> +	int i;
> >> +
> >> +	for (i = 0; i < nr; i++) {
> >> +		struct xen_remove_from_physmap xrp;
> >> +		unsigned long pfn;
> >> +
> >> +		pfn = page_to_pfn(pages[i]);
> >> +
> >> +		xrp.domid = DOMID_SELF;
> >> +		xrp.gpfn = pfn;
> >> +		(void)HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> > 
> > Why drop the warning we had before?
> 
> Because map failures (and hence) unmap failures are expected and would
> result in log spam (particularly with an x86 HVM guest).

Ah right, the explanation is already in the commit message. Good.


> >> --- a/include/xen/xen-ops.h
> >> +++ b/include/xen/xen-ops.h
> >> @@ -34,6 +34,16 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> >>  			       struct page **pages);
> >>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> >>  			       int numpgs, struct page **pages);
> >> +#ifdef CONFIG_XEN_AUTO_XLATE
> >> +int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
> >> +			      unsigned long addr,
> >> +			      xen_pfn_t gfn, int nr,
> >> +			      pgprot_t prot,
> >> +			      unsigned domid, 
> >> +			      struct page **pages);
> >> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
> >> +			      int nr, struct page **pages);
> >> +#endif
> > 
> > I don't think we actually need the #ifdef in the header file?
> 
> It serves as useful documentation if nothing else.

git serves as useful documentation

[PATCHv6 3/3] xen/privcmd: improve performance of MMAPBATCH_V2

[David Vrabel]

Make the IOCTL_PRIVCMD_MMAPBATCH_V2 (and older V1 version) map
multiple frames at a time rather than one at a time, despite the pages
being non-consecutive GFNs.

xen_remap_foreign_mfn_array() is added which maps an array of GFNs
(instead of a consecutive range of GFNs).

Migrate times are significantly improved (when using a PV toolstack
domain).  For example, for an idle 12 GiB PV guest:

        Before     After
  real  0m38.179s  0m26.868s
  user  0m15.096s  0m13.652s
  sys   0m28.988s  0m18.732s

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/arm/xen/enlighten.c |   20 ++++++--
 arch/x86/xen/mmu.c       |  102 ++++++++++++++++++++++++++++++++--------
 drivers/xen/privcmd.c    |  117 ++++++++++++++++++++++++++++++++--------------
 drivers/xen/xlate_mmu.c  |   50 ++++++++++++--------
 include/xen/xen-ops.h    |   47 +++++++++++++++++--
 5 files changed, 253 insertions(+), 83 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 5c04389..9929cb6 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -53,15 +53,27 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
 
 static __read_mostly int xen_events_irq = -1;
 
-int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
+int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
 			       unsigned long addr,
-			       xen_pfn_t mfn, int nr,
-			       pgprot_t prot, unsigned domid,
+			       xen_pfn_t *mfn, int nr,
+			       int *err_ptr, pgprot_t prot,
+			       unsigned domid,
 			       struct page **pages)
 {
-	return xen_xlate_remap_gfn_range(vma, addr, mfn, nr,
+	return xen_xlate_remap_gfn_array(vma, addr, mfn, nr, err_ptr,
 					 prot, domid, pages);
 }
+EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_array);
+
+/* Not used in ARM. Use xen_remap_domain_mfn_array(). */
+int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
+                              unsigned long addr,
+                              xen_pfn_t mfn, int nr,
+                              pgprot_t prot, unsigned domid,
+                              struct page **pages)
+{
+	return -ENOSYS;
+}
 EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
 
 int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 3d536a5..78d352f 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -2439,7 +2439,8 @@ void __init xen_hvm_init_mmu_ops(void)
 #define REMAP_BATCH_SIZE 16
 
 struct remap_data {
-	unsigned long mfn;
+	xen_pfn_t *mfn;
+	bool contiguous;
 	pgprot_t prot;
 	struct mmu_update *mmu_update;
 };
@@ -2448,7 +2449,14 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, pgtable_t token,
 				 unsigned long addr, void *data)
 {
 	struct remap_data *rmd = data;
-	pte_t pte = pte_mkspecial(mfn_pte(rmd->mfn++, rmd->prot));
+	pte_t pte = pte_mkspecial(mfn_pte(*rmd->mfn, rmd->prot));
+
+	/* If we have a contigious range, just update the mfn itself,
+	   else update pointer to be "next mfn". */
+	if (rmd->contiguous)
+		(*rmd->mfn)++;
+	else
+		rmd->mfn++;
 
 	rmd->mmu_update->ptr = virt_to_machine(ptep).maddr;
 	rmd->mmu_update->val = pte_val_ma(pte);
@@ -2457,26 +2465,26 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, pgtable_t token,
 	return 0;
 }
 
-int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
-			       unsigned long addr,
-			       xen_pfn_t mfn, int nr,
-			       pgprot_t prot, unsigned domid,
-			       struct page **pages)
-
+static int do_remap_mfn(struct vm_area_struct *vma,
+			unsigned long addr,
+			xen_pfn_t *mfn, int nr,
+			int *err_ptr, pgprot_t prot,
+			unsigned domid,
+			struct page **pages)
 {
+	int err = 0;
 	struct remap_data rmd;
 	struct mmu_update mmu_update[REMAP_BATCH_SIZE];
-	int batch;
 	unsigned long range;
-	int err = 0;
+	int mapped = 0;
 
 	BUG_ON(!((vma->vm_flags & (VM_PFNMAP | VM_IO)) == (VM_PFNMAP | VM_IO)));
 
 	if (xen_feature(XENFEAT_auto_translated_physmap)) {
 #ifdef CONFIG_XEN_PVH
 		/* We need to update the local page tables and the xen HAP */
-		return xen_xlate_remap_gfn_range(vma, addr, mfn, nr, prot,
-						 domid, pages);
+		return xen_xlate_remap_gfn_array(vma, addr, mfn, nr, err_ptr,
+						 prot, domid, pages);
 #else
 		return -EINVAL;
 #endif
@@ -2484,9 +2492,15 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 
 	rmd.mfn = mfn;
 	rmd.prot = prot;
+	/* We use the err_ptr to indicate if there we are doing a contigious
+	 * mapping or a discontigious mapping. */
+	rmd.contiguous = !err_ptr;
 
 	while (nr) {
-		batch = min(REMAP_BATCH_SIZE, nr);
+		int index = 0;
+		int done = 0;
+		int batch = min(REMAP_BATCH_SIZE, nr);
+		int batch_left = batch;
 		range = (unsigned long)batch << PAGE_SHIFT;
 
 		rmd.mmu_update = mmu_update;
@@ -2495,23 +2509,72 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 		if (err)
 			goto out;
 
-		err = HYPERVISOR_mmu_update(mmu_update, batch, NULL, domid);
-		if (err < 0)
-			goto out;
+		/* We record the error for each page that gives an error, but
+		 * continue mapping until the whole set is done */
+		do {
+			int i;
+
+			err = HYPERVISOR_mmu_update(&mmu_update[index],
+						    batch_left, &done, domid);
+
+			/*
+			 * @err_ptr may be the same buffer as @mfn, so
+			 * only clear it after each chunk of @mfn is
+			 * used.
+			 */
+			if (err_ptr) {
+				for (i = index; i < index + done; i++)
+					err_ptr[i] = 0;
+			}
+			if (err < 0) {
+				if (!err_ptr)
+					goto out;
+				err_ptr[i] = err;
+				done++; /* Skip failed frame. */
+			} else
+				mapped += done;
+			batch_left -= done;
+			index += done;
+		} while (batch_left);
 
 		nr -= batch;
 		addr += range;
+		if (err_ptr)
+			err_ptr += batch;
 	}
-
-	err = 0;
 out:
 
 	xen_flush_tlb_all();
 
-	return err;
+	return err < 0 ? err : mapped;
+}
+
+int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
+			       unsigned long addr,
+			       xen_pfn_t mfn, int nr,
+			       pgprot_t prot, unsigned domid,
+			       struct page **pages)
+{
+	return do_remap_mfn(vma, addr, &mfn, nr, NULL, prot, domid, pages);
 }
 EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
 
+int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
+			       unsigned long addr,
+			       xen_pfn_t *mfn, int nr,
+			       int *err_ptr, pgprot_t prot,
+			       unsigned domid, struct page **pages)
+{
+	/* We BUG_ON because it's a programmer error to pass a NULL err_ptr,
+	 * and the consequences later is quite hard to detect what the actual
+	 * cause of "wrong memory was mapped in".
+	 */
+	BUG_ON(err_ptr == NULL);
+	return do_remap_mfn(vma, addr, mfn, nr, err_ptr, prot, domid, pages);
+}
+EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_array);
+
+
 /* Returns: 0 success */
 int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
 			       int numpgs, struct page **pages)
@@ -2526,3 +2589,4 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
 #endif
 }
 EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
+
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 59ac71c..b667c99 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -159,6 +159,40 @@ static int traverse_pages(unsigned nelem, size_t size,
 	return ret;
 }
 
+/*
+ * Similar to traverse_pages, but use each page as a "block" of
+ * data to be processed as one unit.
+ */
+static int traverse_pages_block(unsigned nelem, size_t size,
+				struct list_head *pos,
+				int (*fn)(void *data, int nr, void *state),
+				void *state)
+{
+	void *pagedata;
+	unsigned pageidx;
+	int ret = 0;
+
+	BUG_ON(size > PAGE_SIZE);
+
+	pageidx = PAGE_SIZE;
+
+	while (nelem) {
+		int nr = (PAGE_SIZE/size);
+		struct page *page;
+		if (nr > nelem)
+			nr = nelem;
+		pos = pos->next;
+		page = list_entry(pos, struct page, lru);
+		pagedata = page_address(page);
+		ret = (*fn)(pagedata, nr, state);
+		if (ret)
+			break;
+		nelem -= nr;
+	}
+
+	return ret;
+}
+
 struct mmap_mfn_state {
 	unsigned long va;
 	struct vm_area_struct *vma;
@@ -274,39 +308,25 @@ struct mmap_batch_state {
 /* auto translated dom0 note: if domU being created is PV, then mfn is
  * mfn(addr on bus). If it's auto xlated, then mfn is pfn (input to HAP).
  */
-static int mmap_batch_fn(void *data, void *state)
+static int mmap_batch_fn(void *data, int nr, void *state)
 {
 	xen_pfn_t *mfnp = data;
 	struct mmap_batch_state *st = state;
 	struct vm_area_struct *vma = st->vma;
 	struct page **pages = vma->vm_private_data;
-	struct page *cur_page = NULL;
+	struct page **cur_pages = NULL;
 	int ret;
 
 	if (xen_feature(XENFEAT_auto_translated_physmap))
-		cur_page = pages[st->index++];
+		cur_pages = &pages[st->index];
 
-	ret = xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, *mfnp, 1,
-					 st->vma->vm_page_prot, st->domain,
-					 &cur_page);
+	BUG_ON(nr < 0);
+	ret = xen_remap_domain_mfn_array(st->vma, st->va & PAGE_MASK, mfnp, nr,
+					 (int *)mfnp, st->vma->vm_page_prot, 
+					 st->domain, cur_pages);
 
-	/* Store error code for second pass. */
-	if (st->version == 1) {
-		if (ret < 0) {
-			/*
-			 * V1 encodes the error codes in the 32bit top nibble of the
-			 * mfn (with its known limitations vis-a-vis 64 bit callers).
-			 */
-			*mfnp |= (ret == -ENOENT) ?
-						PRIVCMD_MMAPBATCH_PAGED_ERROR :
-						PRIVCMD_MMAPBATCH_MFN_ERROR;
-		}
-	} else { /* st->version == 2 */
-		*((int *) mfnp) = ret;
-	}
-
-	/* And see if it affects the global_error. */
-	if (ret < 0) {
+	/* Adjust the global_error? */
+	if (ret != nr) {
 		if (ret == -ENOENT)
 			st->global_error = -ENOENT;
 		else {
@@ -315,23 +335,35 @@ static int mmap_batch_fn(void *data, void *state)
 				st->global_error = 1;
 		}
 	}
-	st->va += PAGE_SIZE;
+	st->va += PAGE_SIZE * nr;
+	st->index += nr;
 
 	return 0;
 }
 
-static int mmap_return_errors(void *data, void *state)
+static int mmap_return_error(int err, struct mmap_batch_state *st)
 {
-	struct mmap_batch_state *st = state;
+	int ret;
 
 	if (st->version == 1) {
-		xen_pfn_t mfnp = *((xen_pfn_t *) data);
-		if (mfnp & PRIVCMD_MMAPBATCH_MFN_ERROR)
-			return __put_user(mfnp, st->user_mfn++);
-		else
+		if (err) {
+			xen_pfn_t mfn;
+
+			ret = __get_user(mfn, st->user_mfn);
+			if (ret < 0)
+				return ret;
+			/*
+			 * V1 encodes the error codes in the 32bit top
+			 * nibble of the mfn (with its known
+			 * limitations vis-a-vis 64 bit callers).
+			 */
+			mfn |= (ret == -ENOENT) ?
+				PRIVCMD_MMAPBATCH_PAGED_ERROR :
+				PRIVCMD_MMAPBATCH_MFN_ERROR;
+			return __put_user(mfn, st->user_mfn++);
+		} else
 			st->user_mfn++;
 	} else { /* st->version == 2 */
-		int err = *((int *) data);
 		if (err)
 			return __put_user(err, st->user_err++);
 		else
@@ -341,6 +373,21 @@ static int mmap_return_errors(void *data, void *state)
 	return 0;
 }
 
+static int mmap_return_errors(void *data, int nr, void *state)
+{
+	struct mmap_batch_state *st = state;
+	int *errs = data;
+	int i;
+	int ret;
+
+	for (i = 0; i < nr; i++) {
+		ret = mmap_return_error(errs[i], st);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 /* Allocate pfns that are then mapped with gmfns from foreign domid. Update
  * the vma with the page info to use later.
  * Returns: 0 if success, otherwise -errno
@@ -472,8 +519,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 	state.version       = version;
 
 	/* mmap_batch_fn guarantees ret == 0 */
-	BUG_ON(traverse_pages(m.num, sizeof(xen_pfn_t),
-			     &pagelist, mmap_batch_fn, &state));
+	BUG_ON(traverse_pages_block(m.num, sizeof(xen_pfn_t),
+				    &pagelist, mmap_batch_fn, &state));
 
 	up_write(&mm->mmap_sem);
 
@@ -481,8 +528,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 		/* Write back errors in second pass. */
 		state.user_mfn = (xen_pfn_t *)m.arr;
 		state.user_err = m.err;
-		ret = traverse_pages(m.num, sizeof(xen_pfn_t),
-							 &pagelist, mmap_return_errors, &state);
+		ret = traverse_pages_block(m.num, sizeof(xen_pfn_t),
+					   &pagelist, mmap_return_errors, &state);
 	} else
 		ret = 0;
 
diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
index 7724d90..581db0f 100644
--- a/drivers/xen/xlate_mmu.c
+++ b/drivers/xen/xlate_mmu.c
@@ -62,13 +62,15 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
 }
 
 struct remap_data {
-	xen_pfn_t fgmfn; /* foreign domain's gmfn */
+	xen_pfn_t *fgmfn; /* foreign domain's gmfn */
 	pgprot_t prot;
 	domid_t  domid;
 	struct vm_area_struct *vma;
 	int index;
 	struct page **pages;
 	struct xen_remap_mfn_info *info;
+	int *err_ptr;
+	int mapped;
 };
 
 static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
@@ -80,38 +82,46 @@ static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
 	pte_t pte = pte_mkspecial(pfn_pte(pfn, info->prot));
 	int rc;
 
-	rc = map_foreign_page(pfn, info->fgmfn, info->domid);
-	if (rc < 0)
-		return rc;
-	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
+	rc = map_foreign_page(pfn, *info->fgmfn, info->domid);
+	*info->err_ptr++ = rc;
+	if (!rc) {
+		set_pte_at(info->vma->vm_mm, addr, ptep, pte);
+		info->mapped++;
+	}
+	info->fgmfn++;
 
 	return 0;
 }
 
-int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
+int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
 			      unsigned long addr,
-			      xen_pfn_t gfn, int nr,
-			      pgprot_t prot, unsigned domid,
+			      xen_pfn_t *mfn, int nr,
+			      int *err_ptr, pgprot_t prot,
+			      unsigned domid,
 			      struct page **pages)
 {
 	int err;
 	struct remap_data data;
-
-	/* TBD: Batching, current sole caller only does page at a time */
-	if (nr > 1)
-		return -EINVAL;
-
-	data.fgmfn = gfn;
-	data.prot = prot;
+	unsigned long range = nr << PAGE_SHIFT;
+ 
+	/* Kept here for the purpose of making sure code doesn't break
+	   x86 PVOPS */
+	BUG_ON(!((vma->vm_flags & (VM_PFNMAP | VM_IO)) == (VM_PFNMAP | VM_IO)));
+ 
+	data.fgmfn = mfn;
+	data.prot  = prot;
 	data.domid = domid;
-	data.vma = vma;
-	data.index = 0;
+	data.vma   = vma;
 	data.pages = pages;
-	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
+	data.index = 0;
+	data.err_ptr = err_ptr;
+	data.mapped = 0;
+
+	err = apply_to_page_range(vma->vm_mm, addr, range,
 				  remap_pte_fn, &data);
-	return err;
+	return err < 0 ? err : data.mapped;
 }
-EXPORT_SYMBOL_GPL(xen_xlate_remap_gfn_range);
+EXPORT_SYMBOL_GPL(xen_xlate_remap_gfn_array);
 
 int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
 			      int nr, struct page **pages)
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index ba7d232..225a2ff 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -27,19 +27,56 @@ int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
 void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order);
 
 struct vm_area_struct;
+
+/*
+ * xen_remap_domain_mfn_array() - map an array of foreign frames
+ * @vma:     VMA to map the pages into
+ * @addr:    Address at which to map the pages
+ * @gfn:     Array of GFNs to map
+ * @nr:      Number entries in the GFN array
+ * @err_ptr: Returns per-GFN error status.
+ * @prot:    page protection mask
+ * @domid:   Domain owning the pages
+ * @pages:   Array of pages if this domain has an auto-translated physmap
+ *
+ * @gfn and @err_ptr may point to the same buffer, the GFNs will be
+ * overwritten by the error codes after they are mapped.
+ *
+ * Returns the number of successfully mapped frames, or a -ve error
+ * code.
+ */
+int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
+			       unsigned long addr,
+			       xen_pfn_t *gfn, int nr,
+			       int *err_ptr, pgprot_t prot,
+			       unsigned domid, 
+			       struct page **pages);
+
+/* xen_remap_domain_mfn_range() - map a range of foreign frames
+ * @vma:     VMA to map the pages into
+ * @addr:    Address at which to map the pages
+ * @gfn:     First GFN to map.
+ * @nr:      Number frames to map
+ * @prot:    page protection mask
+ * @domid:   Domain owning the pages
+ * @pages:   Array of pages if this domain has an auto-translated physmap
+ *
+ * Returns the number of successfully mapped frames, or a -ve error
+ * code.
+ */
 int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 			       unsigned long addr,
-			       xen_pfn_t mfn, int nr,
+			       xen_pfn_t gfn, int nr,
 			       pgprot_t prot, unsigned domid,
 			       struct page **pages);
 int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
 			       int numpgs, struct page **pages);
 #ifdef CONFIG_XEN_AUTO_XLATE
-int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
+int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
 			      unsigned long addr,
-			      xen_pfn_t gfn, int nr,
-			      pgprot_t prot,
-			      unsigned domid, 
+			      xen_pfn_t *gfn, int nr,
+			      int *err_ptr, pgprot_t prot,
+			      unsigned domid,
 			      struct page **pages);
 int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
 			      int nr, struct page **pages);
-- 
1.7.10.4

Re: [PATCHv6 3/3] xen/privcmd: improve performance of MMAPBATCH_V2

[Stefano Stabellini]

On Fri, 6 Mar 2015, David Vrabel wrote:
> Make the IOCTL_PRIVCMD_MMAPBATCH_V2 (and older V1 version) map
> multiple frames at a time rather than one at a time, despite the pages
> being non-consecutive GFNs.
> 
> xen_remap_foreign_mfn_array() is added which maps an array of GFNs
> (instead of a consecutive range of GFNs).
> 
> Migrate times are significantly improved (when using a PV toolstack
> domain).  For example, for an idle 12 GiB PV guest:
> 
>         Before     After
>   real  0m38.179s  0m26.868s
>   user  0m15.096s  0m13.652s
>   sys   0m28.988s  0m18.732s
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Nice!


>  arch/arm/xen/enlighten.c |   20 ++++++--
>  arch/x86/xen/mmu.c       |  102 ++++++++++++++++++++++++++++++++--------
>  drivers/xen/privcmd.c    |  117 ++++++++++++++++++++++++++++++++--------------
>  drivers/xen/xlate_mmu.c  |   50 ++++++++++++--------
>  include/xen/xen-ops.h    |   47 +++++++++++++++++--
>  5 files changed, 253 insertions(+), 83 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 5c04389..9929cb6 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -53,15 +53,27 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
>  
>  static __read_mostly int xen_events_irq = -1;
>  
> -int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> +int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
>  			       unsigned long addr,
> -			       xen_pfn_t mfn, int nr,
> -			       pgprot_t prot, unsigned domid,
> +			       xen_pfn_t *mfn, int nr,
> +			       int *err_ptr, pgprot_t prot,
> +			       unsigned domid,
>  			       struct page **pages)
>  {
> -	return xen_xlate_remap_gfn_range(vma, addr, mfn, nr,
> +	return xen_xlate_remap_gfn_array(vma, addr, mfn, nr, err_ptr,
>  					 prot, domid, pages);
>  }
> +EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_array);
> +
> +/* Not used in ARM. Use xen_remap_domain_mfn_array(). */
> +int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> +                              unsigned long addr,
> +                              xen_pfn_t mfn, int nr,
> +                              pgprot_t prot, unsigned domid,
> +                              struct page **pages)
> +{
> +	return -ENOSYS;
> +}
>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);

I understand that it is not used, but since you have already introduced
xen_remap_domain_mfn_array, you might as well implement
xen_remap_domain_mfn_range by calling xen_remap_domain_mfn_array or
xen_xlate_remap_gfn_array.


>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index 3d536a5..78d352f 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -2439,7 +2439,8 @@ void __init xen_hvm_init_mmu_ops(void)
>  #define REMAP_BATCH_SIZE 16
>  
>  struct remap_data {
> -	unsigned long mfn;
> +	xen_pfn_t *mfn;
> +	bool contiguous;
>  	pgprot_t prot;
>  	struct mmu_update *mmu_update;
>  };
> @@ -2448,7 +2449,14 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, pgtable_t token,
>  				 unsigned long addr, void *data)
>  {
>  	struct remap_data *rmd = data;
> -	pte_t pte = pte_mkspecial(mfn_pte(rmd->mfn++, rmd->prot));
> +	pte_t pte = pte_mkspecial(mfn_pte(*rmd->mfn, rmd->prot));
> +
> +	/* If we have a contigious range, just update the mfn itself,
> +	   else update pointer to be "next mfn". */
> +	if (rmd->contiguous)
> +		(*rmd->mfn)++;
> +	else
> +		rmd->mfn++;
>  
>  	rmd->mmu_update->ptr = virt_to_machine(ptep).maddr;
>  	rmd->mmu_update->val = pte_val_ma(pte);
> @@ -2457,26 +2465,26 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, pgtable_t token,
>  	return 0;
>  }
>  
> -int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> -			       unsigned long addr,
> -			       xen_pfn_t mfn, int nr,
> -			       pgprot_t prot, unsigned domid,
> -			       struct page **pages)
> -
> +static int do_remap_mfn(struct vm_area_struct *vma,
> +			unsigned long addr,
> +			xen_pfn_t *mfn, int nr,
> +			int *err_ptr, pgprot_t prot,
> +			unsigned domid,
> +			struct page **pages)
>  {
> +	int err = 0;
>  	struct remap_data rmd;
>  	struct mmu_update mmu_update[REMAP_BATCH_SIZE];
> -	int batch;
>  	unsigned long range;
> -	int err = 0;
> +	int mapped = 0;
>  
>  	BUG_ON(!((vma->vm_flags & (VM_PFNMAP | VM_IO)) == (VM_PFNMAP | VM_IO)));
>  
>  	if (xen_feature(XENFEAT_auto_translated_physmap)) {
>  #ifdef CONFIG_XEN_PVH
>  		/* We need to update the local page tables and the xen HAP */
> -		return xen_xlate_remap_gfn_range(vma, addr, mfn, nr, prot,
> -						 domid, pages);
> +		return xen_xlate_remap_gfn_array(vma, addr, mfn, nr, err_ptr,
> +						 prot, domid, pages);
>  #else
>  		return -EINVAL;
>  #endif
> @@ -2484,9 +2492,15 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  
>  	rmd.mfn = mfn;
>  	rmd.prot = prot;
> +	/* We use the err_ptr to indicate if there we are doing a contigious
> +	 * mapping or a discontigious mapping. */
> +	rmd.contiguous = !err_ptr;
>  
>  	while (nr) {
> -		batch = min(REMAP_BATCH_SIZE, nr);
> +		int index = 0;
> +		int done = 0;
> +		int batch = min(REMAP_BATCH_SIZE, nr);
> +		int batch_left = batch;
>  		range = (unsigned long)batch << PAGE_SHIFT;
>  
>  		rmd.mmu_update = mmu_update;
> @@ -2495,23 +2509,72 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  		if (err)
>  			goto out;
>  
> -		err = HYPERVISOR_mmu_update(mmu_update, batch, NULL, domid);
> -		if (err < 0)
> -			goto out;
> +		/* We record the error for each page that gives an error, but
> +		 * continue mapping until the whole set is done */
> +		do {
> +			int i;
> +
> +			err = HYPERVISOR_mmu_update(&mmu_update[index],
> +						    batch_left, &done, domid);
> +
> +			/*
> +			 * @err_ptr may be the same buffer as @mfn, so
> +			 * only clear it after each chunk of @mfn is
> +			 * used.
> +			 */
> +			if (err_ptr) {
> +				for (i = index; i < index + done; i++)
> +					err_ptr[i] = 0;
> +			}
> +			if (err < 0) {
> +				if (!err_ptr)
> +					goto out;
> +				err_ptr[i] = err;
> +				done++; /* Skip failed frame. */
> +			} else
> +				mapped += done;
> +			batch_left -= done;
> +			index += done;
> +		} while (batch_left);
>  
>  		nr -= batch;
>  		addr += range;
> +		if (err_ptr)
> +			err_ptr += batch;
>  	}
> -
> -	err = 0;
>  out:
>  
>  	xen_flush_tlb_all();
>  
> -	return err;
> +	return err < 0 ? err : mapped;
> +}
> +
> +int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> +			       unsigned long addr,
> +			       xen_pfn_t mfn, int nr,
> +			       pgprot_t prot, unsigned domid,
> +			       struct page **pages)
> +{
> +	return do_remap_mfn(vma, addr, &mfn, nr, NULL, prot, domid, pages);
>  }
>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>  
> +int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
> +			       unsigned long addr,
> +			       xen_pfn_t *mfn, int nr,
> +			       int *err_ptr, pgprot_t prot,
> +			       unsigned domid, struct page **pages)
> +{
> +	/* We BUG_ON because it's a programmer error to pass a NULL err_ptr,
> +	 * and the consequences later is quite hard to detect what the actual
> +	 * cause of "wrong memory was mapped in".
> +	 */
> +	BUG_ON(err_ptr == NULL);
> +	return do_remap_mfn(vma, addr, mfn, nr, err_ptr, prot, domid, pages);
> +}
> +EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_array);
> +
> +
>  /* Returns: 0 success */
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  			       int numpgs, struct page **pages)
> @@ -2526,3 +2589,4 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  #endif
>  }
>  EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> +
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 59ac71c..b667c99 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -159,6 +159,40 @@ static int traverse_pages(unsigned nelem, size_t size,
>  	return ret;
>  }
>  
> +/*
> + * Similar to traverse_pages, but use each page as a "block" of
> + * data to be processed as one unit.
> + */
> +static int traverse_pages_block(unsigned nelem, size_t size,
> +				struct list_head *pos,
> +				int (*fn)(void *data, int nr, void *state),
> +				void *state)
> +{
> +	void *pagedata;
> +	unsigned pageidx;
> +	int ret = 0;
> +
> +	BUG_ON(size > PAGE_SIZE);

I looks like that PAGE_SIZE needs to be a multiple of size. Maybe we can
add a BUG_ON for that too.


> +	pageidx = PAGE_SIZE;
> +
> +	while (nelem) {
> +		int nr = (PAGE_SIZE/size);
> +		struct page *page;
> +		if (nr > nelem)
> +			nr = nelem;
> +		pos = pos->next;
> +		page = list_entry(pos, struct page, lru);
> +		pagedata = page_address(page);
> +		ret = (*fn)(pagedata, nr, state);
> +		if (ret)
> +			break;
> +		nelem -= nr;
> +	}
> +
> +	return ret;
> +}
> +
>  struct mmap_mfn_state {
>  	unsigned long va;
>  	struct vm_area_struct *vma;
> @@ -274,39 +308,25 @@ struct mmap_batch_state {
>  /* auto translated dom0 note: if domU being created is PV, then mfn is
>   * mfn(addr on bus). If it's auto xlated, then mfn is pfn (input to HAP).
>   */
> -static int mmap_batch_fn(void *data, void *state)
> +static int mmap_batch_fn(void *data, int nr, void *state)
>  {
>  	xen_pfn_t *mfnp = data;
>  	struct mmap_batch_state *st = state;
>  	struct vm_area_struct *vma = st->vma;
>  	struct page **pages = vma->vm_private_data;
> -	struct page *cur_page = NULL;
> +	struct page **cur_pages = NULL;
>  	int ret;
>  
>  	if (xen_feature(XENFEAT_auto_translated_physmap))
> -		cur_page = pages[st->index++];
> +		cur_pages = &pages[st->index];
>  
> -	ret = xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, *mfnp, 1,
> -					 st->vma->vm_page_prot, st->domain,
> -					 &cur_page);
> +	BUG_ON(nr < 0);
> +	ret = xen_remap_domain_mfn_array(st->vma, st->va & PAGE_MASK, mfnp, nr,
> +					 (int *)mfnp, st->vma->vm_page_prot, 
> +					 st->domain, cur_pages);
>  
> -	/* Store error code for second pass. */
> -	if (st->version == 1) {
> -		if (ret < 0) {
> -			/*
> -			 * V1 encodes the error codes in the 32bit top nibble of the
> -			 * mfn (with its known limitations vis-a-vis 64 bit callers).
> -			 */
> -			*mfnp |= (ret == -ENOENT) ?
> -						PRIVCMD_MMAPBATCH_PAGED_ERROR :
> -						PRIVCMD_MMAPBATCH_MFN_ERROR;
> -		}
> -	} else { /* st->version == 2 */
> -		*((int *) mfnp) = ret;
> -	}
> -
> -	/* And see if it affects the global_error. */
> -	if (ret < 0) {
> +	/* Adjust the global_error? */
> +	if (ret != nr) {
>  		if (ret == -ENOENT)
>  			st->global_error = -ENOENT;
>  		else {
> @@ -315,23 +335,35 @@ static int mmap_batch_fn(void *data, void *state)
>  				st->global_error = 1;
>  		}
>  	}
> -	st->va += PAGE_SIZE;
> +	st->va += PAGE_SIZE * nr;
> +	st->index += nr;
>  
>  	return 0;
>  }
>  
> -static int mmap_return_errors(void *data, void *state)
> +static int mmap_return_error(int err, struct mmap_batch_state *st)
>  {
> -	struct mmap_batch_state *st = state;
> +	int ret;
>  
>  	if (st->version == 1) {
> -		xen_pfn_t mfnp = *((xen_pfn_t *) data);
> -		if (mfnp & PRIVCMD_MMAPBATCH_MFN_ERROR)
> -			return __put_user(mfnp, st->user_mfn++);
> -		else
> +		if (err) {
> +			xen_pfn_t mfn;
> +
> +			ret = __get_user(mfn, st->user_mfn);
> +			if (ret < 0)
> +				return ret;
> +			/*
> +			 * V1 encodes the error codes in the 32bit top
> +			 * nibble of the mfn (with its known
> +			 * limitations vis-a-vis 64 bit callers).
> +			 */
> +			mfn |= (ret == -ENOENT) ?
> +				PRIVCMD_MMAPBATCH_PAGED_ERROR :
> +				PRIVCMD_MMAPBATCH_MFN_ERROR;
> +			return __put_user(mfn, st->user_mfn++);
> +		} else
>  			st->user_mfn++;

Instead of having to resort to __get_user, couldn't you examine err and
base you the choice between PRIVCMD_MMAPBATCH_MFN_ERROR and
PRIVCMD_MMAPBATCH_PAGED_ERROR on it, similarly to what we did before?

Also I think you should spend a few more words in the commit message
regarding the changes you are making to the error reporting path.

>  	} else { /* st->version == 2 */
> -		int err = *((int *) data);
>  		if (err)
>  			return __put_user(err, st->user_err++);
>  		else
> @@ -341,6 +373,21 @@ static int mmap_return_errors(void *data, void *state)
>  	return 0;
>  }
>  
> +static int mmap_return_errors(void *data, int nr, void *state)
> +{
> +	struct mmap_batch_state *st = state;
> +	int *errs = data;
> +	int i;
> +	int ret;
> +
> +	for (i = 0; i < nr; i++) {
> +		ret = mmap_return_error(errs[i], st);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
>
>  /* Allocate pfns that are then mapped with gmfns from foreign domid. Update
>   * the vma with the page info to use later.
>   * Returns: 0 if success, otherwise -errno
> @@ -472,8 +519,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>  	state.version       = version;
>  
>  	/* mmap_batch_fn guarantees ret == 0 */
> -	BUG_ON(traverse_pages(m.num, sizeof(xen_pfn_t),
> -			     &pagelist, mmap_batch_fn, &state));
> +	BUG_ON(traverse_pages_block(m.num, sizeof(xen_pfn_t),
> +				    &pagelist, mmap_batch_fn, &state));
>  
>  	up_write(&mm->mmap_sem);
>  
> @@ -481,8 +528,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>  		/* Write back errors in second pass. */
>  		state.user_mfn = (xen_pfn_t *)m.arr;
>  		state.user_err = m.err;
> -		ret = traverse_pages(m.num, sizeof(xen_pfn_t),
> -							 &pagelist, mmap_return_errors, &state);
> +		ret = traverse_pages_block(m.num, sizeof(xen_pfn_t),
> +					   &pagelist, mmap_return_errors, &state);
>  	} else
>  		ret = 0;
>  
> diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
> index 7724d90..581db0f 100644
> --- a/drivers/xen/xlate_mmu.c
> +++ b/drivers/xen/xlate_mmu.c
> @@ -62,13 +62,15 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
>  }
>  
>  struct remap_data {
> -	xen_pfn_t fgmfn; /* foreign domain's gmfn */
> +	xen_pfn_t *fgmfn; /* foreign domain's gmfn */
>  	pgprot_t prot;
>  	domid_t  domid;
>  	struct vm_area_struct *vma;
>  	int index;
>  	struct page **pages;
>  	struct xen_remap_mfn_info *info;
> +	int *err_ptr;
> +	int mapped;
>  };
>  
>  static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> @@ -80,38 +82,46 @@ static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
>  	pte_t pte = pte_mkspecial(pfn_pte(pfn, info->prot));
>  	int rc;
>  
> -	rc = map_foreign_page(pfn, info->fgmfn, info->domid);
> -	if (rc < 0)
> -		return rc;
> -	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> +	rc = map_foreign_page(pfn, *info->fgmfn, info->domid);
> +	*info->err_ptr++ = rc;
> +	if (!rc) {
> +		set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> +		info->mapped++;
> +	}
> +	info->fgmfn++;
>  
>  	return 0;
>  }
>  
> -int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
> +int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
>  			      unsigned long addr,
> -			      xen_pfn_t gfn, int nr,
> -			      pgprot_t prot, unsigned domid,
> +			      xen_pfn_t *mfn, int nr,
> +			      int *err_ptr, pgprot_t prot,
> +			      unsigned domid,
>  			      struct page **pages)
>  {
>  	int err;
>  	struct remap_data data;
> -
> -	/* TBD: Batching, current sole caller only does page at a time */
> -	if (nr > 1)
> -		return -EINVAL;
> -
> -	data.fgmfn = gfn;
> -	data.prot = prot;
> +	unsigned long range = nr << PAGE_SHIFT;
> + 
> +	/* Kept here for the purpose of making sure code doesn't break
> +	   x86 PVOPS */
> +	BUG_ON(!((vma->vm_flags & (VM_PFNMAP | VM_IO)) == (VM_PFNMAP | VM_IO)));
> + 
> +	data.fgmfn = mfn;
> +	data.prot  = prot;
>  	data.domid = domid;
> -	data.vma = vma;
> -	data.index = 0;
> +	data.vma   = vma;
>  	data.pages = pages;
> -	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> +	data.index = 0;
> +	data.err_ptr = err_ptr;
> +	data.mapped = 0;
> +
> +	err = apply_to_page_range(vma->vm_mm, addr, range,
>  				  remap_pte_fn, &data);
> -	return err;
> +	return err < 0 ? err : data.mapped;
>  }
> -EXPORT_SYMBOL_GPL(xen_xlate_remap_gfn_range);
> +EXPORT_SYMBOL_GPL(xen_xlate_remap_gfn_array);
>  
>  int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
>  			      int nr, struct page **pages)
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index ba7d232..225a2ff 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -27,19 +27,56 @@ int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
>  void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order);
>  
>  struct vm_area_struct;
> +
> +/*
> + * xen_remap_domain_mfn_array() - map an array of foreign frames
> + * @vma:     VMA to map the pages into
> + * @addr:    Address at which to map the pages
> + * @gfn:     Array of GFNs to map
> + * @nr:      Number entries in the GFN array
> + * @err_ptr: Returns per-GFN error status.
> + * @prot:    page protection mask
> + * @domid:   Domain owning the pages
> + * @pages:   Array of pages if this domain has an auto-translated physmap
> + *
> + * @gfn and @err_ptr may point to the same buffer, the GFNs will be
> + * overwritten by the error codes after they are mapped.
> + *
> + * Returns the number of successfully mapped frames, or a -ve error
> + * code.
> + */
> +int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
> +			       unsigned long addr,
> +			       xen_pfn_t *gfn, int nr,
> +			       int *err_ptr, pgprot_t prot,
> +			       unsigned domid, 
> +			       struct page **pages);
> +
> +/* xen_remap_domain_mfn_range() - map a range of foreign frames
> + * @vma:     VMA to map the pages into
> + * @addr:    Address at which to map the pages
> + * @gfn:     First GFN to map.
> + * @nr:      Number frames to map
> + * @prot:    page protection mask
> + * @domid:   Domain owning the pages
> + * @pages:   Array of pages if this domain has an auto-translated physmap
> + *
> + * Returns the number of successfully mapped frames, or a -ve error
> + * code.
> + */
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
> -			       xen_pfn_t mfn, int nr,
> +			       xen_pfn_t gfn, int nr,
>  			       pgprot_t prot, unsigned domid,
>  			       struct page **pages);
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>  			       int numpgs, struct page **pages);
>  #ifdef CONFIG_XEN_AUTO_XLATE
> -int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
> +int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
>  			      unsigned long addr,
> -			      xen_pfn_t gfn, int nr,
> -			      pgprot_t prot,
> -			      unsigned domid, 
> +			      xen_pfn_t *gfn, int nr,
> +			      int *err_ptr, pgprot_t prot,
> +			      unsigned domid,
>  			      struct page **pages);
>  int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
>  			      int nr, struct page **pages);

Since we are introducing a new internal interface, shouldn't we try to
aim for something more generic, maybe like a scatter gather list?
We had one consecutive range and now we also have a list of pages. It
makes sense to jump straight into a list of ranges, right?

Re: [PATCHv6 3/3] xen/privcmd: improve performance of MMAPBATCH_V2

[David Vrabel]

On 09/03/15 12:25, Stefano Stabellini wrote:
> On Fri, 6 Mar 2015, David Vrabel wrote:
>> Make the IOCTL_PRIVCMD_MMAPBATCH_V2 (and older V1 version) map
>> multiple frames at a time rather than one at a time, despite the pages
>> being non-consecutive GFNs.
>>
>> xen_remap_foreign_mfn_array() is added which maps an array of GFNs
>> (instead of a consecutive range of GFNs).
>>
>> Migrate times are significantly improved (when using a PV toolstack
>> domain).  For example, for an idle 12 GiB PV guest:
>>
>>         Before     After
>>   real  0m38.179s  0m26.868s
>>   user  0m15.096s  0m13.652s
>>   sys   0m28.988s  0m18.732s
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> Nice!

Note that is is for PV toolstack domains only.  Someone else would need
to implement the hypercall batching for auto-xlated physmap guests.

>> +/* Not used in ARM. Use xen_remap_domain_mfn_array(). */
>> +int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>> +                              unsigned long addr,
>> +                              xen_pfn_t mfn, int nr,
>> +                              pgprot_t prot, unsigned domid,
>> +                              struct page **pages)
>> +{
>> +	return -ENOSYS;
>> +}
>>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> 
> I understand that it is not used, but since you have already introduced
> xen_remap_domain_mfn_array, you might as well implement
> xen_remap_domain_mfn_range by calling xen_remap_domain_mfn_array or
> xen_xlate_remap_gfn_array.

I should I spend time implementing something that isn't used?

>> +			ret = __get_user(mfn, st->user_mfn);
>> +			if (ret < 0)
>> +				return ret;
>> +			/*
>> +			 * V1 encodes the error codes in the 32bit top
>> +			 * nibble of the mfn (with its known
>> +			 * limitations vis-a-vis 64 bit callers).
>> +			 */
>> +			mfn |= (ret == -ENOENT) ?
>> +				PRIVCMD_MMAPBATCH_PAGED_ERROR :
>> +				PRIVCMD_MMAPBATCH_MFN_ERROR;
>> +			return __put_user(mfn, st->user_mfn++);
>> +		} else
>>  			st->user_mfn++;
> 
> Instead of having to resort to __get_user, couldn't you examine err and
> base you the choice between PRIVCMD_MMAPBATCH_MFN_ERROR and
> PRIVCMD_MMAPBATCH_PAGED_ERROR on it, similarly to what we did before?

The get_user() is required because we're or'ing in bits into the
original user buffer.

However, I can see where your confusion comes from because it should be:

   mfn |= (err == -ENOENT) ? ...

> Also I think you should spend a few more words in the commit message
> regarding the changes you are making to the error reporting path.

It's only moving around a bit.  I'm not sure what you want to be
specifically called out here.

>>  #ifdef CONFIG_XEN_AUTO_XLATE
>> -int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
>> +int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
>>  			      unsigned long addr,
>> -			      xen_pfn_t gfn, int nr,
>> -			      pgprot_t prot,
>> -			      unsigned domid, 
>> +			      xen_pfn_t *gfn, int nr,
>> +			      int *err_ptr, pgprot_t prot,
>> +			      unsigned domid,
>>  			      struct page **pages);
>>  int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
>>  			      int nr, struct page **pages);
> 
> Since we are introducing a new internal interface, shouldn't we try to
> aim for something more generic, maybe like a scatter gather list?
> We had one consecutive range and now we also have a list of pages. It
> makes sense to jump straight into a list of ranges, right?

Again, I don't see why I should have to spend time of an even more
generic interface we don't need.

David

Re: [PATCHv6 3/3] xen/privcmd: improve performance of MMAPBATCH_V2

[Stefano Stabellini]

On Mon, 9 Mar 2015, David Vrabel wrote:
> On 09/03/15 12:25, Stefano Stabellini wrote:
> > On Fri, 6 Mar 2015, David Vrabel wrote:
> >> Make the IOCTL_PRIVCMD_MMAPBATCH_V2 (and older V1 version) map
> >> multiple frames at a time rather than one at a time, despite the pages
> >> being non-consecutive GFNs.
> >>
> >> xen_remap_foreign_mfn_array() is added which maps an array of GFNs
> >> (instead of a consecutive range of GFNs).
> >>
> >> Migrate times are significantly improved (when using a PV toolstack
> >> domain).  For example, for an idle 12 GiB PV guest:
> >>
> >>         Before     After
> >>   real  0m38.179s  0m26.868s
> >>   user  0m15.096s  0m13.652s
> >>   sys   0m28.988s  0m18.732s
> >>
> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> > 
> > Nice!
> 
> Note that is is for PV toolstack domains only.  Someone else would need
> to implement the hypercall batching for auto-xlated physmap guests.
> 
> >> +/* Not used in ARM. Use xen_remap_domain_mfn_array(). */
> >> +int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> >> +                              unsigned long addr,
> >> +                              xen_pfn_t mfn, int nr,
> >> +                              pgprot_t prot, unsigned domid,
> >> +                              struct page **pages)
> >> +{
> >> +	return -ENOSYS;
> >> +}
> >>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> > 
> > I understand that it is not used, but since you have already introduced
> > xen_remap_domain_mfn_array, you might as well implement
> > xen_remap_domain_mfn_range by calling xen_remap_domain_mfn_array or
> > xen_xlate_remap_gfn_array.
> 
> I should I spend time implementing something that isn't used?

In that case, please change the comment to:

/* Not used by XENFEAT_auto_translated guests. */


> >> +			ret = __get_user(mfn, st->user_mfn);
> >> +			if (ret < 0)
> >> +				return ret;
> >> +			/*
> >> +			 * V1 encodes the error codes in the 32bit top
> >> +			 * nibble of the mfn (with its known
> >> +			 * limitations vis-a-vis 64 bit callers).
> >> +			 */
> >> +			mfn |= (ret == -ENOENT) ?
> >> +				PRIVCMD_MMAPBATCH_PAGED_ERROR :
> >> +				PRIVCMD_MMAPBATCH_MFN_ERROR;
> >> +			return __put_user(mfn, st->user_mfn++);
> >> +		} else
> >>  			st->user_mfn++;
> > 
> > Instead of having to resort to __get_user, couldn't you examine err and
> > base you the choice between PRIVCMD_MMAPBATCH_MFN_ERROR and
> > PRIVCMD_MMAPBATCH_PAGED_ERROR on it, similarly to what we did before?
> 
> The get_user() is required because we're or'ing in bits into the
> original user buffer.
> 
> However, I can see where your confusion comes from because it should be:
> 
>    mfn |= (err == -ENOENT) ? ...

Ah! I see now!


> > Also I think you should spend a few more words in the commit message
> > regarding the changes you are making to the error reporting path.
> 
> It's only moving around a bit.  I'm not sure what you want to be
> specifically called out here.

If it was just code movement, I wouldn't have to ask :-)
The additional get_user call for example would be something to write
down.


> >>  #ifdef CONFIG_XEN_AUTO_XLATE
> >> -int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
> >> +int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
> >>  			      unsigned long addr,
> >> -			      xen_pfn_t gfn, int nr,
> >> -			      pgprot_t prot,
> >> -			      unsigned domid, 
> >> +			      xen_pfn_t *gfn, int nr,
> >> +			      int *err_ptr, pgprot_t prot,
> >> +			      unsigned domid,
> >>  			      struct page **pages);
> >>  int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
> >>  			      int nr, struct page **pages);
> > 
> > Since we are introducing a new internal interface, shouldn't we try to
> > aim for something more generic, maybe like a scatter gather list?
> > We had one consecutive range and now we also have a list of pages. It
> > makes sense to jump straight into a list of ranges, right?
> 
> Again, I don't see why I should have to spend time of an even more
> generic interface we don't need.

Switching from a single range to a list of pages without considering to
jump directly to a list of ranges seems a bit short sighted to me.
Changing internal interfaces can be painful and since we are going
through that pain now, we might as well do it right.

That said, I am going to leave this with the other maintainers.

Re: [PATCHv6 3/3] xen/privcmd: improve performance of MMAPBATCH_V2

[David Vrabel]

On 09/03/15 12:25, Stefano Stabellini wrote:
> On Fri, 6 Mar 2015, David Vrabel wrote:
>> 
>> +/*
>> + * Similar to traverse_pages, but use each page as a "block" of
>> + * data to be processed as one unit.
>> + */
>> +static int traverse_pages_block(unsigned nelem, size_t size,
>> +				struct list_head *pos,
>> +				int (*fn)(void *data, int nr, void *state),
>> +				void *state)
>> +{
>> +	void *pagedata;
>> +	unsigned pageidx;
>> +	int ret = 0;
>> +
>> +	BUG_ON(size > PAGE_SIZE);
> 
> I looks like that PAGE_SIZE needs to be a multiple of size. Maybe we can
> add a BUG_ON for that too.

There is no such requirement because...

>> +	pageidx = PAGE_SIZE;
>> +
>> +	while (nelem) {
>> +		int nr = (PAGE_SIZE/size);

...the number of elements per pages is rounded down here.

>> +		struct page *page;
>> +		if (nr > nelem)
>> +			nr = nelem;
>> +		pos = pos->next;
>> +		page = list_entry(pos, struct page, lru);
>> +		pagedata = page_address(page);
>> +		ret = (*fn)(pagedata, nr, state);
>> +		if (ret)
>> +			break;
>> +		nelem -= nr;
>> +	}
>> +
>> +	return ret;
>> +}

David