| Message ID | 1473711511-11931-9-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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. > @@ -1158,7 +1160,38 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > for( ; ; ); /* not reached */ > } > > -#ifndef CONFIG_ARM /* TODO - runtime service support */ > +#ifndef CONFIG_ARM > + > +#define EBMALLOC_SIZE MB(1) > + > +static char __section(".bss.page_aligned") __aligned(PAGE_SIZE) ebmalloc_mem[EBMALLOC_SIZE]; Long line. > +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? Jan
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. [...] > > +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? Daniel
>>> 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. Arguably the one static function may better be, as other workarounds to avoid the "unused" compiler warning wouldn't be any better. >> > +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? Jan
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
>>> On 20.09.16 at 11:45, <daniel.kiper@oracle.com> wrote: > 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... 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. >> >> > +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. Jan
On Tue, Sep 20, 2016 at 03:57:19AM -0600, Jan Beulich wrote: > >>> On 20.09.16 at 11:45, <daniel.kiper@oracle.com> wrote: > > 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... > > 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
>>> On 20.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: > On Tue, Sep 20, 2016 at 03:57:19AM -0600, Jan Beulich wrote: >> >>> On 20.09.16 at 11:45, <daniel.kiper@oracle.com> wrote: >> > 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... >> >> 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? That's not the static function, is it? The function you name should actually be called on ARM too (as I did point out elsewhere already), just to not leave a trap open for someone to fall into when (s)he adds a first use of the allocator on ARM. >> >> >> > +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? Of course. But please may I ask you to read the spec instead? Jan
On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote: > >>> On 20.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: > > On Tue, Sep 20, 2016 at 03:57:19AM -0600, Jan Beulich wrote: > >> >>> On 20.09.16 at 11:45, <daniel.kiper@oracle.com> wrote: > >> > 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... > >> > >> 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? > > That's not the static function, is it? The function you name should > actually be called on ARM too (as I did point out elsewhere already), > just to not leave a trap open for someone to fall into when (s)he > adds a first use of the allocator on ARM. Well, I think that sane person doing that would analyze how ebmalloc works on x86 and then move (align to ARM needs if required) all its machinery (including free_ebmalloc_unused_mem()) to run on ARM. At least I would do that. This way he/she would avoid issues mentioned by you. However, if you still prefer your way I can do that. Though I am not sure about the rest of ebmalloc stuff. Should I move it out of #ifndef CONFIG_ARM? Looking at your earlier replies I see that yes. Am I correct? > >> >> >> > +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? > > Of course. But please may I ask you to read the spec instead? Thanks! Sure thing! Daniel
>>> On 20.09.16 at 20:45, <daniel.kiper@oracle.com> wrote: > On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote: >> >>> On 20.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: >> > On Tue, Sep 20, 2016 at 03:57:19AM -0600, Jan Beulich wrote: >> >> >>> On 20.09.16 at 11:45, <daniel.kiper@oracle.com> wrote: >> >> > 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... >> >> >> >> 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? >> >> That's not the static function, is it? The function you name should >> actually be called on ARM too (as I did point out elsewhere already), >> just to not leave a trap open for someone to fall into when (s)he >> adds a first use of the allocator on ARM. > > Well, I think that sane person doing that would analyze how ebmalloc works > on x86 and then move (align to ARM needs if required) all its machinery > (including free_ebmalloc_unused_mem()) to run on ARM. At least I would do > that. This way he/she would avoid issues mentioned by you. However, if you > still prefer your way I can do that. Though I am not sure about the rest of > ebmalloc stuff. Should I move it out of #ifndef CONFIG_ARM? Looking at your > earlier replies I see that yes. Am I correct? Yes. Jan
On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote: > >>> On 20.09.16 at 20:45, <daniel.kiper@oracle.com> wrote: > > On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote: > >> >>> On 20.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: [...] > >> > 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? > >> > >> That's not the static function, is it? The function you name should > >> actually be called on ARM too (as I did point out elsewhere already), > >> just to not leave a trap open for someone to fall into when (s)he > >> adds a first use of the allocator on ARM. > > > > Well, I think that sane person doing that would analyze how ebmalloc works > > on x86 and then move (align to ARM needs if required) all its machinery > > (including free_ebmalloc_unused_mem()) to run on ARM. At least I would do > > that. This way he/she would avoid issues mentioned by you. However, if you > > still prefer your way I can do that. Though I am not sure about the rest of > > ebmalloc stuff. Should I move it out of #ifndef CONFIG_ARM? Looking at your > > earlier replies I see that yes. Am I correct? > > Yes. This does not work because if I build Xen for ARM then I got: boot.c:589:21: error: 'ebmalloc' defined but not used [-Werror=unused-function] static void __init *ebmalloc(size_t size) I think about following solutions: - leave all ebmalloc stuff in #ifndef CONFIG_ARM as it is right now, - if we wish to have ebmalloc stuff in common code but does not use it yet then we must add __used attribute to ebmalloc() or drop the static, - use ebmalloc on ARM and x86; the best option but I do not think that we should do this right now before freeze. What is your choice? Or do you have better ideas? Daniel
>>> On 22.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: > On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote: >> >>> On 20.09.16 at 20:45, <daniel.kiper@oracle.com> wrote: >> > On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote: >> >> >>> On 20.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: > > [...] > >> >> > 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? >> >> >> >> That's not the static function, is it? The function you name should >> >> actually be called on ARM too (as I did point out elsewhere already), >> >> just to not leave a trap open for someone to fall into when (s)he >> >> adds a first use of the allocator on ARM. >> > >> > Well, I think that sane person doing that would analyze how ebmalloc works >> > on x86 and then move (align to ARM needs if required) all its machinery >> > (including free_ebmalloc_unused_mem()) to run on ARM. At least I would do >> > that. This way he/she would avoid issues mentioned by you. However, if you >> > still prefer your way I can do that. Though I am not sure about the rest of >> > ebmalloc stuff. Should I move it out of #ifndef CONFIG_ARM? Looking at your >> > earlier replies I see that yes. Am I correct? >> >> Yes. > > This does not work because if I build Xen for ARM then I got: > boot.c:589:21: error: 'ebmalloc' defined but not used > [-Werror=unused-function] > static void __init *ebmalloc(size_t size) Quote from an earlier reply of mine: Arguably the one static function may better be, as other workarounds to avoid the "unused" compiler warning wouldn't be any better. and then later You misunderstood - I said that for this one (unused) static function such an #ifdef is probably okay, as the presumably smallest possible workaround. I really have no idea what else to say. Jan
On Thu, Sep 22, 2016 at 05:25:46AM -0600, Jan Beulich wrote: > >>> On 22.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: > > On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote: > >> >>> On 20.09.16 at 20:45, <daniel.kiper@oracle.com> wrote: > >> > On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote: > >> >> >>> On 20.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: > > > > [...] > > > >> >> > 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? > >> >> > >> >> That's not the static function, is it? The function you name should > >> >> actually be called on ARM too (as I did point out elsewhere already), > >> >> just to not leave a trap open for someone to fall into when (s)he > >> >> adds a first use of the allocator on ARM. > >> > > >> > Well, I think that sane person doing that would analyze how ebmalloc works > >> > on x86 and then move (align to ARM needs if required) all its machinery > >> > (including free_ebmalloc_unused_mem()) to run on ARM. At least I would do > >> > that. This way he/she would avoid issues mentioned by you. However, if you > >> > still prefer your way I can do that. Though I am not sure about the rest of > >> > ebmalloc stuff. Should I move it out of #ifndef CONFIG_ARM? Looking at your > >> > earlier replies I see that yes. Am I correct? > >> > >> Yes. > > > > This does not work because if I build Xen for ARM then I got: > > boot.c:589:21: error: 'ebmalloc' defined but not used > > [-Werror=unused-function] > > static void __init *ebmalloc(size_t size) > > Quote from an earlier reply of mine: > > Arguably the one static function may better be, as other > workarounds to avoid the "unused" compiler warning wouldn't > be any better. > > and then later > > You misunderstood - I said that for this one (unused) static > function such an #ifdef is probably okay, as the presumably > smallest possible workaround. > > I really have no idea what else to say. Sorry, however, sometimes it is very difficult to understand what are you referring to. If you could be more specific then I will be happy. Anyway, now it looks that you wish something like that: #ifndef CONFIG_ARM /* Whole x86 ebmalloc stuff. */ ... #else void __init free_ebmalloc_unused_mem(void) { } #endif and then call free_ebmalloc_unused_mem() from e.g. xen/arch/arm/setup.c:init_done(). Am I right? Daniel
>>> On 22.09.16 at 14:07, <daniel.kiper@oracle.com> wrote: > On Thu, Sep 22, 2016 at 05:25:46AM -0600, Jan Beulich wrote: >> >>> On 22.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: >> > On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote: >> >> >>> On 20.09.16 at 20:45, <daniel.kiper@oracle.com> wrote: >> >> > On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote: >> >> >> >>> On 20.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: >> > >> > [...] >> > >> >> >> > 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? >> >> >> >> >> >> That's not the static function, is it? The function you name should >> >> >> actually be called on ARM too (as I did point out elsewhere already), >> >> >> just to not leave a trap open for someone to fall into when (s)he >> >> >> adds a first use of the allocator on ARM. >> >> > >> >> > Well, I think that sane person doing that would analyze how ebmalloc works >> >> > on x86 and then move (align to ARM needs if required) all its machinery >> >> > (including free_ebmalloc_unused_mem()) to run on ARM. At least I would do >> >> > that. This way he/she would avoid issues mentioned by you. However, if you >> >> > still prefer your way I can do that. Though I am not sure about the rest > of >> >> > ebmalloc stuff. Should I move it out of #ifndef CONFIG_ARM? Looking at > your >> >> > earlier replies I see that yes. Am I correct? >> >> >> >> Yes. >> > >> > This does not work because if I build Xen for ARM then I got: >> > boot.c:589:21: error: 'ebmalloc' defined but not used >> > [-Werror=unused-function] >> > static void __init *ebmalloc(size_t size) >> >> Quote from an earlier reply of mine: >> >> Arguably the one static function may better be, as other >> workarounds to avoid the "unused" compiler warning wouldn't >> be any better. >> >> and then later >> >> You misunderstood - I said that for this one (unused) static >> function such an #ifdef is probably okay, as the presumably >> smallest possible workaround. >> >> I really have no idea what else to say. > > Sorry, however, sometimes it is very difficult to understand what are > you referring to. If you could be more specific then I will be happy. > > Anyway, now it looks that you wish something like that: > > #ifndef CONFIG_ARM > /* Whole x86 ebmalloc stuff. */ > ... > #else > void __init free_ebmalloc_unused_mem(void) > { > } > #endif > > and then call free_ebmalloc_unused_mem() from e.g. > xen/arch/arm/setup.c:init_done(). Am I right? No. I want the #ifdef to _only_ cover the unused static function. Jan
Hi Daniel, On 22/09/16 13:07, Daniel Kiper wrote: > On Thu, Sep 22, 2016 at 05:25:46AM -0600, Jan Beulich wrote: >>>>> On 22.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: >>> On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote: >>>>>>> On 20.09.16 at 20:45, <daniel.kiper@oracle.com> wrote: >>>>> On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote: >>>>>>>>> On 20.09.16 at 12:52, <daniel.kiper@oracle.com> wrote: >>> >>> [...] >>> >>>>>>> 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? >>>>>> >>>>>> That's not the static function, is it? The function you name should >>>>>> actually be called on ARM too (as I did point out elsewhere already), >>>>>> just to not leave a trap open for someone to fall into when (s)he >>>>>> adds a first use of the allocator on ARM. >>>>> >>>>> Well, I think that sane person doing that would analyze how ebmalloc works >>>>> on x86 and then move (align to ARM needs if required) all its machinery >>>>> (including free_ebmalloc_unused_mem()) to run on ARM. At least I would do >>>>> that. This way he/she would avoid issues mentioned by you. However, if you >>>>> still prefer your way I can do that. Though I am not sure about the rest of >>>>> ebmalloc stuff. Should I move it out of #ifndef CONFIG_ARM? Looking at your >>>>> earlier replies I see that yes. Am I correct? >>>> >>>> Yes. >>> >>> This does not work because if I build Xen for ARM then I got: >>> boot.c:589:21: error: 'ebmalloc' defined but not used >>> [-Werror=unused-function] >>> static void __init *ebmalloc(size_t size) >> >> Quote from an earlier reply of mine: >> >> Arguably the one static function may better be, as other >> workarounds to avoid the "unused" compiler warning wouldn't >> be any better. >> >> and then later >> >> You misunderstood - I said that for this one (unused) static >> function such an #ifdef is probably okay, as the presumably >> smallest possible workaround. >> >> I really have no idea what else to say. > > Sorry, however, sometimes it is very difficult to understand what are > you referring to. If you could be more specific then I will be happy. > > Anyway, now it looks that you wish something like that: > > #ifndef CONFIG_ARM > /* Whole x86 ebmalloc stuff. */ > ... > #else > void __init free_ebmalloc_unused_mem(void) > { > } > #endif > > and then call free_ebmalloc_unused_mem() from e.g. > xen/arch/arm/setup.c:init_done(). Am I right? Bear in mind that the EFI loader on ARM is standalone. It cannot interact with Xen. The main goal of the EFI stub is to load the different images on the memory and then will jump at the same starting point as when Xen is loaded without EFI. So anything in bss will be zeroed. Regards,
Hi Julien, On Thu, Sep 22, 2016 at 06:07:26PM +0100, Julien Grall wrote: [...] > >#ifndef CONFIG_ARM > >/* Whole x86 ebmalloc stuff. */ > >... > >#else > >void __init free_ebmalloc_unused_mem(void) > >{ > >} > >#endif > > > >and then call free_ebmalloc_unused_mem() from e.g. > >xen/arch/arm/setup.c:init_done(). Am I right? > > Bear in mind that the EFI loader on ARM is standalone. It cannot > interact with Xen. > > The main goal of the EFI stub is to load the different images on the > memory and then will jump at the same starting point as when Xen is > loaded without EFI. So anything in bss will be zeroed. AIUI, on ARM EFI we have following call sequence: - efi_start(), - efi_xen_start(), - real_start() - ... - el2() which zeroes BSS... ;-((( We had the same situation on x86. So, we moved BSS init just before efi_start() call and disabled later zero BSS if we are booted via EFI. Could we do the same on ARM? As I can see Jan wish to use ebmalloc on ARM too. Does it make sense for you? Thank you for pointing this issue out. Daniel
On 23/09/16 11:50, Daniel Kiper wrote: > Hi Julien, Hi Daniel, > > On Thu, Sep 22, 2016 at 06:07:26PM +0100, Julien Grall wrote: > > [...] > >>> #ifndef CONFIG_ARM >>> /* Whole x86 ebmalloc stuff. */ >>> ... >>> #else >>> void __init free_ebmalloc_unused_mem(void) >>> { >>> } >>> #endif >>> >>> and then call free_ebmalloc_unused_mem() from e.g. >>> xen/arch/arm/setup.c:init_done(). Am I right? >> >> Bear in mind that the EFI loader on ARM is standalone. It cannot >> interact with Xen. >> >> The main goal of the EFI stub is to load the different images on the >> memory and then will jump at the same starting point as when Xen is >> loaded without EFI. So anything in bss will be zeroed. > > AIUI, on ARM EFI we have following call sequence: > - efi_start(), > - efi_xen_start(), > - real_start() > - ... > - el2() which zeroes BSS... ;-((( > > We had the same situation on x86. So, we moved BSS init just before > efi_start() call and disabled later zero BSS if we are booted via EFI. > Could we do the same on ARM? As I can see Jan wish to use ebmalloc on > ARM too. Does it make sense for you? The EFI on ARM has been designed to be standalone and disable page tables, flush the cache before hand and then jump in the startup beginning of the binary (as it would be done without EFI). The problem I can see here, is ebmalloc_mem is allocated in bss rather than in init. I understand this is an optimization, to shrink down the size of the binary. But, I am not in favor to start differing in startup code if we have EFI enabled just for that. IHMO, anything related to the stub should be in the init section and therefore will be freed when Xen has finished to boot. Regards,
On Fri, Sep 23, 2016 at 12:07:14PM +0100, Julien Grall wrote: > On 23/09/16 11:50, Daniel Kiper wrote: > >Hi Julien, > > Hi Daniel, > > > > >On Thu, Sep 22, 2016 at 06:07:26PM +0100, Julien Grall wrote: > > > >[...] > > > >>>#ifndef CONFIG_ARM > >>>/* Whole x86 ebmalloc stuff. */ > >>>... > >>>#else > >>>void __init free_ebmalloc_unused_mem(void) > >>>{ > >>>} > >>>#endif > >>> > >>>and then call free_ebmalloc_unused_mem() from e.g. > >>>xen/arch/arm/setup.c:init_done(). Am I right? > >> > >>Bear in mind that the EFI loader on ARM is standalone. It cannot > >>interact with Xen. > >> > >>The main goal of the EFI stub is to load the different images on the > >>memory and then will jump at the same starting point as when Xen is > >>loaded without EFI. So anything in bss will be zeroed. > > > >AIUI, on ARM EFI we have following call sequence: > > - efi_start(), > > - efi_xen_start(), > > - real_start() > > - ... > > - el2() which zeroes BSS... ;-((( > > > >We had the same situation on x86. So, we moved BSS init just before > >efi_start() call and disabled later zero BSS if we are booted via EFI. > >Could we do the same on ARM? As I can see Jan wish to use ebmalloc on > >ARM too. Does it make sense for you? > > The EFI on ARM has been designed to be standalone and disable page > tables, flush the cache before hand and then jump in the startup > beginning of the binary (as it would be done without EFI). > > The problem I can see here, is ebmalloc_mem is allocated in bss > rather than in init. I understand this is an optimization, to shrink > down the size of the binary. > > But, I am not in favor to start differing in startup code if we have > EFI enabled just for that. > > IHMO, anything related to the stub should be in the init section and > therefore will be freed when Xen has finished to boot. One early allocator for both platforms would be nice. And I have a feeling that this is the Jan's goal. However, I am not going to insist because you know ARM platforms better than I. So, I think that Jan should say what is his idea in this situation. Daniel
On 23/09/16 12:35, Daniel Kiper wrote: > On Fri, Sep 23, 2016 at 12:07:14PM +0100, Julien Grall wrote: >> On 23/09/16 11:50, Daniel Kiper wrote: >>> Hi Julien, >> >> Hi Daniel, >> >>> >>> On Thu, Sep 22, 2016 at 06:07:26PM +0100, Julien Grall wrote: >>> >>> [...] >>> >>>>> #ifndef CONFIG_ARM >>>>> /* Whole x86 ebmalloc stuff. */ >>>>> ... >>>>> #else >>>>> void __init free_ebmalloc_unused_mem(void) >>>>> { >>>>> } >>>>> #endif >>>>> >>>>> and then call free_ebmalloc_unused_mem() from e.g. >>>>> xen/arch/arm/setup.c:init_done(). Am I right? >>>> >>>> Bear in mind that the EFI loader on ARM is standalone. It cannot >>>> interact with Xen. >>>> >>>> The main goal of the EFI stub is to load the different images on the >>>> memory and then will jump at the same starting point as when Xen is >>>> loaded without EFI. So anything in bss will be zeroed. >>> >>> AIUI, on ARM EFI we have following call sequence: >>> - efi_start(), >>> - efi_xen_start(), >>> - real_start() >>> - ... >>> - el2() which zeroes BSS... ;-((( >>> >>> We had the same situation on x86. So, we moved BSS init just before >>> efi_start() call and disabled later zero BSS if we are booted via EFI. >>> Could we do the same on ARM? As I can see Jan wish to use ebmalloc on >>> ARM too. Does it make sense for you? >> >> The EFI on ARM has been designed to be standalone and disable page >> tables, flush the cache before hand and then jump in the startup >> beginning of the binary (as it would be done without EFI). >> >> The problem I can see here, is ebmalloc_mem is allocated in bss >> rather than in init. I understand this is an optimization, to shrink >> down the size of the binary. >> >> But, I am not in favor to start differing in startup code if we have >> EFI enabled just for that. >> >> IHMO, anything related to the stub should be in the init section and >> therefore will be freed when Xen has finished to boot. > > One early allocator for both platforms would be nice. And I have a feeling > that this is the Jan's goal. However, I am not going to insist because you > know ARM platforms better than I. So, I think that Jan should say what is > his idea in this situation. Out of interest, what is the reason behind putting the early allocator in bss rather than init? Regards,
On Fri, Sep 23, 2016 at 12:42:10PM +0100, Julien Grall wrote: > > > On 23/09/16 12:35, Daniel Kiper wrote: > >On Fri, Sep 23, 2016 at 12:07:14PM +0100, Julien Grall wrote: > >>On 23/09/16 11:50, Daniel Kiper wrote: > >>>Hi Julien, > >> > >>Hi Daniel, > >> > >>> > >>>On Thu, Sep 22, 2016 at 06:07:26PM +0100, Julien Grall wrote: > >>> > >>>[...] > >>> > >>>>>#ifndef CONFIG_ARM > >>>>>/* Whole x86 ebmalloc stuff. */ > >>>>>... > >>>>>#else > >>>>>void __init free_ebmalloc_unused_mem(void) > >>>>>{ > >>>>>} > >>>>>#endif > >>>>> > >>>>>and then call free_ebmalloc_unused_mem() from e.g. > >>>>>xen/arch/arm/setup.c:init_done(). Am I right? > >>>> > >>>>Bear in mind that the EFI loader on ARM is standalone. It cannot > >>>>interact with Xen. > >>>> > >>>>The main goal of the EFI stub is to load the different images on the > >>>>memory and then will jump at the same starting point as when Xen is > >>>>loaded without EFI. So anything in bss will be zeroed. > >>> > >>>AIUI, on ARM EFI we have following call sequence: > >>> - efi_start(), > >>> - efi_xen_start(), > >>> - real_start() > >>> - ... > >>> - el2() which zeroes BSS... ;-((( > >>> > >>>We had the same situation on x86. So, we moved BSS init just before > >>>efi_start() call and disabled later zero BSS if we are booted via EFI. > >>>Could we do the same on ARM? As I can see Jan wish to use ebmalloc on > >>>ARM too. Does it make sense for you? > >> > >>The EFI on ARM has been designed to be standalone and disable page > >>tables, flush the cache before hand and then jump in the startup > >>beginning of the binary (as it would be done without EFI). > >> > >>The problem I can see here, is ebmalloc_mem is allocated in bss > >>rather than in init. I understand this is an optimization, to shrink > >>down the size of the binary. > >> > >>But, I am not in favor to start differing in startup code if we have > >>EFI enabled just for that. > >> > >>IHMO, anything related to the stub should be in the init section and > >>therefore will be freed when Xen has finished to boot. > > > >One early allocator for both platforms would be nice. And I have a feeling > >that this is the Jan's goal. However, I am not going to insist because you > >know ARM platforms better than I. So, I think that Jan should say what is > >his idea in this situation. > > Out of interest, what is the reason behind putting the early > allocator in bss rather than init? First of all some data must live longer than just init phase. Later we do not want to increase image size. You could find full explanation in the commit message for this patch. However, if you still have questions drop me a line. Daniel
>>> On 23.09.16 at 13:35, <daniel.kiper@oracle.com> wrote: > One early allocator for both platforms would be nice. And I have a feeling > that this is the Jan's goal. However, I am not going to insist because you > know ARM platforms better than I. So, I think that Jan should say what is > his idea in this situation. The question of what section to put the data in is pretty orthogonal to my request to make the allocator platform independent. In fact, if desired we could have a per-arch override to specify the section this should go into. Jan
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 10985721..42cc5f8 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -117,7 +117,7 @@ static void __init relocate_trampoline(unsigned long phys) static void __init place_string(u32 *addr, const char *s) { - static char *__initdata alloc = start; + char *alloc = NULL; if ( s && *s ) { @@ -125,7 +125,7 @@ static void __init place_string(u32 *addr, const char *s) const char *old = (char *)(long)*addr; size_t len2 = *addr ? strlen(old) + 1 : 0; - alloc -= len1 + len2; + alloc = ebmalloc(len1 + len2); /* * Insert new string before already existing one. This is needed * for options passed on the command line to override options from @@ -208,12 +208,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) { - place_string(&mbi.mem_upper, NULL); - mbi.mem_upper -= map_size; - mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR); - if ( mbi.mem_upper < xen_phys_start ) - return NULL; - return (void *)(long)mbi.mem_upper; + return ebmalloc(map_size); } static void __init efi_arch_pre_exit_boot(void) diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 0bfaa74..014739a 100644 --- 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) { } void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { } diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 06f3970..5b17783 100644 --- 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(); + /* MUST be done prior to removing .init data. */ unregister_init_virtual_region(); @@ -1080,8 +1082,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) if ( !xen_phys_start ) panic("Not enough memory to relocate Xen."); - reserve_e820_ram(&boot_e820, efi_enabled(EFI_LOADER) ? mbi->mem_upper : __pa(&_start), - __pa(&_end)); + reserve_e820_ram(&boot_e820, __pa(&_start), __pa(&_end)); /* Late kexec reservation (dynamic start address). */ kexec_reserve_area(&boot_e820); diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 1ef5d0b..192c0b8 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -79,6 +79,8 @@ static size_t wstrlen(const CHAR16 * s); static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); +static void *ebmalloc(size_t size); + static const EFI_BOOT_SERVICES *__initdata efi_bs; static UINT32 __initdata efi_bs_revision; static EFI_HANDLE __initdata efi_ih; @@ -1158,7 +1160,38 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) for( ; ; ); /* not reached */ } -#ifndef CONFIG_ARM /* TODO - runtime service support */ +#ifndef CONFIG_ARM + +#define EBMALLOC_SIZE MB(1) + +static char __section(".bss.page_aligned") __aligned(PAGE_SIZE) ebmalloc_mem[EBMALLOC_SIZE]; +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); + + if ( ebmalloc_allocated > sizeof(ebmalloc_mem) ) + blexit(L"Out of static memory\r\n"); + + return ptr; +} + +void __init free_ebmalloc_unused_mem(void) +{ + unsigned long start, end; + + start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated); + end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem); + + destroy_xen_mappings(start, end); + init_xenheap_pages(__pa(start), __pa(end)); + + printk(XENLOG_INFO "Freed %lukB unused BSS memory\n", (end - start) >> 10); +} static bool_t __initdata efi_map_uc; diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h index 68c68a8..6c7f601 100644 --- a/xen/include/xen/efi.h +++ b/xen/include/xen/efi.h @@ -32,6 +32,7 @@ struct xenpf_efi_runtime_call; struct compat_pf_efi_runtime_call; bool efi_enabled(unsigned int feature); +void free_ebmalloc_unused_mem(void); void efi_init_memory(void); bool_t efi_rs_using_pgtables(void); unsigned long efi_get_time(void);
There is a problem with place_string() which is used as early memory allocator. It gets memory chunks starting from start symbol and goes down. Sadly this does not work when Xen is loaded using multiboot2 protocol because then the start lives on 1 MiB address and we should not allocate a memory from below of it. So, I tried to use mem_lower address calculated by GRUB2. However, this solution works only on some machines. There are machines in the wild (e.g. Dell PowerEdge R820) which uses first ~640 KiB for boot services code or data... :-((( Hence, we need new memory allocator for Xen EFI boot code which is quite simple and generic and could be used by place_string() and efi_arch_allocate_mmap_buffer(). I think about following solutions: 1) We could use native EFI allocation functions (e.g. AllocatePool() or AllocatePages()) to get memory chunk. However, later (somewhere in __start_xen()) we must copy its contents to safe place or reserve it in e820 memory map and map it in Xen virtual address space. This means that the code referring to Xen command line, loaded modules and EFI memory map, mostly in __start_xen(), will be further complicated and diverge from legacy BIOS cases. Additionally, both former things have to be placed below 4 GiB because their addresses are stored in multiboot_info_t structure which has 32-bit relevant members. 2) We may allocate memory area statically somewhere in Xen code which could be used as memory pool for early dynamic allocations. Looks quite simple. Additionally, it would not depend on EFI at all and could be used on legacy BIOS platforms if we need it. However, we must carefully choose size of this pool. We do not want increase Xen binary size too much and waste too much memory but also we must fit at least memory map on x86 EFI platforms. As I saw on small machine, e.g. IBM System x3550 M2 with 8 GiB RAM, memory map may contain more than 200 entries. Every entry on x86-64 platform is 40 bytes in size. So, it means that we need more than 8 KiB for EFI memory map only. Additionally, if we use this memory pool for Xen and modules command line storage (it would be used when xen.efi is executed as EFI application) then we should add, I think, about 1 KiB. In this case, to be on safe side, we should assume at least 64 KiB pool for early memory allocations. Which is about 4 times of our earlier calculations. However, during discussion on Xen-devel Jan Beulich suggested that just in case we should use 1 MiB memory pool like it is in original place_string() implementation. So, let's use 1 MiB as it was proposed. If we think that we should not waste unallocated memory in the pool on running system then we can mark this region as __initdata and move all required data to dynamically allocated places somewhere in __start_xen(). 2a) We could put memory pool into .bss.page_aligned section. Then allocate memory chunks starting from the lowest address. After init phase we can free unused portion of the memory pool as in case of .init.text or .init.data sections. This way we do not need to allocate any space in image file and freeing of unused area in the memory pool is very simple. Now #2a solution is implemented because it is quite simple and requires limited number of changes, especially in __start_xen(). Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v6 - suggestions/fixes: - optimize ebmalloc allocator, - move ebmalloc machinery to xen/common/efi/boot.c (suggested by Jan Beulich), - enforce PAGE_SIZE ebmalloc_mem alignment (suggested by Jan Beulich), - ebmalloc() must allocate properly aligned memory regions (suggested by Jan Beulich), - printk() should use XENLOG_INFO (suggested by Jan Beulich). v4 - suggestions/fixes: - move from #2 solution to #2a solution, - improve commit message. --- xen/arch/x86/efi/efi-boot.h | 11 +++-------- xen/arch/x86/efi/stub.c | 2 ++ xen/arch/x86/setup.c | 5 +++-- xen/common/efi/boot.c | 35 ++++++++++++++++++++++++++++++++++- xen/include/xen/efi.h | 1 + 5 files changed, 43 insertions(+), 11 deletions(-)