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

[Jan Beulich]

>>> On 27.03.15 at 13:57, <address@hidden> wrote:
> On Mon, Mar 02, 2015 at 05:23:49PM +0000, Jan Beulich wrote:
>> >>> On 30.01.15 at 18:54, <address@hidden> wrote:
>> > +{
>> > +    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.
> 
> I can understand why __malloc_mem should be let's say page aligned
> but I am not sure why we should care here about alignment inside
> of __malloc_mem array like...
> 
>> > @@ -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);
> 
> ...here...

Specifically with the later mentioned potential for sharing this with
ARM I think you have to make sure that things are suitably aligned,
or else you cause aborts.

>> > -    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).
> 
> So, go or no go this route?

As long as it's being done properly, I'm not wildly opposed.

Jan