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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.08.16 at 00:43, <daniel.kiper@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -103,9 +103,56 @@ static void __init relocate_trampoline(unsigned long 
> phys)
>          *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
>  }
>  
> +#define EBMALLOC_SIZE        MB(1)
> +
> +static char __section(".bss.page_aligned") ebmalloc_mem[EBMALLOC_SIZE];

You need to specify the alignment of the object (using the relatively
new __aligned() construct).

> +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.

> +    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?

> +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

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.

Jan


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