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

From: "Jan Beulich" <JBeulich@suse.com>
To: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, andrew.cooper3@citrix.com,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
	qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v5 12/16] x86/efi: create new early memory allocator
Date: Mon, 05 Sep 2016 06:33:57 -0600	[thread overview]
Message-ID: <57CD8255020000780010BC22@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1471646606-28519-13-git-send-email-daniel.kiper@oracle.com>

>>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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@lists.xen.org
https://lists.xen.org/xen-devel