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

From: Daniel Kiper <daniel.kiper@oracle.com>
To: Jan Beulich <JBeulich@suse.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: Thu, 8 Sep 2016 10:29:11 +0200	[thread overview]
Message-ID: <20160908082911.GS20645@olila.local.net-space.pl> (raw)
In-Reply-To: <57D039DB020000780010CA64@prv-mh.provo.novell.com>

On Wed, Sep 07, 2016 at 08:01:31AM -0600, Jan Beulich wrote:
> >>> On 07.09.16 at 14:05, <daniel.kiper@oracle.com> wrote:
> > On Mon, Sep 05, 2016 at 06:33:57AM -0600, Jan Beulich wrote:
> >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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.
>
> Which all is in line with my request of improving the comment.

OK.

> >> > +    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?
>
> To avoid the penalty unaligned accesses incur? And that's alongside
> the fact that it's simply bad practice to knowingly but without actual
> need cause unaligned accesses even if they work fine.

I expected that but I do not think it is very important here. Anyway,
I am still not sure why you say "at least pointer size". Because
sizeof(void *) assures proper alignment on any architecture?
Additionally, will this alignment sufficiently replace alignment
provided by current efi_arch_allocate_mmap_buffer() implementation?

> >> 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.
>
> Being architecture independent it has all reasons to be moved
> there. Agreed there may be compiler warnings for these then
> being unused static functions, but I'd rather see this code get
> #ifdef-ed out for ARM for the time being than it needing to be

OK.

> moved over later on. And of course a question to be asked first
> is whether in fact there is something in common or ARM specific
> code that could benefit from using this new allocator, if you
> already introduce it.

I think that it is x86 specific stuff and should stay here as is.
However, potentially it can be common allocator for both architectures.
Though I do not see gains on ARM itself.

Daniel

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