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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Sep 07, 2016 at 08:01:31AM -0600, Jan Beulich wrote:
> >>> On 07.09.16 at 14:05, <daniel.kiper@xxxxxxxxxx> wrote:
> > 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.
>
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel