Xen devel - May 2013 - [PATCH] xl, e820_host, PV passthrough: Fix guests crashing when memory == maxmem

The code had an obvious bug where it would assume that the balloon
amount would always be _something_ and add an E820_RAM entry at the
end of the E820 array. The added E820_RAM would contain the balloon amount
plus the delta of memory that had to be subtracted b/c of the various
E820 entries. That assumption is certainly true when maxmem != mem,
but if guest config has maxmem = memory that is incorrect (as balloon
value is zero). The end result is that the E820 that is constructed
is missing a swath of "delta" memory and in most cases ends up with
only one E820_RAM entry that is of 512MB size on many Intel systems.

Reported-by: Christian Holpert <christian@holpert.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl_x86.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index a17f6ae..a78c91d 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -182,7 +182,7 @@ static int e820_sanitize(libxl_ctx *ctx, struct e820entry
src[],
         idx++;
     }
     /* At this point we have the mapped RAM + E820 entries from src. */
-    if (balloon_kb) {
+    if (balloon_kb || delta_kb) {
         /* and if we truncated the RAM region, then add it to the end. */
         e820[idx].type = E820_RAM;
         e820[idx].addr = (uint64_t)(1ULL << 32) > last ?
-- 
1.7.7.6

On Fri, 2013-05-24 at 09:36 -0400, Konrad Rzeszutek Wilk
wrote:
> The code had an obvious bug where it would assume that the balloon
> amount would always be _something_ and add an E820_RAM entry at the
> end of the E820 array. The added E820_RAM would contain the balloon amount
> plus the delta of memory that had to be subtracted b/c of the various
> E820 entries. That assumption is certainly true when maxmem != mem,
> but if guest config has maxmem = memory that is incorrect (as balloon
> value is zero). The end result is that the E820 that is constructed
> is missing a swath of "delta" memory and in most cases ends up
with
> only one E820_RAM entry that is of 512MB size on many Intel systems.
> 
> Reported-by: Christian Holpert <christian@holpert.de>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl_x86.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index a17f6ae..a78c91d 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -182,7 +182,7 @@ static int e820_sanitize(libxl_ctx *ctx, struct
e820entry src[],
>          idx++;
>      }
>      /* At this point we have the mapped RAM + E820 entries from src. */
> -    if (balloon_kb) {
> +    if (balloon_kb || delta_kb) {
>          /* and if we truncated the RAM region, then add it to the end. */
>          e820[idx].type = E820_RAM;
>          e820[idx].addr = (uint64_t)(1ULL << 32) > last ?

On Fri, May 24, 2013 at 02:58:24PM +0100, Ian Campbell
wrote:
> On Fri, 2013-05-24 at 09:36 -0400, Konrad Rzeszutek Wilk wrote:
> > The code had an obvious bug where it would assume that the balloon
> > amount would always be _something_ and add an E820_RAM entry at the
> > end of the E820 array. The added E820_RAM would contain the balloon
amount
> > plus the delta of memory that had to be subtracted b/c of the various
> > E820 entries. That assumption is certainly true when maxmem != mem,
> > but if guest config has maxmem = memory that is incorrect (as balloon
> > value is zero). The end result is that the E820 that is constructed
> > is missing a swath of "delta" memory and in most cases ends
up with
> > only one E820_RAM entry that is of 512MB size on many Intel systems.
> > 
> > Reported-by: Christian Holpert <christian@holpert.de>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
George,
Would you be OK putting this in Xen 4.3? It is only exposed if a user
 1) launches a PV guest
 2). Has in the guest config: pci=[''blabla'']\ne820_host=1
 3). Is not that freq used as only a couple of people seem to use it
> 
> > ---
> >  tools/libxl/libxl_x86.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> > index a17f6ae..a78c91d 100644
> > --- a/tools/libxl/libxl_x86.c
> > +++ b/tools/libxl/libxl_x86.c
> > @@ -182,7 +182,7 @@ static int e820_sanitize(libxl_ctx *ctx, struct
e820entry src[],
> >          idx++;
> >      }
> >      /* At this point we have the mapped RAM + E820 entries from src.
*/
> > -    if (balloon_kb) {
> > +    if (balloon_kb || delta_kb) {
> >          /* and if we truncated the RAM region, then add it to the
end. */
> >          e820[idx].type = E820_RAM;
> >          e820[idx].addr = (uint64_t)(1ULL << 32) > last ?
> 
>

On Fri, 2013-05-24 at 10:51 -0400, Konrad Rzeszutek Wilk
wrote:
> On Fri, May 24, 2013 at 02:58:24PM +0100, Ian Campbell wrote:
> > On Fri, 2013-05-24 at 09:36 -0400, Konrad Rzeszutek Wilk wrote:
> > > The code had an obvious bug where it would assume that the
balloon
> > > amount would always be _something_ and add an E820_RAM entry at
the
> > > end of the E820 array. The added E820_RAM would contain the
balloon amount
> > > plus the delta of memory that had to be subtracted b/c of the
various
> > > E820 entries. That assumption is certainly true when maxmem !=
mem,
> > > but if guest config has maxmem = memory that is incorrect (as
balloon
> > > value is zero). The end result is that the E820 that is
constructed
> > > is missing a swath of "delta" memory and in most cases
ends up with
> > > only one E820_RAM entry that is of 512MB size on many Intel
systems.
> > > 
> > > Reported-by: Christian Holpert <christian@holpert.de>
> > > Signed-off-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> George,
> Would you be OK putting this in Xen 4.3? It is only exposed if a user
>  1) launches a PV guest
>  2). Has in the guest config: pci=[''blabla'']\ne820_host=1
>  3). Is not that freq used as only a couple of people seem to use it
TBH I was just going to commit it as an obvious bug fix in my next sweep
of my queue (probably on Tuesday at this rate...)

I guess George has until then to object ;-)

Ian

On 24/05/13 15:55, Ian Campbell wrote:
> On Fri, 2013-05-24 at 10:51 -0400, Konrad Rzeszutek Wilk wrote:
>> On Fri, May 24, 2013 at 02:58:24PM +0100, Ian Campbell wrote:
>>> On Fri, 2013-05-24 at 09:36 -0400, Konrad Rzeszutek Wilk wrote:
>>>> The code had an obvious bug where it would assume that the
balloon
>>>> amount would always be _something_ and add an E820_RAM entry at
the
>>>> end of the E820 array. The added E820_RAM would contain the
balloon amount
>>>> plus the delta of memory that had to be subtracted b/c of the
various
>>>> E820 entries. That assumption is certainly true when maxmem !=
mem,
>>>> but if guest config has maxmem = memory that is incorrect (as
balloon
>>>> value is zero). The end result is that the E820 that is
constructed
>>>> is missing a swath of "delta" memory and in most
cases ends up with
>>>> only one E820_RAM entry that is of 512MB size on many Intel
systems.
>>>>
>>>> Reported-by: Christian Holpert <christian@holpert.de>
>>>> Signed-off-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>> George,
>> Would you be OK putting this in Xen 4.3? It is only exposed if a user
>>   1) launches a PV guest
>>   2). Has in the guest config:
pci=[''blabla'']\ne820_host=1
>>   3). Is not that freq used as only a couple of people seem to use it
> TBH I was just going to commit it as an obvious bug fix in my next sweep
> of my queue (probably on Tuesday at this rate...)
>
> I guess George has until then to object ;-)
Sounds like a bug fix -- no objections. :-)

  -George

On Fri, 2013-05-24 at 14:58 +0100, Ian Campbell wrote:
> On Fri, 2013-05-24 at 09:36 -0400, Konrad Rzeszutek Wilk wrote:
> > The code had an obvious bug where it would assume that the balloon
> > amount would always be _something_ and add an E820_RAM entry at the
> > end of the E820 array. The added E820_RAM would contain the balloon
amount
> > plus the delta of memory that had to be subtracted b/c of the various
> > E820 entries. That assumption is certainly true when maxmem != mem,
> > but if guest config has maxmem = memory that is incorrect (as balloon
> > value is zero). The end result is that the E820 that is constructed
> > is missing a swath of "delta" memory and in most cases ends
up with
> > only one E820_RAM entry that is of 512MB size on many Intel systems.
> > 
> > Reported-by: Christian Holpert <christian@holpert.de>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
And applied.