Re: [Xen-devel] [PATCH v6 08/15] x86/efi: create new early memory allocator

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Tue, Sep 20, 2016 at 03:57:19AM -0600, Jan Beulich wrote:
> >>> On 20.09.16 at 11:45, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Mon, Sep 19, 2016 at 09:17:50AM -0600, Jan Beulich wrote:
> >> >>> On 19.09.16 at 17:04, <daniel.kiper@xxxxxxxxxx> wrote:
> >> > On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
> >> >> >>> On 12.09.16 at 22:18, <daniel.kiper@xxxxxxxxxx> wrote:
> >> >> > --- a/xen/arch/x86/setup.c
> >> >> > +++ b/xen/arch/x86/setup.c
> >> >> > @@ -520,6 +520,8 @@ static void noinline init_done(void)
> >> >> >
> >> >> >      system_state = SYS_STATE_active;
> >> >> >
> >> >> > +    free_ebmalloc_unused_mem();
> >> >>
> >> >> Now that the allocator properly lives in common code, this appears
> >> >> to lack an ARM side counterpart.
> >> >
> >> > Why? It is called only from xen/arch/x86/setup.c:__start_xen() and all
> >> > ebmalloc stuff is in #ifndef CONFIG_ARM. So, free_ebmalloc_unused_mem()
> >> > will be needed only if we add ARM support here.
> >>
> >> Well, it being inside that conditional is part of the problem - there's
> >> no apparent point for all of it to be.
> >
> > I can agree that this is potentially generic stuff (well, I understand that
> > it is our final goal but unreachable yet due to various things). However,
> > right know it is only used on x86. So, I am not sure what is the problem
> > with #ifndef CONFIG_ARM right now...
>
> It is a fact that these should actually not be there, so we ought to
> at least limit them to the smallest possible count and scopes.
>
> >> Arguably the one static function may better be, as other workarounds
> >> to avoid the "unused" compiler warning wouldn't be any better.
> >
> > Do you mean static function with empty body for ARM? It is not needed
> > right now because it is never called on ARM. Am I missing something?
>
> You misunderstood - I said that for this one (unused) static
> function such an #ifdef is probably okay, as the presumably
> smallest possible workaround.

Do you suggest that I should move out of #ifndef CONFIG_ARM all ebmalloc stuff
except free_ebmalloc_unused_mem(). Even if it is not used on ARM right now?

> >> >> > +static unsigned long __initdata ebmalloc_allocated;
> >> >> > +
> >> >> > +/* EFI boot allocator. */
> >> >> > +static void __init *ebmalloc(size_t size)
> >> >> > +{
> >> >> > +    void *ptr = ebmalloc_mem + ebmalloc_allocated;
> >> >> > +
> >> >> > +    ebmalloc_allocated += (size + sizeof(void *) - 1) & 
> >> >> > ~((typeof(size))sizeof(void *) - 1);
> >> >>
> >> >> What's the point of this ugly cast?
> >> >
> >> > In general ALIGN_UP() would be nice here. However, there is no such thing
> >> > in Xen headers (or I cannot find it). Should I add one? As separate 
> >> > patch?
> >>
> >> I understand what you want the expression for, but you didn't
> >> answer my question. Or do you not realize that all this cast is
> >> about is a strange way of converting an expression of type
> >> size_t to type size_t?
> >
> > Does sizeof() returns size_t type? I was thinking that it returns
> > a number calculated during compilation, however, it does not have
> > specific type.
>
> Every expression needs to have a well specified type. Even
> plain numbers do.

Hmmm... So, what is a type e.g. 5 without "U" and/or "L"? int?

Daniel

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