Re: [PATCH 17/18] x86/efi: create new early memory allocator

[Roy Franz <roy.franz@linaro.org>]

On Mon, Mar 2, 2015 at 9:23 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 30.01.15 at 18:54, <daniel.kiper@oracle.com> wrote:
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -103,9 +103,35 @@ static void __init relocate_trampoline(unsigned long phys)
>>          *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
>>  }
>>
>> +#define __MALLOC_SIZE        65536
>> +
>> +static char __malloc_mem[__MALLOC_SIZE];
>> +static char *__malloc_free = NULL;
>> +
>> +static void __init *__malloc(size_t size)
>
> Please do away with all these prefixing underscores.
>
>> +{
>> +    void *ptr;
>> +
>> +    /*
>> +     * Init __malloc_free on runtime. Static initialization
>> +     * will not work because it puts virtual address there.
>> +     */
>> +    if ( __malloc_free == NULL )
>> +        __malloc_free = __malloc_mem;
>> +
>> +    ptr = __malloc_free;
>> +
>> +    __malloc_free += size;
>> +
>> +    if ( __malloc_free - __malloc_mem > sizeof(__malloc_mem) )
>> +        blexit(L"Out of static memory\r\n");
>> +
>> +    return ptr;
>> +}
>
> You're ignoring alignment requirements here altogether.
>
>> @@ -192,12 +218,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 __malloc(*map_size);
>>  }
>
> Which then even suggests that _if_ we go this route, this could be
> shared with ARM (and hence become common code again).
>
> Jan
>

We could do the same thing on ARM.  For ARM, 2 allocations are done: 1
for the FDT, and
this one for the EFI memory map.  Both of these are currently
allocated with EFI allocation
functions, so don't have fixed size limits.  If we go with the fixed
size pool, I don't think that 64k
will be enough for the ARM case, as FDTs can be 20-50k in size.

Roy