Re: [PATCHv11 3/9] kexec: add infrastructure for handling kexec images

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

On 18/11/13 08:07, Jan Beulich wrote:
>>>> On 15.11.13 at 19:31, David Vrabel <david.vrabel@citrix.com> wrote:
>> On 15/11/13 14:35, Jan Beulich wrote:
>>>>>> On 08.11.13 at 13:50, David Vrabel <david.vrabel@citrix.com> wrote:
>>>> Add the code needed to handle and load kexec images into Xen memory or
>>>> into the crash region.  This is needed for the new KEXEC_CMD_load and
>>>> KEXEC_CMD_unload hypercall sub-ops.
>>>
>>> I know it's late in the game, but just now I started getting the
>>> impression that this introduced a new limitation that needs to
>>> be taken into consideration elsewhere: With the old
>>> implementation it was the kernel's responsibility to write to
>>> the reserved space or, where Xen needed to touch the space,
>>> it did so via fixmap entries. Hence there was no need for the
>>> area to have corresponding struct page_info.
>>>
>>> The new code, however, appears to make assumptions that
>>> the memory used here is part of the range covered by the
>>> frame table, and hence setup.c's determination of the base
>>> address would need to be adjusted accordingly. (I realize
>>> that this only matters on systems having more RAM than the
>>> hypervisor can make use of.)
>>
>> The relocation code wrote the image into the crash region, not the
>> kernel, but I take your point.
>>
>> Is this a real problem or just a theoretical one for now?
> 
> Not sure what "theoretical" here means - I know of actual systems
> (even if perhaps not commercially available yet) that would be
> affected by this.

The administrator has to configure the location of the crash region.  I
was asking if there are systems that configure the crash region such
that it would would end in the wrong place.

It does appear that the simplest crashkernel configuration would get it
wrong.  e.g., crashkernel=0-:64M

>> I don't think
>> it's unreasonable to require the crash region to be within the frame table.
> 
> Right - as I assume you don't want to change all of your mapping
> code, the only alternative is for the restriction to be enforced when
> allocating the memory block.

The

   map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
                     kexec_crash_area.start >> PAGE_SHIFT,
                     PFN_UP(kexec_crash_area.size), PAGE_HYPERVISOR);

call in __start_xen() suggests that this isn't a new problem.

This seems like a minor issue and if no one finds the time to fix it, I
think simply adding a release note would do.

David