Re: [Xen-devel] [PATCH v2 19/23] x86/efi: create new early memory allocator

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Jul 20, 2015 at 04:29:14PM +0200, Daniel Kiper 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

s/So, I tried to use/With/
> mem_lower address calculated by GRUB2. However, it works only on some

s/GRUB2. However, //

> 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

s/only/to only/
> 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.

s/I think ...two solutions/There are 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.
> 
> 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

Aren't we gziping the binary? In which you do not waste that much disk space.

>    at least memory map on x86 EFI platforms. As I saw on small machine,

s/at least memory map on/on small memory/

s/As I saw on small machine/For example on/

>    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.

s/entries. Every entry on .. is/wherein every entry is/

>    So, it means that we need more than 8 KiB for EFI memory map only.

s/So, it means that we need more than/Brings it up to/

>    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,

s/,I think, //

>    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.

s/If we think that we should not/To not/

>    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().
> 
> Now solution #2 is implemented but maybe we should consider #1 one day.

s/Now ../This patch implements #2 solution./

> 
> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> with the
adjustments mentioned.
> ---
>  xen/arch/x86/efi/efi-boot.h |   38 ++++++++++++++++++++++++++++++--------
>  xen/arch/x86/setup.c        |    3 +--
>  2 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 2dd69f6..3d25c48 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -103,9 +103,36 @@ static void __init relocate_trampoline(unsigned long 
> phys)
>          *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
>  }
>  
> +#define EBMALLOC_SIZE        MB(1)
> +
> +static char __initdata ebmalloc_mem[EBMALLOC_SIZE];
> +static char __initdata *ebmalloc_free = NULL;
> +
> +/* EFI boot allocator. */
> +static void __init *ebmalloc(size_t size)
> +{
> +    void *ptr;
> +
> +    /*
> +     * Init ebmalloc_free on runtime. Static initialization
> +     * will not work because it puts virtual address there.

And why is that bad?

> +     */
> +    if ( ebmalloc_free == NULL )
> +        ebmalloc_free = ebmalloc_mem;
> +
> +    ptr = ebmalloc_free;
> +
> +    ebmalloc_free += size;
> +
> +    if ( ebmalloc_free - ebmalloc_mem > sizeof(ebmalloc_mem) )
> +        blexit(L"Out of static memory\r\n");
> +
> +    return ptr;
> +}
> +
>  static void __init place_string(u32 *addr, const char *s)
>  {
> -    static char *__initdata alloc = start;
> +    char *alloc = NULL;
>  
>      if ( s && *s )
>      {
> @@ -113,7 +140,7 @@ static void __init place_string(u32 *addr, const char *s)
>          const char *old = (char *)(long)*addr;
>          size_t len2 = *addr ? strlen(old) + 1 : 0;
>  
> -        alloc -= len1 + len2;
> +        alloc = ebmalloc(len1 + len2);
>          /*
>           * Insert new string before already existing one. This is needed
>           * for options passed on the command line to override options from
> @@ -196,12 +223,7 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>  
>  static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
>  {
> -    place_string(&mbi.mem_upper, NULL);
> -    mbi.mem_upper -= map_size;
> -    mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
> -    if ( mbi.mem_upper < xen_phys_start )
> -        return NULL;
> -    return (void *)(long)mbi.mem_upper;
> +    return ebmalloc(map_size);
>  }
>  
>  static void __init efi_arch_pre_exit_boot(void)
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index bce708c..a59fc4e 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -994,8 +994,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      if ( !xen_phys_start )
>          panic("Not enough memory to relocate Xen.");
> -    reserve_e820_ram(&boot_e820, efi_enabled(EFI_PLATFORM) ? mbi->mem_upper 
> : __pa(&_start),
> -                     __pa(&_end));
> +    reserve_e820_ram(&boot_e820, __pa(&_start), __pa(&_end));
>  
>      /* Late kexec reservation (dynamic start address). */
>      kexec_reserve_area(&boot_e820);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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