KEXEC: Clean up logic to choose a range for the crash kernel

RFC In review Andrew Cooper 2 participants 8 messages Nov 25, 2011 – Nov 28, 2011

lore

Share & tools

lore

Andrew Cooper

author

2011-11-25 15:12 UTC #0 lore raw

RFC 1/1 KEXEC: Clean up logic to choose a range for the crash kernel

Signature · 3 lines

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

# HG changeset patch
# Parent 031be098e227bc0dcceef880a1a4c8f9c81647df
KEXEC: Clean up logic to choose a range for the crash kernel.

This is a fairly large change but I can't see an easy way to split it up without things breaking.

Before this patch, using "classic" crashdump= syntax (<size>@<start>) causes an exact size and location to be reserved in the e820 map before Xen and modules are relocated. However, using "new" syntax (<start>-[<end>]:<size>) encounteres a myriad of problems, most notibly not considering the range when allocating its region.

This patch tries to unify both codepaths, and fix the broken logic when using "newer" syntax.

Changes addressed:

1) Remove use of kexec_crash_area from parse_crashkernel(). Use ranges[0] in preference, as there is no support for multiple ranges using classic syntax. This prevents some wierd logic in __setup_xen() depending on whether kexec_crash_area.size is set or not.

2) Change set_kexec_crash_area_size() to choose_kexec_range() The new method of choosing a range provides an entire rather than just setting kexec_crash_area.size. Also, fix the very broken logic which will only consider a range if its .end is greater than system_ram. Additionally, prefer range with the largest size if we have a choice of valid ones, and prefer ranges with more flexibility in their limits.

3) Move kexec_reserve_area() from setup.c to kexec.c
It makes more sense here, Also, change its functionality a little to always work from the kexec range provided by choose_kexec_range()

4) Remove the kexec reservation code when considering modules This code only had any effect for people using the "newer" syntax on the command line, as people using the "classic" syntax would reserve a memory region before considering modules.

Signed-off-by Andrew Cooper <[email protected]>

xen/arch/x86/setup.c +5 ~~-42~~

diff -r 0a0c02a61676 xen/arch/x86/setup.c
@@ -482,32 +482,6 @@ static void __init parse_video_info(void
     }
 }
 
-static void __init kexec_reserve_area(struct e820map *e820)
-{
-    unsigned long kdump_start = kexec_crash_area.start;
-    unsigned long kdump_size  = kexec_crash_area.size;
-    static int is_reserved = 0;
-
-    kdump_size = (kdump_size + PAGE_SIZE - 1) & PAGE_MASK;
-
-    if ( (kdump_start == 0) || (kdump_size == 0) || is_reserved )
-        return;
-
-    is_reserved = 1;
-
-    if ( !reserve_e820_ram(e820, kdump_start, kdump_start + kdump_size) )
-    {
-        printk("Kdump: DISABLED (failed to reserve %luMB (%lukB) at 0x%lx)"
-               "\n", kdump_size >> 20, kdump_size >> 10, kdump_start);
-        kexec_crash_area.start = kexec_crash_area.size = 0;
-    }
-    else
-    {
-        printk("Kdump: %luMB (%lukB) at 0x%lx\n",
-               kdump_size >> 20, kdump_size >> 10, kdump_start);
-    }
-}
-
 void init_done(void)
 {
     /* Free (or page-protect) the init areas. */

@@ -759,13 +733,15 @@ void __init __start_xen(unsigned long mb
     /* Create a temporary copy of the E820 map. */
     memcpy(&boot_e820, &e820, sizeof(e820));
 
-    /* Early kexec reservation (explicit static start address). */
+    /* Calculate nr_pages from the e820 map. */
     nr_pages = 0;
     for ( i = 0; i < e820.nr_map; i++ )
         if ( e820.map[i].type == E820_RAM )
             nr_pages += e820.map[i].size >> PAGE_SHIFT;
-    set_kexec_crash_area_size((u64)nr_pages << PAGE_SHIFT);
-    kexec_reserve_area(&boot_e820);
+
+    /* Early kexec reservation. */
+    if( choose_kexec_range((u64)nr_pages << PAGE_SHIFT) )
+        kexec_reserve_area(&boot_e820);
 
     initial_images = mod;
     nr_initial_images = mbi->mods_count;

@@ -939,19 +915,6 @@ void __init __start_xen(unsigned long mb
                 mod[j].reserved = 1;
             }
         }
-
-#ifdef CONFIG_X86_32
-        /* Confine the kexec area to below 4Gb. */
-        e = min_t(uint64_t, e, 1ULL << 32);
-#endif
-        /* Don't overlap with modules. */
-        e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size),
-                             mod, mbi->mods_count, -1);
-        if ( !kexec_crash_area.start && (s < e) )
-        {
-            e = (e - kexec_crash_area.size) & PAGE_MASK;
-            kexec_crash_area.start = e;
-        }
     }
 
     if ( modules_headroom && !mod->reserved )

xen/common/kexec.c +118 ~~-12~~

160 lines not on this page · raw message

xen/include/xen/kexec.h +3 -1

diff -r 0a0c02a61676 xen/include/xen/kexec.h
@@ -4,6 +4,7 @@
 #include <public/kexec.h>
 #include <asm/percpu.h>
 #include <xen/elfcore.h>
+#include <asm/e820.h>
 
 typedef struct xen_kexec_reserve {
     unsigned long size;

@@ -14,7 +15,8 @@ extern xen_kexec_reserve_t kexec_crash_a
 
 extern bool_t kexecing;
 
-void set_kexec_crash_area_size(u64 system_ram);
+bool_t choose_kexec_range(u64 system_ram);
+void kexec_reserve_area(struct e820map *e820);
 
 /* We have space for 4 images to support atomic update
  * of images. This is important for CRASH images since

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Jan Beulich

2011-11-25 15:36 UTC #1 lore raw

Quoted 6 lines

Quoted 1 line
Quoted 1 line · Andrew Cooper
Before this patch, using "classic" crashdump= syntax (<size>@<start>) causes an exact size and location to be reserved in the e820 map before Xen and modules are relocated. However, using "new" syntax (<start>-[<end>]:<size>) encounteres a myriad of problems, most notibly not considering the range when allocating its region.

I'm sure I tested this (with some basic values), so the "myriad of problems" make me curious as to what they are.

What does "range" refer to here, and what is "its region"?

You also seem to overlook that even the old syntax allowed for only specifying a size.

Quoted 10 lines

This patch tries to unify both codepaths, and fix the broken logic when using "newer" syntax.
Changes addressed:
1) Remove use of kexec_crash_area from parse_crashkernel(). Use ranges[0] in preference, as there is no support for multiple ranges using classic syntax. This prevents some wierd logic in __setup_xen() depending on whether kexec_crash_area.size is set or not.

What weird about that?

Quoted 2 lines

2) Change set_kexec_crash_area_size() to choose_kexec_range() The new method of choosing a range provides an entire rather than

And entire what?

Quoted 3 lines

just setting kexec_crash_area.size. Also, fix the very broken logic which will only consider a range if its .end is greater than system_ram.

Where's that happening? Are you perhaps mis-interpreting the purpose of these ranges?

Quoted 4 lines

Additionally, prefer range with the largest
size if we have a choice of valid ones, and prefer ranges with more flexibility in their limits.

Quoted 8 lines

3) Move kexec_reserve_area() from setup.c to kexec.c
It makes more sense here, Also, change its functionality a little to always work from the kexec range provided by choose_kexec_range()
4) Remove the kexec reservation code when considering modules This code only had any effect for people using the "newer" syntax on the command line, as people using the "classic" syntax would reserve a memory region before considering modules.

Are you sure? How do you prevent the kexec area from overlapping any of the modules (in particular the initrd, which can get passed in place to Dom0)?

Jan

Andrew Cooper

author

2011-11-25 16:03 UTC #2 lore raw

Quoted 8 lines · Jan Beulich

Quoted 6 lines
Quoted 1 line
Quoted 1 line · Andrew Cooper
Before this patch, using "classic" crashdump= syntax (<size>@<start>) causes an exact size and location to be reserved in the e820 map before Xen and modules are relocated. However, using "new" syntax (<start>-[<end>]:<size>) encounteres a myriad of problems, most notibly not considering the range when allocating its region.
I'm sure I tested this (with some basic values), so the "myriad of problems" make me curious as to what they are.

The comment was a bit old from before I twigged what some of the code was doing. The two key problems were only considering the size, and a bug in set_kexec_crash_area_size() which will only consider a range as being valid if its end is greater than system_ram. This bug prevents specifying a range as "128M anywhere in 1G-2G" actually working on a box with more than 2G of ram.

Quoted 1 line

What does "range" refer to here, and what is "its region"?

I will need to clean up my use of the term range and region, although it doesn't help that there is ambiguity in the source code as well. Loosely, I have been using "range" to mean a possible range as given on the command line, and "region" to be the actual area in memory where the kdump kernel is going to be put.

Quoted 2 lines

You also seem to overlook that even the old syntax allowed for only specifying a size.

No. In the case of using the old syntax, both kexec_crash_area.{start,size} are set, which caused the earlier call to kexec_reserve_region() in __setup_xen() to attempt to reserve an exact region at an exact address. Using the newer syntax, kexec_crash_area.size did not get set in the parsing function, and got considered in set_kexec_crash_area_size(), which only sets the size. The subsiquent call to kexec_reverse_area() on the next line fails silently because kexec_crash_area.start is 0. Later, when considering the modules, kexec_crash_area.start gets set to e-size, at which point the call to kexec_reserve_area() actually does attempt to reserve a region, based on the limits of an arbitrary e820 entry, not on the limits provided on the command line.

Quoted 11 lines

Quoted 10 lines
This patch tries to unify both codepaths, and fix the broken logic when using "newer" syntax.
Changes addressed:
1) Remove use of kexec_crash_area from parse_crashkernel(). Use ranges[0] in preference, as there is no support for multiple ranges using classic syntax. This prevents some wierd logic in __setup_xen() depending on whether kexec_crash_area.size is set or not.
What weird about that?

As described above.

Quoted 3 lines

Quoted 2 lines
2) Change set_kexec_crash_area_size() to choose_kexec_range() The new method of choosing a range provides an entire rather than
And entire what?

Oops. An entire "kexec_range" entry giving a start, end and size value, rather than the current case which takes either an exact size/start pair from the 'classic' case, or a size anywhere in memory for the 'newer' case.

Quoted 5 lines

Quoted 3 lines
just setting kexec_crash_area.size. Also, fix the very broken logic which will only consider a range if its .end is greater than system_ram.
Where's that happening? Are you perhaps mis-interpreting the purpose of these ranges?

I might possibly be misinterpreting these ranges as the only documentation is the code, but I believe I have got it correct based on the upstream Linux code. What are your interpretations of the ranges?

Quoted 8 lines

Quoted 8 lines
Additionally, prefer range with the largest
size if we have a choice of valid ones, and prefer ranges with more flexibility in their limits.
3) Move kexec_reserve_area() from setup.c to kexec.c
It makes more sense here, Also, change its functionality a little to always work from the kexec range provided by choose_kexec_range()

Quoted 7 lines

Quoted 4 lines
4) Remove the kexec reservation code when considering modules This code only had any effect for people using the "newer" syntax on the command line, as people using the "classic" syntax would reserve a memory region before considering modules.
Are you sure? How do you prevent the kexec area from overlapping any of the modules (in particular the initrd, which can get passed in place to Dom0)?

Good point, which is why this is an RFC. Note that the "classic" syntax, which everyone refers to in documents on the subject, will never consider any of the modules. I am not really sure which is best, but I would consider positioning the kdump kernel more important than the location of initrd. The user is likely to know more about why their kdump kernel needs to be located where specified than Xen does. Perhaps after deciding where the kdump kernel should go, we should move any overlapping modules?

Quoted 2 lines

Jan

Signature · 2 lines

Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Andrew Cooper

author

2011-11-25 16:09 UTC #3 lore raw

Quoted 82 lines · Andrew Cooper

Quoted 8 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · Andrew Cooper
Before this patch, using "classic" crashdump= syntax (<size>@<start>) causes an exact size and location to be reserved in the e820 map before Xen and modules are relocated. However, using "new" syntax (<start>-[<end>]:<size>) encounteres a myriad of problems, most notibly not considering the range when allocating its region.
I'm sure I tested this (with some basic values), so the "myriad of problems" make me curious as to what they are.
The comment was a bit old from before I twigged what some of the code was doing. The two key problems were only considering the size, and a bug in set_kexec_crash_area_size() which will only consider a range as being valid if its end is greater than system_ram. This bug prevents specifying a range as "128M anywhere in 1G-2G" actually working on a box with more than 2G of ram.
Quoted 1 line
What does "range" refer to here, and what is "its region"?
I will need to clean up my use of the term range and region, although it doesn't help that there is ambiguity in the source code as well. Loosely, I have been using "range" to mean a possible range as given on the command line, and "region" to be the actual area in memory where the kdump kernel is going to be put.
Quoted 2 lines
You also seem to overlook that even the old syntax allowed for only specifying a size.
No. In the case of using the old syntax, both kexec_crash_area.{start,size} are set, which caused the earlier call to kexec_reserve_region() in __setup_xen() to attempt to reserve an exact region at an exact address. Using the newer syntax, kexec_crash_area.size did not get set in the parsing function, and got considered in set_kexec_crash_area_size(), which only sets the size. The subsiquent call to kexec_reverse_area() on the next line fails silently because kexec_crash_area.start is 0. Later, when considering the modules, kexec_crash_area.start gets set to e-size, at which point the call to kexec_reserve_area() actually does attempt to reserve a region, based on the limits of an arbitrary e820 entry, not on the limits provided on the command line.
Quoted 11 lines
Quoted 10 lines
This patch tries to unify both codepaths, and fix the broken logic when using "newer" syntax.
Changes addressed:
1) Remove use of kexec_crash_area from parse_crashkernel(). Use ranges[0] in preference, as there is no support for multiple ranges using classic syntax. This prevents some wierd logic in __setup_xen() depending on whether kexec_crash_area.size is set or not.
What weird about that?
As described above.
Quoted 3 lines
Quoted 2 lines
2) Change set_kexec_crash_area_size() to choose_kexec_range() The new method of choosing a range provides an entire rather than
And entire what?
Oops. An entire "kexec_range" entry giving a start, end and size value, rather than the current case which takes either an exact size/start pair from the 'classic' case, or a size anywhere in memory for the 'newer' case.
Quoted 5 lines
Quoted 3 lines
just setting kexec_crash_area.size. Also, fix the very broken logic which will only consider a range if its .end is greater than system_ram.
Where's that happening? Are you perhaps mis-interpreting the purpose of these ranges?
I might possibly be misinterpreting these ranges as the only documentation is the code, but I believe I have got it correct based on the upstream Linux code. What are your interpretations of the ranges?
Quoted 8 lines
Quoted 8 lines
Additionally, prefer range with the largest
size if we have a choice of valid ones, and prefer ranges with more flexibility in their limits.
3) Move kexec_reserve_area() from setup.c to kexec.c
It makes more sense here, Also, change its functionality a little to always work from the kexec range provided by choose_kexec_range()
Quoted 7 lines
Quoted 4 lines
4) Remove the kexec reservation code when considering modules This code only had any effect for people using the "newer" syntax on the command line, as people using the "classic" syntax would reserve a memory region before considering modules.
Are you sure? How do you prevent the kexec area from overlapping any of the modules (in particular the initrd, which can get passed in place to Dom0)?

Quoted 8 lines

Good point, which is why this is an RFC. Note that the "classic" syntax, which everyone refers to in documents on the subject, will never consider any of the modules. I am not really sure which is best, but I would consider positioning the kdump kernel more important than the location of initrd. The user is likely to know more about why their kdump kernel needs to be located where specified than Xen does. Perhaps after deciding where the kdump kernel should go, we should move any overlapping modules?

On second thoughts, how about this?

If a user provides an exact position and size for the kdump kernel, place the kdump kernel there and move any overlapping modules.

If the user provides a range, attempt to fit the kdump kernel around other modules, and in the case where we cant, move the offending module(s) elsewhere.

Quoted 2 lines

Quoted 2 lines
Jan

Signature · 2 lines

Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Jan Beulich

2011-11-25 16:49 UTC #4 lore raw

In reply to Andrew Cooper

Quoted 9 lines

Quoted 1 line
Quoted 1 line · Andrew Cooper
Quoted 3 lines · Jan Beulich
Quoted 1 line
Quoted 1 line
Quoted 1 line · Andrew Cooper
Where's that happening? Are you perhaps mis-interpreting the purpose of these ranges?
I might possibly be misinterpreting these ranges as the only documentation is the code, but I believe I have got it correct based on the upstream Linux code. What are your interpretations of the ranges?

The ranges are to say "if the system has between <start> and <end> amount of memory, then allocate a <size> block (possibly located at <offset>).

Quoted 11 lines

Quoted 7 lines
Quoted 4 lines
4) Remove the kexec reservation code when considering modules This code only had any effect for people using the "newer" syntax on the command line, as people using the "classic" syntax would reserve a memory region before considering modules.
Are you sure? How do you prevent the kexec area from overlapping any of the modules (in particular the initrd, which can get passed in place to Dom0)?
Good point, which is why this is an RFC. Note that the "classic" syntax, which everyone refers to in documents on the subject, will never consider any of the modules.

It won't in either case when addresses are specified. It will in both cases when only sizes are provided.

Quoted 6 lines

I am not really sure which is best, but I
would consider positioning the kdump kernel more important than the location of initrd. The user is likely to know more about why their kdump kernel needs to be located where specified than Xen does. Perhaps after deciding where the kdump kernel should go, we should move any overlapping modules?

And that's already happening (or is at least supposed to be).

Jan

Andrew Cooper

author

2011-11-25 17:03 UTC #5 lore raw

Quoted 11 lines · Jan Beulich

Quoted 8 lines
Quoted 1 line
Quoted 1 line · Andrew Cooper
Quoted 3 lines · Jan Beulich
Quoted 1 line
Quoted 1 line
Quoted 1 line · Andrew Cooper
Where's that happening? Are you perhaps mis-interpreting the purpose of these ranges?
I might possibly be misinterpreting these ranges as the only documentation is the code, but I believe I have got it correct based on the upstream Linux code. What are your interpretations of the ranges?
The ranges are to say "if the system has between <start> and <end> amount of memory, then allocate a <size> block (possibly located at <offset>).

Ah ok. That actually makes more logical sense. Is there some documentation as to exactly what is permitted to specify? I have not been able to find any.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
4) Remove the kexec reservation code when considering modules This code only had any effect for people using the "newer" syntax on the command line, as people using the "classic" syntax would reserve a memory region before considering modules.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Are you sure? How do you prevent the kexec area from overlapping any of the modules (in particular the initrd, which can get passed in place to Dom0)?
Good point, which is why this is an RFC. Note that the "classic" syntax, which everyone refers to in documents on the subject, will never consider any of the modules.
It won't in either case when addresses are specified. It will in both cases when only sizes are provided.

How can you specify an offset while using the range syntax? This leaves the classic syntax as the only way to specific an exact location in memory.

Quoted 10 lines

Quoted 6 lines
I am not really sure which is best, but I
would consider positioning the kdump kernel more important than the location of initrd. The user is likely to know more about why their kdump kernel needs to be located where specified than Xen does. Perhaps after deciding where the kdump kernel should go, we should move any overlapping modules?
And that's already happening (or is at least supposed to be).
Jan

Signature · 2 lines

Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Andrew Cooper

author

2011-11-25 17:49 UTC #6 lore raw

Quoted 24 lines · Andrew Cooper

Quoted 11 lines · Jan Beulich
Quoted 8 lines
Quoted 1 line
Quoted 1 line · Andrew Cooper
Quoted 3 lines · Jan Beulich
Quoted 1 line
Quoted 1 line
Quoted 1 line · Andrew Cooper
Where's that happening? Are you perhaps mis-interpreting the purpose of these ranges?
I might possibly be misinterpreting these ranges as the only documentation is the code, but I believe I have got it correct based on the upstream Linux code. What are your interpretations of the ranges?
The ranges are to say "if the system has between <start> and <end> amount of memory, then allocate a <size> block (possibly located at <offset>).
Ah ok. That actually makes more logical sense. Is there some documentation as to exactly what is permitted to specify? I have not been able to find any.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
4) Remove the kexec reservation code when considering modules This code only had any effect for people using the "newer" syntax on the command line, as people using the "classic" syntax would reserve a memory region before considering modules.
Are you sure? How do you prevent the kexec area from overlapping any of the modules (in particular the initrd, which can get passed in place to Dom0)?

Quoted 17 lines

Quoted 5 lines
Quoted 3 lines
Good point, which is why this is an RFC. Note that the "classic" syntax, which everyone refers to in documents on the subject, will never consider any of the modules.
It won't in either case when addresses are specified. It will in both cases when only sizes are provided.
How can you specify an offset while using the range syntax? This leaves the classic syntax as the only way to specific an exact location in memory.
Quoted 9 lines
Quoted 6 lines
I am not really sure which is best, but I
would consider positioning the kdump kernel more important than the location of initrd. The user is likely to know more about why their kdump kernel needs to be located where specified than Xen does. Perhaps after deciding where the kdump kernel should go, we should move any overlapping modules?
And that's already happening (or is at least supposed to be).
Jan

After comparing the Xen parsing code with the Linux parsing code, with the correct idea of what we are trying to do, Xen still lacks the ability to specify an exact offset using the newer style. I will implement this.

Signature · 2 lines

Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Jan Beulich

2011-11-28 07:45 UTC #7 lore raw

Quoted 5 lines

Quoted 1 line
Quoted 1 line · Andrew Cooper
After comparing the Xen parsing code with the Linux parsing code, with the correct idea of what we are trying to do, Xen still lacks the ability to specify an exact offset using the newer style. I will implement this.

What am I overlooking? set_kexec_crash_area_size() takes the new syntax into consideration, and kexec_reserve_area() gets called only afterwards.

Jan