Re: [Xen-devel] [PATCH v3 12/16 - RFC] x86/efi: create new early memory allocator

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.05.16 at 21:48, <daniel.kiper@xxxxxxxxxx> wrote:
> On Wed, May 25, 2016 at 02:39:57AM -0600, Jan Beulich wrote:
>> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> wrote:
>> > There is a problem with place_string() which is used as early memory
>> > allocator. It gets memory chunks starting from start symbol and
>> > going down. Sadly this does not work when Xen is loaded using multiboot2
>> > protocol because start lives on 1 MiB address. So, I tried to use
>> > mem_lower address calculated by GRUB2. However, it works only on some
>> > machines. There are machines in the wild (e.g. Dell PowerEdge R820)
>> > which uses first ~640 KiB for boot services code or data... :-(((
>> >
>> > In case of multiboot2 protocol we need that place_string() only allocate
>> > memory chunk for EFI memory map. However, I think that it should be fixed
>> > instead of making another function used just in one case. I thought about
>> > two solutions.
>> >
>> > 1) We could use native EFI allocation functions (e.g. AllocatePool()
>> >    or AllocatePages()) to get memory chunk. However, later (somewhere
>> >    in __start_xen()) we must copy its contents to safe place or reserve
>> >    this in e820 memory map and map it in Xen virtual address space.
>> >    In later case we must also care about conflicts with e.g. crash
>> >    kernel regions which could be quite difficult.
>>
>> I don't see why that would be: Simply use an allocation type that
>> doesn't lead to the area getting consumed as normal RAM. Nor do
>> I see the kexec collision potential. Furthermore (and I think I've
>> said so before) ARM is already using AllocatePool() - just with an
>> unsuitable memory type -, so doing so on x86 too would allow for
> 
> Nope, they are using standard EfiLoaderData.

Note how I said "just with an unsuitable memory type"?

>> > Jan Beulich added 1b) Do away with efi_arch_allocate_mmap_buffer() and use
>> >    AllocatePages() uniformly, perhaps with a per-arch specified memory type
>> >    (by means of which you can control whether the memory contents will 
>> > remain
>> >    preserved until the time you want to look at it). That will eliminate 
>> > the
>> >    only place_string() you're concerned about, with a patch with better
>> >    diffstat (largely due to the questionable arch hook gone).
>> >
>> > However, this solution does not solve conflicts problem described in #1
>> > because EFI memory map is needed during Xen runtime after init phase.
>> > So, finally we would get back to #1. Hmmm... Should I check how Linux
>> > and others cope with that problem?
>>
>> Ah, here you mention it actually. Yet you don't explain what conflict
>> potential you see once using EfiRuntimeServicesData for the allocation.
> 
> Good point! IMO, if crash kernel region conflicts with EfiRuntimeServices*
> then we should display warning that it cannot be allocated. By the way,
> once you mentioned that you have in your queue (I suppose that it is
> extremely long) kdump patch which adds functionality to automatically
> establish crash kernel region placement. I think that could solve (at
> least partially) problem with conflicts. Could you post it?

For one, unless asked to be at a specific location, we already dynamically
place that area. The patch that I believe you think of just enhances the
placement to have something between "fully dynamic" and "at a fixed
place". I've never posted it (or even ported it to -unstable) because I've
never got positive feedback on it by those who it was originally created
for. If you think it could be useful, I can certainly revive it.

Jan


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