Re: kexec's v1 compatibility code

[David Vrabel <david.vrabel@citrix.com>]

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