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

[Daniel Kiper]

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

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

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

[Daniel Kiper]

On Mon, Oct 24, 2016 at 03:57:22AM -0600, Jan Beulich wrote:
> >>> On 24.10.16 at 11:03,  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

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

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

[Julien Grall]

Hi Jan,

On 12/10/16 13:59, Jan Beulich wrote:

On 12.10.16 at 14:51,  wrote:

Hello Jan,

On 12/10/2016 12:45, Jan Beulich wrote:

On 11.10.16 at 15:39,  wrote:

On 06/10/16 13:21, Jan Beulich wrote:

On 05.10.16 at 20:30,  wrote:

On 30/09/2016 02:46, Jan Beulich wrote:

On 29.09.16 at 23:42,  wrote:

+#else
+static void __init free_ebmalloc_unused_mem(void)
+{
+}
+#endif


Did you build test this for ARM? The function ought to be unused,
as ...


@@ -1251,6 +1301,8 @@ void __init efi_init_memory(void)
 } *extra, *extra_head = NULL;
 #endif

+free_ebmalloc_unused_mem();


... the whole function here doesn't get built on ARM.

Julien - we're still awaiting your input on general aspects here.


efi_init_memory would need to be called during Xen boot on ARM. I am not
sure where as I we don't yet have runtime support on ARM.

Other than that, the patch looks good to me.


But that wasn't the question. My goal is to have as little code
inside #ifndef CONFIG_ARM as possible, and hence I'd like to have
as much of this new code as possible outside of such conditionals.
So the question really is whether that alternative approach would
be fine with you, or what problems you might see.


I am not sure to get it. The current approach looks good to me, however,
the implementation should not be exposed to ARM until all the TODOs
mentioned by Daniel are fixed.


Which is precisely the opposite of what I'm aiming at. Once again:
Don't you think it is desirable to keep the #ifndef CONFIG_ARM
instances to cover as little code as possible? Not all of the named
TODOs really need to be addressed in order to compile most of
what comprises this new allocator; in fact none of them really
needs addressing:
- if the size estimation turns out to low once ARM starts actually
  using this, let's just bump it (perhaps by making it a per-arch
  constant),
- if the section chosen needs to be different (which it really
  shouldn't be), let's simply adjust it,


If we keep the section in BSS, then we really need to move the
initialization of BSS earlier.


Right, but that should be simple enough. Or we do ...


This TODO really needs to be fixed now otherwise it will be a nightmare
to debug later on.


- as we've already figured there's no need for the stub
  free_ebmalloc_unused_mem() right now anyway.


But we would need to call free_ebmalloc_unused_mem from somewhere. The
idea to not expose the early memory allocator on ARM is avoid to have an
implementation with may not fully work on ARM because of known missing
pieces.


And then (as another alternative) we have the option of ARM
simply defining EBMALLOC_SIZE to zero for the time being. That
would eliminate the need to actually call free_ebmalloc_unused_mem()
and turn the other two items into non-issues.


... this, which you didn't comment on at all.


I skipped this part by mistake. That would work to, assuming there is a 
proper comment on top of EBMALLOC_SIZE explaining what needs to be done 
in order to fully support the early allocator on ARM.


Both solutions are fine by me.

Cheers,

--
Julien Grall

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

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

[Jan Beulich]

>>> On 24.10.16 at 11:03,  wrote:
> On Wed, Oct 12, 2016 at 06:59:52AM -0600, Jan Beulich wrote:
>> >>> On 12.10.16 at 14:51,  wrote:
>> > Hello Jan,
>> >
>> > On 12/10/2016 12:45, Jan Beulich wrote:
>> > On 11.10.16 at 15:39,  wrote:
>> >>> On 06/10/16 13:21, Jan Beulich wrote:
>> >>> On 05.10.16 at 20:30,  wrote:
>> > On 30/09/2016 02:46, Jan Beulich wrote:
>> > On 29.09.16 at 23:42,  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

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

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

[Daniel Kiper]

On Wed, Oct 12, 2016 at 06:59:52AM -0600, Jan Beulich wrote:
> >>> On 12.10.16 at 14:51,  wrote:
> > Hello Jan,
> >
> > On 12/10/2016 12:45, Jan Beulich wrote:
> > On 11.10.16 at 15:39,  wrote:
> >>> On 06/10/16 13:21, Jan Beulich wrote:
> >>> On 05.10.16 at 20:30,  wrote:
> > On 30/09/2016 02:46, Jan Beulich wrote:
> > On 29.09.16 at 23:42,  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

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

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

[Jan Beulich]

>>> On 12.10.16 at 14:51,  wrote:
> Hello Jan,
> 
> On 12/10/2016 12:45, Jan Beulich wrote:
> On 11.10.16 at 15:39,  wrote:
>>> On 06/10/16 13:21, Jan Beulich wrote:
>>> On 05.10.16 at 20:30,  wrote:
> On 30/09/2016 02:46, Jan Beulich wrote:
> On 29.09.16 at 23:42,  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


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

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

[Julien Grall]

Hello Jan,

On 12/10/2016 12:45, Jan Beulich wrote:

On 11.10.16 at 15:39,  wrote:

On 06/10/16 13:21, Jan Beulich wrote:

On 05.10.16 at 20:30,  wrote:

On 30/09/2016 02:46, Jan Beulich wrote:

On 29.09.16 at 23:42,  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,

--
Julien Grall

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

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

[Jan Beulich]

>>> On 11.10.16 at 15:39,  wrote:
> Hello Jan,
> 
> On 06/10/16 13:21, Jan Beulich wrote:
> On 05.10.16 at 20:30,  wrote:
>>> On 30/09/2016 02:46, Jan Beulich wrote:
>>> On 29.09.16 at 23:42,  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


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

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

[Julien Grall]

Hello Jan,

On 06/10/16 13:21, Jan Beulich wrote:

On 05.10.16 at 20:30,  wrote:

On 30/09/2016 02:46, Jan Beulich wrote:

On 29.09.16 at 23:42,  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,

--
Julien Grall

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

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

[Jan Beulich]

>>> On 05.10.16 at 20:30,  wrote:
> On 30/09/2016 02:46, Jan Beulich wrote:
> On 29.09.16 at 23:42,  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


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

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

[Julien Grall]

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

--
Julien Grall

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

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

[Julien Grall]

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

--
Julien Grall

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

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

[Daniel Kiper]

On Fri, Sep 30, 2016 at 03:46:54AM -0600, Jan Beulich wrote:
> >>> On 29.09.16 at 23:42,  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

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

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

[Daniel Kiper]

On Fri, Sep 30, 2016 at 03:46:54AM -0600, Jan Beulich wrote:
> >>> On 29.09.16 at 23:42,  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

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

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

[Jan Beulich]

>>> On 29.09.16 at 23:42,  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


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