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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 12/10/16 13:59, Jan Beulich wrote:
> > > > On 12.10.16 at 14:51, <julien.grall@xxxxxxx> wrote:
> > Hello Jan,
> >
> > On 12/10/2016 12:45, Jan Beulich wrote:
> > > > > > On 11.10.16 at 15:39, <julien.grall@xxxxxxx> wrote:
> > > > On 06/10/16 13:21, Jan Beulich wrote:
> > > > > > > > On 05.10.16 at 20:30, <julien.grall@xxxxxxx> wrote:
> > > > > > On 30/09/2016 02:46, Jan Beulich wrote:
> > > > > > > > > > On 29.09.16 at 23:42, <daniel.kiper@xxxxxxxxxx> wrote:
> > > > > > > > +#else
> > > > > > > > +static void __init free_ebmalloc_unused_mem(void)
> > > > > > > > +{
> > > > > > > > +}
> > > > > > > > +#endif
> > > > > > > Did you build test this for ARM? The function ought to be unused,
> > > > > > > as ...
> > > > > > >
> > > > > > > > @@ -1251,6 +1301,8 @@ void __init efi_init_memory(void)
> > > > > > > >      } *extra, *extra_head = NULL;
> > > > > > > >  #endif
> > > > > > > >
> > > > > > > > +    free_ebmalloc_unused_mem();
> > > > > > > ... the whole function here doesn't get built on ARM.
> > > > > > >
> > > > > > > Julien - we're still awaiting your input on general aspects here.
> > > > > > efi_init_memory would need to be called during Xen boot on ARM. I am not
> > > > > > sure where as I we don't yet have runtime support on ARM.
> > > > > >
> > > > > > Other than that, the patch looks good to me.
> > > > > But that wasn't the question. My goal is to have as little code
> > > > > inside #ifndef CONFIG_ARM as possible, and hence I'd like to have
> > > > > as much of this new code as possible outside of such conditionals.
> > > > > So the question really is whether that alternative approach would
> > > > > be fine with you, or what problems you might see.
> > > > I am not sure to get it. The current approach looks good to me, however,
> > > > the implementation should not be exposed to ARM until all the TODOs
> > > > mentioned by Daniel are fixed.
> > > Which is precisely the opposite of what I'm aiming at. Once again:
> > > Don't you think it is desirable to keep the #ifndef CONFIG_ARM
> > > instances to cover as little code as possible? Not all of the named
> > > TODOs really need to be addressed in order to compile most of
> > > what comprises this new allocator; in fact none of them really
> > > needs addressing:
> > > - if the size estimation turns out to low once ARM starts actually
> > >   using this, let's just bump it (perhaps by making it a per-arch
> > >   constant),
> > > - if the section chosen needs to be different (which it really
> > >   shouldn't be), let's simply adjust it,
> > If we keep the section in BSS, then we really need to move the
> > initialization of BSS earlier.
> Right, but that should be simple enough. Or we do ...
>
> > This TODO really needs to be fixed now otherwise it will be a nightmare
> > to debug later on.
> >
> > > - as we've already figured there's no need for the stub
> > >   free_ebmalloc_unused_mem() right now anyway.
> > But we would need to call free_ebmalloc_unused_mem from somewhere. The
> > idea to not expose the early memory allocator on ARM is avoid to have an
> > implementation with may not fully work on ARM because of known missing
> > pieces.
> >
> > > And then (as another alternative) we have the option of ARM
> > > simply defining EBMALLOC_SIZE to zero for the time being. That
> > > would eliminate the need to actually call free_ebmalloc_unused_mem()
> > > and turn the other two items into non-issues.
> ... this, which you didn't comment on at all.
I skipped this part by mistake. That would work to, assuming there is a 
proper comment on top of EBMALLOC_SIZE explaining what needs to be done 
in order to fully support the early allocator on ARM.
Both solutions are fine by me.

Cheers,

--
Julien Grall

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