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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, Mar 27, 2015 at 01:35:11PM +0000, Jan Beulich wrote:
> >>> On 27.03.15 at 13:57, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Mon, Mar 02, 2015 at 05:23:49PM +0000, Jan Beulich wrote:
> >> >>> On 30.01.15 at 18:54, <daniel.kiper@xxxxxxxxxx> 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

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