kexec: Clear notes during setup

kexec: Clear notes during setup

[Andrew Cooper]

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

I know that xen-unstable is on a feature freeze, and this is not
strictly a bugfix (yet; see below), but as it is safe and designed to
help clarity in the case of a crash, so I request that it be considered
for inclusion.

I have constructed an artificial case where the information reported in
1 per-cpu crash note was stale by using low_crashinfo mode, crashing
Xen, allowing it to reboot, offlining a CPU then re-crashing Xen.  This
leaves stale register state written into the offlined CPU crash
information.  In this case, the information was stale but correct, due
to the predictable nature of the Xen crash path from the 'C' debug key,
but there is no guarantee that in the case of a real crash, the same
will still be true.

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


[-- Attachment #2: kexec-clear-notes.patch --]
[-- Type: text/x-patch, Size: 1502 bytes --]

# HG changeset patch
# Parent 98fe3b2a572d4ffe704124e75c7aa8d94dbb51bc
kexec: clear notes during setup

Explicity zero the memory backing the crash notes during setup.

This allows the crash environment to be rather more certain whether the crash
notes were actually written, rather than trusting that the memory was clear
beforehand.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 98fe3b2a572d xen/common/kexec.c
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -401,7 +401,7 @@ static int kexec_init_cpu_notes(const un
 
     /* If we dont care about the position of allocation, malloc. */
     if ( low_crashinfo_mode == LOW_CRASHINFO_NONE )
-        note = xmalloc_bytes(nr_bytes);
+        note = xzalloc_bytes(nr_bytes);
 
     /* Protect the write into crash_notes[] with a spinlock, as this function
      * is on a hotplug path and a hypercall path. */
@@ -505,7 +505,7 @@ static int __init kexec_init(void)
 
     if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
     {
-        size_t crash_heap_size;
+        size_t crash_heap_size, i;
 
         /* This calculation is safe even if the machine is booted in
          * uniprocessor mode. */
@@ -520,6 +520,9 @@ static int __init kexec_init(void)
         if ( ! crash_heap_current )
             return -ENOMEM;
 
+        for ( i=0; i< (crash_heap_size >> PAGE_SHIFT); ++i )
+            clear_page(crash_heap_current + (i << PAGE_SHIFT));
+
         crash_heap_end = crash_heap_current + crash_heap_size;
     }
 

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

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

Re: kexec: Clear notes during setup

[Jan Beulich]

>>> On 04.05.12 at 13:42, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> I know that xen-unstable is on a feature freeze, and this is not
> strictly a bugfix (yet; see below), but as it is safe and designed to
> help clarity in the case of a crash, so I request that it be considered
> for inclusion.
> 
> I have constructed an artificial case where the information reported in
> 1 per-cpu crash note was stale by using low_crashinfo mode, crashing
> Xen, allowing it to reboot, offlining a CPU then re-crashing Xen.  This
> leaves stale register state written into the offlined CPU crash
> information.  In this case, the information was stale but correct, due
> to the predictable nature of the Xen crash path from the 'C' debug key,
> but there is no guarantee that in the case of a real crash, the same
> will still be true.

Apart from the missing blanks in the for() statement (which could as
well be a simple memset() afaict),
Acked-by: Jan Beulich <jbeulich@suse.com>

I personally would think that this can go in as a bug fix.

Jan

Re: kexec: Clear notes during setup

[Andrew Cooper]

On 04/05/12 13:51, Jan Beulich wrote:
>>>> On 04.05.12 at 13:42, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> I know that xen-unstable is on a feature freeze, and this is not
>> strictly a bugfix (yet; see below), but as it is safe and designed to
>> help clarity in the case of a crash, so I request that it be considered
>> for inclusion.
>>
>> I have constructed an artificial case where the information reported in
>> 1 per-cpu crash note was stale by using low_crashinfo mode, crashing
>> Xen, allowing it to reboot, offlining a CPU then re-crashing Xen.  This
>> leaves stale register state written into the offlined CPU crash
>> information.  In this case, the information was stale but correct, due
>> to the predictable nature of the Xen crash path from the 'C' debug key,
>> but there is no guarantee that in the case of a real crash, the same
>> will still be true.
> Apart from the missing blanks in the for() statement (which could as
> well be a simple memset() afaict),
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> I personally would think that this can go in as a bug fix.
>
> Jan

How would you format the for loop differently? (Not that I mind - just
so I know for next time)

As for clear_page vs memset - clear_page is faster, and liable to be
conditionally tuned more in the future.

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

Re: kexec: Clear notes during setup

[Jan Beulich]

>>> On 04.05.12 at 17:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 04/05/12 13:51, Jan Beulich wrote:
>>>>> On 04.05.12 at 13:42, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> I know that xen-unstable is on a feature freeze, and this is not
>>> strictly a bugfix (yet; see below), but as it is safe and designed to
>>> help clarity in the case of a crash, so I request that it be considered
>>> for inclusion.
>>>
>>> I have constructed an artificial case where the information reported in
>>> 1 per-cpu crash note was stale by using low_crashinfo mode, crashing
>>> Xen, allowing it to reboot, offlining a CPU then re-crashing Xen.  This
>>> leaves stale register state written into the offlined CPU crash
>>> information.  In this case, the information was stale but correct, due
>>> to the predictable nature of the Xen crash path from the 'C' debug key,
>>> but there is no guarantee that in the case of a real crash, the same
>>> will still be true.
>> Apart from the missing blanks in the for() statement (which could as
>> well be a simple memset() afaict),
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> I personally would think that this can go in as a bug fix.
>>
>> Jan
> 
> How would you format the for loop differently? (Not that I mind - just
> so I know for next time)

        for ( i = 0; i < (crash_heap_size >> PAGE_SHIFT); ++i )

> As for clear_page vs memset - clear_page is faster, and liable to be
> conditionally tuned more in the future.

Certainly, but does this matter here?

Jan

Re: kexec: Clear notes during setup

[Andrew Cooper]

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

On 04/05/12 16:25, Jan Beulich wrote:
>>>> On 04.05.12 at 17:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 04/05/12 13:51, Jan Beulich wrote:
>>>>>> On 04.05.12 at 13:42, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> I know that xen-unstable is on a feature freeze, and this is not
>>>> strictly a bugfix (yet; see below), but as it is safe and designed to
>>>> help clarity in the case of a crash, so I request that it be considered
>>>> for inclusion.
>>>>
>>>> I have constructed an artificial case where the information reported in
>>>> 1 per-cpu crash note was stale by using low_crashinfo mode, crashing
>>>> Xen, allowing it to reboot, offlining a CPU then re-crashing Xen.  This
>>>> leaves stale register state written into the offlined CPU crash
>>>> information.  In this case, the information was stale but correct, due
>>>> to the predictable nature of the Xen crash path from the 'C' debug key,
>>>> but there is no guarantee that in the case of a real crash, the same
>>>> will still be true.
>>> Apart from the missing blanks in the for() statement (which could as
>>> well be a simple memset() afaict),
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> I personally would think that this can go in as a bug fix.
>>>
>>> Jan
>> How would you format the for loop differently? (Not that I mind - just
>> so I know for next time)
>         for ( i = 0; i < (crash_heap_size >> PAGE_SHIFT); ++i )

Ok - refreshed the patch as such

>
>> As for clear_page vs memset - clear_page is faster, and liable to be
>> conditionally tuned more in the future.
> Certainly, but does this matter here?
>
> Jan
>

crash_heap_size scales linearly with the number of PCPUs on the system,
so very large boxes might start noticing a difference in boot speed. 
(Probably not in the grand scheme of things, but as we are explicitly
allocating pages, it makes sense to clear them as pages)

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


[-- Attachment #2: kexec-clear-notes.patch --]
[-- Type: text/x-patch, Size: 1505 bytes --]

# HG changeset patch
# Parent 98fe3b2a572d4ffe704124e75c7aa8d94dbb51bc
kexec: clear notes during setup

Explicity zero the memory backing the crash notes during setup.

This allows the crash environment to be rather more certain whether the crash
notes were actually written, rather than trusting that the memory was clear
beforehand.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 98fe3b2a572d xen/common/kexec.c
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -401,7 +401,7 @@ static int kexec_init_cpu_notes(const un
 
     /* If we dont care about the position of allocation, malloc. */
     if ( low_crashinfo_mode == LOW_CRASHINFO_NONE )
-        note = xmalloc_bytes(nr_bytes);
+        note = xzalloc_bytes(nr_bytes);
 
     /* Protect the write into crash_notes[] with a spinlock, as this function
      * is on a hotplug path and a hypercall path. */
@@ -505,7 +505,7 @@ static int __init kexec_init(void)
 
     if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
     {
-        size_t crash_heap_size;
+        size_t crash_heap_size, i;
 
         /* This calculation is safe even if the machine is booted in
          * uniprocessor mode. */
@@ -520,6 +520,9 @@ static int __init kexec_init(void)
         if ( ! crash_heap_current )
             return -ENOMEM;
 
+        for ( i = 0; i < (crash_heap_size >> PAGE_SHIFT); ++i )
+            clear_page(crash_heap_current + (i << PAGE_SHIFT));
+
         crash_heap_end = crash_heap_current + crash_heap_size;
     }
 

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

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

Re: kexec: Clear notes during setup

[David Vrabel]

On 04/05/12 16:39, Andrew Cooper wrote:
> On 04/05/12 16:25, Jan Beulich wrote:
>>>>> On 04.05.12 at 17:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 04/05/12 13:51, Jan Beulich wrote:
>>>>>>> On 04.05.12 at 13:42, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>>> I know that xen-unstable is on a feature freeze, and this is not
>>>>> strictly a bugfix (yet; see below), but as it is safe and designed to
>>>>> help clarity in the case of a crash, so I request that it be considered
>>>>> for inclusion.
>>>>>
>>>>> I have constructed an artificial case where the information reported in
>>>>> 1 per-cpu crash note was stale by using low_crashinfo mode, crashing
>>>>> Xen, allowing it to reboot, offlining a CPU then re-crashing Xen.  This
>>>>> leaves stale register state written into the offlined CPU crash
>>>>> information.  In this case, the information was stale but correct, due
>>>>> to the predictable nature of the Xen crash path from the 'C' debug key,
>>>>> but there is no guarantee that in the case of a real crash, the same
>>>>> will still be true.
>>>> Apart from the missing blanks in the for() statement (which could as
>>>> well be a simple memset() afaict),
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> I personally would think that this can go in as a bug fix.
>>>>
>>>> Jan
>>> How would you format the for loop differently? (Not that I mind - just
>>> so I know for next time)
>>         for ( i = 0; i < (crash_heap_size >> PAGE_SHIFT); ++i )
> 
> Ok - refreshed the patch as such

See also CODING_STYLE in the top level of the Xen source tree.

>>> As for clear_page vs memset - clear_page is faster, and liable to be
>>> conditionally tuned more in the future.
>> Certainly, but does this matter here?
>>
>> Jan
>>
> 
> crash_heap_size scales linearly with the number of PCPUs on the system,
> so very large boxes might start noticing a difference in boot speed. 
> (Probably not in the grand scheme of things, but as we are explicitly
> allocating pages, it makes sense to clear them as pages)

If this is important then there should be a alloc_zeroed_xenheap_pages()
function for this and not an open-coded loop.  I'd just use a memset() here.

David