kexec's v1 compatibility code

kexec's v1 compatibility code

[Jan Beulich]

David,

now that we're putting Xen 4.4.x underneath an older distro (SLE11)
we've got to see that kexec doesn't work there. Initial investigation
of our kexec person revealed that the destinations attempted to be
written to by kexec_reloc()'s code following the is_source and
is_zero labels have no mappings in the kexec page tables. Comparing
kexec_do_load_v1() with kexec_load() I wonder whether the former
isn't simply lacking a call to kimage_load_segments().

He worked around this (I haven't seen the code yet that he used)
to then find that the dump kernel (other than an "ordinary" kexec
one) also expects at least the low 640k to be mapped. He's
suggesting that Linux'es kexec code sets up an identity mapping of
all memory, and that we should do the same. I can't say I'm
convinced of this though, as it seems bogus to me that the dump
kernel should depend on anything beyond a bare minimum
environment it is being handed control in; I would instead expect
the crash kernel to be responsible for any such specific needs.

May I also ask whether that compatibility code got tested?

Jan

Re: kexec's v1 compatibility code

[Andrew Cooper]

On 08/05/15 14:34, Jan Beulich wrote:
> David,
>
> now that we're putting Xen 4.4.x underneath an older distro (SLE11)
> we've got to see that kexec doesn't work there. Initial investigation
> of our kexec person revealed that the destinations attempted to be
> written to by kexec_reloc()'s code following the is_source and
> is_zero labels have no mappings in the kexec page tables. Comparing
> kexec_do_load_v1() with kexec_load() I wonder whether the former
> isn't simply lacking a call to kimage_load_segments().
>
> He worked around this (I haven't seen the code yet that he used)
> to then find that the dump kernel (other than an "ordinary" kexec
> one) also expects at least the low 640k to be mapped. He's
> suggesting that Linux'es kexec code sets up an identity mapping of
> all memory, and that we should do the same. I can't say I'm
> convinced of this though, as it seems bogus to me that the dump
> kernel should depend on anything beyond a bare minimum
> environment it is being handed control in; I would instead expect
> the crash kernel to be responsible for any such specific needs.
>
> May I also ask whether that compatibility code got tested?

This is all from a while ago.  It is quite possible that we didn't
actually tested the compatibility case with a 64bit dom0 kernel,
although I certainly did test earlier versions of the series with a
32bit dom0 kernel.  The work was done long before XenServer moved to a
64bit dom0, and was done by deleting everything and starting from scratch.

IIRC, the low 640k mappings is a purgatory bug rather than Linux, and
has been fixed upstream in kexec-tools since.  (I recall that it used to
take a backup copy of the IVT for some reason)

~Andrew

Re: kexec's v1 compatibility code

[David Vrabel]

On 08/05/15 14:34, Jan Beulich wrote:
> David,
> 
> now that we're putting Xen 4.4.x underneath an older distro (SLE11)
> we've got to see that kexec doesn't work there. Initial investigation
> of our kexec person revealed that the destinations attempted to be
> written to by kexec_reloc()'s code following the is_source and
> is_zero labels have no mappings in the kexec page tables. Comparing
> kexec_do_load_v1() with kexec_load() I wonder whether the former
> isn't simply lacking a call to kimage_load_segments().

I think I only tested the V1 path with 32-bit images which did not need
page tables.

The caller of the V1 kexec_load has already loaded the segments into
their (potentially intermediate) destination so the apparently missing
kimage_load_segments() is deliberate.

I think kimage_build_ind() needs to call machine_kexec_add_page()
appropriately.

> He worked around this (I haven't seen the code yet that he used)
> to then find that the dump kernel (other than an "ordinary" kexec
> one) also expects at least the low 640k to be mapped. He's
> suggesting that Linux'es kexec code sets up an identity mapping of
> all memory, and that we should do the same. I can't say I'm
> convinced of this though, as it seems bogus to me that the dump
> kernel should depend on anything beyond a bare minimum
> environment it is being handed control in; I would instead expect
> the crash kernel to be responsible for any such specific needs.

The version of kexec-tools that works with the V2 ABI, adds an extra
segment (with no source buffer) to add a mapping of 0-1MIB that
purgatory expects.  Perhaps the v1 compat code needs to do the same?

> May I also ask whether that compatibility code got tested?

Only with 32-bit images -- hence the page table related problems.

David

Re: kexec's v1 compatibility code

[Jan Beulich]

>>> On 08.05.15 at 17:53, <dvrabel@cantab.net> wrote:
> On 08/05/15 14:34, Jan Beulich wrote:
>> now that we're putting Xen 4.4.x underneath an older distro (SLE11)
>> we've got to see that kexec doesn't work there. Initial investigation
>> of our kexec person revealed that the destinations attempted to be
>> written to by kexec_reloc()'s code following the is_source and
>> is_zero labels have no mappings in the kexec page tables. Comparing
>> kexec_do_load_v1() with kexec_load() I wonder whether the former
>> isn't simply lacking a call to kimage_load_segments().
> 
> I think I only tested the V1 path with 32-bit images which did not need
> page tables.
> 
> The caller of the V1 kexec_load has already loaded the segments into
> their (potentially intermediate) destination so the apparently missing
> kimage_load_segments() is deliberate.
> 
> I think kimage_build_ind() needs to call machine_kexec_add_page()
> appropriately.

Okay, iiuc IND_SOURCE and IND_DONE don't need any adjustment.
Would the below therefore look okay, or did I simply not find where
the indirection pages get handled?

Thanks, Jan

--- a/xen/common/kimage.c
+++ b/xen/common/kimage.c
@@ -863,9 +863,14 @@ int kimage_build_ind(struct kexec_image 
 {
     void *page;
     kimage_entry_t *entry;
-    int ret = 0;
+    int ret;
     paddr_t dest = KIMAGE_NO_DEST;
 
+    ret = machine_kexec_add_page(image, pfn_to_paddr(ind_mfn),
+                                 pfn_to_paddr(ind_mfn));
+    if ( ret < 0 )
+        return ret;
+
     page = map_domain_page(ind_mfn);
     if ( !page )
         return -ENOMEM;
@@ -887,10 +892,16 @@ int kimage_build_ind(struct kexec_image 
         case IND_DESTINATION:
             dest = (paddr_t)mfn << PAGE_SHIFT;
             ret = kimage_set_destination(image, dest);
+            if ( !ret )
+                ret = machine_kexec_add_page(image, dest, dest);
             if ( ret < 0 )
                 goto done;
             break;
         case IND_INDIRECTION:
+            ret = machine_kexec_add_page(image, pfn_to_paddr(mfn),
+                                         pfn_to_paddr(mfn));
+            if ( ret < 0 )
+                goto done;
             unmap_domain_page(page);
             page = map_domain_page(mfn);
             entry = page;

Re: kexec's v1 compatibility code

[David Vrabel]

On 11/05/15 10:25, Jan Beulich wrote:
>>>> On 08.05.15 at 17:53, <dvrabel@cantab.net> wrote:
>> On 08/05/15 14:34, Jan Beulich wrote:
>>> now that we're putting Xen 4.4.x underneath an older distro (SLE11)
>>> we've got to see that kexec doesn't work there. Initial investigation
>>> of our kexec person revealed that the destinations attempted to be
>>> written to by kexec_reloc()'s code following the is_source and
>>> is_zero labels have no mappings in the kexec page tables. Comparing
>>> kexec_do_load_v1() with kexec_load() I wonder whether the former
>>> isn't simply lacking a call to kimage_load_segments().
>>
>> I think I only tested the V1 path with 32-bit images which did not need
>> page tables.
>>
>> The caller of the V1 kexec_load has already loaded the segments into
>> their (potentially intermediate) destination so the apparently missing
>> kimage_load_segments() is deliberate.
>>
>> I think kimage_build_ind() needs to call machine_kexec_add_page()
>> appropriately.
> 
> Okay, iiuc IND_SOURCE and IND_DONE don't need any adjustment.
> Would the below therefore look okay, or did I simply not find where
> the indirection pages get handled?
> 
> Thanks, Jan
> 
> --- a/xen/common/kimage.c
> +++ b/xen/common/kimage.c
> @@ -863,9 +863,14 @@ int kimage_build_ind(struct kexec_image 
>  {
>      void *page;
>      kimage_entry_t *entry;
> -    int ret = 0;
> +    int ret;
>      paddr_t dest = KIMAGE_NO_DEST;
>  
> +    ret = machine_kexec_add_page(image, pfn_to_paddr(ind_mfn),
> +                                 pfn_to_paddr(ind_mfn));
> +    if ( ret < 0 )
> +        return ret;
> +

You don't need this one because after building the new indirection
pages, we discard the guest supplied one.

>      page = map_domain_page(ind_mfn);
>      if ( !page )
>          return -ENOMEM;
> @@ -887,10 +892,16 @@ int kimage_build_ind(struct kexec_image 
>          case IND_DESTINATION:
>              dest = (paddr_t)mfn << PAGE_SHIFT;
>              ret = kimage_set_destination(image, dest);
> +            if ( !ret )
> +                ret = machine_kexec_add_page(image, dest, dest);

This is the one that was missing.  It matches the
machine_kexec_add_page() call for the destinations in kimage_load_segment().

>              if ( ret < 0 )
>                  goto done;
>              break;
>          case IND_INDIRECTION:
> +            ret = machine_kexec_add_page(image, pfn_to_paddr(mfn),
> +                                         pfn_to_paddr(mfn));
> +            if ( ret < 0 )
> +                goto done;

You don't need this one either, because this MFN is another
guest-supplied indirection page we're not going to use.

>              unmap_domain_page(page);
>              page = map_domain_page(mfn);
>              entry = page;

David