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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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