LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] xen: p2m: correctly initialize partial p2m leave
@ 2011-01-20 14:38 Stefan Bader
  2011-01-20 15:10 ` Konrad Rzeszutek Wilk
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Stefan Bader @ 2011-01-20 14:38 UTC (permalink / raw)
  To: Jeremy Fitzhardinge, Konrad Rzeszutek Wilk
  Cc: xen-devel, Linux Kernel Mailing List

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

There have been changes and code been moved around, so this is just a quick
rebase of the change I tested on a 2.6.37 based kernel. The basic problem seem
still valid, though.

Initially I thought of adding a cc to stable into the s-o-b, but the patch needs
to be adapted anyway (I can supply that version if the way I fixed the issue
looks ok).

Regards,
Stefan

[-- Attachment #2: 0001-xen-mmu-correctly-initialize-partial-p2m-leave.patch --]
[-- Type: text/x-diff, Size: 3124 bytes --]

>From 1e9c9514caf0399c88ae9288e6db8e3d1c4b4be5 Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
Date: Thu, 20 Jan 2011 11:37:43 +0100
Subject: [PATCH] xen: p2m: correctly initialize partial p2m leave

After changing the p2m mapping to a tree by

  commit 58e05027b530ff081ecea68e38de8d59db8f87e0
    xen: convert p2m to a 3 level tree

and trying to boot a DomU with 615MB of memory, the following crash was
observed in the dump:

kernel direct mapping tables up to 26f00000 @ 1ec4000-1fff000
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<c0107397>] xen_set_pte+0x27/0x60
*pdpt = 0000000000000000 *pde = 0000000000000000

Adding further debug statements showed that when trying to set up
pfn=0x26700 the returned mapping was invalid.

pfn=0x266ff calling set_pte(0xc1fe77f8, 0x6b3003)
pfn=0x26700 calling set_pte(0xc1fe7800, 0x3)

Although the last_pfn obtained from the startup info is 0x26700, which
should in turn not be hit, the additional 8MB which are added as extra
memory normally seem to be ok. This lead to looking into the initial
p2m tree construction, which uses the smaller value and assuming that
there is other code handling the extra memory.

When the p2m tree is set up, the leaves are directly pointed to the
array which the domain builder set up. But if the mapping is not on a
boundary that fits into one p2m page, this will result in the last leaf
being only partially valid. And as the invalid entries are not
initialized in that case, things go badly wrong.

I am trying to fix that by checking whether the current leaf is a
complete map and if not, allocate a completely new page and copy only
the valid pointers there. This may not be the most efficient or elegant
solution, but at least it seems to allow me booting DomUs with memory
assignments all over the range.

BugLink: http://bugs.launchpad.net/bugs/686692

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 arch/x86/xen/p2m.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 8f2251d..c9307ec 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -237,7 +237,25 @@ void __init xen_build_dynamic_phys_to_machine(void)
 			p2m_top[topidx] = mid;
 		}
 
-		p2m_top[topidx][mididx] = &mfn_list[pfn];
+		/*
+		 * As long as the mfn_list has enough entries to completely
+		 * fill a p2m page, pointing into the array is ok. But if
+		 * not the entries beyond the last pfn will be undefined.
+		 * And guessing that the 'what-ever-there-is' does not take it
+		 * too kindly when changing it to invalid markers, a new page
+		 * is allocated, initialized and filled with the valid part.
+		 */
+		if (unlikely(pfn + P2M_PER_PAGE > max_pfn)) {
+			unsigned long p2midx;
+			unsigned long **p2m = extend_brk(PAGE_SIZE, PAGE_SIZE);
+			p2m_init(p2m);
+
+			for (p2midx = 0; pfn + p2midx < max_pfn; p2midx++) {
+				p2m[p2midx] = mfn_list[pfn + p2midx];
+			}
+			p2m_top[topidx][mididx] = p2m;
+		} else
+			p2m_top[topidx][mididx] = &mfn_list[pfn];
 	}
 
 	m2p_override_init();
-- 
1.7.0.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xen: p2m: correctly initialize partial p2m leave
  2011-01-20 14:38 [PATCH] xen: p2m: correctly initialize partial p2m leave Stefan Bader
@ 2011-01-20 15:10 ` Konrad Rzeszutek Wilk
  2011-01-24  4:49   ` [Xen-devel] " Jeremy Fitzhardinge
  2011-01-21 14:26 ` [Xen-devel] " Konrad Rzeszutek Wilk
  2011-01-25 16:54 ` Ian Campbell
  2 siblings, 1 reply; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2011-01-20 15:10 UTC (permalink / raw)
  To: Stefan Bader; +Cc: Jeremy Fitzhardinge, xen-devel, Linux Kernel Mailing List

On Thu, Jan 20, 2011 at 03:38:23PM +0100, Stefan Bader wrote:
> There have been changes and code been moved around, so this is just a quick
> rebase of the change I tested on a 2.6.37 based kernel. The basic problem seem
> still valid, though.

Nice catch..
> 
> Initially I thought of adding a cc to stable into the s-o-b, but the patch needs
> to be adapted anyway (I can supply that version if the way I fixed the issue
> looks ok).
> 
> Regards,
> Stefan

> >From 1e9c9514caf0399c88ae9288e6db8e3d1c4b4be5 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Thu, 20 Jan 2011 11:37:43 +0100
> Subject: [PATCH] xen: p2m: correctly initialize partial p2m leave
> 
> After changing the p2m mapping to a tree by
> 
>   commit 58e05027b530ff081ecea68e38de8d59db8f87e0
>     xen: convert p2m to a 3 level tree
> 
> and trying to boot a DomU with 615MB of memory, the following crash was
> observed in the dump:
> 
> kernel direct mapping tables up to 26f00000 @ 1ec4000-1fff000
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<c0107397>] xen_set_pte+0x27/0x60
> *pdpt = 0000000000000000 *pde = 0000000000000000
> 
> Adding further debug statements showed that when trying to set up
> pfn=0x26700 the returned mapping was invalid.
> 
> pfn=0x266ff calling set_pte(0xc1fe77f8, 0x6b3003)
> pfn=0x26700 calling set_pte(0xc1fe7800, 0x3)
> 
> Although the last_pfn obtained from the startup info is 0x26700, which
> should in turn not be hit, the additional 8MB which are added as extra
> memory normally seem to be ok. This lead to looking into the initial
> p2m tree construction, which uses the smaller value and assuming that
> there is other code handling the extra memory.
> 
> When the p2m tree is set up, the leaves are directly pointed to the
> array which the domain builder set up. But if the mapping is not on a
> boundary that fits into one p2m page, this will result in the last leaf
> being only partially valid. And as the invalid entries are not
> initialized in that case, things go badly wrong.
> 
> I am trying to fix that by checking whether the current leaf is a
> complete map and if not, allocate a completely new page and copy only
> the valid pointers there. This may not be the most efficient or elegant
> solution, but at least it seems to allow me booting DomUs with memory
> assignments all over the range.
> 
> BugLink: http://bugs.launchpad.net/bugs/686692
> 
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  arch/x86/xen/p2m.c |   20 +++++++++++++++++++-
>  1 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 8f2251d..c9307ec 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -237,7 +237,25 @@ void __init xen_build_dynamic_phys_to_machine(void)
>  			p2m_top[topidx] = mid;
>  		}
>  
> -		p2m_top[topidx][mididx] = &mfn_list[pfn];
> +		/*
> +		 * As long as the mfn_list has enough entries to completely
> +		 * fill a p2m page, pointing into the array is ok. But if
> +		 * not the entries beyond the last pfn will be undefined.
> +		 * And guessing that the 'what-ever-there-is' does not take it
> +		 * too kindly when changing it to invalid markers, a new page
> +		 * is allocated, initialized and filled with the valid part.
> +		 */
> +		if (unlikely(pfn + P2M_PER_PAGE > max_pfn)) {
> +			unsigned long p2midx;
> +			unsigned long **p2m = extend_brk(PAGE_SIZE, PAGE_SIZE);
> +			p2m_init(p2m);
> +
> +			for (p2midx = 0; pfn + p2midx < max_pfn; p2midx++) {
> +				p2m[p2midx] = mfn_list[pfn + p2midx];
> +			}
> +			p2m_top[topidx][mididx] = p2m;
> +		} else
> +			p2m_top[topidx][mididx] = &mfn_list[pfn];
>  	}
>  
>  	m2p_override_init();
> -- 
> 1.7.0.4
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Xen-devel] [PATCH] xen: p2m: correctly initialize partial p2m leave
  2011-01-20 14:38 [PATCH] xen: p2m: correctly initialize partial p2m leave Stefan Bader
  2011-01-20 15:10 ` Konrad Rzeszutek Wilk
@ 2011-01-21 14:26 ` Konrad Rzeszutek Wilk
  2011-01-21 14:42   ` Stefan Bader
  2011-01-25 16:54 ` Ian Campbell
  2 siblings, 1 reply; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2011-01-21 14:26 UTC (permalink / raw)
  To: Stefan Bader; +Cc: Jeremy Fitzhardinge, xen-devel, Linux Kernel Mailing List

On Thu, Jan 20, 2011 at 03:38:23PM +0100, Stefan Bader wrote:
> There have been changes and code been moved around, so this is just a quick
> rebase of the change I tested on a 2.6.37 based kernel. The basic problem seem
> still valid, though.

Yup.
> 
> Initially I thought of adding a cc to stable into the s-o-b, but the patch needs
> to be adapted anyway (I can supply that version if the way I fixed the issue
> looks ok).

OK, let me send this upstream to Linus for adaption. I fixed one compile warning
but otherwsie it is the same. Look below for details.

And when that is done I would appreciate you sending a copy to stable.

> 
> Regards,
> Stefan

> >From 1e9c9514caf0399c88ae9288e6db8e3d1c4b4be5 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Thu, 20 Jan 2011 11:37:43 +0100
> Subject: [PATCH] xen: p2m: correctly initialize partial p2m leave
> 
> After changing the p2m mapping to a tree by
> 
>   commit 58e05027b530ff081ecea68e38de8d59db8f87e0
>     xen: convert p2m to a 3 level tree
> 
> and trying to boot a DomU with 615MB of memory, the following crash was
> observed in the dump:
> 
> kernel direct mapping tables up to 26f00000 @ 1ec4000-1fff000
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<c0107397>] xen_set_pte+0x27/0x60
> *pdpt = 0000000000000000 *pde = 0000000000000000
> 
> Adding further debug statements showed that when trying to set up
> pfn=0x26700 the returned mapping was invalid.
> 
> pfn=0x266ff calling set_pte(0xc1fe77f8, 0x6b3003)
> pfn=0x26700 calling set_pte(0xc1fe7800, 0x3)
> 
> Although the last_pfn obtained from the startup info is 0x26700, which
> should in turn not be hit, the additional 8MB which are added as extra
> memory normally seem to be ok. This lead to looking into the initial
> p2m tree construction, which uses the smaller value and assuming that
> there is other code handling the extra memory.
> 
> When the p2m tree is set up, the leaves are directly pointed to the
> array which the domain builder set up. But if the mapping is not on a
> boundary that fits into one p2m page, this will result in the last leaf
> being only partially valid. And as the invalid entries are not
> initialized in that case, things go badly wrong.
> 
> I am trying to fix that by checking whether the current leaf is a
> complete map and if not, allocate a completely new page and copy only
> the valid pointers there. This may not be the most efficient or elegant
> solution, but at least it seems to allow me booting DomUs with memory
> assignments all over the range.
> 
> BugLink: http://bugs.launchpad.net/bugs/686692
> 
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  arch/x86/xen/p2m.c |   20 +++++++++++++++++++-
>  1 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 8f2251d..c9307ec 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -237,7 +237,25 @@ void __init xen_build_dynamic_phys_to_machine(void)
>  			p2m_top[topidx] = mid;
>  		}
>  
> -		p2m_top[topidx][mididx] = &mfn_list[pfn];
> +		/*
> +		 * As long as the mfn_list has enough entries to completely
> +		 * fill a p2m page, pointing into the array is ok. But if
> +		 * not the entries beyond the last pfn will be undefined.
> +		 * And guessing that the 'what-ever-there-is' does not take it
> +		 * too kindly when changing it to invalid markers, a new page
> +		 * is allocated, initialized and filled with the valid part.
> +		 */
> +		if (unlikely(pfn + P2M_PER_PAGE > max_pfn)) {
> +			unsigned long p2midx;
> +			unsigned long **p2m = extend_brk(PAGE_SIZE, PAGE_SIZE);

			unsigned long  *p2m.


> +			p2m_init(p2m);
> +
> +			for (p2midx = 0; pfn + p2midx < max_pfn; p2midx++) {
> +				p2m[p2midx] = mfn_list[pfn + p2midx];
> +			}
> +			p2m_top[topidx][mididx] = p2m;
> +		} else
> +			p2m_top[topidx][mididx] = &mfn_list[pfn];
>  	}
>  
>  	m2p_override_init();
> -- 
> 1.7.0.4
> 

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Xen-devel] [PATCH] xen: p2m: correctly initialize partial p2m leave
  2011-01-21 14:26 ` [Xen-devel] " Konrad Rzeszutek Wilk
@ 2011-01-21 14:42   ` Stefan Bader
  0 siblings, 0 replies; 8+ messages in thread
From: Stefan Bader @ 2011-01-21 14:42 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Jeremy Fitzhardinge, xen-devel, Linux Kernel Mailing List

On 01/21/2011 03:26 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 20, 2011 at 03:38:23PM +0100, Stefan Bader wrote:
>> There have been changes and code been moved around, so this is just a quick
>> rebase of the change I tested on a 2.6.37 based kernel. The basic problem seem
>> still valid, though.
> 
> Yup.
>>
>> Initially I thought of adding a cc to stable into the s-o-b, but the patch needs
>> to be adapted anyway (I can supply that version if the way I fixed the issue
>> looks ok).
> 
> OK, let me send this upstream to Linus for adaption. I fixed one compile warning
> but otherwsie it is the same. Look below for details.
> 
> And when that is done I would appreciate you sending a copy to stable.
> 
Sure, will do that as soon as the change hits 2.6.38 upstream.

-Stefan

>>
>> Regards,
>> Stefan
> 
>> >From 1e9c9514caf0399c88ae9288e6db8e3d1c4b4be5 Mon Sep 17 00:00:00 2001
>> From: Stefan Bader <stefan.bader@canonical.com>
>> Date: Thu, 20 Jan 2011 11:37:43 +0100
>> Subject: [PATCH] xen: p2m: correctly initialize partial p2m leave
>>
>> After changing the p2m mapping to a tree by
>>
>>   commit 58e05027b530ff081ecea68e38de8d59db8f87e0
>>     xen: convert p2m to a 3 level tree
>>
>> and trying to boot a DomU with 615MB of memory, the following crash was
>> observed in the dump:
>>
>> kernel direct mapping tables up to 26f00000 @ 1ec4000-1fff000
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP: [<c0107397>] xen_set_pte+0x27/0x60
>> *pdpt = 0000000000000000 *pde = 0000000000000000
>>
>> Adding further debug statements showed that when trying to set up
>> pfn=0x26700 the returned mapping was invalid.
>>
>> pfn=0x266ff calling set_pte(0xc1fe77f8, 0x6b3003)
>> pfn=0x26700 calling set_pte(0xc1fe7800, 0x3)
>>
>> Although the last_pfn obtained from the startup info is 0x26700, which
>> should in turn not be hit, the additional 8MB which are added as extra
>> memory normally seem to be ok. This lead to looking into the initial
>> p2m tree construction, which uses the smaller value and assuming that
>> there is other code handling the extra memory.
>>
>> When the p2m tree is set up, the leaves are directly pointed to the
>> array which the domain builder set up. But if the mapping is not on a
>> boundary that fits into one p2m page, this will result in the last leaf
>> being only partially valid. And as the invalid entries are not
>> initialized in that case, things go badly wrong.
>>
>> I am trying to fix that by checking whether the current leaf is a
>> complete map and if not, allocate a completely new page and copy only
>> the valid pointers there. This may not be the most efficient or elegant
>> solution, but at least it seems to allow me booting DomUs with memory
>> assignments all over the range.
>>
>> BugLink: http://bugs.launchpad.net/bugs/686692
>>
>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>> ---
>>  arch/x86/xen/p2m.c |   20 +++++++++++++++++++-
>>  1 files changed, 19 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
>> index 8f2251d..c9307ec 100644
>> --- a/arch/x86/xen/p2m.c
>> +++ b/arch/x86/xen/p2m.c
>> @@ -237,7 +237,25 @@ void __init xen_build_dynamic_phys_to_machine(void)
>>  			p2m_top[topidx] = mid;
>>  		}
>>  
>> -		p2m_top[topidx][mididx] = &mfn_list[pfn];
>> +		/*
>> +		 * As long as the mfn_list has enough entries to completely
>> +		 * fill a p2m page, pointing into the array is ok. But if
>> +		 * not the entries beyond the last pfn will be undefined.
>> +		 * And guessing that the 'what-ever-there-is' does not take it
>> +		 * too kindly when changing it to invalid markers, a new page
>> +		 * is allocated, initialized and filled with the valid part.
>> +		 */
>> +		if (unlikely(pfn + P2M_PER_PAGE > max_pfn)) {
>> +			unsigned long p2midx;
>> +			unsigned long **p2m = extend_brk(PAGE_SIZE, PAGE_SIZE);
> 
> 			unsigned long  *p2m.
> 
> 
>> +			p2m_init(p2m);
>> +
>> +			for (p2midx = 0; pfn + p2midx < max_pfn; p2midx++) {
>> +				p2m[p2midx] = mfn_list[pfn + p2midx];
>> +			}
>> +			p2m_top[topidx][mididx] = p2m;
>> +		} else
>> +			p2m_top[topidx][mididx] = &mfn_list[pfn];
>>  	}
>>  
>>  	m2p_override_init();
>> -- 
>> 1.7.0.4
>>
> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Xen-devel] Re: [PATCH] xen: p2m: correctly initialize partial p2m leave
  2011-01-20 15:10 ` Konrad Rzeszutek Wilk
@ 2011-01-24  4:49   ` Jeremy Fitzhardinge
  2011-01-24  9:06     ` Stefan Bader
  0 siblings, 1 reply; 8+ messages in thread
From: Jeremy Fitzhardinge @ 2011-01-24  4:49 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Stefan Bader, Jeremy Fitzhardinge, xen-devel, Linux Kernel Mailing List

On 01/20/2011 07:10 AM, Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 20, 2011 at 03:38:23PM +0100, Stefan Bader wrote:
>> There have been changes and code been moved around, so this is just a quick
>> rebase of the change I tested on a 2.6.37 based kernel. The basic problem seem
>> still valid, though.
> Nice catch..

Indeed!

>> Initially I thought of adding a cc to stable into the s-o-b, but the patch needs
>> to be adapted anyway (I can supply that version if the way I fixed the issue
>> looks ok).
>>
>> Regards,
>> Stefan
>> >From 1e9c9514caf0399c88ae9288e6db8e3d1c4b4be5 Mon Sep 17 00:00:00 2001
>> From: Stefan Bader <stefan.bader@canonical.com>
>> Date: Thu, 20 Jan 2011 11:37:43 +0100
>> Subject: [PATCH] xen: p2m: correctly initialize partial p2m leave
>>
>> After changing the p2m mapping to a tree by
>>
>>   commit 58e05027b530ff081ecea68e38de8d59db8f87e0
>>     xen: convert p2m to a 3 level tree
>>
>> and trying to boot a DomU with 615MB of memory, the following crash was
>> observed in the dump:
>>
>> kernel direct mapping tables up to 26f00000 @ 1ec4000-1fff000
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP: [<c0107397>] xen_set_pte+0x27/0x60
>> *pdpt = 0000000000000000 *pde = 0000000000000000
>>
>> Adding further debug statements showed that when trying to set up
>> pfn=0x26700 the returned mapping was invalid.
>>
>> pfn=0x266ff calling set_pte(0xc1fe77f8, 0x6b3003)
>> pfn=0x26700 calling set_pte(0xc1fe7800, 0x3)
>>
>> Although the last_pfn obtained from the startup info is 0x26700, which
>> should in turn not be hit, the additional 8MB which are added as extra
>> memory normally seem to be ok. This lead to looking into the initial
>> p2m tree construction, which uses the smaller value and assuming that
>> there is other code handling the extra memory.
>>
>> When the p2m tree is set up, the leaves are directly pointed to the
>> array which the domain builder set up. But if the mapping is not on a
>> boundary that fits into one p2m page, this will result in the last leaf
>> being only partially valid. And as the invalid entries are not
>> initialized in that case, things go badly wrong.
>>
>> I am trying to fix that by checking whether the current leaf is a
>> complete map and if not, allocate a completely new page and copy only
>> the valid pointers there. This may not be the most efficient or elegant
>> solution, but at least it seems to allow me booting DomUs with memory
>> assignments all over the range.

Since the p2m page is just a normal page that happens to have been
initialized by the domain builder, I think we can just fill the tail of
the page with INVALID_P2M_ENTRY in place, rather than having to allocate
a new one.

    J

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Xen-devel] Re: [PATCH] xen: p2m: correctly initialize partial p2m leave
  2011-01-24  4:49   ` [Xen-devel] " Jeremy Fitzhardinge
@ 2011-01-24  9:06     ` Stefan Bader
  0 siblings, 0 replies; 8+ messages in thread
From: Stefan Bader @ 2011-01-24  9:06 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Konrad Rzeszutek Wilk, Jeremy Fitzhardinge, xen-devel,
	Linux Kernel Mailing List

On 01/24/2011 05:49 AM, Jeremy Fitzhardinge wrote:
> On 01/20/2011 07:10 AM, Konrad Rzeszutek Wilk wrote:
>> On Thu, Jan 20, 2011 at 03:38:23PM +0100, Stefan Bader wrote:
>>> There have been changes and code been moved around, so this is just a quick
>>> rebase of the change I tested on a 2.6.37 based kernel. The basic problem seem
>>> still valid, though.
>> Nice catch..
> 
> Indeed!
> 
>>> Initially I thought of adding a cc to stable into the s-o-b, but the patch needs
>>> to be adapted anyway (I can supply that version if the way I fixed the issue
>>> looks ok).
>>>
>>> Regards,
>>> Stefan
>>> >From 1e9c9514caf0399c88ae9288e6db8e3d1c4b4be5 Mon Sep 17 00:00:00 2001
>>> From: Stefan Bader <stefan.bader@canonical.com>
>>> Date: Thu, 20 Jan 2011 11:37:43 +0100
>>> Subject: [PATCH] xen: p2m: correctly initialize partial p2m leave
>>>
>>> After changing the p2m mapping to a tree by
>>>
>>>   commit 58e05027b530ff081ecea68e38de8d59db8f87e0
>>>     xen: convert p2m to a 3 level tree
>>>
>>> and trying to boot a DomU with 615MB of memory, the following crash was
>>> observed in the dump:
>>>
>>> kernel direct mapping tables up to 26f00000 @ 1ec4000-1fff000
>>> BUG: unable to handle kernel NULL pointer dereference at (null)
>>> IP: [<c0107397>] xen_set_pte+0x27/0x60
>>> *pdpt = 0000000000000000 *pde = 0000000000000000
>>>
>>> Adding further debug statements showed that when trying to set up
>>> pfn=0x26700 the returned mapping was invalid.
>>>
>>> pfn=0x266ff calling set_pte(0xc1fe77f8, 0x6b3003)
>>> pfn=0x26700 calling set_pte(0xc1fe7800, 0x3)
>>>
>>> Although the last_pfn obtained from the startup info is 0x26700, which
>>> should in turn not be hit, the additional 8MB which are added as extra
>>> memory normally seem to be ok. This lead to looking into the initial
>>> p2m tree construction, which uses the smaller value and assuming that
>>> there is other code handling the extra memory.
>>>
>>> When the p2m tree is set up, the leaves are directly pointed to the
>>> array which the domain builder set up. But if the mapping is not on a
>>> boundary that fits into one p2m page, this will result in the last leaf
>>> being only partially valid. And as the invalid entries are not
>>> initialized in that case, things go badly wrong.
>>>
>>> I am trying to fix that by checking whether the current leaf is a
>>> complete map and if not, allocate a completely new page and copy only
>>> the valid pointers there. This may not be the most efficient or elegant
>>> solution, but at least it seems to allow me booting DomUs with memory
>>> assignments all over the range.
> 
> Since the p2m page is just a normal page that happens to have been
> initialized by the domain builder, I think we can just fill the tail of
> the page with INVALID_P2M_ENTRY in place, rather than having to allocate
> a new one.
> 
>     J

That was exactly the detail I was not sure about and did not want to make
assumptions after only spending a little bit digging around in the code. The
safest assumption was to expect other data to be possibly located after the p2m
array. Please feel free to clean up the code any time.

-Stefan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Xen-devel] [PATCH] xen: p2m: correctly initialize partial p2m leave
  2011-01-20 14:38 [PATCH] xen: p2m: correctly initialize partial p2m leave Stefan Bader
  2011-01-20 15:10 ` Konrad Rzeszutek Wilk
  2011-01-21 14:26 ` [Xen-devel] " Konrad Rzeszutek Wilk
@ 2011-01-25 16:54 ` Ian Campbell
  2011-01-25 18:48   ` Stefan Bader
  2 siblings, 1 reply; 8+ messages in thread
From: Ian Campbell @ 2011-01-25 16:54 UTC (permalink / raw)
  To: Stefan Bader
  Cc: Jeremy Fitzhardinge, Konrad Rzeszutek Wilk, xen-devel,
	Linux Kernel Mailing List

Please always include an inline copy of a patch for easier
review-by-reply, even if you also include an attachment because your
mailer mangles patches.

Anyway, I suspect the following comment is obsoleted by Jeremy's "just
do it in place" suggestion but:

On Thu, 2011-01-20 at 14:38 +0000, Stefan Bader wrote:
> [...]
> +                       unsigned long **p2m = extend_brk(PAGE_SIZE, PAGE_SIZE);

I think this would need to be matched by a corresponding RESERVE_BRK of some sort.

Ian.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Xen-devel] [PATCH] xen: p2m: correctly initialize partial p2m leave
  2011-01-25 16:54 ` Ian Campbell
@ 2011-01-25 18:48   ` Stefan Bader
  0 siblings, 0 replies; 8+ messages in thread
From: Stefan Bader @ 2011-01-25 18:48 UTC (permalink / raw)
  To: Ian Campbell
  Cc: Jeremy Fitzhardinge, Konrad Rzeszutek Wilk, xen-devel,
	Linux Kernel Mailing List

On 01/25/2011 05:54 PM, Ian Campbell wrote:
> Please always include an inline copy of a patch for easier
> review-by-reply, even if you also include an attachment because your
> mailer mangles patches.
> 
Will try to remember. It easy to forget when the mailer used does show textual
attachments inline.

> Anyway, I suspect the following comment is obsoleted by Jeremy's "just
> do it in place" suggestion but:
> 
> On Thu, 2011-01-20 at 14:38 +0000, Stefan Bader wrote:
>> [...]
>> +                       unsigned long **p2m = extend_brk(PAGE_SIZE, PAGE_SIZE);
> 
> I think this would need to be matched by a corresponding RESERVE_BRK of some sort.
> 
> Ian.
> 
> 
Currently the version extending brk is upstream. So we would need this going
upstream as well...

--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -61,6 +61,7 @@ static RESERVE_BRK_ARRAY(unsigned long *, p2m_top_mfn_p, P2M_T

 RESERVE_BRK(p2m_mid, PAGE_SIZE * (MAX_DOMAIN_PAGES / (P2M_PER_PAGE * P2M_MID_PE
 RESERVE_BRK(p2m_mid_mfn, PAGE_SIZE * (MAX_DOMAIN_PAGES / (P2M_PER_PAGE * P2M_MI
+RESERVE_BRK(p2m_node, PAGE_SIZE);

 static inline unsigned p2m_top_index(unsigned long pfn)
 {

(code above not tested at all)

And for the "do it inline" this could possibly look like this? (I know I am
repeating myself though the value at max_pfn always seemed to be 3L which seemed
to be too consistent for coincidence)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index ddc81a0..fd12d7c 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -241,21 +241,15 @@ void __init xen_build_dynamic_phys_to_machine(void)
                 * As long as the mfn_list has enough entries to completely
                 * fill a p2m page, pointing into the array is ok. But if
                 * not the entries beyond the last pfn will be undefined.
-                * And guessing that the 'what-ever-there-is' does not take it
-                * too kindly when changing it to invalid markers, a new page
-                * is allocated, initialized and filled with the valid part.
                 */
                if (unlikely(pfn + P2M_PER_PAGE > max_pfn)) {
                        unsigned long p2midx;
-                       unsigned long *p2m = extend_brk(PAGE_SIZE, PAGE_SIZE);
-                       p2m_init(p2m);
-
-                       for (p2midx = 0; pfn + p2midx < max_pfn; p2midx++) {
-                               p2m[p2midx] = mfn_list[pfn + p2midx];
-                       }
-                       p2m_top[topidx][mididx] = p2m;
-               } else
-                       p2m_top[topidx][mididx] = &mfn_list[pfn];
+
+                       p2midx = max_pfn % P2M_PER_PAGE;
+                       for ( ; p2midx < P2M_PER_PAGE; p2midx++)
+                               mfn_list[pfn + p2midx] = INVALID_P2M_ENTRY;
+               }
+               p2m_top[topidx][mididx] = &mfn_list[pfn];
        }

        m2p_override_init();

(again completely (not even compile) tested)

-Stefan

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2011-01-25 18:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-20 14:38 [PATCH] xen: p2m: correctly initialize partial p2m leave Stefan Bader
2011-01-20 15:10 ` Konrad Rzeszutek Wilk
2011-01-24  4:49   ` [Xen-devel] " Jeremy Fitzhardinge
2011-01-24  9:06     ` Stefan Bader
2011-01-21 14:26 ` [Xen-devel] " Konrad Rzeszutek Wilk
2011-01-21 14:42   ` Stefan Bader
2011-01-25 16:54 ` Ian Campbell
2011-01-25 18:48   ` Stefan Bader

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).