[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 15:28:44 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Mar 27, 2015 at 01:35:11PM +0000, Jan Beulich wrote:
> >>> 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.
So, AIUI, general idea is OK but fix all stuff which should be fixed. Right?
Daniel