[PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[David Vrabel]

This set of pages avoids the need to call vmalloc_sync_all() when
mapping foreign pages.  Two new functions are adding for mapping
foreign pages onto RAM pages instead of vmalloc space.

This does waste a page of RAM for each mapped page.  In the future a
ballooned page could be used instead (once the API for getting a
ballooned page with the right GFP flags is available).

David

[PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[David Vrabel]

This set of pages avoids the need to call vmalloc_sync_all() when
mapping foreign pages.  Two new functions are adding for mapping
foreign pages onto RAM pages instead of vmalloc space.

This does waste a page of RAM for each mapped page.  In the future a
ballooned page could be used instead (once the API for getting a
ballooned page with the right GFP flags is available).

David

[PATCH 1/6] xen: add functions for mapping foreign pages over pages

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Add xenbus_map_ring_page() and xenbus_unmap_ring_page() which map and
unmap foreign pages over a page instead of mapping them into vmalloc
address space.  This avoids having to do an expensive
vmalloc_sync_all().

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/xen/xenbus/xenbus_client.c |   68 ++++++++++++++++++++++++++++++++++++
 include/xen/xenbus.h               |    3 ++
 2 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index cdacf92..504325b 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -32,6 +32,7 @@
 
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/mm.h>
 #include <linux/vmalloc.h>
 #include <asm/xen/hypervisor.h>
 #include <xen/interface/xen.h>
@@ -39,6 +40,7 @@
 #include <xen/events.h>
 #include <xen/grant_table.h>
 #include <xen/xenbus.h>
+#include <xen/page.h>
 
 const char *xenbus_strstate(enum xenbus_state state)
 {
@@ -509,6 +511,44 @@ EXPORT_SYMBOL_GPL(xenbus_map_ring);
 
 
 /**
+ * xenbus_map_ring_page - map a foreign page into a kernel page
+ * @dev: xenbus device
+ * @gnt_ref: grant reference
+ * @page: return the page the foreign page has been mapped to
+ *
+ * Map a foreign page from another domain's grant table into a newly
+ * allocated page in this domain.  The page must be unmapped with
+ * xenbus_unmap_ring_page().
+ *
+ * Returns 0 on success, and -ENOMEM or GNTST_* (see
+ * include/xen/interface/grant_table.h) on error.
+ */
+int xenbus_map_ring_page(struct xenbus_device *dev, int gnt_ref,
+			 struct page **page)
+{
+	struct page *new_page;
+	grant_ref_t handle;
+	int ret;
+
+	new_page = alloc_page(GFP_KERNEL);
+	if (!new_page)
+		return -ENOMEM;
+
+	ret = xenbus_map_ring(dev, gnt_ref, &handle, page_address(new_page));
+	if (ret < 0)
+		goto err;
+
+	new_page->private = handle;
+	*page = new_page;
+	return 0;
+
+err:
+	__free_page(new_page);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(xenbus_map_ring_page);
+
+/**
  * xenbus_unmap_ring_vfree
  * @dev: xenbus device
  * @vaddr: addr to unmap
@@ -593,6 +633,34 @@ int xenbus_unmap_ring(struct xenbus_device *dev,
 }
 EXPORT_SYMBOL_GPL(xenbus_unmap_ring);
 
+/**
+ * xenbus_unmap_ring_page - unmap an foreign page from a kernel page
+ * @dev: xenbus device
+ * @page: page the foreign page was mapped to
+ *
+ * Unmap a foreign page previously mapped with xenbus_map_ring_page().
+ * The page is freed.
+ */
+void xenbus_unmap_ring_page(struct xenbus_device *dev, struct page *page)
+{
+	int ret;
+
+	ret = xenbus_unmap_ring(dev, page->private, page_address(page));
+	if (ret < 0)
+		return;
+
+	/*
+	 * Restore the original PTE of this page before freeing it.
+	 */
+	ret = HYPERVISOR_update_va_mapping(
+		(unsigned long)page_address(page),
+		mfn_pte(get_phys_to_machine(page_to_pfn(page)), PAGE_KERNEL),
+		0);
+	BUG_ON(ret);
+
+	__free_page(page);
+}
+EXPORT_SYMBOL_GPL(xenbus_unmap_ring_page);
 
 /**
  * xenbus_read_driver_state
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index aceeca7..ebde2fd 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -212,10 +212,13 @@ int xenbus_map_ring_valloc(struct xenbus_device *dev,
 			   int gnt_ref, void **vaddr);
 int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
 			   grant_handle_t *handle, void *vaddr);
+int xenbus_map_ring_page(struct xenbus_device *dev, int gnt_ref,
+			 struct page **page);
 
 int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr);
 int xenbus_unmap_ring(struct xenbus_device *dev,
 		      grant_handle_t handle, void *vaddr);
+void xenbus_unmap_ring_page(struct xenbus_device *dev, struct page *page);
 
 int xenbus_alloc_evtchn(struct xenbus_device *dev, int *port);
 int xenbus_bind_evtchn(struct xenbus_device *dev, int remote_port, int *port);
-- 
1.7.2.5

[PATCH 2/6] block: xen-blkback: use API provided by xenbus module to map rings

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

The xenbus module provides xenbus_map_ring_page() and
xenbus_map_ring_page().  Use these to map the ring pages granted by
the frontend.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/block/xen-blkback/common.h |    5 +--
 drivers/block/xen-blkback/xenbus.c |   55 +++++------------------------------
 2 files changed, 9 insertions(+), 51 deletions(-)

diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index 9e40b28..65ef268 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -139,7 +139,7 @@ struct xen_blkif {
 	/* Comms information. */
 	enum blkif_protocol	blk_protocol;
 	union blkif_back_rings	blk_rings;
-	struct vm_struct	*blk_ring_area;
+	struct page		*blk_ring_page;
 	/* The VBD attached to this interface. */
 	struct xen_vbd		vbd;
 	/* Back pointer to the backend_info. */
@@ -163,9 +163,6 @@ struct xen_blkif {
 	int			st_wr_sect;
 
 	wait_queue_head_t	waiting_to_free;
-
-	grant_handle_t		shmem_handle;
-	grant_ref_t		shmem_ref;
 };
 
 
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 3f129b4..7e21ef4 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -120,38 +120,6 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
 	return blkif;
 }
 
-static int map_frontend_page(struct xen_blkif *blkif, unsigned long shared_page)
-{
-	struct gnttab_map_grant_ref op;
-
-	gnttab_set_map_op(&op, (unsigned long)blkif->blk_ring_area->addr,
-			  GNTMAP_host_map, shared_page, blkif->domid);
-
-	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status) {
-		DPRINTK("Grant table operation failure !\n");
-		return op.status;
-	}
-
-	blkif->shmem_ref = shared_page;
-	blkif->shmem_handle = op.handle;
-
-	return 0;
-}
-
-static void unmap_frontend_page(struct xen_blkif *blkif)
-{
-	struct gnttab_unmap_grant_ref op;
-
-	gnttab_set_unmap_op(&op, (unsigned long)blkif->blk_ring_area->addr,
-			    GNTMAP_host_map, blkif->shmem_handle);
-
-	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
-		BUG();
-}
-
 static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page,
 			 unsigned int evtchn)
 {
@@ -161,35 +129,30 @@ static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page,
 	if (blkif->irq)
 		return 0;
 
-	blkif->blk_ring_area = alloc_vm_area(PAGE_SIZE);
-	if (!blkif->blk_ring_area)
-		return -ENOMEM;
-
-	err = map_frontend_page(blkif, shared_page);
-	if (err) {
-		free_vm_area(blkif->blk_ring_area);
+	err = xenbus_map_ring_page(blkif->be->dev, shared_page,
+				   &blkif->blk_ring_page);
+	if (err < 0)
 		return err;
-	}
 
 	switch (blkif->blk_protocol) {
 	case BLKIF_PROTOCOL_NATIVE:
 	{
 		struct blkif_sring *sring;
-		sring = (struct blkif_sring *)blkif->blk_ring_area->addr;
+		sring = page_address(blkif->blk_ring_page);
 		BACK_RING_INIT(&blkif->blk_rings.native, sring, PAGE_SIZE);
 		break;
 	}
 	case BLKIF_PROTOCOL_X86_32:
 	{
 		struct blkif_x86_32_sring *sring_x86_32;
-		sring_x86_32 = (struct blkif_x86_32_sring *)blkif->blk_ring_area->addr;
+		sring_x86_32 = page_address(blkif->blk_ring_page);
 		BACK_RING_INIT(&blkif->blk_rings.x86_32, sring_x86_32, PAGE_SIZE);
 		break;
 	}
 	case BLKIF_PROTOCOL_X86_64:
 	{
 		struct blkif_x86_64_sring *sring_x86_64;
-		sring_x86_64 = (struct blkif_x86_64_sring *)blkif->blk_ring_area->addr;
+		sring_x86_64 = page_address(blkif->blk_ring_page);
 		BACK_RING_INIT(&blkif->blk_rings.x86_64, sring_x86_64, PAGE_SIZE);
 		break;
 	}
@@ -201,8 +164,7 @@ static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page,
 						    xen_blkif_be_int, 0,
 						    "blkif-backend", blkif);
 	if (err < 0) {
-		unmap_frontend_page(blkif);
-		free_vm_area(blkif->blk_ring_area);
+		xenbus_unmap_ring_page(blkif->be->dev, blkif->blk_ring_page);
 		blkif->blk_rings.common.sring = NULL;
 		return err;
 	}
@@ -228,8 +190,7 @@ static void xen_blkif_disconnect(struct xen_blkif *blkif)
 	}
 
 	if (blkif->blk_rings.common.sring) {
-		unmap_frontend_page(blkif);
-		free_vm_area(blkif->blk_ring_area);
+		xenbus_unmap_ring_page(blkif->be->dev, blkif->blk_ring_page);
 		blkif->blk_rings.common.sring = NULL;
 	}
 }
-- 
1.7.2.5

[PATCH 2/6] block: xen-blkback: use API provided by xenbus module to map rings

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

The xenbus module provides xenbus_map_ring_page() and
xenbus_map_ring_page().  Use these to map the ring pages granted by
the frontend.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/block/xen-blkback/common.h |    5 +--
 drivers/block/xen-blkback/xenbus.c |   55 +++++------------------------------
 2 files changed, 9 insertions(+), 51 deletions(-)

diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index 9e40b28..65ef268 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -139,7 +139,7 @@ struct xen_blkif {
 	/* Comms information. */
 	enum blkif_protocol	blk_protocol;
 	union blkif_back_rings	blk_rings;
-	struct vm_struct	*blk_ring_area;
+	struct page		*blk_ring_page;
 	/* The VBD attached to this interface. */
 	struct xen_vbd		vbd;
 	/* Back pointer to the backend_info. */
@@ -163,9 +163,6 @@ struct xen_blkif {
 	int			st_wr_sect;
 
 	wait_queue_head_t	waiting_to_free;
-
-	grant_handle_t		shmem_handle;
-	grant_ref_t		shmem_ref;
 };
 
 
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 3f129b4..7e21ef4 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -120,38 +120,6 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
 	return blkif;
 }
 
-static int map_frontend_page(struct xen_blkif *blkif, unsigned long shared_page)
-{
-	struct gnttab_map_grant_ref op;
-
-	gnttab_set_map_op(&op, (unsigned long)blkif->blk_ring_area->addr,
-			  GNTMAP_host_map, shared_page, blkif->domid);
-
-	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status) {
-		DPRINTK("Grant table operation failure !\n");
-		return op.status;
-	}
-
-	blkif->shmem_ref = shared_page;
-	blkif->shmem_handle = op.handle;
-
-	return 0;
-}
-
-static void unmap_frontend_page(struct xen_blkif *blkif)
-{
-	struct gnttab_unmap_grant_ref op;
-
-	gnttab_set_unmap_op(&op, (unsigned long)blkif->blk_ring_area->addr,
-			    GNTMAP_host_map, blkif->shmem_handle);
-
-	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
-		BUG();
-}
-
 static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page,
 			 unsigned int evtchn)
 {
@@ -161,35 +129,30 @@ static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page,
 	if (blkif->irq)
 		return 0;
 
-	blkif->blk_ring_area = alloc_vm_area(PAGE_SIZE);
-	if (!blkif->blk_ring_area)
-		return -ENOMEM;
-
-	err = map_frontend_page(blkif, shared_page);
-	if (err) {
-		free_vm_area(blkif->blk_ring_area);
+	err = xenbus_map_ring_page(blkif->be->dev, shared_page,
+				   &blkif->blk_ring_page);
+	if (err < 0)
 		return err;
-	}
 
 	switch (blkif->blk_protocol) {
 	case BLKIF_PROTOCOL_NATIVE:
 	{
 		struct blkif_sring *sring;
-		sring = (struct blkif_sring *)blkif->blk_ring_area->addr;
+		sring = page_address(blkif->blk_ring_page);
 		BACK_RING_INIT(&blkif->blk_rings.native, sring, PAGE_SIZE);
 		break;
 	}
 	case BLKIF_PROTOCOL_X86_32:
 	{
 		struct blkif_x86_32_sring *sring_x86_32;
-		sring_x86_32 = (struct blkif_x86_32_sring *)blkif->blk_ring_area->addr;
+		sring_x86_32 = page_address(blkif->blk_ring_page);
 		BACK_RING_INIT(&blkif->blk_rings.x86_32, sring_x86_32, PAGE_SIZE);
 		break;
 	}
 	case BLKIF_PROTOCOL_X86_64:
 	{
 		struct blkif_x86_64_sring *sring_x86_64;
-		sring_x86_64 = (struct blkif_x86_64_sring *)blkif->blk_ring_area->addr;
+		sring_x86_64 = page_address(blkif->blk_ring_page);
 		BACK_RING_INIT(&blkif->blk_rings.x86_64, sring_x86_64, PAGE_SIZE);
 		break;
 	}
@@ -201,8 +164,7 @@ static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page,
 						    xen_blkif_be_int, 0,
 						    "blkif-backend", blkif);
 	if (err < 0) {
-		unmap_frontend_page(blkif);
-		free_vm_area(blkif->blk_ring_area);
+		xenbus_unmap_ring_page(blkif->be->dev, blkif->blk_ring_page);
 		blkif->blk_rings.common.sring = NULL;
 		return err;
 	}
@@ -228,8 +190,7 @@ static void xen_blkif_disconnect(struct xen_blkif *blkif)
 	}
 
 	if (blkif->blk_rings.common.sring) {
-		unmap_frontend_page(blkif);
-		free_vm_area(blkif->blk_ring_area);
+		xenbus_unmap_ring_page(blkif->be->dev, blkif->blk_ring_page);
 		blkif->blk_rings.common.sring = NULL;
 	}
 }
-- 
1.7.2.5

[PATCH 3/6] net: xen-netback: use API provided by xenbus module to map rings

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

The xenbus module provides xenbus_map_ring_page() and
xenbus_map_ring_page().  Use these to map the ring pages granted by
the frontend.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netback/common.h  |   13 +++---
 drivers/net/xen-netback/netback.c |   79 +++++++-----------------------------
 2 files changed, 23 insertions(+), 69 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 161f207..05e0a8e 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -58,10 +58,6 @@ struct xenvif {
 	u8               fe_dev_addr[6];
 
 	/* Physical parameters of the comms window. */
-	grant_handle_t   tx_shmem_handle;
-	grant_ref_t      tx_shmem_ref;
-	grant_handle_t   rx_shmem_handle;
-	grant_ref_t      rx_shmem_ref;
 	unsigned int     irq;
 
 	/* List of frontends to notify after a batch of frames sent. */
@@ -70,8 +66,8 @@ struct xenvif {
 	/* The shared rings and indexes. */
 	struct xen_netif_tx_back_ring tx;
 	struct xen_netif_rx_back_ring rx;
-	struct vm_struct *tx_comms_area;
-	struct vm_struct *rx_comms_area;
+	struct page *tx_ring_page;
+	struct page *rx_ring_page;
 
 	/* Frontend feature information. */
 	u8 can_sg:1;
@@ -106,6 +102,11 @@ struct xenvif {
 	wait_queue_head_t waiting_to_free;
 };
 
+static inline struct xenbus_device *xenvif_to_xenbus_device(struct xenvif *vif)
+{
+	return to_xenbus_device(vif->dev->dev.parent);
+}
+
 #define XEN_NETIF_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, PAGE_SIZE)
 #define XEN_NETIF_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, PAGE_SIZE)
 
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index fd00f25..2a017b28 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1577,88 +1577,41 @@ static int xen_netbk_kthread(void *data)
 
 void xen_netbk_unmap_frontend_rings(struct xenvif *vif)
 {
-	struct gnttab_unmap_grant_ref op;
-
-	if (vif->tx.sring) {
-		gnttab_set_unmap_op(&op, (unsigned long)vif->tx_comms_area->addr,
-				    GNTMAP_host_map, vif->tx_shmem_handle);
-
-		if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
-			BUG();
-	}
-
-	if (vif->rx.sring) {
-		gnttab_set_unmap_op(&op, (unsigned long)vif->rx_comms_area->addr,
-				    GNTMAP_host_map, vif->rx_shmem_handle);
-
-		if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
-			BUG();
-	}
-	if (vif->rx_comms_area)
-		free_vm_area(vif->rx_comms_area);
-	if (vif->tx_comms_area)
-		free_vm_area(vif->tx_comms_area);
+	if (vif->tx.sring)
+		xenbus_unmap_ring_page(xenvif_to_xenbus_device(vif),
+				       vif->tx_ring_page);
+	if (vif->rx.sring)
+		xenbus_unmap_ring_page(xenvif_to_xenbus_device(vif),
+				       vif->rx_ring_page);
 }
 
 int xen_netbk_map_frontend_rings(struct xenvif *vif,
 				 grant_ref_t tx_ring_ref,
 				 grant_ref_t rx_ring_ref)
 {
-	struct gnttab_map_grant_ref op;
 	struct xen_netif_tx_sring *txs;
 	struct xen_netif_rx_sring *rxs;
 
 	int err = -ENOMEM;
 
-	vif->tx_comms_area = alloc_vm_area(PAGE_SIZE);
-	if (vif->tx_comms_area == NULL)
+	err = xenbus_map_ring_page(xenvif_to_xenbus_device(vif),
+				   tx_ring_ref, &vif->tx_ring_page);
+	if (err)
 		goto err;
 
-	vif->rx_comms_area = alloc_vm_area(PAGE_SIZE);
-	if (vif->rx_comms_area == NULL)
-		goto err;
-
-	gnttab_set_map_op(&op, (unsigned long)vif->tx_comms_area->addr,
-			  GNTMAP_host_map, tx_ring_ref, vif->domid);
-
-	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status) {
-		netdev_warn(vif->dev,
-			    "failed to map tx ring. err=%d status=%d\n",
-			    err, op.status);
-		err = op.status;
-		goto err;
-	}
-
-	vif->tx_shmem_ref    = tx_ring_ref;
-	vif->tx_shmem_handle = op.handle;
-
-	txs = (struct xen_netif_tx_sring *)vif->tx_comms_area->addr;
+	txs = page_address(vif->tx_ring_page);
 	BACK_RING_INIT(&vif->tx, txs, PAGE_SIZE);
 
-	gnttab_set_map_op(&op, (unsigned long)vif->rx_comms_area->addr,
-			  GNTMAP_host_map, rx_ring_ref, vif->domid);
-
-	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status) {
-		netdev_warn(vif->dev,
-			    "failed to map rx ring. err=%d status=%d\n",
-			    err, op.status);
-		err = op.status;
+	err = xenbus_map_ring_page(xenvif_to_xenbus_device(vif),
+				   rx_ring_ref, &vif->rx_ring_page);
+	if (err)
 		goto err;
-	}
-
-	vif->rx_shmem_ref     = rx_ring_ref;
-	vif->rx_shmem_handle  = op.handle;
-	vif->rx_req_cons_peek = 0;
 
-	rxs = (struct xen_netif_rx_sring *)vif->rx_comms_area->addr;
+	rxs = page_address(vif->rx_ring_page);
 	BACK_RING_INIT(&vif->rx, rxs, PAGE_SIZE);
 
+	vif->rx_req_cons_peek = 0;
+
 	return 0;
 
 err:
-- 
1.7.2.5

[PATCH 4/6] xen: xen-pciback: use xenbus_map_ring_page() to map rings

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

xenbus_map_ring_valloc() is deprecated in favour of
xenbus_map_ring_page().

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/xen/xen-pciback/pciback.h |    1 +
 drivers/xen/xen-pciback/xenbus.c  |    7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
index a0e131a..956aef9 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -32,6 +32,7 @@ struct xen_pcibk_device {
 	struct xenbus_watch be_watch;
 	u8 be_watching;
 	int evtchn_irq;
+	struct page *ring_page;
 	struct xen_pci_sharedinfo *sh_info;
 	unsigned long flags;
 	struct work_struct op_work;
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index 978d2c6..41bfefc 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -79,7 +79,7 @@ static void xen_pcibk_disconnect(struct xen_pcibk_device *pdev)
 
 	spin_lock(&pdev->dev_lock);
 	if (pdev->sh_info != NULL) {
-		xenbus_unmap_ring_vfree(pdev->xdev, pdev->sh_info);
+		xenbus_unmap_ring_page(pdev->xdev, pdev->ring_page);
 		pdev->sh_info = NULL;
 	}
 	spin_unlock(&pdev->dev_lock);
@@ -107,13 +107,12 @@ static int xen_pcibk_do_attach(struct xen_pcibk_device *pdev, int gnt_ref,
 			     int remote_evtchn)
 {
 	int err = 0;
-	void *vaddr;
 
 	dev_dbg(&pdev->xdev->dev,
 		"Attaching to frontend resources - gnt_ref=%d evtchn=%d\n",
 		gnt_ref, remote_evtchn);
 
-	err = xenbus_map_ring_valloc(pdev->xdev, gnt_ref, &vaddr);
+	err = xenbus_map_ring_page(pdev->xdev, gnt_ref, &pdev->ring_page);
 	if (err < 0) {
 		xenbus_dev_fatal(pdev->xdev, err,
 				"Error mapping other domain page in ours.");
@@ -121,7 +120,7 @@ static int xen_pcibk_do_attach(struct xen_pcibk_device *pdev, int gnt_ref,
 	}
 
 	spin_lock(&pdev->dev_lock);
-	pdev->sh_info = vaddr;
+	pdev->sh_info = page_address(pdev->ring_page);
 	spin_unlock(&pdev->dev_lock);
 
 	err = bind_interdomain_evtchn_to_irqhandler(
-- 
1.7.2.5

[PATCH 5/6] xen: xenbus: remove xenbus_map_ring_valloc() and xenbus_map_ring_vfree()

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

xenbus_map_ring_valloc() and xenbus_map_ring_vfree() are no longer
used.  Drivers should use xenbus_map_ring_page() instead.

xenbus_map_ring() and xenbus_unmap_ring also have no users outside of
the file and are made static.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/ia64/xen/grant-table.c        |    2 +-
 drivers/xen/xenbus/xenbus_client.c |  119 ++----------------------------------
 include/xen/xenbus.h               |    9 +---
 3 files changed, 7 insertions(+), 123 deletions(-)

diff --git a/arch/ia64/xen/grant-table.c b/arch/ia64/xen/grant-table.c
index 48cca37..4b72424 100644
--- a/arch/ia64/xen/grant-table.c
+++ b/arch/ia64/xen/grant-table.c
@@ -54,7 +54,7 @@ struct vm_struct *xen_alloc_vm_area(unsigned long size)
 	area->size = size;
 	area->pages = NULL;
 	area->nr_pages = nr_pages;
-	area->phys_addr = 0;	/* xenbus_map_ring_valloc uses this field!  */
+	area->phys_addr = 0;
 
 	return area;
 
diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 504325b..3c64be8 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -419,58 +419,6 @@ int xenbus_free_evtchn(struct xenbus_device *dev, int port)
 }
 EXPORT_SYMBOL_GPL(xenbus_free_evtchn);
 
-
-/**
- * xenbus_map_ring_valloc
- * @dev: xenbus device
- * @gnt_ref: grant reference
- * @vaddr: pointer to address to be filled out by mapping
- *
- * Based on Rusty Russell's skeleton driver's map_page.
- * Map a page of memory into this domain from another domain's grant table.
- * xenbus_map_ring_valloc allocates a page of virtual address space, maps the
- * page to that address, and sets *vaddr to that address.
- * Returns 0 on success, and GNTST_* (see xen/include/interface/grant_table.h)
- * or -ENOMEM on error. If an error is returned, device will switch to
- * XenbusStateClosing and the error message will be saved in XenStore.
- */
-int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr)
-{
-	struct gnttab_map_grant_ref op = {
-		.flags = GNTMAP_host_map,
-		.ref   = gnt_ref,
-		.dom   = dev->otherend_id,
-	};
-	struct vm_struct *area;
-
-	*vaddr = NULL;
-
-	area = xen_alloc_vm_area(PAGE_SIZE);
-	if (!area)
-		return -ENOMEM;
-
-	op.host_addr = (unsigned long)area->addr;
-
-	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status != GNTST_okay) {
-		xen_free_vm_area(area);
-		xenbus_dev_fatal(dev, op.status,
-				 "mapping in shared page %d from domain %d",
-				 gnt_ref, dev->otherend_id);
-		return op.status;
-	}
-
-	/* Stuff the handle in an unused field */
-	area->phys_addr = (unsigned long)op.handle;
-
-	*vaddr = area->addr;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
-
-
 /**
  * xenbus_map_ring
  * @dev: xenbus device
@@ -485,8 +433,8 @@ EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
  * or -ENOMEM on error. If an error is returned, device will switch to
  * XenbusStateClosing and the error message will be saved in XenStore.
  */
-int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
-		    grant_handle_t *handle, void *vaddr)
+static int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
+			   grant_handle_t *handle, void *vaddr)
 {
 	struct gnttab_map_grant_ref op = {
 		.host_addr = (unsigned long)vaddr,
@@ -502,13 +450,12 @@ int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
 		xenbus_dev_fatal(dev, op.status,
 				 "mapping in shared page %d from domain %d",
 				 gnt_ref, dev->otherend_id);
+		*handle = 0;
 	} else
 		*handle = op.handle;
 
 	return op.status;
 }
-EXPORT_SYMBOL_GPL(xenbus_map_ring);
-
 
 /**
  * xenbus_map_ring_page - map a foreign page into a kernel page
@@ -549,61 +496,6 @@ err:
 EXPORT_SYMBOL_GPL(xenbus_map_ring_page);
 
 /**
- * xenbus_unmap_ring_vfree
- * @dev: xenbus device
- * @vaddr: addr to unmap
- *
- * Based on Rusty Russell's skeleton driver's unmap_page.
- * Unmap a page of memory in this domain that was imported from another domain.
- * Use xenbus_unmap_ring_vfree if you mapped in your memory with
- * xenbus_map_ring_valloc (it will free the virtual address space).
- * Returns 0 on success and returns GNTST_* on error
- * (see xen/include/interface/grant_table.h).
- */
-int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
-{
-	struct vm_struct *area;
-	struct gnttab_unmap_grant_ref op = {
-		.host_addr = (unsigned long)vaddr,
-	};
-
-	/* It'd be nice if linux/vmalloc.h provided a find_vm_area(void *addr)
-	 * method so that we don't have to muck with vmalloc internals here.
-	 * We could force the user to hang on to their struct vm_struct from
-	 * xenbus_map_ring_valloc, but these 6 lines considerably simplify
-	 * this API.
-	 */
-	read_lock(&vmlist_lock);
-	for (area = vmlist; area != NULL; area = area->next) {
-		if (area->addr == vaddr)
-			break;
-	}
-	read_unlock(&vmlist_lock);
-
-	if (!area) {
-		xenbus_dev_error(dev, -ENOENT,
-				 "can't find mapped virtual address %p", vaddr);
-		return GNTST_bad_virt_addr;
-	}
-
-	op.handle = (grant_handle_t)area->phys_addr;
-
-	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
-		BUG();
-
-	if (op.status == GNTST_okay)
-		xen_free_vm_area(area);
-	else
-		xenbus_dev_error(dev, op.status,
-				 "unmapping page at handle %d error %d",
-				 (int16_t)area->phys_addr, op.status);
-
-	return op.status;
-}
-EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
-
-
-/**
  * xenbus_unmap_ring
  * @dev: xenbus device
  * @handle: grant handle
@@ -613,8 +505,8 @@ EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
  * Returns 0 on success and returns GNTST_* on error
  * (see xen/include/interface/grant_table.h).
  */
-int xenbus_unmap_ring(struct xenbus_device *dev,
-		      grant_handle_t handle, void *vaddr)
+static int xenbus_unmap_ring(struct xenbus_device *dev,
+			     grant_handle_t handle, void *vaddr)
 {
 	struct gnttab_unmap_grant_ref op = {
 		.host_addr = (unsigned long)vaddr,
@@ -631,7 +523,6 @@ int xenbus_unmap_ring(struct xenbus_device *dev,
 
 	return op.status;
 }
-EXPORT_SYMBOL_GPL(xenbus_unmap_ring);
 
 /**
  * xenbus_unmap_ring_page - unmap an foreign page from a kernel page
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index ebde2fd..d73d320 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -208,16 +208,9 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
 
 int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
 int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
-int xenbus_map_ring_valloc(struct xenbus_device *dev,
-			   int gnt_ref, void **vaddr);
-int xenbus_map_ring(struct xenbus_device *dev, int gnt_ref,
-			   grant_handle_t *handle, void *vaddr);
+
 int xenbus_map_ring_page(struct xenbus_device *dev, int gnt_ref,
 			 struct page **page);
-
-int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr);
-int xenbus_unmap_ring(struct xenbus_device *dev,
-		      grant_handle_t handle, void *vaddr);
 void xenbus_unmap_ring_page(struct xenbus_device *dev, struct page *page);
 
 int xenbus_alloc_evtchn(struct xenbus_device *dev, int *port);
-- 
1.7.2.5

[PATCH 6/6] mm: remove vmalloc_sync_all() from alloc_vm_area()

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

The address space allocated by the remaining user of alloc_vm_area()
is not accessed during a hypercall.  Therefore, the normal mechanism
of populating the vmalloc page tables during a fault works so the
vmalloc_sync_all() is not required and it can be removed.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 mm/vmalloc.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 5016f19..b0b4550 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2117,9 +2117,7 @@ static int f(pte_t *pte, pgtable_t table, unsigned long addr, void *data)
  *
  *	This function reserves a range of kernel address space, and
  *	allocates pagetables to map that range.  No actual mappings
- *	are created.  If the kernel address space is not shared
- *	between processes, it syncs the pagetable across all
- *	processes.
+ *	are created.
  */
 struct vm_struct *alloc_vm_area(size_t size)
 {
@@ -2140,14 +2138,6 @@ struct vm_struct *alloc_vm_area(size_t size)
 		return NULL;
 	}
 
-	/*
-	 * If the allocated address space is passed to a hypercall
-	 * before being used then we cannot rely on a page fault to
-	 * trigger an update of the page tables.  So sync all the page
-	 * tables here.
-	 */
-	vmalloc_sync_all();
-
 	return area;
 }
 EXPORT_SYMBOL_GPL(alloc_vm_area);
-- 
1.7.2.5

Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[Jeremy Fitzhardinge]

On 09/15/2011 05:40 AM, David Vrabel wrote:
> This set of pages avoids the need to call vmalloc_sync_all() when
> mapping foreign pages.  Two new functions are adding for mapping
> foreign pages onto RAM pages instead of vmalloc space.
>
> This does waste a page of RAM for each mapped page.  In the future a
> ballooned page could be used instead (once the API for getting a
> ballooned page with the right GFP flags is available).

You can allocate a pfn, free ("balloon out") the underlying mfn, and
replace it with a granted page; it doesn't need any particular new
infrastructure.

But that said, if you want to allocate virtual addresses for mapping
granted pages, then alloc_vm_area() is the right thing to use.  You need
to make sure you touch the pages from within the kernel before passing
those addresses to a hypercall to make sure the mappings are established
within the current task (possibly within a no-preempt region to
guarantee that nothing odd happens).  Or alternatively, you could switch
the current pagetable to init_mm for the hypercall (if it isn't already)
- since that's the reference pagetable - assuming you're not passing
usermode virtual addresses to the hypercall.

This series is relying on regular ram mappings are already synced to all
tasks, but I'm not sure that's necessarily guaranteed (for example, if
you hotplug new memory into the domain, the new pages won't be mapped
into every mm unless they're synced).

    J

Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[Stefano Stabellini]

On Thu, 15 Sep 2011, Jeremy Fitzhardinge wrote:
> On 09/15/2011 05:40 AM, David Vrabel wrote:
> > This set of pages avoids the need to call vmalloc_sync_all() when
> > mapping foreign pages.  Two new functions are adding for mapping
> > foreign pages onto RAM pages instead of vmalloc space.
> >
> > This does waste a page of RAM for each mapped page.  In the future a
> > ballooned page could be used instead (once the API for getting a
> > ballooned page with the right GFP flags is available).
> 
> You can allocate a pfn, free ("balloon out") the underlying mfn, and
> replace it with a granted page; it doesn't need any particular new
> infrastructure.

David, give a look at drivers/xen/balloon.c:alloc_xenballooned_pages, in
particular after my changes to it:

http://marc.info/?l=linux-kernel&m=131552369305998&w=2

it allows the caller to allocate a lowmem page and balloon it out right
away.


> But that said, if you want to allocate virtual addresses for mapping
> granted pages, then alloc_vm_area() is the right thing to use.  You need
> to make sure you touch the pages from within the kernel before passing
> those addresses to a hypercall to make sure the mappings are established
> within the current task (possibly within a no-preempt region to
> guarantee that nothing odd happens).  Or alternatively, you could switch
> the current pagetable to init_mm for the hypercall (if it isn't already)
> - since that's the reference pagetable - assuming you're not passing
> usermode virtual addresses to the hypercall.

This problem is not very different from the one we are trying to solve
with alloc_xenballooned_pages: in case we want a page that is mapped in
kernel space we just use allocate a lowmem page, balloon it out, change
the p2m and add the page to the m2p_override.
Why should we solve that issue differently?


> This series is relying on regular ram mappings are already synced to all
> tasks, but I'm not sure that's necessarily guaranteed (for example, if
> you hotplug new memory into the domain, the new pages won't be mapped
> into every mm unless they're synced).

the series is using GFP_KERNEL, so this problem shouldn't occur, right?

Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[Stefano Stabellini]

On Thu, 15 Sep 2011, Jeremy Fitzhardinge wrote:
> On 09/15/2011 05:40 AM, David Vrabel wrote:
> > This set of pages avoids the need to call vmalloc_sync_all() when
> > mapping foreign pages.  Two new functions are adding for mapping
> > foreign pages onto RAM pages instead of vmalloc space.
> >
> > This does waste a page of RAM for each mapped page.  In the future a
> > ballooned page could be used instead (once the API for getting a
> > ballooned page with the right GFP flags is available).
> 
> You can allocate a pfn, free ("balloon out") the underlying mfn, and
> replace it with a granted page; it doesn't need any particular new
> infrastructure.

David, give a look at drivers/xen/balloon.c:alloc_xenballooned_pages, in
particular after my changes to it:

http://marc.info/?l=linux-kernel&m=131552369305998&w=2

it allows the caller to allocate a lowmem page and balloon it out right
away.


> But that said, if you want to allocate virtual addresses for mapping
> granted pages, then alloc_vm_area() is the right thing to use.  You need
> to make sure you touch the pages from within the kernel before passing
> those addresses to a hypercall to make sure the mappings are established
> within the current task (possibly within a no-preempt region to
> guarantee that nothing odd happens).  Or alternatively, you could switch
> the current pagetable to init_mm for the hypercall (if it isn't already)
> - since that's the reference pagetable - assuming you're not passing
> usermode virtual addresses to the hypercall.

This problem is not very different from the one we are trying to solve
with alloc_xenballooned_pages: in case we want a page that is mapped in
kernel space we just use allocate a lowmem page, balloon it out, change
the p2m and add the page to the m2p_override.
Why should we solve that issue differently?


> This series is relying on regular ram mappings are already synced to all
> tasks, but I'm not sure that's necessarily guaranteed (for example, if
> you hotplug new memory into the domain, the new pages won't be mapped
> into every mm unless they're synced).

the series is using GFP_KERNEL, so this problem shouldn't occur, right?

Re: [Xen-devel] Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[David Vrabel]

On 15/09/11 22:37, Jeremy Fitzhardinge wrote:
> On 09/15/2011 05:40 AM, David Vrabel wrote:
>> This set of pages avoids the need to call vmalloc_sync_all() when
>> mapping foreign pages.  Two new functions are adding for mapping
>> foreign pages onto RAM pages instead of vmalloc space.
>>
>> [...]
> 
> But that said, if you want to allocate virtual addresses for mapping
> granted pages, then alloc_vm_area() is the right thing to use.  You need
> to make sure you touch the pages from within the kernel before passing
> those addresses to a hypercall to make sure the mappings are established
> within the current task (possibly within a no-preempt region to
> guarantee that nothing odd happens). Or alternatively, you could switch
> the current pagetable to init_mm for the hypercall (if it isn't already)
> - since that's the reference pagetable - assuming you're not passing
> usermode virtual addresses to the hypercall.

Making alloc_vm_area() fill in a array of pte_t *'s and passing the pointer
to the PTE (using the GNTMAP_contains_pte flag) in the hypercall should
allow Xen to update the PTEs in init_mm directly.

However, I'm not sure what to do about ia64 where GNTMAP_contains_pte
is not supported.  Any ideas?

Here's the untested patch I have so far.

diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
index 49ba9b5..77348e8 100644
--- a/arch/x86/xen/grant-table.c
+++ b/arch/x86/xen/grant-table.c
@@ -71,7 +71,7 @@ int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
 
 	if (shared == NULL) {
 		struct vm_struct *area =
-			alloc_vm_area(PAGE_SIZE * max_nr_gframes);
+			alloc_vm_area(PAGE_SIZE * max_nr_gframes, NULL);
 		BUG_ON(area == NULL);
 		shared = area->addr;
 		*__shared = shared;
diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index cdacf92..75eb179 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -435,19 +435,20 @@ EXPORT_SYMBOL_GPL(xenbus_free_evtchn);
 int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr)
 {
 	struct gnttab_map_grant_ref op = {
-		.flags = GNTMAP_host_map,
+		.flags = GNTMAP_host_map | GNTMAP_contains_pte,
 		.ref   = gnt_ref,
 		.dom   = dev->otherend_id,
 	};
 	struct vm_struct *area;
+	pte_t *pte;
 
 	*vaddr = NULL;
 
-	area = alloc_vm_area(PAGE_SIZE);
+	area = alloc_vm_area(PAGE_SIZE, &pte);
 	if (!area)
 		return -ENOMEM;
 
-	op.host_addr = (unsigned long)area->addr;
+	op.host_addr = (unsigned long)pte;
 
 	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
 		BUG();
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 9332e52..1a77252 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -118,7 +118,7 @@ unmap_kernel_range(unsigned long addr, unsigned long size)
 #endif
 
 /* Allocate/destroy a 'vmalloc' VM area. */
-extern struct vm_struct *alloc_vm_area(size_t size);
+extern struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes);
 extern void free_vm_area(struct vm_struct *area);
 
 /* for /dev/kmem */
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 5016f19..786b4f6 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2105,23 +2105,30 @@ void  __attribute__((weak)) vmalloc_sync_all(void)
 
 static int f(pte_t *pte, pgtable_t table, unsigned long addr, void *data)
 {
-	/* apply_to_page_range() does all the hard work. */
+	pte_t ***p = data;
+
+	if (p) {
+		*(*p) = pte;
+		(*p)++;
+	}
 	return 0;
 }
 
 /**
  *	alloc_vm_area - allocate a range of kernel address space
  *	@size:		size of the area
+ *	@ptes:		returns the PTEs for the address space
  *
  *	Returns:	NULL on failure, vm_struct on success
  *
  *	This function reserves a range of kernel address space, and
  *	allocates pagetables to map that range.  No actual mappings
- *	are created.  If the kernel address space is not shared
- *	between processes, it syncs the pagetable across all
- *	processes.
+ *	are created.
+ *
+ *	If @ptes is non-NULL, pointers to the PTEs (in init_mm)
+ *	allocated for the VM area are returned.
  */
-struct vm_struct *alloc_vm_area(size_t size)
+struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes)
 {
 	struct vm_struct *area;
 
@@ -2135,19 +2142,11 @@ struct vm_struct *alloc_vm_area(size_t size)
 	 * of kernel virtual address space and mapped into init_mm.
 	 */
 	if (apply_to_page_range(&init_mm, (unsigned long)area->addr,
-				area->size, f, NULL)) {
+				area->size, f, ptes ? &ptes : NULL)) {
 		free_vm_area(area);
 		return NULL;
 	}
 
-	/*
-	 * If the allocated address space is passed to a hypercall
-	 * before being used then we cannot rely on a page fault to
-	 * trigger an update of the page tables.  So sync all the page
-	 * tables here.
-	 */
-	vmalloc_sync_all();
-
 	return area;
 }
 EXPORT_SYMBOL_GPL(alloc_vm_area);

David

Re: [Xen-devel] Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[Jeremy Fitzhardinge]

On 09/21/2011 03:42 AM, Stefano Stabellini wrote:
> On Thu, 15 Sep 2011, Jeremy Fitzhardinge wrote:
>> This series is relying on regular ram mappings are already synced to all
>> tasks, but I'm not sure that's necessarily guaranteed (for example, if
>> you hotplug new memory into the domain, the new pages won't be mapped
>> into every mm unless they're synced).
> the series is using GFP_KERNEL, so this problem shouldn't occur, right?

What properties do you think GFP_KERNEL guarantees?

    J

Re: [Xen-devel] Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[Stefano Stabellini]

On Wed, 21 Sep 2011, Jeremy Fitzhardinge wrote:
> On 09/21/2011 03:42 AM, Stefano Stabellini wrote:
> > On Thu, 15 Sep 2011, Jeremy Fitzhardinge wrote:
> >> This series is relying on regular ram mappings are already synced to all
> >> tasks, but I'm not sure that's necessarily guaranteed (for example, if
> >> you hotplug new memory into the domain, the new pages won't be mapped
> >> into every mm unless they're synced).
> > the series is using GFP_KERNEL, so this problem shouldn't occur, right?
> 
> What properties do you think GFP_KERNEL guarantees?

That the memory is below 4G and always mapped in the kernel 1:1 region.

Regarding memory hotplug it looks like that x86_32 is mapping new memory
ZONE_HIGHMEM, therefore avoiding any problems with GFP_KERNEL allocations.
On the other hand x86_64 is mapping the memory ZONE_NORMAL and calling
init_memory_mapping on the new range right away. AFAICT changes to
the 1:1 mapping in init_mm are automatically synced across all mm's
because the pgd is shared?

Re: [Xen-devel] Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[Jeremy Fitzhardinge]

On 09/22/2011 04:06 AM, Stefano Stabellini wrote:
> On Wed, 21 Sep 2011, Jeremy Fitzhardinge wrote:
>> On 09/21/2011 03:42 AM, Stefano Stabellini wrote:
>>> On Thu, 15 Sep 2011, Jeremy Fitzhardinge wrote:
>>>> This series is relying on regular ram mappings are already synced to all
>>>> tasks, but I'm not sure that's necessarily guaranteed (for example, if
>>>> you hotplug new memory into the domain, the new pages won't be mapped
>>>> into every mm unless they're synced).
>>> the series is using GFP_KERNEL, so this problem shouldn't occur, right?
>> What properties do you think GFP_KERNEL guarantees?
> That the memory is below 4G and always mapped in the kernel 1:1 region.

Hm, but that's not quite the same thing as "mapped into every
pagetable".  Lowmem pages always have a kernel virtual address, and its
always OK to touch them at any point in kernel code[*] because one can
rely on the fault handler to create mappings as needed - but that
doesn't mean they're necessarily mapped by present ptes in the current
pagetable.

[*] - except NMI handlers

> Regarding memory hotplug it looks like that x86_32 is mapping new memory
> ZONE_HIGHMEM, therefore avoiding any problems with GFP_KERNEL allocations.
> On the other hand x86_64 is mapping the memory ZONE_NORMAL and calling
> init_memory_mapping on the new range right away. AFAICT changes to
> the 1:1 mapping in init_mm are automatically synced across all mm's
> because the pgd is shared?

TBH I'm not sure.  vmalloc_sync_one/all does seem to do *something* on
64-bit, but I was never completely sure what regions of the address
space were already shared.  I *think* it might be that the pgd and pud
are not shared, but the pmd down is, so if you add a new pmd you need to
sync it into all the puds (and puds into pgds if you add a new one of
those).

But I'd be happier pretending that vmalloc_sync_* just doesn't exist,
and deal with it at the hypercall level - in the short term, by just
making sure that the callers touch all those pages before passing them
into the hypercall.

    J

Re: [Xen-devel] Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[Stefano Stabellini]

On Thu, 22 Sep 2011, Jeremy Fitzhardinge wrote:
> On 09/22/2011 04:06 AM, Stefano Stabellini wrote:
> > On Wed, 21 Sep 2011, Jeremy Fitzhardinge wrote:
> >> On 09/21/2011 03:42 AM, Stefano Stabellini wrote:
> >>> On Thu, 15 Sep 2011, Jeremy Fitzhardinge wrote:
> >>>> This series is relying on regular ram mappings are already synced to all
> >>>> tasks, but I'm not sure that's necessarily guaranteed (for example, if
> >>>> you hotplug new memory into the domain, the new pages won't be mapped
> >>>> into every mm unless they're synced).
> >>> the series is using GFP_KERNEL, so this problem shouldn't occur, right?
> >> What properties do you think GFP_KERNEL guarantees?
> > That the memory is below 4G and always mapped in the kernel 1:1 region.
> 
> Hm, but that's not quite the same thing as "mapped into every
> pagetable".  Lowmem pages always have a kernel virtual address, and its
> always OK to touch them at any point in kernel code[*] because one can
> rely on the fault handler to create mappings as needed - but that
> doesn't mean they're necessarily mapped by present ptes in the current
> pagetable.
> 
> [*] - except NMI handlers

Is that really true?
I quickly went through the fault handler and I couldn't see anything
related to the kernel 1:1 region.


> > Regarding memory hotplug it looks like that x86_32 is mapping new memory
> > ZONE_HIGHMEM, therefore avoiding any problems with GFP_KERNEL allocations.
> > On the other hand x86_64 is mapping the memory ZONE_NORMAL and calling
> > init_memory_mapping on the new range right away. AFAICT changes to
> > the 1:1 mapping in init_mm are automatically synced across all mm's
> > because the pgd is shared?
> 
> TBH I'm not sure.  vmalloc_sync_one/all does seem to do *something* on
> 64-bit, but I was never completely sure what regions of the address
> space were already shared.  I *think* it might be that the pgd and pud
> are not shared, but the pmd down is, so if you add a new pmd you need to
> sync it into all the puds (and puds into pgds if you add a new one of
> those).
> 
> But I'd be happier pretending that vmalloc_sync_* just doesn't exist,
> and deal with it at the hypercall level - in the short term, by just
> making sure that the callers touch all those pages before passing them
> into the hypercall.

That would certainly be an improvement over what we have now.

However I am worried about the gntdev stuff: if I am right and the 1:1
mapping is guaranteed to be sync'ed, then it is OK and we can use
alloc_xenballooned_pages everywhere, otherwise we should fix or remove
alloc_xenballooned_pages from gntdev too.

Re: Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[Stefano Stabellini]

On Thu, 22 Sep 2011, Jeremy Fitzhardinge wrote:
> On 09/22/2011 04:06 AM, Stefano Stabellini wrote:
> > On Wed, 21 Sep 2011, Jeremy Fitzhardinge wrote:
> >> On 09/21/2011 03:42 AM, Stefano Stabellini wrote:
> >>> On Thu, 15 Sep 2011, Jeremy Fitzhardinge wrote:
> >>>> This series is relying on regular ram mappings are already synced to all
> >>>> tasks, but I'm not sure that's necessarily guaranteed (for example, if
> >>>> you hotplug new memory into the domain, the new pages won't be mapped
> >>>> into every mm unless they're synced).
> >>> the series is using GFP_KERNEL, so this problem shouldn't occur, right?
> >> What properties do you think GFP_KERNEL guarantees?
> > That the memory is below 4G and always mapped in the kernel 1:1 region.
> 
> Hm, but that's not quite the same thing as "mapped into every
> pagetable".  Lowmem pages always have a kernel virtual address, and its
> always OK to touch them at any point in kernel code[*] because one can
> rely on the fault handler to create mappings as needed - but that
> doesn't mean they're necessarily mapped by present ptes in the current
> pagetable.
> 
> [*] - except NMI handlers

Is that really true?
I quickly went through the fault handler and I couldn't see anything
related to the kernel 1:1 region.


> > Regarding memory hotplug it looks like that x86_32 is mapping new memory
> > ZONE_HIGHMEM, therefore avoiding any problems with GFP_KERNEL allocations.
> > On the other hand x86_64 is mapping the memory ZONE_NORMAL and calling
> > init_memory_mapping on the new range right away. AFAICT changes to
> > the 1:1 mapping in init_mm are automatically synced across all mm's
> > because the pgd is shared?
> 
> TBH I'm not sure.  vmalloc_sync_one/all does seem to do *something* on
> 64-bit, but I was never completely sure what regions of the address
> space were already shared.  I *think* it might be that the pgd and pud
> are not shared, but the pmd down is, so if you add a new pmd you need to
> sync it into all the puds (and puds into pgds if you add a new one of
> those).
> 
> But I'd be happier pretending that vmalloc_sync_* just doesn't exist,
> and deal with it at the hypercall level - in the short term, by just
> making sure that the callers touch all those pages before passing them
> into the hypercall.

That would certainly be an improvement over what we have now.

However I am worried about the gntdev stuff: if I am right and the 1:1
mapping is guaranteed to be sync'ed, then it is OK and we can use
alloc_xenballooned_pages everywhere, otherwise we should fix or remove
alloc_xenballooned_pages from gntdev too.

Re: [Xen-devel] Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[David Vrabel]

On 22/09/11 22:19, Jeremy Fitzhardinge wrote:
> 
> But I'd be happier pretending that vmalloc_sync_* just doesn't exist,
> and deal with it at the hypercall level - in the short term, by just
> making sure that the callers touch all those pages before passing them
> into the hypercall.

I don't think you can access the pages at the addresses allocated with
alloc_vm_area() because the pages don't exist yet and there's a check of
pte_present() in vmalloc_fault() and I think an Oops will be reported.

David

Re: Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[David Vrabel]

On 22/09/11 22:19, Jeremy Fitzhardinge wrote:
> 
> But I'd be happier pretending that vmalloc_sync_* just doesn't exist,
> and deal with it at the hypercall level - in the short term, by just
> making sure that the callers touch all those pages before passing them
> into the hypercall.

I don't think you can access the pages at the addresses allocated with
alloc_vm_area() because the pages don't exist yet and there's a check of
pte_present() in vmalloc_fault() and I think an Oops will be reported.

David

Re: [Xen-devel] Re: [PATCH 0/6] xen: don't call vmalloc_sync_all() when mapping foreign pages

[David Vrabel]

On 21/09/11 15:44, David Vrabel wrote:
> On 15/09/11 22:37, Jeremy Fitzhardinge wrote:
>> On 09/15/2011 05:40 AM, David Vrabel wrote:
>>> This set of pages avoids the need to call vmalloc_sync_all() when
>>> mapping foreign pages.  Two new functions are adding for mapping
>>> foreign pages onto RAM pages instead of vmalloc space.
>>>
>>> [...]
>>
>> But that said, if you want to allocate virtual addresses for mapping
>> granted pages, then alloc_vm_area() is the right thing to use.  You need
>> to make sure you touch the pages from within the kernel before passing
>> those addresses to a hypercall to make sure the mappings are established
>> within the current task (possibly within a no-preempt region to
>> guarantee that nothing odd happens). Or alternatively, you could switch
>> the current pagetable to init_mm for the hypercall (if it isn't already)
>> - since that's the reference pagetable - assuming you're not passing
>> usermode virtual addresses to the hypercall.
> 
> Making alloc_vm_area() fill in a array of pte_t *'s and passing the pointer
> to the PTE (using the GNTMAP_contains_pte flag) in the hypercall should
> allow Xen to update the PTEs in init_mm directly.
> 
> However, I'm not sure what to do about ia64 where GNTMAP_contains_pte
> is not supported.  Any ideas?
> 
> Here's the untested patch I have so far.

This (sort of) works with some hacks/changes (described below).

> diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c
> index 49ba9b5..77348e8 100644
> --- a/arch/x86/xen/grant-table.c
> +++ b/arch/x86/xen/grant-table.c
> @@ -71,7 +71,7 @@ int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
>  
>  	if (shared == NULL) {
>  		struct vm_struct *area =
> -			alloc_vm_area(PAGE_SIZE * max_nr_gframes);
> +			alloc_vm_area(PAGE_SIZE * max_nr_gframes, NULL);
>  		BUG_ON(area == NULL);
>  		shared = area->addr;
>  		*__shared = shared;
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index cdacf92..75eb179 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -435,19 +435,20 @@ EXPORT_SYMBOL_GPL(xenbus_free_evtchn);
>  int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr)
>  {
>  	struct gnttab_map_grant_ref op = {
> -		.flags = GNTMAP_host_map,
> +		.flags = GNTMAP_host_map | GNTMAP_contains_pte,
>  		.ref   = gnt_ref,
>  		.dom   = dev->otherend_id,
>  	};
>  	struct vm_struct *area;
> +	pte_t *pte;

alloc_vm_area() is allocating two pages instead of the one we ask for.
I hacked this with

       pte_t *pte[2];

>  	*vaddr = NULL;
>  
> -	area = alloc_vm_area(PAGE_SIZE);
> +	area = alloc_vm_area(PAGE_SIZE, &pte);
>  	if (!area)
>  		return -ENOMEM;
>  
> -	op.host_addr = (unsigned long)area->addr;
> +	op.host_addr = (unsigned long)pte;

With the GNTMAP_contains_pte flags this needs a machine address here so:

        op.host_addr = arbitrary_virt_to_machine(pte).maddr;

>  	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
>  		BUG();

Also need to do something with xenbus_map_ring_vfree() which doesn't work.

> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 9332e52..1a77252 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -118,7 +118,7 @@ unmap_kernel_range(unsigned long addr, unsigned long size)
>  #endif
>  
>  /* Allocate/destroy a 'vmalloc' VM area. */
> -extern struct vm_struct *alloc_vm_area(size_t size);
> +extern struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes);
>  extern void free_vm_area(struct vm_struct *area);
>  
>  /* for /dev/kmem */
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 5016f19..786b4f6 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2105,23 +2105,30 @@ void  __attribute__((weak)) vmalloc_sync_all(void)
>  
>  static int f(pte_t *pte, pgtable_t table, unsigned long addr, void *data)
>  {
> -	/* apply_to_page_range() does all the hard work. */
> +	pte_t ***p = data;
> +
> +	if (p) {
> +		*(*p) = pte;
> +		(*p)++;
> +	}
>  	return 0;
>  }
>  
>  /**
>   *	alloc_vm_area - allocate a range of kernel address space
>   *	@size:		size of the area
> + *	@ptes:		returns the PTEs for the address space
>   *
>   *	Returns:	NULL on failure, vm_struct on success
>   *
>   *	This function reserves a range of kernel address space, and
>   *	allocates pagetables to map that range.  No actual mappings
> - *	are created.  If the kernel address space is not shared
> - *	between processes, it syncs the pagetable across all
> - *	processes.
> + *	are created.
> + *
> + *	If @ptes is non-NULL, pointers to the PTEs (in init_mm)
> + *	allocated for the VM area are returned.
>   */
> -struct vm_struct *alloc_vm_area(size_t size)
> +struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes)
>  {
>  	struct vm_struct *area;
>  
> @@ -2135,19 +2142,11 @@ struct vm_struct *alloc_vm_area(size_t size)
>  	 * of kernel virtual address space and mapped into init_mm.
>  	 */
>  	if (apply_to_page_range(&init_mm, (unsigned long)area->addr,
> -				area->size, f, NULL)) {
> +				area->size, f, ptes ? &ptes : NULL)) {
>  		free_vm_area(area);
>  		return NULL;
>  	}
>  
> -	/*
> -	 * If the allocated address space is passed to a hypercall
> -	 * before being used then we cannot rely on a page fault to
> -	 * trigger an update of the page tables.  So sync all the page
> -	 * tables here.
> -	 */
> -	vmalloc_sync_all();
> -
>  	return area;
>  }
>  EXPORT_SYMBOL_GPL(alloc_vm_area);