[PATCH] kexec: allow relaxed placement specification via command line

[PATCH] kexec: allow relaxed placement specification via command line

[Jan Beulich]

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

Rather than just allowing a fixed address or fully automatic placement,
also allow for specifying an upper bound. Especially on EFI systems,
where firmware memory use is commonly less predictable than on legacy
BIOS ones, this makes success of the reservation more likely when
automatic placement is not an option (e.g. because of special DMA
restrictions of devices involved in actually carrying out the dump).

Also take the opportunity to actually add text to the "crashkernel"
entry in the command line option doc.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -458,7 +458,18 @@ Specify the maximum address to allocate
 combination with the `low_crashinfo` command line option.
 
 ### crashkernel
-> `= <ramsize-range>:<size>[,...][@<offset>]`
+> `= <ramsize-range>:<size>[,...][{@,<}<offset>]`
+> `= <size>[{@,<}<offset>]`
+
+Specify sizes and optionally placement of the kexec reservation area.
+The `<ramsize-range>:<size>' pairs indicate how much memory to set
+aside for kexec (`<size>') for a given range of installed RAM
+(`<ramsize-range>').  Each `<ramsize-range>' is of the form
+`<start>-[<end>]'.
+
+A trailing `@<offset>' specifies the exact address this area should be
+placed at, whereas `<' in place of `@' just specifies an upper bound of
+the address range the area should fall into.
 
 ### credit2\_balance\_over
 > `= <integer>`
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1044,13 +1044,19 @@ void __init noreturn __start_xen(unsigne
         }
 
 #ifdef CONFIG_KEXEC
-        /* 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) )
+        while ( !kexec_crash_area.start )
         {
-            e = (e - kexec_crash_area.size) & PAGE_MASK;
-            kexec_crash_area.start = e;
+            /* Don't overlap with modules. */
+            e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size),
+                                 mod, mbi->mods_count, -1);
+            if ( s >= e )
+                break;
+            if ( e > kexec_crash_area_limit )
+            {
+                e = kexec_crash_area_limit & PAGE_MASK;
+                continue;
+            }
+            kexec_crash_area.start = (e - kexec_crash_area.size) & PAGE_MASK;
         }
 #endif
     }
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -60,6 +60,7 @@ static unsigned char vmcoreinfo_data[VMC
 static size_t vmcoreinfo_size = 0;
 
 xen_kexec_reserve_t kexec_crash_area;
+paddr_t __initdata kexec_crash_area_limit = ~(paddr_t)0;
 static struct {
     u64 start, end;
     unsigned long size;
@@ -86,7 +87,7 @@ static void *crash_heap_current = NULL,
 /*
  * Parse command lines in the format
  *
- *   crashkernel=<ramsize-range>:<size>[,...][@<offset>]
+ *   crashkernel=<ramsize-range>:<size>[,...][{@,<}<address>]
  *
  * with <ramsize-range> being of form
  *
@@ -94,7 +95,7 @@ static void *crash_heap_current = NULL,
  *
  * as well as the legacy ones in the format
  *
- *   crashkernel=<size>[@<offset>]
+ *   crashkernel=<size>[{@,<}<address>]
  */
 static void __init parse_crashkernel(const char *str)
 {
@@ -109,7 +110,7 @@ static void __init parse_crashkernel(con
             {
                 printk(XENLOG_WARNING "crashkernel: too many ranges\n");
                 cur = NULL;
-                str = strchr(str, '@');
+                str = strpbrk(str, "@<");
                 break;
             }
 
@@ -154,9 +155,16 @@ static void __init parse_crashkernel(con
     }
     else
         kexec_crash_area.size = parse_size_and_unit(cur = str, &str);
-    if ( cur != str && *str == '@' )
-        kexec_crash_area.start = parse_size_and_unit(cur = str + 1, &str);
-    if ( cur == str )
+    if ( cur != str )
+    {
+        if ( *str == '@' )
+            kexec_crash_area.start = parse_size_and_unit(cur = str + 1, &str);
+        else if ( *str == '<' )
+            kexec_crash_area_limit = parse_size_and_unit(cur = str + 1, &str);
+        else
+            printk(XENLOG_WARNING "crashkernel: '%s' ignored\n", str);
+    }
+    if ( cur && cur == str )
         printk(XENLOG_WARNING "crashkernel: memory value expected\n");
 }
 custom_param("crashkernel", parse_crashkernel);
--- a/xen/include/xen/kexec.h
+++ b/xen/include/xen/kexec.h
@@ -14,6 +14,7 @@ typedef struct xen_kexec_reserve {
 } xen_kexec_reserve_t;
 
 extern xen_kexec_reserve_t kexec_crash_area;
+extern paddr_t kexec_crash_area_limit;
 
 extern bool_t kexecing;
 



[-- Attachment #2: kexec-relaxed-placement.patch --]
[-- Type: text/plain, Size: 4838 bytes --]

kexec: allow relaxed placement specification via command line

Rather than just allowing a fixed address or fully automatic placement,
also allow for specifying an upper bound. Especially on EFI systems,
where firmware memory use is commonly less predictable than on legacy
BIOS ones, this makes success of the reservation more likely when
automatic placement is not an option (e.g. because of special DMA
restrictions of devices involved in actually carrying out the dump).

Also take the opportunity to actually add text to the "crashkernel"
entry in the command line option doc.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -458,7 +458,18 @@ Specify the maximum address to allocate
 combination with the `low_crashinfo` command line option.
 
 ### crashkernel
-> `= <ramsize-range>:<size>[,...][@<offset>]`
+> `= <ramsize-range>:<size>[,...][{@,<}<offset>]`
+> `= <size>[{@,<}<offset>]`
+
+Specify sizes and optionally placement of the kexec reservation area.
+The `<ramsize-range>:<size>' pairs indicate how much memory to set
+aside for kexec (`<size>') for a given range of installed RAM
+(`<ramsize-range>').  Each `<ramsize-range>' is of the form
+`<start>-[<end>]'.
+
+A trailing `@<offset>' specifies the exact address this area should be
+placed at, whereas `<' in place of `@' just specifies an upper bound of
+the address range the area should fall into.
 
 ### credit2\_balance\_over
 > `= <integer>`
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1044,13 +1044,19 @@ void __init noreturn __start_xen(unsigne
         }
 
 #ifdef CONFIG_KEXEC
-        /* 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) )
+        while ( !kexec_crash_area.start )
         {
-            e = (e - kexec_crash_area.size) & PAGE_MASK;
-            kexec_crash_area.start = e;
+            /* Don't overlap with modules. */
+            e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size),
+                                 mod, mbi->mods_count, -1);
+            if ( s >= e )
+                break;
+            if ( e > kexec_crash_area_limit )
+            {
+                e = kexec_crash_area_limit & PAGE_MASK;
+                continue;
+            }
+            kexec_crash_area.start = (e - kexec_crash_area.size) & PAGE_MASK;
         }
 #endif
     }
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -60,6 +60,7 @@ static unsigned char vmcoreinfo_data[VMC
 static size_t vmcoreinfo_size = 0;
 
 xen_kexec_reserve_t kexec_crash_area;
+paddr_t __initdata kexec_crash_area_limit = ~(paddr_t)0;
 static struct {
     u64 start, end;
     unsigned long size;
@@ -86,7 +87,7 @@ static void *crash_heap_current = NULL,
 /*
  * Parse command lines in the format
  *
- *   crashkernel=<ramsize-range>:<size>[,...][@<offset>]
+ *   crashkernel=<ramsize-range>:<size>[,...][{@,<}<address>]
  *
  * with <ramsize-range> being of form
  *
@@ -94,7 +95,7 @@ static void *crash_heap_current = NULL,
  *
  * as well as the legacy ones in the format
  *
- *   crashkernel=<size>[@<offset>]
+ *   crashkernel=<size>[{@,<}<address>]
  */
 static void __init parse_crashkernel(const char *str)
 {
@@ -109,7 +110,7 @@ static void __init parse_crashkernel(con
             {
                 printk(XENLOG_WARNING "crashkernel: too many ranges\n");
                 cur = NULL;
-                str = strchr(str, '@');
+                str = strpbrk(str, "@<");
                 break;
             }
 
@@ -154,9 +155,16 @@ static void __init parse_crashkernel(con
     }
     else
         kexec_crash_area.size = parse_size_and_unit(cur = str, &str);
-    if ( cur != str && *str == '@' )
-        kexec_crash_area.start = parse_size_and_unit(cur = str + 1, &str);
-    if ( cur == str )
+    if ( cur != str )
+    {
+        if ( *str == '@' )
+            kexec_crash_area.start = parse_size_and_unit(cur = str + 1, &str);
+        else if ( *str == '<' )
+            kexec_crash_area_limit = parse_size_and_unit(cur = str + 1, &str);
+        else
+            printk(XENLOG_WARNING "crashkernel: '%s' ignored\n", str);
+    }
+    if ( cur && cur == str )
         printk(XENLOG_WARNING "crashkernel: memory value expected\n");
 }
 custom_param("crashkernel", parse_crashkernel);
--- a/xen/include/xen/kexec.h
+++ b/xen/include/xen/kexec.h
@@ -14,6 +14,7 @@ typedef struct xen_kexec_reserve {
 } xen_kexec_reserve_t;
 
 extern xen_kexec_reserve_t kexec_crash_area;
+extern paddr_t kexec_crash_area_limit;
 
 extern bool_t kexecing;
 

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

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

Re: [PATCH] kexec: allow relaxed placement specification via command line

[Andrew Cooper]

On 30/05/16 14:48, Jan Beulich wrote:
> Rather than just allowing a fixed address or fully automatic placement,
> also allow for specifying an upper bound. Especially on EFI systems,
> where firmware memory use is commonly less predictable than on legacy
> BIOS ones, this makes success of the reservation more likely when
> automatic placement is not an option (e.g. because of special DMA
> restrictions of devices involved in actually carrying out the dump).
>
> Also take the opportunity to actually add text to the "crashkernel"
> entry in the command line option doc.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -458,7 +458,18 @@ Specify the maximum address to allocate
>  combination with the `low_crashinfo` command line option.
>  
>  ### crashkernel
> -> `= <ramsize-range>:<size>[,...][@<offset>]`
> +> `= <ramsize-range>:<size>[,...][{@,<}<offset>]`
> +> `= <size>[{@,<}<offset>]`
> +
> +Specify sizes and optionally placement of the kexec reservation area.
> +The `<ramsize-range>:<size>' pairs indicate how much memory to set

For markdown, you need to use matching ` ` pairs for formatting the
containing text as monospace.

Other than this, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH] kexec: allow relaxed placement specification via command line

[Jan Beulich]

>>> On 31.05.16 at 12:24, <andrew.cooper3@citrix.com> wrote:
> On 30/05/16 14:48, Jan Beulich wrote:
>> Rather than just allowing a fixed address or fully automatic placement,
>> also allow for specifying an upper bound. Especially on EFI systems,
>> where firmware memory use is commonly less predictable than on legacy
>> BIOS ones, this makes success of the reservation more likely when
>> automatic placement is not an option (e.g. because of special DMA
>> restrictions of devices involved in actually carrying out the dump).
>>
>> Also take the opportunity to actually add text to the "crashkernel"
>> entry in the command line option doc.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -458,7 +458,18 @@ Specify the maximum address to allocate
>>  combination with the `low_crashinfo` command line option.
>>  
>>  ### crashkernel
>> -> `= <ramsize-range>:<size>[,...][@<offset>]`
>> +> `= <ramsize-range>:<size>[,...][{@,<}<offset>]`
>> +> `= <size>[{@,<}<offset>]`
>> +
>> +Specify sizes and optionally placement of the kexec reservation area.
>> +The `<ramsize-range>:<size>' pairs indicate how much memory to set
> 
> For markdown, you need to use matching ` ` pairs for formatting the
> containing text as monospace.

Oh, okay. I meant to copy what was there, and now I see that
I didn't look right. Fixed.

> Other than this, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks, Jan


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

Re: [PATCH] kexec: allow relaxed placement specification via command line

[David Vrabel]

On 30/05/16 14:48, Jan Beulich wrote:
> Rather than just allowing a fixed address or fully automatic placement,
> also allow for specifying an upper bound. Especially on EFI systems,
> where firmware memory use is commonly less predictable than on legacy
> BIOS ones, this makes success of the reservation more likely when
> automatic placement is not an option (e.g. because of special DMA
> restrictions of devices involved in actually carrying out the dump).
> 
> Also take the opportunity to actually add text to the "crashkernel"
> entry in the command line option doc.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1044,13 +1044,19 @@ void __init noreturn __start_xen(unsigne
>          }
>  
>  #ifdef CONFIG_KEXEC
> -        /* 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) )

I think we want a comment here.

/*
 * Looking backwards from the crash area limit, find a large enough
 * crash area that does not overlap with modules.
 */

> +        while ( !kexec_crash_area.start )

Does this mean that if an @<offset> is specified we no longer check for
overlapping modules?

>          {
> -            e = (e - kexec_crash_area.size) & PAGE_MASK;
> -            kexec_crash_area.start = e;
> +            /* Don't overlap with modules. */
> +            e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size),
> +                                 mod, mbi->mods_count, -1);
> +            if ( s >= e )
> +                break;
> +            if ( e > kexec_crash_area_limit )
> +            {
> +                e = kexec_crash_area_limit & PAGE_MASK;
> +                continue;
> +            }
> +            kexec_crash_area.start = (e - kexec_crash_area.size) & PAGE_MASK;
>          }
>  #endif
>      }

David


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

Re: [PATCH] kexec: allow relaxed placement specification via command line

[Jan Beulich]

>>> On 31.05.16 at 12:30, <david.vrabel@citrix.com> wrote:
> On 30/05/16 14:48, Jan Beulich wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1044,13 +1044,19 @@ void __init noreturn __start_xen(unsigne
>>          }
>>  
>>  #ifdef CONFIG_KEXEC
>> -        /* 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) )
> 
> I think we want a comment here.
> 
> /*
>  * Looking backwards from the crash area limit, find a large enough
>  * crash area that does not overlap with modules.
>  */

Sure, added.

>> +        while ( !kexec_crash_area.start )
> 
> Does this mean that if an @<offset> is specified we no longer check for
> overlapping modules?

We didn't do any more checking before. If you look at the old
code above, we called consider_modules() only to possibly alter
e. All the rest of the old code was similarly dependent upon
!kexec_crash_area.start. That other case is being taken care
of earlier anyway - see kexec_reserve_area()'s first invocation.

But yes, it looks like there's an overlap check missing there (iiuc
relevant really only for the initrd, as that's the only thing the
memory of which may not get copied but simply directly handed
to Dom0).

Jan


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

Re: [PATCH] kexec: allow relaxed placement specification via command line

[David Vrabel]

On 31/05/16 13:44, Jan Beulich wrote:
>>>> On 31.05.16 at 12:30, <david.vrabel@citrix.com> wrote:
>> On 30/05/16 14:48, Jan Beulich wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1044,13 +1044,19 @@ void __init noreturn __start_xen(unsigne
>>>          }
>>>  
>>>  #ifdef CONFIG_KEXEC
>>> -        /* 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) )
>>
>> I think we want a comment here.
>>
>> /*
>>  * Looking backwards from the crash area limit, find a large enough
>>  * crash area that does not overlap with modules.
>>  */
> 
> Sure, added.
> 
>>> +        while ( !kexec_crash_area.start )
>>
>> Does this mean that if an @<offset> is specified we no longer check for
>> overlapping modules?
> 
> We didn't do any more checking before. If you look at the old
> code above, we called consider_modules() only to possibly alter
> e. All the rest of the old code was similarly dependent upon
> !kexec_crash_area.start. That other case is being taken care
> of earlier anyway - see kexec_reserve_area()'s first invocation.
> 
> But yes, it looks like there's an overlap check missing there (iiuc
> relevant really only for the initrd, as that's the only thing the
> memory of which may not get copied but simply directly handed
> to Dom0).

Ok.  Any additional improvement can be done later so if you add the comment,

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

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

Re: [PATCH] kexec: allow relaxed placement specification via command line

[Daniel Kiper]

On Mon, May 30, 2016 at 07:48:26AM -0600, Jan Beulich wrote:
> Rather than just allowing a fixed address or fully automatic placement,
> also allow for specifying an upper bound. Especially on EFI systems,
> where firmware memory use is commonly less predictable than on legacy
> BIOS ones, this makes success of the reservation more likely when
> automatic placement is not an option (e.g. because of special DMA
> restrictions of devices involved in actually carrying out the dump).
>
> Also take the opportunity to actually add text to the "crashkernel"
> entry in the command line option doc.

Thank you for posting this.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -458,7 +458,18 @@ Specify the maximum address to allocate
>  combination with the `low_crashinfo` command line option.
>
>  ### crashkernel
> -> `= <ramsize-range>:<size>[,...][@<offset>]`
> +> `= <ramsize-range>:<size>[,...][{@,<}<offset>]`
> +> `= <size>[{@,<}<offset>]`
> +
> +Specify sizes and optionally placement of the kexec reservation area.

Should not we use "crash kernel reservation" instead of "kexec reservation"?
Kexec is a bit different thing and does not need upfront memory reservations.

> +The `<ramsize-range>:<size>' pairs indicate how much memory to set
> +aside for kexec (`<size>') for a given range of installed RAM

Ditto.

Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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

Re: [PATCH] kexec: allow relaxed placement specification via command line

[Jan Beulich]

>>> On 01.06.16 at 12:26, <daniel.kiper@oracle.com> wrote:
> On Mon, May 30, 2016 at 07:48:26AM -0600, Jan Beulich wrote:
>> Rather than just allowing a fixed address or fully automatic placement,
>> also allow for specifying an upper bound. Especially on EFI systems,
>> where firmware memory use is commonly less predictable than on legacy
>> BIOS ones, this makes success of the reservation more likely when
>> automatic placement is not an option (e.g. because of special DMA
>> restrictions of devices involved in actually carrying out the dump).
>>
>> Also take the opportunity to actually add text to the "crashkernel"
>> entry in the command line option doc.
> 
> Thank you for posting this.
> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -458,7 +458,18 @@ Specify the maximum address to allocate
>>  combination with the `low_crashinfo` command line option.
>>
>>  ### crashkernel
>> -> `= <ramsize-range>:<size>[,...][@<offset>]`
>> +> `= <ramsize-range>:<size>[,...][{@,<}<offset>]`
>> +> `= <size>[{@,<}<offset>]`
>> +
>> +Specify sizes and optionally placement of the kexec reservation area.
> 
> Should not we use "crash kernel reservation" instead of "kexec reservation"?
> Kexec is a bit different thing and does not need upfront memory 
> reservations.

Good idea, done.

>> +The `<ramsize-range>:<size>' pairs indicate how much memory to set
>> +aside for kexec (`<size>') for a given range of installed RAM
> 
> Ditto.
> 
> Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Thanks, Jan


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