x86: fix kexec crash regression

x86: fix kexec crash regression

[David Vrabel]

A recent commit causes Xen to crash if a kexec crash images is loaded
into a crash area below 5 TiB (and a non-debug Xen is used).

This series reverts the bad commit and then re-implements the part of
the original fix that does cause a regression.

This is an important bug fix necessary for 4.4.

David

[PATCH 1/2] Revert "kexec/x86: do not map crash kernel area"

[David Vrabel]

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

This reverts commit 7113a45451a9f656deeff070e47672043ed83664.

The kexec code uses map_domain_page() on pages within the crash
region, so this mapping is required if the crash region is within the
direct map area.

Wthout this revert, loading a crash kernel may cause a fatal page
fault when trying to zero the first control page allocated from the
crash area.  The fault will occur on non-debug builds of Xen when the
crash area is below 5 TiB (which will be most systems).

Reported-by: Don Slutz <dslutz@verizon.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/setup.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 4833ca3..f07ee2b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1097,7 +1097,9 @@ void __init __start_xen(unsigned long mbi_p)
                          mod[i].mod_start,
                          PFN_UP(mod[i].mod_end), PAGE_HYPERVISOR);
     }
-
+    map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
+                     kexec_crash_area.start >> PAGE_SHIFT,
+                     PFN_UP(kexec_crash_area.size), PAGE_HYPERVISOR);
     xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) - 1) &
                    ~((1UL << L2_PAGETABLE_SHIFT) - 1);
     destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE);
-- 
1.7.2.5

[PATCH 2/2] x86: map portion of kexec crash area that is within the direct map area

[David Vrabel]

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

map_domain_page() is used to temporarily map the crash area when
loading a kexec crash image.

If some or all of the crash area is within the direct map area then
this portion must have a direct mapping, otherwise map_domain_page()
(on non-debug builds) will return a direct map VA that is not actually
mapped.

Parts of the crash area outside the direct map area (i.e., above 5 TiB)
do not need and should not have such a mapping.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/setup.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f07ee2b..8e10bdf 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1097,9 +1097,17 @@ void __init __start_xen(unsigned long mbi_p)
                          mod[i].mod_start,
                          PFN_UP(mod[i].mod_end), PAGE_HYPERVISOR);
     }
-    map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
-                     kexec_crash_area.start >> PAGE_SHIFT,
-                     PFN_UP(kexec_crash_area.size), PAGE_HYPERVISOR);
+
+    if ( kexec_crash_area.size )
+    {
+        unsigned long s = PFN_DOWN(kexec_crash_area.start);
+        unsigned long e = min(s + PFN_UP(kexec_crash_area.size),
+                              PFN_UP(__pa(HYPERVISOR_VIRT_END - 1)));
+
+        map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
+                         s, e - s, PAGE_HYPERVISOR);
+    }
+
     xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) - 1) &
                    ~((1UL << L2_PAGETABLE_SHIFT) - 1);
     destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE);
-- 
1.7.2.5

Re: x86: fix kexec crash regression

[Andrew Cooper]

On 08/01/14 15:56, David Vrabel wrote:
> A recent commit causes Xen to crash if a kexec crash images is loaded
> into a crash area below 5 TiB (and a non-debug Xen is used).
>
> This series reverts the bad commit and then re-implements the part of
> the original fix that does cause a regression.
>
> This is an important bug fix necessary for 4.4.
>
> David

Both Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew

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

Re: [PATCH 1/2] Revert "kexec/x86: do not map crash kernel area"

[Jan Beulich]

>>> On 08.01.14 at 16:56, David Vrabel <david.vrabel@citrix.com> wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> This reverts commit 7113a45451a9f656deeff070e47672043ed83664.

As indicated before - I don't think reverting is the right thing here.
Just add the necessary code in the same (or another, if more
suitable) place.

Jan

> The kexec code uses map_domain_page() on pages within the crash
> region, so this mapping is required if the crash region is within the
> direct map area.
> 
> Wthout this revert, loading a crash kernel may cause a fatal page
> fault when trying to zero the first control page allocated from the
> crash area.  The fault will occur on non-debug builds of Xen when the
> crash area is below 5 TiB (which will be most systems).
> 
> Reported-by: Don Slutz <dslutz@verizon.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/arch/x86/setup.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 4833ca3..f07ee2b 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1097,7 +1097,9 @@ void __init __start_xen(unsigned long mbi_p)
>                           mod[i].mod_start,
>                           PFN_UP(mod[i].mod_end), PAGE_HYPERVISOR);
>      }
> -
> +    map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
> +                     kexec_crash_area.start >> PAGE_SHIFT,
> +                     PFN_UP(kexec_crash_area.size), PAGE_HYPERVISOR);
>      xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) - 1) &
>                     ~((1UL << L2_PAGETABLE_SHIFT) - 1);
>      destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + 
> BOOTSTRAP_MAP_BASE);
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH 1/2] Revert "kexec/x86: do not map crash kernel area"

[Ian Campbell]

On Wed, 2014-01-08 at 16:27 +0000, Jan Beulich wrote:
> >>> On 08.01.14 at 16:56, David Vrabel <david.vrabel@citrix.com> wrote:
> > From: David Vrabel <david.vrabel@citrix.com>
> > 
> > This reverts commit 7113a45451a9f656deeff070e47672043ed83664.
> 
> As indicated before - I don't think reverting is the right thing here.
> Just add the necessary code in the same (or another, if more
> suitable) place.

Isn't that what the second patch does? Perhaps this would be better
structured as a single fix rather than a revert + do it properly, but
the latter approach seems reasonable enough...

Ian.

Re: [PATCH 2/2] x86: map portion of kexec crash area that is within the direct map area

[Jan Beulich]

>>> On 08.01.14 at 16:56, David Vrabel <david.vrabel@citrix.com> wrote:
> +    if ( kexec_crash_area.size )

Wouldn't this better also include a kexec_crash_area.start range
check?

> +    {
> +        unsigned long s = PFN_DOWN(kexec_crash_area.start);
> +        unsigned long e = min(s + PFN_UP(kexec_crash_area.size),
> +                              PFN_UP(__pa(HYPERVISOR_VIRT_END - 1)));
> +
> +        map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
> +                         s, e - s, PAGE_HYPERVISOR);

map_pages_to_xen() doesn't tolerate a huge count resulting when
e < s (which is possible due to the min() above).

Furthermore, with PFN compression and a badly specified address
range (overlapping a hole) this would likely crash during boot. While
that mistake might later also lead to problems, I think it would be
better if booting nevertheless succeeded.

And of course the whole thing will break the moment we allow RAM
to fall into PFN compression holes (either by not using some small
amount of memory in order to be able to cover a larger total
amount, like would be desirable in cases where we need to chop
off a large piece at the top, but could do with not using a couple
of gigabytes relocated from the range below 4Gb, or via command
line option), which is why I told you that using map_domain_page()
for the kexec crash area is a bad thing in the first place.

Jan

> +    }

Re: [PATCH 1/2] Revert "kexec/x86: do not map crash kernel area"

[Jan Beulich]

>>> On 08.01.14 at 17:34, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-01-08 at 16:27 +0000, Jan Beulich wrote:
>> >>> On 08.01.14 at 16:56, David Vrabel <david.vrabel@citrix.com> wrote:
>> > From: David Vrabel <david.vrabel@citrix.com>
>> > 
>> > This reverts commit 7113a45451a9f656deeff070e47672043ed83664.
>> 
>> As indicated before - I don't think reverting is the right thing here.
>> Just add the necessary code in the same (or another, if more
>> suitable) place.
> 
> Isn't that what the second patch does? Perhaps this would be better
> structured as a single fix rather than a revert + do it properly, but
> the latter approach seems reasonable enough...

Right - that's what I'm asking for: Do this as a single patch.

Jan

Re: [PATCH 2/2] x86: map portion of kexec crash area that is within the direct map area

[David Vrabel]

On 08/01/14 16:38, Jan Beulich wrote:
>>>> On 08.01.14 at 16:56, David Vrabel <david.vrabel@citrix.com> wrote:
>> +    if ( kexec_crash_area.size )
> 
> Wouldn't this better also include a kexec_crash_area.start range
> check?

It's a "if there is a crash area" check.  It seems fine as-is to me.

>> +    {
>> +        unsigned long s = PFN_DOWN(kexec_crash_area.start);
>> +        unsigned long e = min(s + PFN_UP(kexec_crash_area.size),
>> +                              PFN_UP(__pa(HYPERVISOR_VIRT_END - 1)));
>> +
>> +        map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
>> +                         s, e - s, PAGE_HYPERVISOR);
> 
> map_pages_to_xen() doesn't tolerate a huge count resulting when
> e < s (which is possible due to the min() above).

Yes, you're right.  This needs to be:

   if ( e > s )
       map_pages_to_xen(...)

David