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

[Jan Beulich]

>>> On 23.09.16 at 13:35,  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


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

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

[Daniel Kiper]

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

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

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

[Julien Grall]

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,

--
Julien Grall

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

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

[Daniel Kiper]

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

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

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

[Julien Grall]

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,

--
Julien Grall

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

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

[Daniel Kiper]

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

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

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

[Julien Grall]

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

On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote:

On 20.09.16 at 20:45,  wrote:

On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote:

On 20.09.16 at 12:52,  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,

--
Julien Grall

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

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

[Jan Beulich]

>>> On 22.09.16 at 14:07,  wrote:
> On Thu, Sep 22, 2016 at 05:25:46AM -0600, Jan Beulich wrote:
>> >>> On 22.09.16 at 12:52,  wrote:
>> > On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote:
>> >> >>> On 20.09.16 at 20:45,  wrote:
>> >> > On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote:
>> >> >> >>> On 20.09.16 at 12:52,  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


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

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

[Daniel Kiper]

On Thu, Sep 22, 2016 at 05:25:46AM -0600, Jan Beulich wrote:
> >>> On 22.09.16 at 12:52,  wrote:
> > On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote:
> >> >>> On 20.09.16 at 20:45,  wrote:
> >> > On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote:
> >> >> >>> On 20.09.16 at 12:52,  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

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

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

[Jan Beulich]

>>> On 22.09.16 at 12:52,  wrote:
> On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote:
>> >>> On 20.09.16 at 20:45,  wrote:
>> > On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote:
>> >> >>> On 20.09.16 at 12:52,  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


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

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

[Daniel Kiper]

On Wed, Sep 21, 2016 at 03:42:09AM -0600, Jan Beulich wrote:
> >>> On 20.09.16 at 20:45,  wrote:
> > On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote:
> >> >>> On 20.09.16 at 12:52,  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

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

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

[Jan Beulich]

>>> On 20.09.16 at 20:45,  wrote:
> On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote:
>> >>> On 20.09.16 at 12:52,  wrote:
>> > On Tue, Sep 20, 2016 at 03:57:19AM -0600, Jan Beulich wrote:
>> >> >>> On 20.09.16 at 11:45,  wrote:
>> >> > On Mon, Sep 19, 2016 at 09:17:50AM -0600, Jan Beulich wrote:
>> >> >> >>> On 19.09.16 at 17:04,  wrote:
>> >> >> > On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
>> >> >> >> >>> On 12.09.16 at 22:18,  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


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

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

[Daniel Kiper]

On Tue, Sep 20, 2016 at 07:46:56AM -0600, Jan Beulich wrote:
> >>> On 20.09.16 at 12:52,  wrote:
> > On Tue, Sep 20, 2016 at 03:57:19AM -0600, Jan Beulich wrote:
> >> >>> On 20.09.16 at 11:45,  wrote:
> >> > On Mon, Sep 19, 2016 at 09:17:50AM -0600, Jan Beulich wrote:
> >> >> >>> On 19.09.16 at 17:04,  wrote:
> >> >> > On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
> >> >> >> >>> On 12.09.16 at 22:18,  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

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

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

[Jan Beulich]

>>> On 20.09.16 at 12:52,  wrote:
> On Tue, Sep 20, 2016 at 03:57:19AM -0600, Jan Beulich wrote:
>> >>> On 20.09.16 at 11:45,  wrote:
>> > On Mon, Sep 19, 2016 at 09:17:50AM -0600, Jan Beulich wrote:
>> >> >>> On 19.09.16 at 17:04,  wrote:
>> >> > On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
>> >> >> >>> On 12.09.16 at 22:18,  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


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

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

[Daniel Kiper]

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

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

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

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

Daniel

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

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

[Jan Beulich]

>>> On 20.09.16 at 11:45,  wrote:
> On Mon, Sep 19, 2016 at 09:17:50AM -0600, Jan Beulich wrote:
>> >>> On 19.09.16 at 17:04,  wrote:
>> > On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
>> >> >>> On 12.09.16 at 22:18,  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


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

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

[Daniel Kiper]

On Mon, Sep 19, 2016 at 09:17:50AM -0600, Jan Beulich wrote:
> >>> On 19.09.16 at 17:04,  wrote:
> > On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
> >> >>> On 12.09.16 at 22:18,  wrote:
> >> > --- a/xen/arch/x86/setup.c
> >> > +++ b/xen/arch/x86/setup.c
> >> > @@ -520,6 +520,8 @@ static void noinline init_done(void)
> >> >
> >> >  system_state = SYS_STATE_active;
> >> >
> >> > +free_ebmalloc_unused_mem();
> >>
> >> Now that the allocator properly lives in common code, this appears
> >> to lack an ARM side counterpart.
> >
> > Why? It is called only from xen/arch/x86/setup.c:__start_xen() and all
> > ebmalloc stuff is in #ifndef CONFIG_ARM. So, free_ebmalloc_unused_mem()
> > will be needed only if we add ARM support here.
>
> Well, it being inside that conditional is part of the problem - there's
> no apparent point for all of it to be.

I can agree that this is potentially generic stuff (well, I understand that
it is our final goal but unreachable yet due to various things). However,
right know it is only used on x86. So, I am not sure what is the problem
with #ifndef CONFIG_ARM right now...

> Arguably the one static function may better be, as other workarounds
> to avoid the "unused" compiler warning wouldn't be any better.

Do you mean static function with empty body for ARM? It is not needed
right now because it is never called on ARM. Am I missing something?

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

Does sizeof() returns size_t type? I was thinking that it returns
a number calculated during compilation, however, it does not have
specific type.

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

Daniel

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

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

[Daniel Kiper]

On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
> >>> On 12.09.16 at 22:18,  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

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

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

[Jan Beulich]

>>> On 19.09.16 at 17:04,  wrote:
> On Mon, Sep 19, 2016 at 06:12:35AM -0600, Jan Beulich wrote:
>> >>> On 12.09.16 at 22:18,  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


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

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

[Jan Beulich]

>>> On 12.09.16 at 22:18,  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_SIZEMB(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


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