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

From: Daniel Kiper <daniel.kiper@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, andrew.cooper3@citrix.com,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
	qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v6 08/15] x86/efi: create new early memory allocator
Date: Tue, 20 Sep 2016 11:45:09 +0200	[thread overview]
Message-ID: <20160920094509.GA3500@olila.local.net-space.pl> (raw)
In-Reply-To: <57E01DBE02000078001102EE@prv-mh.provo.novell.com>

On Mon, Sep 19, 2016 at 09:17:50AM -0600, Jan Beulich wrote:
> >>> On 19.09.16 at 17:04, <daniel.kiper@oracle.com> wrote:
> > On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
> >> >>> On 12.09.16 at 22:18, <daniel.kiper@oracle.com> 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...

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

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

Anyway, I think that probably ALIGN_UP()/ALING_DOWN() would be useful
in many places. This way we can avoid such long constructs (it will
be long even if we remove cast).

Daniel

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