[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 17/18] x86/efi: create new early memory allocator
From: |
Daniel Kiper |
Subject: |
Re: [PATCH 17/18] x86/efi: create new early memory allocator |
Date: |
Fri, 27 Mar 2015 13:57:01 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Mar 02, 2015 at 05:23:49PM +0000, Jan Beulich wrote:
> >>> On 30.01.15 at 18:54, <address@hidden> 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.
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...
> > - 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? If not what do you suggest?
Daniel