| Message ID | 1475185362-14198-7-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. Jan
On Fri, Sep 30, 2016 at 03:46:54AM -0600, Jan Beulich wrote: > >>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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 ... Nope. > > @@ -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. Err... I thought that it does somehow. It looks that at least one test build is always good idea. Daniel
On Fri, Sep 30, 2016 at 03:46:54AM -0600, Jan Beulich wrote: > >>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. Julien, ping? I would like to go forward with this patchset. Could you tell us is current solution OK for you (of course except this stupid build issue spotted by Jan)? Should anything change? Daniel
Hello Daniel, On 05/10/2016 00:02, Daniel Kiper wrote: > On Fri, Sep 30, 2016 at 03:46:54AM -0600, Jan Beulich wrote: >>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. > > Julien, ping? I would like to go forward with this patchset. Could > you tell us is current solution OK for you (of course except this > stupid build issue spotted by Jan)? Should anything change? Sorry I have been in conferences for the past 2 weeks with limited review bandwidth. I will try to give a look today. Cheers,
Hi Jan, Sorry for the late answer, I have been traveling the past 2 weeks. On 30/09/2016 02:46, Jan Beulich wrote: >>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. Regards,
>>> On 05.10.16 at 20:30, <julien.grall@arm.com> wrote: > On 30/09/2016 02:46, Jan Beulich wrote: >>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. Jan
Hello Jan, On 06/10/16 13:21, Jan Beulich wrote: >>>> On 05.10.16 at 20:30, <julien.grall@arm.com> wrote: >> On 30/09/2016 02:46, Jan Beulich wrote: >>>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. I would be happy to review any patches addressing the TODOs. Regards,
>>> On 11.10.16 at 15:39, <julien.grall@arm.com> wrote: > Hello Jan, > > On 06/10/16 13:21, Jan Beulich wrote: >>>>> On 05.10.16 at 20:30, <julien.grall@arm.com> wrote: >>> On 30/09/2016 02:46, Jan Beulich wrote: >>>>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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, - as we've already figured there's no need for the stub free_ebmalloc_unused_mem() right now anyway. 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. > I would be happy to review any patches addressing the TODOs. This, I'm sorry, gets me to raise another question: When is this finally going to happen? Shared EFI code was introduced for 4.5, and we're now looking to release 4.8 still with runtime support unimplemented on ARM. How much longer is this going to take? Jan
Hello Jan, On 12/10/2016 12:45, Jan Beulich wrote: >>>> On 11.10.16 at 15:39, <julien.grall@arm.com> wrote: >> On 06/10/16 13:21, Jan Beulich wrote: >>>>>> On 05.10.16 at 20:30, <julien.grall@arm.com> wrote: >>>> On 30/09/2016 02:46, Jan Beulich wrote: >>>>>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. 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. > >> I would be happy to review any patches addressing the TODOs. > > This, I'm sorry, gets me to raise another question: When is this > finally going to happen? Shared EFI code was introduced for 4.5, > and we're now looking to release 4.8 still with runtime support > unimplemented on ARM. How much longer is this going to take? Xen is a community project, features are added by contributors when they need it. I personally don't have any bandwidth to work on EFI runtimes services at the moment (note that the item is in my TODO list as a low priority). I welcome any contribution on EFI runtime support for ARM and will be happy to review any series. Cheers,
>>> On 12.10.16 at 14:51, <julien.grall@arm.com> wrote: > Hello Jan, > > On 12/10/2016 12:45, Jan Beulich wrote: >>>>> On 11.10.16 at 15:39, <julien.grall@arm.com> wrote: >>> On 06/10/16 13:21, Jan Beulich wrote: >>>>>>> On 05.10.16 at 20:30, <julien.grall@arm.com> wrote: >>>>> On 30/09/2016 02:46, Jan Beulich wrote: >>>>>>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. Jan
On Wed, Oct 12, 2016 at 06:59:52AM -0600, Jan Beulich wrote: > >>> On 12.10.16 at 14:51, <julien.grall@arm.com> wrote: > > Hello Jan, > > > > On 12/10/2016 12:45, Jan Beulich wrote: > >>>>> On 11.10.16 at 15:39, <julien.grall@arm.com> wrote: > >>> On 06/10/16 13:21, Jan Beulich wrote: > >>>>>>> On 05.10.16 at 20:30, <julien.grall@arm.com> wrote: > >>>>> On 30/09/2016 02:46, Jan Beulich wrote: > >>>>>>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. Julien, Jan could you finally agree how this should be done? It looks that it is last thing which blocks whole patch series. Daniel
>>> On 24.10.16 at 11:03, <daniel.kiper@oracle.com> wrote: > On Wed, Oct 12, 2016 at 06:59:52AM -0600, Jan Beulich wrote: >> >>> On 12.10.16 at 14:51, <julien.grall@arm.com> wrote: >> > Hello Jan, >> > >> > On 12/10/2016 12:45, Jan Beulich wrote: >> >>>>> On 11.10.16 at 15:39, <julien.grall@arm.com> wrote: >> >>> On 06/10/16 13:21, Jan Beulich wrote: >> >>>>>>> On 05.10.16 at 20:30, <julien.grall@arm.com> wrote: >> >>>>> On 30/09/2016 02:46, Jan Beulich wrote: >> >>>>>>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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. > > Julien, Jan could you finally agree how this should be done? Well, the ball is in Julien's court right now afaict from the above. > It looks that it is last thing which blocks whole patch series. I don't think so - Andrew has (not via mail) already indicated he'd like to comment on the not insignificant amount of assembly code getting added, some of which presumably could (and hence should) better be done in C. I've specifically avoided so far to respond with any R-b or ack on the two main (in this regard) patches. Jan
Hi Jan, On 12/10/16 13:59, Jan Beulich wrote: >>>> On 12.10.16 at 14:51, <julien.grall@arm.com> wrote: >> Hello Jan, >> >> On 12/10/2016 12:45, Jan Beulich wrote: >>>>>> On 11.10.16 at 15:39, <julien.grall@arm.com> wrote: >>>> On 06/10/16 13:21, Jan Beulich wrote: >>>>>>>> On 05.10.16 at 20:30, <julien.grall@arm.com> wrote: >>>>>> On 30/09/2016 02:46, Jan Beulich wrote: >>>>>>>>>> On 29.09.16 at 23:42, <daniel.kiper@oracle.com> 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,
On Mon, Oct 24, 2016 at 03:57:22AM -0600, Jan Beulich wrote: > >>> On 24.10.16 at 11:03, <daniel.kiper@oracle.com> wrote: [...] > > It looks that it is last thing which blocks whole patch series. > > I don't think so - Andrew has (not via mail) already indicated he'd > like to comment on the not insignificant amount of assembly code > getting added, some of which presumably could (and hence should) > better be done in C. I've specifically avoided so far to respond with > any R-b or ack on the two main (in this regard) patches. Andrew, ping? Could you send me your comments? Jan
On Thu, Nov 03, 2016 at 02:48:26PM +0100, Daniel Kiper wrote: > On Mon, Oct 24, 2016 at 03:57:22AM -0600, Jan Beulich wrote: > > >>> On 24.10.16 at 11:03, <daniel.kiper@oracle.com> wrote: > > [...] > > > > It looks that it is last thing which blocks whole patch series. > > > > I don't think so - Andrew has (not via mail) already indicated he'd > > like to comment on the not insignificant amount of assembly code > > getting added, some of which presumably could (and hence should) > > better be done in C. I've specifically avoided so far to respond with > > any R-b or ack on the two main (in this regard) patches. > > Andrew, ping? Could you send me your comments? Ping? Daniel
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 388c4ea..62c010e 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -114,7 +114,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 ) { @@ -122,7 +122,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 @@ -205,12 +205,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/setup.c b/xen/arch/x86/setup.c index 06f3970..9f50eb0 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1080,8 +1080,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..46559f0 100644 --- 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, + * - expose full blown free_ebmalloc_unused_mem() instead of its stub. + */ + +#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) & ~(sizeof(void *) - 1); + + if ( ebmalloc_allocated > sizeof(ebmalloc_mem) ) + blexit(L"Out of static memory\r\n"); + + return ptr; +} + +static 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); +} +#else +static void __init free_ebmalloc_unused_mem(void) +{ +} +#endif + /* * Include architecture specific implementation here, which references the * static globals defined above. @@ -1251,6 +1301,8 @@ void __init efi_init_memory(void) } *extra, *extra_head = NULL; #endif + free_ebmalloc_unused_mem(); + if ( !efi_enabled(EFI_BOOT) ) return;
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(). New allocator is quite generic and can be used on ARM platforms too. Though it is not enabled on ARM yet due to lack of some prereq. List of them is placed before ebmalloc code. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v9 - suggestions/fixes: - call free_ebmalloc_unused_mem() from efi_init_memory() instead of xen/arch/arm/setup.c:init_done() (suggested by Jan Beulich), - improve comments. v8 - suggestions/fixes: - disable whole ebmalloc machinery on ARM platforms, - add comment saying what should be done before enabling ebmalloc on ARM, (suggested by Julien Grall), - move ebmalloc code before efi-boot.h inclusion and remove unneeded forward declaration (suggested by Jan Beulich), - remove free_ebmalloc_unused_mem() call from xen/arch/arm/setup.c:init_done() (suggested by Julien Grall), - improve commit message. v7 - suggestions/fixes: - enable most of ebmalloc machinery on ARM platforms (suggested by Jan Beulich), - remove unneeded cast (suggested by Jan Beulich), - wrap long line (suggested by Jan Beulich), - improve commit message. 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/setup.c | 3 +-- xen/common/efi/boot.c | 52 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 10 deletions(-)