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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Jul 06, 2016 at 01:22:24AM -0600, Jan Beulich wrote:
> >>> On 05.07.16 at 20:26, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Fri, Apr 15, 2016 at 02:33:12PM +0200, Daniel Kiper wrote:
> >> 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.
> >
> > I have checked Linux kernel code. It allocates buffer for memory map using
> > EFI API and later reserve it in e820 memory map. Simple. This should work
> > for us too but...
> >
> >>    In later case we must also care about conflicts with e.g. crash
> >>    kernel regions which could be quite difficult.
> >
> > This is not a problem since Xen can choose dynamically placement of kdump
> > region during boot phase and there is no requirement to specify it in boot
> > command line. This means that it will avoid all allocated/reserved regions
> > including EFI memory map. However, there is one potential problem which
> > cannot be avoided simply with current EFI spec. I think about conflicts
> > with trampoline. It must live below 1 MiB. However, there is not something
> > similar to "AllocateMaxAddress" option for AllocatePages() which would
> > ask EFI to allocate memory above a given address (Hmmm... Why UEFI designers
> > did not added such option, e.g. "AllocateMinAddress"? For me it is obvious
> > thing if we have "AllocateMaxAddress").
>
> Not obvious to me at all. Allowing an upper bound is natural (for
> both DMA purposes and arbitrary other addressing restrictions).
> Allowing a lower bound to be specified isn't.

I think that I have shown above that on some platforms this could be useful 
option.

> > So, it means that we cannot simply
> > say "give me a memory chunk above 1 MiB". AIUI, Linux guys do not care,
> > hope that all EFI platforms are smart and AllocatePages() tries hard to
> > avoid everything below 1 MiB. We can go this way too. However, I am almost
> > sure that sooner or later we will find crazy platforms which allocate memory
> > from 0-1 MiB region. We can avoid this by getting EFI memory map, looking 
> > for
> > free regions above 1 MiB and then trying to allocate memory chunk using
> > AllocatePages() with "AllocateAddress". Does it make sense?
>
> I don't see the point of all that, as I don't see why any EFI
> implementation would want to deviate from the first line principle
> of satisfying allocation requests as high as possible.

In general this is good idea. However, I have not seen such requirement in
UEFI spec. So, I suppose that bad things may happen on some EFI implementations
and that is why I proposed a bit "smarter" approach. On the other hand if Linux
does allocations in "simple" way (just AllocatePages() call) then it means that
this solution works on most platforms if not all. So, probably "simple" solution
would work for us too.

Anyway, I think that it is worth considering all potential issues if we are
aware of them in advance.

> Apart from that using (only) EFI allocation mechanisms for
> obtaining the trampoline area won't work anyway, as we already
> know there are systems where all of the memory below 1Mb is
> in use by EFI (mostly with boot kind allocations, i.e. becoming
> available after ExitBootServices()).

I know about that. However, I am talking here about memory allocation
for EFI memory map. As I said above this region may potentially (well,
it looks that probability is low but as I said earlier we should think
and discuss this issue here) conflict with trampoline region. Though
I am not saying how this region (for trampoline) should be allocated
because current solution works well.

> >> 2) We may allocate memory area statically somewhere in Xen code which
> >>    could be used as memory pool for early dynamic allocations. Looks
> >>    quite simple. Additionally, it would not depend on EFI at all and
> >>    could be used on legacy BIOS platforms if we need it. However, we
> >>    must carefully choose size of this pool. We do not want increase
> >>    Xen binary size too much and waste too much memory but also we must fit
> >>    at least memory map on x86 EFI platforms. As I saw on small machine,
> >>    e.g. IBM System x3550 M2 with 8 GiB RAM, memory map may contain more
> >>    than 200 entries. Every entry on x86-64 platform is 40 bytes in size.
> >>    So, it means that we need more than 8 KiB for EFI memory map only.
> >>    Additionally, if we want to use this memory pool for Xen and modules
> >>    command line storage (it would be used when xen.efi is executed as EFI
> >>    application) then we should add, I think, about 1 KiB. In this case,
> >>    to be on safe side, we should assume at least 64 KiB pool for early
> >>    memory allocations, which is about 4 times of our earlier calculations.
> >>    However, during discussion on Xen-devel Jan Beulich suggested that
> >>    just in case we should use 1 MiB memory pool like it was in original
> >>    place_string() implementation. So, let's use 1 MiB as it was proposed.
> >>    If we think that we should not waste unallocated memory in the pool
> >>    on running system then we can mark this region as __initdata and move
> >>    all required data to dynamically allocated places somewhere in 
> >> __start_xen().
> >
> > 2a) We can create something like .init.bss and put this thing at the end of
> >     regular .bss section. Then allocate memory chunks starting from lowest
> >     address. After init phase we can free unused memory as in case of 
> > .init.text
> >     or .init.data sections. This way we do not need allocate any space in
> >     image file and freeing of unused memory should be simple. What do you
> >     think about that one?
>
> With (again) the caveat of how to size such a region.

Yep, you are right.

> Bottom line - I continue to be unconvinced that we need something
> "new" here at all.

Why? I think that I have shown all currently existing issues related
to place_string() usage. Am I missing something? And please remember
that we are talking here about allocating memory for EFI memory map
not for trampoline.

Daniel

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