[PATCH] virtio-balloon: do not attempt to release more than available pages

[PATCH] virtio-balloon: do not attempt to release more than available pages

[Marcelo Tosatti]

Handle the case where the balloon target is larger than total ram size.

BUG: unable to handle kernel paging request at 0000000000100100
IP: [<ffffffff881970f9>] :virtio_balloon:leak__balloon+0x2e/0xbe

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: virtio/virtio_balloon.c
===================================================================
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -122,10 +122,21 @@ static void release_pages_by_pfn(const u
 	}
 }
 
+static void update_target_size(struct virtio_balloon *vb)
+{
+	__le32 num_pages = cpu_to_le32(vb->num_pages);
+
+	vb->vdev->config->set(vb->vdev,
+			      offsetof(struct virtio_balloon_config, num_pages),
+			      &num_pages, sizeof(num_pages));
+}
+
+
 static void leak_balloon(struct virtio_balloon *vb, size_t num)
 {
 	struct page *page;
 
+	num = min_t(unsigned int, vb->num_pages, num);
 	/* We can only do one array worth at a time. */
 	num = min(num, ARRAY_SIZE(vb->pfns));
 
@@ -136,6 +147,12 @@ static void leak_balloon(struct virtio_b
 		vb->num_pages--;
 	}
 
+	/* Have nothing to release? update target accordingly. */
+	if (vb->num_pfns == 0) {
+		update_target_size(vb);
+		return;
+	}
+
 	if (vb->tell_host_first) {
 		tell_host(vb, vb->deflate_vq);
 		release_pages_by_pfn(vb->pfns, vb->num_pfns);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Avi Kivity]

Marcelo Tosatti wrote:
> Handle the case where the balloon target is larger than total ram size.
>
> BUG: unable to handle kernel paging request at 0000000000100100
> IP: [<ffffffff881970f9>] :virtio_balloon:leak__balloon+0x2e/0xbe
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: virtio/virtio_balloon.c
> ===================================================================
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -122,10 +122,21 @@ static void release_pages_by_pfn(const u
>  	}
>  }
>  
> +static void update_target_size(struct virtio_balloon *vb)
> +{
> +	__le32 num_pages = cpu_to_le32(vb->num_pages);
> +
> +	vb->vdev->config->set(vb->vdev,
> +			      offsetof(struct virtio_balloon_config, num_pages),
> +			      &num_pages, sizeof(num_pages));
> +}
>   

The target is host-owned; moreover the problem may be temporary, but 
you've changed the target permanently.

Suggest sending the host a message (like the page list) indicating it 
couldn't allocate any more.

Also, we may have driven the guest close to oom with this.  We need to 
notify the host when the guest gets into a low-memory cannot swap condition.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Marcelo Tosatti]

On Wed, Mar 05, 2008 at 06:59:10PM +0200, Avi Kivity wrote:
> Marcelo Tosatti wrote:
> >Handle the case where the balloon target is larger than total ram size.
> >
> >BUG: unable to handle kernel paging request at 0000000000100100
> >IP: [<ffffffff881970f9>] :virtio_balloon:leak__balloon+0x2e/0xbe
> >
> >Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >
> >Index: virtio/virtio_balloon.c
> >===================================================================
> >--- a/drivers/virtio/virtio_balloon.c
> >+++ b/drivers/virtio/virtio_balloon.c
> >@@ -122,10 +122,21 @@ static void release_pages_by_pfn(const u
> > 	}
> > }
> > 
> >+static void update_target_size(struct virtio_balloon *vb)
> >+{
> >+	__le32 num_pages = cpu_to_le32(vb->num_pages);
> >+
> >+	vb->vdev->config->set(vb->vdev,
> >+			      offsetof(struct virtio_balloon_config, 
> >num_pages),
> >+			      &num_pages, sizeof(num_pages));
> >+}
> >  
> 
> The target is host-owned; moreover the problem may be temporary, but 
> you've changed the target permanently.
> 
> Suggest sending the host a message (like the page list) indicating it 
> couldn't allocate any more.
> 
> Also, we may have driven the guest close to oom with this.  We need to 
> notify the host when the guest gets into a low-memory cannot swap condition.

I guess the description was not clear, you understood the opposite.

The problem is when the target for total guest pages (not balloon target
size) is set to be larger than the amount of total pages the guest has
booted with. What happens then is that the driver tries to release pages
from the balloon, without checking if there are any:

static void leak_balloon(struct virtio_balloon *vb, size_t num)
{
        struct page *page;

	/* 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++) {
                page = list_first_entry(&vb->pages, struct page, lru);
                list_del(&page->lru);
                vb->pfns[vb->num_pfns] = page_to_pfn(page);
                vb->num_pages--;
        }

vp->pages is empty here.

So the patch checks for the availability of ballooned pages before
attempting to release any, and sets num_pages to match that. 

The host should not allow that to condition to happen, but its still
fragile code in the guest driver.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Avi Kivity]

Marcelo Tosatti wrote:
> On Wed, Mar 05, 2008 at 06:59:10PM +0200, Avi Kivity wrote:
>   
>> Marcelo Tosatti wrote:
>>     
>>> Handle the case where the balloon target is larger than total ram size.
>>>
>>> BUG: unable to handle kernel paging request at 0000000000100100
>>> IP: [<ffffffff881970f9>] :virtio_balloon:leak__balloon+0x2e/0xbe
>>>
>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>
>>> Index: virtio/virtio_balloon.c
>>> ===================================================================
>>> --- a/drivers/virtio/virtio_balloon.c
>>> +++ b/drivers/virtio/virtio_balloon.c
>>> @@ -122,10 +122,21 @@ static void release_pages_by_pfn(const u
>>> 	}
>>> }
>>>
>>> +static void update_target_size(struct virtio_balloon *vb)
>>> +{
>>> +	__le32 num_pages = cpu_to_le32(vb->num_pages);
>>> +
>>> +	vb->vdev->config->set(vb->vdev,
>>> +			      offsetof(struct virtio_balloon_config, 
>>> num_pages),
>>> +			      &num_pages, sizeof(num_pages));
>>> +}
>>>  
>>>       
>> The target is host-owned; moreover the problem may be temporary, but 
>> you've changed the target permanently.
>>
>> Suggest sending the host a message (like the page list) indicating it 
>> couldn't allocate any more.
>>
>> Also, we may have driven the guest close to oom with this.  We need to 
>> notify the host when the guest gets into a low-memory cannot swap condition.
>>     
>
> I guess the description was not clear, you understood the opposite.
>
> The problem is when the target for total guest pages (not balloon target
> size) is set to be larger than the amount of total pages the guest has
> booted with. What happens then is that the driver tries to release pages
> from the balloon, without checking if there are any:
>
> static void leak_balloon(struct virtio_balloon *vb, size_t num)
> {
>         struct page *page;
>
> 	/* 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++) {
>                 page = list_first_entry(&vb->pages, struct page, lru);
>                 list_del(&page->lru);
>                 vb->pfns[vb->num_pfns] = page_to_pfn(page);
>                 vb->num_pages--;
>         }
>
> vp->pages is empty here.
>
> So the patch checks for the availability of ballooned pages before
> attempting to release any, and sets num_pages to match that. 
>
> The host should not allow that to condition to happen, but its still
> fragile code in the guest driver.
>
>   

Ah, I see.  We could simply ignore it, or adjust the target as you did.  
Not sure what is better.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Anthony Liguori]

Marcelo Tosatti wrote:
> On Wed, Mar 05, 2008 at 06:59:10PM +0200, Avi Kivity wrote:
>   
>> Marcelo Tosatti wrote:
>>     
>>> Handle the case where the balloon target is larger than total ram size.
>>>
>>> BUG: unable to handle kernel paging request at 0000000000100100
>>> IP: [<ffffffff881970f9>] :virtio_balloon:leak__balloon+0x2e/0xbe
>>>
>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>
>>> Index: virtio/virtio_balloon.c
>>> ===================================================================
>>> --- a/drivers/virtio/virtio_balloon.c
>>> +++ b/drivers/virtio/virtio_balloon.c
>>> @@ -122,10 +122,21 @@ static void release_pages_by_pfn(const u
>>> 	}
>>> }
>>>
>>> +static void update_target_size(struct virtio_balloon *vb)
>>> +{
>>> +	__le32 num_pages = cpu_to_le32(vb->num_pages);
>>> +
>>> +	vb->vdev->config->set(vb->vdev,
>>> +			      offsetof(struct virtio_balloon_config, 
>>> num_pages),
>>> +			      &num_pages, sizeof(num_pages));
>>> +}
>>>  
>>>       
>> The target is host-owned; moreover the problem may be temporary, but 
>> you've changed the target permanently.
>>
>> Suggest sending the host a message (like the page list) indicating it 
>> couldn't allocate any more.
>>
>> Also, we may have driven the guest close to oom with this.  We need to 
>> notify the host when the guest gets into a low-memory cannot swap condition.
>>     
>
> I guess the description was not clear, you understood the opposite.
>
> The problem is when the target for total guest pages (not balloon target
> size) is set to be larger than the amount of total pages the guest has
> booted with. What happens then is that the driver tries to release pages
> from the balloon, without checking if there are any:
>   

target in the config space is target balloon size, not target for total 
guest pages.  So how is it ever possible for this condition to occur?

Regards,

Anthony Liguori

> static void leak_balloon(struct virtio_balloon *vb, size_t num)
> {
>         struct page *page;
>
> 	/* 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++) {
>                 page = list_first_entry(&vb->pages, struct page, lru);
>                 list_del(&page->lru);
>                 vb->pfns[vb->num_pfns] = page_to_pfn(page);
>                 vb->num_pages--;
>         }
>
> vp->pages is empty here.
>
> So the patch checks for the availability of ballooned pages before
> attempting to release any, and sets num_pages to match that. 
>
> The host should not allow that to condition to happen, but its still
> fragile code in the guest driver.
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2008.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Marcelo Tosatti]

> >I guess the description was not clear, you understood the opposite.
> >
> >The problem is when the target for total guest pages (not balloon target
> >size) is set to be larger than the amount of total pages the guest has
> >booted with. What happens then is that the driver tries to release pages
> >from the balloon, without checking if there are any:
> >  
> 
> target in the config space is target balloon size, not target for total 
> guest pages.  So how is it ever possible for this condition to occur?

Set the target in QEMU to be larger than guest ram size. The config
space variable will be set negatively, so guest attempts to release
pages from the balloon.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Anthony Liguori]

Marcelo Tosatti wrote:
>>> I guess the description was not clear, you understood the opposite.
>>>
>>> The problem is when the target for total guest pages (not balloon target
>>> size) is set to be larger than the amount of total pages the guest has
>>> booted with. What happens then is that the driver tries to release pages
>>>       
>> >from the balloon, without checking if there are any:
>>     
>>>  
>>>       
>> target in the config space is target balloon size, not target for total 
>> guest pages.  So how is it ever possible for this condition to occur?
>>     
>
> Set the target in QEMU to be larger than guest ram size. The config
> space variable will be set negatively, so guest attempts to release
> pages from the balloon.
>
>   

Is an __le32 signed?  If so, we should just use an unsigned type.

Regards,

Anthony Liguori


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Avi Kivity]

Anthony Liguori wrote:
>>
>> Set the target in QEMU to be larger than guest ram size. The config
>> space variable will be set negatively, so guest attempts to release
>> pages from the balloon.
>>
>>   
>>     
>
> Is an __le32 signed?  If so, we should just use an unsigned type.
>
>   

That would balloon the guest into oblivion if the same condition happened.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Anthony Liguori]

Avi Kivity wrote:
> Marcelo Tosatti wrote:
>   
>> Handle the case where the balloon target is larger than total ram size.
>>
>> BUG: unable to handle kernel paging request at 0000000000100100
>> IP: [<ffffffff881970f9>] :virtio_balloon:leak__balloon+0x2e/0xbe
>>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> Index: virtio/virtio_balloon.c
>> ===================================================================
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -122,10 +122,21 @@ static void release_pages_by_pfn(const u
>>  	}
>>  }
>>  
>> +static void update_target_size(struct virtio_balloon *vb)
>> +{
>> +	__le32 num_pages = cpu_to_le32(vb->num_pages);
>> +
>> +	vb->vdev->config->set(vb->vdev,
>> +			      offsetof(struct virtio_balloon_config, num_pages),
>> +			      &num_pages, sizeof(num_pages));
>> +}
>>   
>>     
>
> The target is host-owned; moreover the problem may be temporary, but 
> you've changed the target permanently.
>
> Suggest sending the host a message (like the page list) indicating it 
> couldn't allocate any more.
>   

This is what the actual field in the config space is meant for.

> Also, we may have driven the guest close to oom with this.  We need to 
> notify the host when the guest gets into a low-memory cannot swap condition.
>   

We need an OOM handler and a high seek-cost shrinker to ensure we do not 
go too low I imagine.  We can notify the host of when we refuse to go 
lower with actual in the config space.

Regards,

Anthony Liguori



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Rusty Russell]

On Thursday 06 March 2008 03:28:32 Marcelo Tosatti wrote:
> Handle the case where the balloon target is larger than total ram size.
>
> BUG: unable to handle kernel paging request at 0000000000100100
> IP: [<ffffffff881970f9>] :virtio_balloon:leak__balloon+0x2e/0xbe
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Hi Marcelo,

    My deepest concern is that we're papering over some real bug, if you 
tripped this.  Zeroing num_pages makes sense as a hack, but we're clearly in 
trouble at this point and there should be some indication of that.

    If I may ask, how did this happen?

Thanks,
Rusty.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Marcelo Tosatti]

On Thu, Mar 06, 2008 at 09:39:48AM +1100, Rusty Russell wrote:
> On Thursday 06 March 2008 03:28:32 Marcelo Tosatti wrote:
> > Handle the case where the balloon target is larger than total ram size.
> >
> > BUG: unable to handle kernel paging request at 0000000000100100
> > IP: [<ffffffff881970f9>] :virtio_balloon:leak__balloon+0x2e/0xbe
> >
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Hi Marcelo,
> 
>     My deepest concern is that we're papering over some real bug, if you 
> tripped this.  Zeroing num_pages makes sense as a hack, but we're clearly in 
> trouble at this point and there should be some indication of that.
> 
>     If I may ask, how did this happen?

Rusty,

QEMU allows the user to set a target larger than the memory size which
the guest was booted with:

        dev->num_pages = (ram_size - target) >> TARGET_PAGE_BITS;
        virtio_notify_config(&dev->vdev);

num_pages goes negative, so driver attempts deflate back -num_pages from
its balloon.

There is no other bug here, its just a matter of guest driver being
fragile. You can avoid that condition in the host backend:

--- virtio-balloon.c.orig       2008-03-08 14:39:51.000000000 -0300
+++ virtio-balloon.c    2008-03-08 14:41:08.000000000 -0300
@@ -113,6 +113,8 @@
     VirtIOBalloon *dev = opaque;

     if (target) {
+       if (target > ram_size)
+               target = ram_size;
        dev->num_pages = (ram_size - target) >> TARGET_PAGE_BITS;
        virtio_notify_config(&dev->vdev);
     }

But making the driver robust against it seems sensate. I agree that
zeroing num_pages is hackish. What do you suggest?

Thanks

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Rusty Russell]

On Sunday 09 March 2008 06:06:38 Marcelo Tosatti wrote:
> But making the driver robust against it seems sensate. I agree that
> zeroing num_pages is hackish. What do you suggest?

OK, after more discussion on IRC, this seems like the correct thing to do.

1) Allow more than 2G of pages.  It doesn't cost us much to break the 8TB
   barrier.  This is an ABI change, so best to do this now.
2) Handle the case where we get nonsense from the host, which causes us to
   wrap around.

How's this (compile-tested only!):


diff -r fd0c80dbbd95 drivers/virtio/virtio_balloon.c
--- a/drivers/virtio/virtio_balloon.c	Tue Mar 11 09:21:00 2008 +1100
+++ b/drivers/virtio/virtio_balloon.c	Tue Mar 11 11:25:52 2008 +1100
@@ -43,12 +43,12 @@ struct virtio_balloon
 	bool tell_host_first;
 
 	/* The pages we've told the Host we're not using. */
-	unsigned int num_pages;
+	unsigned long num_pages;
 	struct list_head pages;
 
 	/* The array of pfns we tell the Host about. */
 	unsigned int num_pfns;
-	u32 pfns[256];
+	u64 pfns[256];
 };
 
 static struct virtio_device_id id_table[] = {
@@ -83,17 +83,17 @@ static void tell_host(struct virtio_ball
 	wait_for_completion(&vb->acked);
 }
 
-static void fill_balloon(struct virtio_balloon *vb, size_t num)
+static void fill_balloon(struct virtio_balloon *vb, u64 num)
 {
 	/* We can only do one array worth at a time. */
-	num = min(num, ARRAY_SIZE(vb->pfns));
+	num = min_t(u64, num, ARRAY_SIZE(vb->pfns));
 
 	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
 		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY);
 		if (!page) {
 			if (printk_ratelimit())
 				dev_printk(KERN_INFO, &vb->vdev->dev,
-					   "Out of puff! Can't get %zu pages\n",
+					   "Out of puff! Can't get %llu pages\n",
 					   num);
 			/* Sleep for at least 1/5 of a second before retry. */
 			msleep(200);
@@ -112,7 +112,7 @@ static void fill_balloon(struct virtio_b
 	tell_host(vb, vb->inflate_vq);
 }
 
-static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
+static void release_pages_by_pfn(const u64 pfns[], unsigned int num)
 {
 	unsigned int i;
 
@@ -122,12 +122,12 @@ static void release_pages_by_pfn(const u
 	}
 }
 
-static void leak_balloon(struct virtio_balloon *vb, size_t num)
+static void leak_balloon(struct virtio_balloon *vb, u64 num)
 {
 	struct page *page;
 
 	/* We can only do one array worth at a time. */
-	num = min(num, ARRAY_SIZE(vb->pfns));
+	num = min_t(u64, num, ARRAY_SIZE(vb->pfns));
 
 	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
 		page = list_first_entry(&vb->pages, struct page, lru);
@@ -152,12 +152,19 @@ static void virtballoon_changed(struct v
 	wake_up(&vb->config_change);
 }
 
-static inline int towards_target(struct virtio_balloon *vb)
+static inline s64 towards_target(struct virtio_balloon *vb)
 {
-	u32 v;
+	u64 v;
 	__virtio_config_val(vb->vdev,
 			    offsetof(struct virtio_balloon_config, num_pages),
 			    &v);
+
+	/* If they ask for something which will wrap, we ignore it. */
+	if (v & 0x8000000000000000ULL) {
+		dev_warn(&vb->vdev->dev, "Insane target 0x%llx\n", v);
+		return 0;
+	}
+
 	return v - vb->num_pages;
 }
 
@@ -176,7 +183,7 @@ static int balloon(void *_vballoon)
 
 	set_freezable();
 	while (!kthread_should_stop()) {
-		int diff;
+		s64 diff;
 
 		try_to_freeze();
 		wait_event_interruptible(vb->config_change,
diff -r fd0c80dbbd95 include/linux/virtio_balloon.h
--- a/include/linux/virtio_balloon.h	Tue Mar 11 09:21:00 2008 +1100
+++ b/include/linux/virtio_balloon.h	Tue Mar 11 11:25:52 2008 +1100
@@ -11,8 +11,8 @@ struct virtio_balloon_config
 struct virtio_balloon_config
 {
 	/* Number of pages host wants Guest to give up. */
-	__le32 num_pages;
+	__le64 num_pages;
 	/* Number of pages we've actually got in balloon. */
-	__le32 actual;
+	__le64 actual;
 };
 #endif /* _LINUX_VIRTIO_BALLOON_H */


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

Rusty Russell wrote:
> On Sunday 09 March 2008 06:06:38 Marcelo Tosatti wrote:
>   
>> But making the driver robust against it seems sensate. I agree that
>> zeroing num_pages is hackish. What do you suggest?
>>     
>
> OK, after more discussion on IRC, this seems like the correct thing to do.
>
> 1) Allow more than 2G of pages.  It doesn't cost us much to break the 8TB
>    barrier.  This is an ABI change, so best to do this now.
>   

I don't think there's a compelling need to break the ABI.  If we truly 
ever needed to break the 8TB barrier, we could simply introduce another 
ballooning device.  virtio_pci relies on PFNs fitting into a 32-bit 
value so breaking the 8TB barrier here doesn't given it to us for free 
anyway.

Plus, we've released an external drivers package.  Breaking the ABI at 
this stage is going to cause user confusion.  Sure, the ABI isn't stable 
until 2.6.25 ships but I do think we should have a more compelling 
reason to do it if we truly have to.

> 2) Handle the case where we get nonsense from the host, which causes us to
>    wrap around.
>   

Here's my neither compiled nor tested version which should fix this 
problem without breaking the ABI.

Regards,

Anthony Liguori

[-- Attachment #2: virtio:balloon_signage.patch --]
[-- Type: application/mbox, Size: 682 bytes --]

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 158 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Rusty Russell]

On Tuesday 11 March 2008 11:52:46 Anthony Liguori wrote:
> Rusty Russell wrote:
> > 2) Handle the case where we get nonsense from the host, which causes us
> > to wrap around.
>
> Here's my neither compiled nor tested version which should fix this
> problem without breaking the ABI.

Hmm, we could just refuse to grab >= 2G pages.  It is not only reasonable, but
also gives us an opportunity to make another Sophie Blanchard reference![1]

Patch below.

Cheers,
Rusty.
[1] Wow, I actually learned something from a Wikipedia Featured Article.

virtio: handle > 2 billion page balloon targets

If the host asks for a huge target towards_target() can overflow, and
we up oops as we try to release more pages than we have.  The simple
fix is to use a 64-bit value.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r fd0c80dbbd95 drivers/virtio/virtio_balloon.c
--- a/drivers/virtio/virtio_balloon.c	Tue Mar 11 09:21:00 2008 +1100
+++ b/drivers/virtio/virtio_balloon.c	Tue Mar 11 11:25:52 2008 +1100
@@ -152,7 +152,7 @@ static void virtballoon_changed(struct v
 	wake_up(&vb->config_change);
 }
 
-static inline int towards_target(struct virtio_balloon *vb)
+static inline s64 towards_target(struct virtio_balloon *vb)
 {
 	u32 v;
 	__virtio_config_val(vb->vdev,
@@ -176,7 +183,7 @@ static int balloon(void *_vballoon)
 
 	set_freezable();
 	while (!kthread_should_stop()) {
-		int diff;
+		s64 diff;
 
 		try_to_freeze();
 		wait_event_interruptible(vb->config_change,

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/