Re: [Xen-devel] [PATCH v8 06/13] efi: create new early memory allocator

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Sep 29, 2016 at 01:40:44AM -0600, Jan Beulich wrote:
> >>> On 29.09.16 at 00:51, <daniel.kiper@xxxxxxxxxx> wrote:
> > v8 - suggestions/fixes:
> >    - disable whole ebmalloc machinery on ARM platforms,
>
> This is certainly not in line with my understanding of the outcome of
> that discussion.

Well, I understand that you would like to reduce number of changes
needed to enable ebmalloc ARM in the future. However, on the other
hand Julien wish to not have any calls to ebmalloc on ARM now. So,
if the ebmalloc code and data is not referenced anywhere on ARM then
I do not think that it make sens to build it into ARM xen image.
Especially if I am not sure that 1 MiB ebmalloc region is correct
or not on ARM. So, I would like to wait for Julien final opinion
about that. If he likes your proposal (maybe with some adjustments)
then I am not going to object.

> > --- a/xen/arch/x86/efi/stub.c
> > +++ b/xen/arch/x86/efi/stub.c
> > @@ -9,6 +9,8 @@ bool efi_enabled(unsigned int feature)
> >      return false;
> >  }
> >
> > +void __init free_ebmalloc_unused_mem(void) { }
> > +
> >  void __init efi_init_memory(void) { }
>
> Looking at this I wonder - can't the freeing be put into
> efi_init_memory()?

I am not sure that all mem infrastructure needed to free ebmalloc unused
region exists then. Anyway, I will try it. However, I think that init_done()
is more natural place to do that because later we free .init* sections there.

> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -98,6 +98,56 @@ static CHAR16 __initdata newline[] = L"\r\n";
> >  #define PrintStr(s) StdOut->OutputString(StdOut, s)
> >  #define PrintErr(s) StdErr->OutputString(StdErr, s)
> >
> > +#ifndef CONFIG_ARM
> > +
> > +/*
> > + * TODO: Enable EFI boot allocator on ARM.
> > + * This code can be common for x86 and ARM.
> > + * Things TODO on ARM before enabling ebmalloc:
> > + *   - estimate required EBMALLOC_SIZE value,
> > + *   - where (in which section) ebmalloc_mem[]
> > + *     should live; if in .bss.page_aligned
> > + *     then whole BSS zeroing have to be
> > + *     disabled in xen/arch/arm/arm64/head.S;
> > + *     though BSS should be initialized somehow
> > + *     before use of variables living there,
> > + *   - add free_ebmalloc_unused_mem() call to
> > + *     e.g. xen/arch/arm/setup.c:init_done().
> > + */
>
> Please make better use of line length in such longer comments -
> the longest line here ends at column 48 or so.

OK.

Daniel

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