[PATCH 0/3] Bugfixes for virtio balloon driver

[PATCH 0/3] Bugfixes for virtio balloon driver

[David Gibson]

This series contains one cleanup and two bug fixes for the virtio
balloon driver.

[PATCH 1/3] virtio_balloon: Remove unnecessarily persistent state

[David Gibson]

The main virtio_balloon state structure contains the fields num_pfns and
array 'pfns'.  Although they are stored here persistently, the lifetime of
useful data in there is never more than one function - they're essentially
used as though they were local variables.

For the pfns buffer, used to communicate a batch of pfns this is useful to
avoid either transient kmalloc()s or putting too much data on the stack.
For num_pfns, there is no reason it should not be a local, though.

This patch cleans things up by making num_pfns a local in the functions it
is used in.  The pfns array remains, but the comment is updated to clarify
that it contains no truly persistent data.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 drivers/virtio/virtio_balloon.c |   28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 05f0a80..6c07793 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -46,8 +46,8 @@ struct virtio_balloon
 	unsigned int num_pages;
 	struct list_head pages;
 
-	/* The array of pfns we tell the Host about. */
-	unsigned int num_pfns;
+	/* Temporary buffer of pfns to pass to the host */
+	/* Store this here to avoid a too-large local array */
 	u32 pfns[256];
 
 	/* Memory statistics */
@@ -79,11 +79,12 @@ static void balloon_ack(struct virtqueue *vq)
 		complete(&vb->acked);
 }
 
-static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
+static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq,
+		      unsigned int n)
 {
 	struct scatterlist sg;
 
-	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
+	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * n);
 
 	init_completion(&vb->acked);
 
@@ -98,10 +99,12 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
 
 static void fill_balloon(struct virtio_balloon *vb, size_t num)
 {
+	unsigned int n;
+
 	/* We can only do one array worth at a time. */
 	num = min(num, ARRAY_SIZE(vb->pfns));
 
-	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
+	for (n = 0; n < num; n++) {
 		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
 					__GFP_NOMEMALLOC | __GFP_NOWARN);
 		if (!page) {
@@ -113,17 +116,17 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 			msleep(200);
 			break;
 		}
-		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
+		vb->pfns[n] = page_to_balloon_pfn(page);
 		totalram_pages--;
 		vb->num_pages++;
 		list_add(&page->lru, &vb->pages);
 	}
 
 	/* Didn't get any?  Oh well. */
-	if (vb->num_pfns == 0)
+	if (n == 0)
 		return;
 
-	tell_host(vb, vb->inflate_vq);
+	tell_host(vb, vb->inflate_vq, n);
 }
 
 static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
@@ -139,14 +142,15 @@ static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
 static void leak_balloon(struct virtio_balloon *vb, size_t num)
 {
 	struct page *page;
+	unsigned int n;
 
 	/* We can only do one array worth at a time. */
 	num = min(num, ARRAY_SIZE(vb->pfns));
 
-	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
+	for (n = 0; n < num; n++) {
 		page = list_first_entry(&vb->pages, struct page, lru);
 		list_del(&page->lru);
-		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
+		vb->pfns[n] = page_to_balloon_pfn(page);
 		vb->num_pages--;
 	}
 
@@ -155,8 +159,8 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
 	 * is true, we *have* to do it in this order
 	 */
-	tell_host(vb, vb->deflate_vq);
-	release_pages_by_pfn(vb->pfns, vb->num_pfns);
+	tell_host(vb, vb->deflate_vq, n);
+	release_pages_by_pfn(vb->pfns, n);
 }
 
 static inline void update_stat(struct virtio_balloon *vb, int idx,
-- 
1.7.9.5

[PATCH 2/3] virtio_balloon: Fix endian bug

[David Gibson]

Although virtio config space fields are usually in guest-native endian,
the spec for the virtio balloon device explicitly states that both fields
in its config space are little-endian.

However, the current virtio_balloon driver does not have a suitable endian
swap for the 'num_pages' field, although it does have one for the 'actual'
field.  This patch corrects the bug, adding sparse annotation while we're
at it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 drivers/virtio/virtio_balloon.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 6c07793..553cc1f 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -238,11 +238,14 @@ static void virtballoon_changed(struct virtio_device *vdev)
 
 static inline s64 towards_target(struct virtio_balloon *vb)
 {
-	u32 v;
+	__le32 v;
+	s64 target;
+
 	vb->vdev->config->get(vb->vdev,
 			      offsetof(struct virtio_balloon_config, num_pages),
 			      &v, sizeof(v));
-	return (s64)v - vb->num_pages;
+	target = le32_to_cpu(v);
+	return target - vb->num_pages;
 }
 
 static void update_balloon_size(struct virtio_balloon *vb)
-- 
1.7.9.5

[PATCH 3/3] virtio_balloon: Bugfixes for PAGE_SIZE != 4k

[David Gibson]

The virtio_balloon device is specced to always operate on 4k pages.  The
virtio_balloon driver has a feeble attempt at reconciling this with a
lerge kernel page size, but it is (a) exactly wrong (it shifts the pfn in
the wrong direction) and (b) insufficient (it doesn't issue multiple 4k
balloon requests for each guest page, or correct other accounting values
for the different in page size).

This patch fixes the various problems.  It has been tested with a powerpc
guest kernel configured for 64kB base page size, running under qemu.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 drivers/virtio/virtio_balloon.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 553cc1f..834b7f9 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -60,13 +60,20 @@ static struct virtio_device_id id_table[] = {
 	{ 0 },
 };
 
-static u32 page_to_balloon_pfn(struct page *page)
+#define BALLOON_PAGE_ORDER	(PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT)
+#define PAGES_PER_ARRAY(_a)	(ARRAY_SIZE(_a) >> BALLOON_PAGE_ORDER)
+
+static void page_to_balloon_pfns(u32 pfns[], unsigned int n, struct page *page)
 {
-	unsigned long pfn = page_to_pfn(page);
+	unsigned long bpfn = page_to_pfn(page) << BALLOON_PAGE_ORDER;
+	u32 *p = &pfns[n << BALLOON_PAGE_ORDER];
+	int i;
 
 	BUILD_BUG_ON(PAGE_SHIFT < VIRTIO_BALLOON_PFN_SHIFT);
-	/* Convert pfn from Linux page size to balloon page size. */
-	return pfn >> (PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT);
+
+	/* Enter a balloon pfn for each 4k subpage of the Linux page */
+	for (i = 0; i < (1 << BALLOON_PAGE_ORDER); i++)
+		p[i] = bpfn + i;
 }
 
 static void balloon_ack(struct virtqueue *vq)
@@ -84,7 +91,8 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq,
 {
 	struct scatterlist sg;
 
-	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * n);
+	sg_init_one(&sg, vb->pfns,
+		    sizeof(vb->pfns[0]) * (n << BALLOON_PAGE_ORDER));
 
 	init_completion(&vb->acked);
 
@@ -102,7 +110,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 	unsigned int n;
 
 	/* We can only do one array worth at a time. */
-	num = min(num, ARRAY_SIZE(vb->pfns));
+	num = min(num, PAGES_PER_ARRAY(vb->pfns));
 
 	for (n = 0; n < num; n++) {
 		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
@@ -116,7 +124,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 			msleep(200);
 			break;
 		}
-		vb->pfns[n] = page_to_balloon_pfn(page);
+		page_to_balloon_pfns(vb->pfns, n, page);
 		totalram_pages--;
 		vb->num_pages++;
 		list_add(&page->lru, &vb->pages);
@@ -134,7 +142,7 @@ static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
 	unsigned int i;
 
 	for (i = 0; i < num; i++) {
-		__free_page(pfn_to_page(pfns[i]));
+		__free_page(pfn_to_page(pfns[i << BALLOON_PAGE_ORDER]));
 		totalram_pages++;
 	}
 }
@@ -145,12 +153,12 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 	unsigned int n;
 
 	/* We can only do one array worth at a time. */
-	num = min(num, ARRAY_SIZE(vb->pfns));
+	num = min(num, PAGES_PER_ARRAY(vb->pfns));
 
 	for (n = 0; n < num; n++) {
 		page = list_first_entry(&vb->pages, struct page, lru);
 		list_del(&page->lru);
-		vb->pfns[n] = page_to_balloon_pfn(page);
+		page_to_balloon_pfns(vb->pfns, n, page);
 		vb->num_pages--;
 	}
 
@@ -244,13 +252,13 @@ static inline s64 towards_target(struct virtio_balloon *vb)
 	vb->vdev->config->get(vb->vdev,
 			      offsetof(struct virtio_balloon_config, num_pages),
 			      &v, sizeof(v));
-	target = le32_to_cpu(v);
+	target = le32_to_cpu(v) >> BALLOON_PAGE_ORDER;
 	return target - vb->num_pages;
 }
 
 static void update_balloon_size(struct virtio_balloon *vb)
 {
-	__le32 actual = cpu_to_le32(vb->num_pages);
+	__le32 actual = cpu_to_le32(vb->num_pages << BALLOON_PAGE_ORDER);
 
 	vb->vdev->config->set(vb->vdev,
 			      offsetof(struct virtio_balloon_config, actual),
-- 
1.7.9.5

Re: [PATCH 1/3] virtio_balloon: Remove unnecessarily persistent state

[Michael S. Tsirkin]

On Thu, Apr 12, 2012 at 03:36:33PM +1000, David Gibson wrote:
> The main virtio_balloon state structure contains the fields num_pfns and
> array 'pfns'.  Although they are stored here persistently, the lifetime of
> useful data in there is never more than one function - they're essentially
> used as though they were local variables.
> 
> For the pfns buffer, used to communicate a batch of pfns this is useful to
> avoid either transient kmalloc()s or putting too much data on the stack.
> For num_pfns, there is no reason it should not be a local, though.
> 
> This patch cleans things up by making num_pfns a local in the functions it
> is used in.  The pfns array remains, but the comment is updated to clarify
> that it contains no truly persistent data.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

nak

IMO it makes sense to keep the array size around together with the
array: either we use local storage for both or for neither.

OTOH a comment is a good thing to add, just make it more specific,
see below.

> ---
>  drivers/virtio/virtio_balloon.c |   28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 05f0a80..6c07793 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -46,8 +46,8 @@ struct virtio_balloon
>  	unsigned int num_pages;
>  	struct list_head pages;
>  
> -	/* The array of pfns we tell the Host about. */
> -	unsigned int num_pfns;
> +	/* Temporary buffer of pfns to pass to the host */

This needs to be more specific to be informative:
all things are temporary in this world :)

+ /* Valid for the duration of one command only */


> +	/* Store this here to avoid a too-large local array */
>  	u32 pfns[256];
>  
>  	/* Memory statistics */
> @@ -79,11 +79,12 @@ static void balloon_ack(struct virtqueue *vq)
>  		complete(&vb->acked);
>  }
>  
> -static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
> +static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq,
> +		      unsigned int n)
>  {
>  	struct scatterlist sg;
>  
> -	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> +	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * n);
>  
>  	init_completion(&vb->acked);
>  
> @@ -98,10 +99,12 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
>  
>  static void fill_balloon(struct virtio_balloon *vb, size_t num)
>  {
> +	unsigned int n;
> +
>  	/* We can only do one array worth at a time. */
>  	num = min(num, ARRAY_SIZE(vb->pfns));
>  
> -	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
> +	for (n = 0; n < num; n++) {
>  		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
>  					__GFP_NOMEMALLOC | __GFP_NOWARN);
>  		if (!page) {
> @@ -113,17 +116,17 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
>  			msleep(200);
>  			break;
>  		}
> -		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
> +		vb->pfns[n] = page_to_balloon_pfn(page);
>  		totalram_pages--;
>  		vb->num_pages++;
>  		list_add(&page->lru, &vb->pages);
>  	}
>  
>  	/* Didn't get any?  Oh well. */
> -	if (vb->num_pfns == 0)
> +	if (n == 0)
>  		return;
>  
> -	tell_host(vb, vb->inflate_vq);
> +	tell_host(vb, vb->inflate_vq, n);
>  }
>  
>  static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
> @@ -139,14 +142,15 @@ static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
>  static void leak_balloon(struct virtio_balloon *vb, size_t num)
>  {
>  	struct page *page;
> +	unsigned int n;
>  
>  	/* We can only do one array worth at a time. */
>  	num = min(num, ARRAY_SIZE(vb->pfns));
>  
> -	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
> +	for (n = 0; n < num; n++) {
>  		page = list_first_entry(&vb->pages, struct page, lru);
>  		list_del(&page->lru);
> -		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
> +		vb->pfns[n] = page_to_balloon_pfn(page);
>  		vb->num_pages--;
>  	}
>  
> @@ -155,8 +159,8 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
>  	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
>  	 * is true, we *have* to do it in this order
>  	 */
> -	tell_host(vb, vb->deflate_vq);
> -	release_pages_by_pfn(vb->pfns, vb->num_pfns);
> +	tell_host(vb, vb->deflate_vq, n);
> +	release_pages_by_pfn(vb->pfns, n);
>  }
>  
>  static inline void update_stat(struct virtio_balloon *vb, int idx,
> -- 
> 1.7.9.5

Re: [PATCH 1/3] virtio_balloon: Remove unnecessarily persistent state

[Michael S. Tsirkin]

On Thu, Apr 12, 2012 at 03:36:33PM +1000, David Gibson wrote:
> The main virtio_balloon state structure contains the fields num_pfns and
> array 'pfns'.  Although they are stored here persistently, the lifetime of
> useful data in there is never more than one function - they're essentially
> used as though they were local variables.
> 
> For the pfns buffer, used to communicate a batch of pfns this is useful to
> avoid either transient kmalloc()s or putting too much data on the stack.
> For num_pfns, there is no reason it should not be a local, though.
> 
> This patch cleans things up by making num_pfns a local in the functions it
> is used in.  The pfns array remains, but the comment is updated to clarify
> that it contains no truly persistent data.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Further, this is not a bugfix so pls drop it for now
at least until 3.5.

> ---
>  drivers/virtio/virtio_balloon.c |   28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 05f0a80..6c07793 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -46,8 +46,8 @@ struct virtio_balloon
>  	unsigned int num_pages;
>  	struct list_head pages;
>  
> -	/* The array of pfns we tell the Host about. */
> -	unsigned int num_pfns;
> +	/* Temporary buffer of pfns to pass to the host */
> +	/* Store this here to avoid a too-large local array */
>  	u32 pfns[256];
>  
>  	/* Memory statistics */
> @@ -79,11 +79,12 @@ static void balloon_ack(struct virtqueue *vq)
>  		complete(&vb->acked);
>  }
>  
> -static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
> +static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq,
> +		      unsigned int n)
>  {
>  	struct scatterlist sg;
>  
> -	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> +	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * n);
>  
>  	init_completion(&vb->acked);
>  
> @@ -98,10 +99,12 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
>  
>  static void fill_balloon(struct virtio_balloon *vb, size_t num)
>  {
> +	unsigned int n;
> +
>  	/* We can only do one array worth at a time. */
>  	num = min(num, ARRAY_SIZE(vb->pfns));
>  
> -	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
> +	for (n = 0; n < num; n++) {
>  		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
>  					__GFP_NOMEMALLOC | __GFP_NOWARN);
>  		if (!page) {
> @@ -113,17 +116,17 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
>  			msleep(200);
>  			break;
>  		}
> -		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
> +		vb->pfns[n] = page_to_balloon_pfn(page);
>  		totalram_pages--;
>  		vb->num_pages++;
>  		list_add(&page->lru, &vb->pages);
>  	}
>  
>  	/* Didn't get any?  Oh well. */
> -	if (vb->num_pfns == 0)
> +	if (n == 0)
>  		return;
>  
> -	tell_host(vb, vb->inflate_vq);
> +	tell_host(vb, vb->inflate_vq, n);
>  }
>  
>  static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
> @@ -139,14 +142,15 @@ static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
>  static void leak_balloon(struct virtio_balloon *vb, size_t num)
>  {
>  	struct page *page;
> +	unsigned int n;
>  
>  	/* We can only do one array worth at a time. */
>  	num = min(num, ARRAY_SIZE(vb->pfns));
>  
> -	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
> +	for (n = 0; n < num; n++) {
>  		page = list_first_entry(&vb->pages, struct page, lru);
>  		list_del(&page->lru);
> -		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
> +		vb->pfns[n] = page_to_balloon_pfn(page);
>  		vb->num_pages--;
>  	}
>  
> @@ -155,8 +159,8 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
>  	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
>  	 * is true, we *have* to do it in this order
>  	 */
> -	tell_host(vb, vb->deflate_vq);
> -	release_pages_by_pfn(vb->pfns, vb->num_pfns);
> +	tell_host(vb, vb->deflate_vq, n);
> +	release_pages_by_pfn(vb->pfns, n);
>  }
>  
>  static inline void update_stat(struct virtio_balloon *vb, int idx,
> -- 
> 1.7.9.5

Re: [PATCH 2/3] virtio_balloon: Fix endian bug

[Michael S. Tsirkin]

On Thu, Apr 12, 2012 at 03:36:34PM +1000, David Gibson wrote:
> Although virtio config space fields are usually in guest-native endian,
> the spec for the virtio balloon device explicitly states that both fields
> in its config space are little-endian.
> 
> However, the current virtio_balloon driver does not have a suitable endian
> swap for the 'num_pages' field, although it does have one for the 'actual'
> field.  This patch corrects the bug, adding sparse annotation while we're
> at it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Indeed. And qemu byte-swaps according to the spec, too.
Applied for 3.4, thanks.

> ---
>  drivers/virtio/virtio_balloon.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 6c07793..553cc1f 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -238,11 +238,14 @@ static void virtballoon_changed(struct virtio_device *vdev)
>  
>  static inline s64 towards_target(struct virtio_balloon *vb)
>  {
> -	u32 v;
> +	__le32 v;
> +	s64 target;
> +
>  	vb->vdev->config->get(vb->vdev,
>  			      offsetof(struct virtio_balloon_config, num_pages),
>  			      &v, sizeof(v));
> -	return (s64)v - vb->num_pages;
> +	target = le32_to_cpu(v);
> +	return target - vb->num_pages;
>  }
>  
>  static void update_balloon_size(struct virtio_balloon *vb)
> -- 
> 1.7.9.5

Re: [PATCH 3/3] virtio_balloon: Bugfixes for PAGE_SIZE != 4k

[Michael S. Tsirkin]

On Thu, Apr 12, 2012 at 03:36:35PM +1000, David Gibson wrote:
> The virtio_balloon device is specced to always operate on 4k pages.  The
> virtio_balloon driver has a feeble attempt at reconciling this with a
> lerge kernel page size, but it is (a) exactly wrong (it shifts the pfn in
> the wrong direction) and (b) insufficient (it doesn't issue multiple 4k
> balloon requests for each guest page, or correct other accounting values
> for the different in page size).
> 
> This patch fixes the various problems.  It has been tested with a powerpc
> guest kernel configured for 64kB base page size, running under qemu.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  drivers/virtio/virtio_balloon.c |   32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 553cc1f..834b7f9 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -60,13 +60,20 @@ static struct virtio_device_id id_table[] = {
>  	{ 0 },
>  };
>  
> -static u32 page_to_balloon_pfn(struct page *page)
> +#define BALLOON_PAGE_ORDER	(PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT)
> +#define PAGES_PER_ARRAY(_a)	(ARRAY_SIZE(_a) >> BALLOON_PAGE_ORDER)
> +
> +static void page_to_balloon_pfns(u32 pfns[], unsigned int n, struct page *page)
>  {
> -	unsigned long pfn = page_to_pfn(page);
> +	unsigned long bpfn = page_to_pfn(page) << BALLOON_PAGE_ORDER;
> +	u32 *p = &pfns[n << BALLOON_PAGE_ORDER];
> +	int i;
>  
>  	BUILD_BUG_ON(PAGE_SHIFT < VIRTIO_BALLOON_PFN_SHIFT);
> -	/* Convert pfn from Linux page size to balloon page size. */
> -	return pfn >> (PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT);
> +
> +	/* Enter a balloon pfn for each 4k subpage of the Linux page */
> +	for (i = 0; i < (1 << BALLOON_PAGE_ORDER); i++)
> +		p[i] = bpfn + i;
>  }
>  
>  static void balloon_ack(struct virtqueue *vq)
> @@ -84,7 +91,8 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq,
>  {
>  	struct scatterlist sg;
>  
> -	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * n);
> +	sg_init_one(&sg, vb->pfns,
> +		    sizeof(vb->pfns[0]) * (n << BALLOON_PAGE_ORDER));
>  
>  	init_completion(&vb->acked);
>  
> @@ -102,7 +110,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
>  	unsigned int n;
>  
>  	/* We can only do one array worth at a time. */
> -	num = min(num, ARRAY_SIZE(vb->pfns));
> +	num = min(num, PAGES_PER_ARRAY(vb->pfns));
>  
>  	for (n = 0; n < num; n++) {
>  		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
> @@ -116,7 +124,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
>  			msleep(200);
>  			break;
>  		}
> -		vb->pfns[n] = page_to_balloon_pfn(page);
> +		page_to_balloon_pfns(vb->pfns, n, page);
>  		totalram_pages--;
>  		vb->num_pages++;
>  		list_add(&page->lru, &vb->pages);
> @@ -134,7 +142,7 @@ static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
>  	unsigned int i;
>  
>  	for (i = 0; i < num; i++) {
> -		__free_page(pfn_to_page(pfns[i]));
> +		__free_page(pfn_to_page(pfns[i << BALLOON_PAGE_ORDER]));
>  		totalram_pages++;
>  	}
>  }
> @@ -145,12 +153,12 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
>  	unsigned int n;
>  
>  	/* We can only do one array worth at a time. */
> -	num = min(num, ARRAY_SIZE(vb->pfns));
> +	num = min(num, PAGES_PER_ARRAY(vb->pfns));
>  
>  	for (n = 0; n < num; n++) {
>  		page = list_first_entry(&vb->pages, struct page, lru);
>  		list_del(&page->lru);
> -		vb->pfns[n] = page_to_balloon_pfn(page);
> +		page_to_balloon_pfns(vb->pfns, n, page);
>  		vb->num_pages--;
>  	}
>  
> @@ -244,13 +252,13 @@ static inline s64 towards_target(struct virtio_balloon *vb)
>  	vb->vdev->config->get(vb->vdev,
>  			      offsetof(struct virtio_balloon_config, num_pages),
>  			      &v, sizeof(v));
> -	target = le32_to_cpu(v);
> +	target = le32_to_cpu(v) >> BALLOON_PAGE_ORDER;
>  	return target - vb->num_pages;
>  }
>  
>  static void update_balloon_size(struct virtio_balloon *vb)
>  {
> -	__le32 actual = cpu_to_le32(vb->num_pages);
> +	__le32 actual = cpu_to_le32(vb->num_pages << BALLOON_PAGE_ORDER);
>  
>  	vb->vdev->config->set(vb->vdev,
>  			      offsetof(struct virtio_balloon_config, actual),
> -- 
> 1.7.9.5

Good catch!

Unfortunately I find the approach a bit convoluted.
It also looks like when host asks for 5 balloon pages
you interpret this as 0 where 16 is probably saner
on a 64K system.

I think it's easier if we just keep doing math in
balloon pages. I also get confused by shift operations,
IMO / and * are clearer where they are applicable.
Something like the below would make more sense I think.

I also wrote up a detailed commit log, so we have
the bugs and the expected consequences listed explicitly.

I'm out of time for this week - so completely untested, sorry.
Maybe you could try this out? That would be great.
Thanks!

--->
virtio_balloon: fix handling of PAGE_SIZE != 4k

As reported by David Gibson, current code handles PAGE_SIZE != 4k
completely wrong which can lead to guest memory corruption errors.

- page_to_balloon_pfn is wrong: e.g. on system with 64K page size
 it gives the same pfn value for 16 different pages.

- we also need to convert back to linux pfns when we free.

- for each linux page we need to tell host about multiple balloon
  pages, but code only adds one pfn to the array.

Warning: patch is completely untested.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 9e95ca6..e641e35 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -60,13 +60,23 @@ static struct virtio_device_id id_table[] = {
 	{ 0 },
 };
 
+/* Balloon device works in 4K page units. So each page is
+ * pointed to by multiple balloon pages. */
+#define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
+
 static u32 page_to_balloon_pfn(struct page *page)
 {
 	unsigned long pfn = page_to_pfn(page);
 
 	BUILD_BUG_ON(PAGE_SHIFT < VIRTIO_BALLOON_PFN_SHIFT);
 	/* Convert pfn from Linux page size to balloon page size. */
-	return pfn >> (PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT);
+	return pfn * VIRTIO_BALLOON_PAGES_PER_PAGE;
+}
+
+static struct page *balloon_pfn_to_page(u32 pfn)
+{
+	BUG_ON(pfn % VIRTIO_BALLOON_PAGES_PER_PAGE);
+	return pfn_to_page(pfn / VIRTIO_BALLOON_PAGES_PER_PAGE);
 }
 
 static void balloon_ack(struct virtqueue *vq)
@@ -96,12 +106,24 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
 	wait_for_completion(&vb->acked);
 }
 
+static void set_page_pfns(u32 pfns[], struct page *page)
+{
+	unsigned int i;
+
+	/* Set balloon pfns pointing at this page.
+	 * Note that the first pfn points at start of the page. */
+	for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
+		pfns[i] = page_to_balloon_pfn(page) + i;
+}
+
 static void fill_balloon(struct virtio_balloon *vb, size_t num)
 {
 	/* We can only do one array worth at a time. */
 	num = min(num, ARRAY_SIZE(vb->pfns));
 
-	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
+	for (vb->num_pfns = 0; vb->num_pfns < num;
+	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE)
+		int i;
 		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
 					__GFP_NOMEMALLOC | __GFP_NOWARN);
 		if (!page) {
@@ -113,9 +135,9 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 			msleep(200);
 			break;
 		}
-		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
+		set_page_pfns(vb->pfns + vb->num_pfns, page);
+		vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
 		totalram_pages--;
-		vb->num_pages++;
 		list_add(&page->lru, &vb->pages);
 	}
 
@@ -130,8 +152,9 @@ static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
 {
 	unsigned int i;
 
-	for (i = 0; i < num; i++) {
-		__free_page(pfn_to_page(pfns[i]));
+	/* Find pfns pointing at start of each page, get pages and free them. */
+	for (i = 0; i < num; i += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+		__free_page(balloon_pfn_to_page(pfns[i]));
 		totalram_pages++;
 	}
 }
@@ -143,11 +166,12 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 	/* We can only do one array worth at a time. */
 	num = min(num, ARRAY_SIZE(vb->pfns));
 
-	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
+	for (vb->num_pfns = 0; vb->num_pfns < num;
+	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE)
 		page = list_first_entry(&vb->pages, struct page, lru);
 		list_del(&page->lru);
-		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
-		vb->num_pages--;
+		set_page_pfns(vb->pfns, &vb->num_pfns, page);
+		vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
 	}
 
 	/*

Re: [PATCH 3/3] virtio_balloon: Bugfixes for PAGE_SIZE != 4k

[David Gibson]

On Thu, Apr 12, 2012 at 01:14:06PM +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 12, 2012 at 03:36:35PM +1000, David Gibson wrote:
[snip]
> Good catch!
> 
> Unfortunately I find the approach a bit convoluted.
> It also looks like when host asks for 5 balloon pages
> you interpret this as 0 where 16 is probably saner
> on a 64K system.

Hm, true.  Although qemu at least actuall operates in units of
megabytes on the balloon, so I doubt it matters much in practice.

> I think it's easier if we just keep doing math in
> balloon pages. I also get confused by shift operations,
> IMO / and * are clearer where they are applicable.
> Something like the below would make more sense I think.

Sure.  I thught working in local pages was clearer, but I don't really
care.


> I also wrote up a detailed commit log, so we have
> the bugs and the expected consequences listed explicitly.
> 
> I'm out of time for this week - so completely untested, sorry.
> Maybe you could try this out? That would be great.
> Thanks!

Your patch has numerous syntax errors, but once those are fixed seems
to work fine with a 64k ppc64 kernel.  Fixed version below.  I did add
one comment, to note that with this change the num_pages field in the
vb is no longer the same as the number of elements in the pages list.
Nothing in the code relies on that, but it would probably be the first
assumption of someone looking at the structure definition.

Please apply.

From cf62f576ae1b26426d9325da08826e39db7031a1 Mon Sep 17 00:00:00 2001
From: David Gibson <david@gibson.dropbear.id.au>
Date: Fri, 13 Apr 2012 12:57:40 +1000
Subject: [PATCH] virtio_balloon: fix handling of PAGE_SIZE != 4k

As reported by David Gibson, current code handles PAGE_SIZE != 4k
completely wrong which can lead to guest memory corruption errors.

- page_to_balloon_pfn is wrong: e.g. on system with 64K page size
 it gives the same pfn value for 16 different pages.

- we also need to convert back to linux pfns when we free.

- for each linux page we need to tell host about multiple balloon
  pages, but code only adds one pfn to the array.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 drivers/virtio/virtio_balloon.c |   44 +++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 9e95ca6..0c0c1c3 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -43,6 +43,9 @@ struct virtio_balloon
 	struct completion acked;
 
 	/* The pages we've told the Host we're not using. */
+	/* Note that num_pages is measured in 4k balloon pages, so if
+	 * PAGE_SIZE != 4k, it will be a multiple of the actual number
+	 * of elements in the pages list */
 	unsigned int num_pages;
 	struct list_head pages;
 
@@ -60,13 +63,23 @@ static struct virtio_device_id id_table[] = {
 	{ 0 },
 };
 
+/* Balloon device works in 4K page units. So each page is
+ * pointed to by multiple balloon pages. */
+#define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
+
 static u32 page_to_balloon_pfn(struct page *page)
 {
 	unsigned long pfn = page_to_pfn(page);
 
 	BUILD_BUG_ON(PAGE_SHIFT < VIRTIO_BALLOON_PFN_SHIFT);
 	/* Convert pfn from Linux page size to balloon page size. */
-	return pfn >> (PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT);
+	return pfn * VIRTIO_BALLOON_PAGES_PER_PAGE;
+}
+
+static struct page *balloon_pfn_to_page(u32 pfn)
+{
+	BUG_ON(pfn % VIRTIO_BALLOON_PAGES_PER_PAGE);
+	return pfn_to_page(pfn / VIRTIO_BALLOON_PAGES_PER_PAGE);
 }
 
 static void balloon_ack(struct virtqueue *vq)
@@ -96,12 +109,23 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
 	wait_for_completion(&vb->acked);
 }
 
+static void set_page_pfns(u32 pfns[], struct page *page)
+{
+	unsigned int i;
+
+	/* Set balloon pfns pointing at this page.
+	 * Note that the first pfn points at start of the page. */
+	for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
+		pfns[i] = page_to_balloon_pfn(page) + i;
+}
+
 static void fill_balloon(struct virtio_balloon *vb, size_t num)
 {
 	/* We can only do one array worth at a time. */
 	num = min(num, ARRAY_SIZE(vb->pfns));
 
-	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
+	for (vb->num_pfns = 0; vb->num_pfns < num;
+	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
 		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
 					__GFP_NOMEMALLOC | __GFP_NOWARN);
 		if (!page) {
@@ -113,9 +137,9 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
 			msleep(200);
 			break;
 		}
-		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
+		set_page_pfns(vb->pfns + vb->num_pfns, page);
+		vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
 		totalram_pages--;
-		vb->num_pages++;
 		list_add(&page->lru, &vb->pages);
 	}
 
@@ -130,8 +154,9 @@ static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
 {
 	unsigned int i;
 
-	for (i = 0; i < num; i++) {
-		__free_page(pfn_to_page(pfns[i]));
+	/* Find pfns pointing at start of each page, get pages and free them. */
+	for (i = 0; i < num; i += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+		__free_page(balloon_pfn_to_page(pfns[i]));
 		totalram_pages++;
 	}
 }
@@ -143,11 +168,12 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 	/* We can only do one array worth at a time. */
 	num = min(num, ARRAY_SIZE(vb->pfns));
 
-	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
+	for (vb->num_pfns = 0; vb->num_pfns < num;
+	     vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
 		page = list_first_entry(&vb->pages, struct page, lru);
 		list_del(&page->lru);
-		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
-		vb->num_pages--;
+		set_page_pfns(vb->pfns + vb->num_pfns, page);
+		vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
 	}
 
 	/*
-- 
1.7.9.5



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 3/3] virtio_balloon: Bugfixes for PAGE_SIZE != 4k

[Michael S. Tsirkin]

On Fri, Apr 13, 2012 at 01:12:11PM +1000, David Gibson wrote:
> On Thu, Apr 12, 2012 at 01:14:06PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Apr 12, 2012 at 03:36:35PM +1000, David Gibson wrote:
> [snip]
> > Good catch!
> > 
> > Unfortunately I find the approach a bit convoluted.
> > It also looks like when host asks for 5 balloon pages
> > you interpret this as 0 where 16 is probably saner
> > on a 64K system.
> 
> Hm, true.  Although qemu at least actuall operates in units of
> megabytes on the balloon, so I doubt it matters much in practice.
> 
> > I think it's easier if we just keep doing math in
> > balloon pages. I also get confused by shift operations,
> > IMO / and * are clearer where they are applicable.
> > Something like the below would make more sense I think.
> 
> Sure.  I thught working in local pages was clearer, but I don't really
> care.
> 
> 
> > I also wrote up a detailed commit log, so we have
> > the bugs and the expected consequences listed explicitly.
> > 
> > I'm out of time for this week - so completely untested, sorry.
> > Maybe you could try this out? That would be great.
> > Thanks!
> 
> Your patch has numerous syntax errors, but once those are fixed seems
> to work fine with a 64k ppc64 kernel.  Fixed version below.  I did add
> one comment, to note that with this change the num_pages field in the
> vb is no longer the same as the number of elements in the pages list.
> Nothing in the code relies on that, but it would probably be the first
> assumption of someone looking at the structure definition.

Good point. Although this really applies to all other
memory counters as well, so I put this at top of the file.

> Please apply.

Patch applied, thanks very much for the testing!