Re: [Xen-devel] [PATCH v5 12/16] x86/efi: create new early memory allocator

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Sep 05, 2016 at 06:33:57AM -0600, Jan Beulich wrote:
> >>> On 20.08.16 at 00:43, <daniel.kiper@xxxxxxxxxx> wrote:

[...]

> > +static char __initdata *ebmalloc_free = NULL;
> > +
> > +/* EFI boot allocator. */
> > +static void __init *ebmalloc(size_t size)
> > +{
> > +    void *ptr;
> > +
> > +    /*
> > +     * Init ebmalloc_free on runtime. Static initialization
> > +     * will not work because it puts virtual address there.
> > +     */
>
> I don't understand this static allocation comment: We have this issue
> elsewhere (and use bootsym() as needed), and we do not have this
> issue at all in xen.efi (which this code also gets built for). So I think at
> the very least the comment needs improvement. And then, if static
> initialization indeed can't be used, then a static symbol's initializer of
> NULL is pointless and hence should be omitted.

You have to remember that xen/arch/x86/efi/efi-boot.h stuff is build
into xen.efi and xen.gz. Of course xen.efi with

static char __initdata *ebmalloc_free = ebmalloc_mem;

works, however, xen.gz does not. Sadly, I have not found simpler
solution for that issue, so, I do initialization during runtime.

> > +    if ( ebmalloc_free == NULL )
> > +        ebmalloc_free = ebmalloc_mem;
> > +
> > +    ptr = ebmalloc_free;
> > +
> > +    ebmalloc_free += size;
>
> No minimal (at least pointer size) alignment getting enforced
> somewhere here?

For what?

> > +void __init free_ebmalloc_unused_mem(void)
> > +{
> > +    unsigned long start, end;
> > +
> > +    if ( ebmalloc_free )
> > +    {
> > +        start = (unsigned long)ebmalloc_free - xen_phys_start;
> > +        start = PAGE_ALIGN(start + XEN_VIRT_START);
> > +    }
> > +    else
> > +        start = (unsigned long)ebmalloc_mem;
> > +
> > +    end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
> > +
> > +    destroy_xen_mappings(start, end);
> > +    init_xenheap_pages(__pa(start), __pa(end));
> > +
> > +    printk("Freed %lukB unused BSS memory\n", (end - start) >> 10);
>
> XENLOG_INFO

OK.

> And then - wouldn't this better go into xen/common/efi/boot.c,
> even if ARM64 does not have a use for it right away? The code
> certainly isn't really x86-specific.

Sure thing. However, if it is not used by ARM64 then I think ebmalloc
stuff should not be moved to xen/common/efi/boot.c.

Daniel

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