Re: [PATCH 17/18] x86/efi: create new early memory allocator
On Fri, Mar 27, 2015 at 01:35:11PM +, Jan Beulich wrote:
> >>> On 27.03.15 at 13:57, wrote:
> > On Mon, Mar 02, 2015 at 05:23:49PM +, Jan Beulich wrote:
> >> >>> On 30.01.15 at 18:54, wrote:
> >> > +{
> >> > +void *ptr;
> >> > +
> >> > +/*
> >> > + * Init __malloc_free on runtime. Static initialization
> >> > + * will not work because it puts virtual address there.
> >> > + */
> >> > +if ( __malloc_free == NULL )
> >> > +__malloc_free = __malloc_mem;
> >> > +
> >> > +ptr = __malloc_free;
> >> > +
> >> > +__malloc_free += size;
> >> > +
> >> > +if ( __malloc_free - __malloc_mem > sizeof(__malloc_mem) )
> >> > +blexit(L"Out of static memory\r\n");
> >> > +
> >> > +return ptr;
> >> > +}
> >>
> >> You're ignoring alignment requirements here altogether.
> >
> > I can understand why __malloc_mem should be let's say page aligned
> > but I am not sure why we should care here about alignment inside
> > of __malloc_mem array like...
> >
> >> > @@ -192,12 +218,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);
> >
> > ...here...
>
> Specifically with the later mentioned potential for sharing this with
> ARM I think you have to make sure that things are suitably aligned,
> or else you cause aborts.
>
> >> > -if ( mbi.mem_upper < xen_phys_start )
> >> > -return NULL;
> >> > -return (void *)(long)mbi.mem_upper;
> >> > +return __malloc(*map_size);
> >> > }
> >>
> >> Which then even suggests that _if_ we go this route, this could be
> >> shared with ARM (and hence become common code again).
> >
> > So, go or no go this route?
>
> As long as it's being done properly, I'm not wildly opposed.
So, AIUI, general idea is OK but fix all stuff which should be fixed. Right?
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 17/18] x86/efi: create new early memory allocator
>>> On 27.03.15 at 13:57, wrote:
> On Mon, Mar 02, 2015 at 05:23:49PM +, Jan Beulich wrote:
>> >>> On 30.01.15 at 18:54, wrote:
>> > +{
>> > +void *ptr;
>> > +
>> > +/*
>> > + * Init __malloc_free on runtime. Static initialization
>> > + * will not work because it puts virtual address there.
>> > + */
>> > +if ( __malloc_free == NULL )
>> > +__malloc_free = __malloc_mem;
>> > +
>> > +ptr = __malloc_free;
>> > +
>> > +__malloc_free += size;
>> > +
>> > +if ( __malloc_free - __malloc_mem > sizeof(__malloc_mem) )
>> > +blexit(L"Out of static memory\r\n");
>> > +
>> > +return ptr;
>> > +}
>>
>> You're ignoring alignment requirements here altogether.
>
> I can understand why __malloc_mem should be let's say page aligned
> but I am not sure why we should care here about alignment inside
> of __malloc_mem array like...
>
>> > @@ -192,12 +218,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);
>
> ...here...
Specifically with the later mentioned potential for sharing this with
ARM I think you have to make sure that things are suitably aligned,
or else you cause aborts.
>> > -if ( mbi.mem_upper < xen_phys_start )
>> > -return NULL;
>> > -return (void *)(long)mbi.mem_upper;
>> > +return __malloc(*map_size);
>> > }
>>
>> Which then even suggests that _if_ we go this route, this could be
>> shared with ARM (and hence become common code again).
>
> So, go or no go this route?
As long as it's being done properly, I'm not wildly opposed.
Jan
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 17/18] x86/efi: create new early memory allocator
On Mon, Mar 02, 2015 at 05:23:49PM +, Jan Beulich wrote:
> >>> On 30.01.15 at 18:54, wrote:
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -103,9 +103,35 @@ static void __init relocate_trampoline(unsigned long
> > phys)
> > *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
> > }
> >
> > +#define __MALLOC_SIZE 65536
> > +
> > +static char __malloc_mem[__MALLOC_SIZE];
> > +static char *__malloc_free = NULL;
> > +
> > +static void __init *__malloc(size_t size)
>
> Please do away with all these prefixing underscores.
>
> > +{
> > +void *ptr;
> > +
> > +/*
> > + * Init __malloc_free on runtime. Static initialization
> > + * will not work because it puts virtual address there.
> > + */
> > +if ( __malloc_free == NULL )
> > +__malloc_free = __malloc_mem;
> > +
> > +ptr = __malloc_free;
> > +
> > +__malloc_free += size;
> > +
> > +if ( __malloc_free - __malloc_mem > sizeof(__malloc_mem) )
> > +blexit(L"Out of static memory\r\n");
> > +
> > +return ptr;
> > +}
>
> You're ignoring alignment requirements here altogether.
I can understand why __malloc_mem should be let's say page aligned
but I am not sure why we should care here about alignment inside
of __malloc_mem array like...
> > @@ -192,12 +218,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);
...here...
> > -if ( mbi.mem_upper < xen_phys_start )
> > -return NULL;
> > -return (void *)(long)mbi.mem_upper;
> > +return __malloc(*map_size);
> > }
>
> Which then even suggests that _if_ we go this route, this could be
> shared with ARM (and hence become common code again).
So, go or no go this route? If not what do you suggest?
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 17/18] x86/efi: create new early memory allocator
On Tue, Mar 03, 2015 at 08:04:09AM +, Jan Beulich wrote:
> >>> On 02.03.15 at 21:25, wrote:
> > On Mon, Mar 2, 2015 at 9:23 AM, Jan Beulich wrote:
> > On 30.01.15 at 18:54, wrote:
> >>> @@ -192,12 +218,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 __malloc(*map_size);
> >>> }
> >>
> >> Which then even suggests that _if_ we go this route, this could be
> >> shared with ARM (and hence become common code again).
> >
> > We could do the same thing on ARM. For ARM, 2 allocations are done: 1
> > for the FDT, and
> > this one for the EFI memory map. Both of these are currently
> > allocated with EFI allocation
> > functions, so don't have fixed size limits. If we go with the fixed
> > size pool, I don't think that 64k
> > will be enough for the ARM case, as FDTs can be 20-50k in size.
>
> The 64k size here is to be debated anyway I think. We currently have
> about 1Mb in the x86 variant, and I'd much rather see the pool be
> this size initially, properly taking care of releasing to the allocator any
> unused portions of it.
Thanks for your comments guys. I will reply for all of them probably
next week. Now I am busy with bugs in GRUB2. Please stay tuned.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 17/18] x86/efi: create new early memory allocator
>>> On 02.03.15 at 21:25, wrote:
> On Mon, Mar 2, 2015 at 9:23 AM, Jan Beulich wrote:
> On 30.01.15 at 18:54, wrote:
>>> @@ -192,12 +218,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 __malloc(*map_size);
>>> }
>>
>> Which then even suggests that _if_ we go this route, this could be
>> shared with ARM (and hence become common code again).
>
> We could do the same thing on ARM. For ARM, 2 allocations are done: 1
> for the FDT, and
> this one for the EFI memory map. Both of these are currently
> allocated with EFI allocation
> functions, so don't have fixed size limits. If we go with the fixed
> size pool, I don't think that 64k
> will be enough for the ARM case, as FDTs can be 20-50k in size.
The 64k size here is to be debated anyway I think. We currently have
about 1Mb in the x86 variant, and I'd much rather see the pool be
this size initially, properly taking care of releasing to the allocator any
unused portions of it.
Jan
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 17/18] x86/efi: create new early memory allocator
On Mon, Mar 2, 2015 at 9:23 AM, Jan Beulich wrote:
On 30.01.15 at 18:54, wrote:
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -103,9 +103,35 @@ static void __init relocate_trampoline(unsigned long
>> phys)
>> *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
>> }
>>
>> +#define __MALLOC_SIZE65536
>> +
>> +static char __malloc_mem[__MALLOC_SIZE];
>> +static char *__malloc_free = NULL;
>> +
>> +static void __init *__malloc(size_t size)
>
> Please do away with all these prefixing underscores.
>
>> +{
>> +void *ptr;
>> +
>> +/*
>> + * Init __malloc_free on runtime. Static initialization
>> + * will not work because it puts virtual address there.
>> + */
>> +if ( __malloc_free == NULL )
>> +__malloc_free = __malloc_mem;
>> +
>> +ptr = __malloc_free;
>> +
>> +__malloc_free += size;
>> +
>> +if ( __malloc_free - __malloc_mem > sizeof(__malloc_mem) )
>> +blexit(L"Out of static memory\r\n");
>> +
>> +return ptr;
>> +}
>
> You're ignoring alignment requirements here altogether.
>
>> @@ -192,12 +218,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 __malloc(*map_size);
>> }
>
> Which then even suggests that _if_ we go this route, this could be
> shared with ARM (and hence become common code again).
>
> Jan
>
We could do the same thing on ARM. For ARM, 2 allocations are done: 1
for the FDT, and
this one for the EFI memory map. Both of these are currently
allocated with EFI allocation
functions, so don't have fixed size limits. If we go with the fixed
size pool, I don't think that 64k
will be enough for the ARM case, as FDTs can be 20-50k in size.
Roy
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 17/18] x86/efi: create new early memory allocator
>>> On 30.01.15 at 18:54, wrote:
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -103,9 +103,35 @@ static void __init relocate_trampoline(unsigned long
> phys)
> *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
> }
>
> +#define __MALLOC_SIZE65536
> +
> +static char __malloc_mem[__MALLOC_SIZE];
> +static char *__malloc_free = NULL;
> +
> +static void __init *__malloc(size_t size)
Please do away with all these prefixing underscores.
> +{
> +void *ptr;
> +
> +/*
> + * Init __malloc_free on runtime. Static initialization
> + * will not work because it puts virtual address there.
> + */
> +if ( __malloc_free == NULL )
> +__malloc_free = __malloc_mem;
> +
> +ptr = __malloc_free;
> +
> +__malloc_free += size;
> +
> +if ( __malloc_free - __malloc_mem > sizeof(__malloc_mem) )
> +blexit(L"Out of static memory\r\n");
> +
> +return ptr;
> +}
You're ignoring alignment requirements here altogether.
> @@ -192,12 +218,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 __malloc(*map_size);
> }
Which then even suggests that _if_ we go this route, this could be
shared with ARM (and hence become common code again).
Jan
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 17/18] x86/efi: create new early memory allocator
There is a problem with place_string() which is used as early memory
allocator. It gets memory chunks starting from start symbol and
going down. Sadly this does not work when Xen is loaded using multiboot2
protocol because start lives on 1 MiB address. So, I tried to use
mem_lower address calculated by GRUB2. However, it 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... :-(((
In case of multiboot2 protocol we need that place_string() only allocate
memory chunk for EFI memory map. However, I think that it should be fixed
instead of making another function used just in one case. I thought about
two 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
this in e820 memory map and map it in Xen virtual address space.
In later case we must also care about conflicts with e.g. crash
kernel regions which could be quite difficult.
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 my 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 want to 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.
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().
Now solution #2 is implemented but maybe we should consider #1 too.
Signed-off-by: Daniel Kiper
---
xen/arch/x86/efi/efi-boot.h | 37 +
xen/arch/x86/setup.c|3 +--
2 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 3a3b4fe..6e98bc8 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -103,9 +103,35 @@ static void __init relocate_trampoline(unsigned long phys)
*(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
}
+#define __MALLOC_SIZE 65536
+
+static char __malloc_mem[__MALLOC_SIZE];
+static char *__malloc_free = NULL;
+
+static void __init *__malloc(size_t size)
+{
+void *ptr;
+
+/*
+ * Init __malloc_free on runtime. Static initialization
+ * will not work because it puts virtual address there.
+ */
+if ( __malloc_free == NULL )
+__malloc_free = __malloc_mem;
+
+ptr = __malloc_free;
+
+__malloc_free += size;
+
+if ( __malloc_free - __malloc_mem > sizeof(__malloc_mem) )
+blexit(L"Out of static memory\r\n");
+
+return ptr;
+}
+
static void __init place_string(u32 *addr, const char *s)
{
-static char *__initdata alloc = start;
+char *alloc = NULL;
if ( s && *s )
{
@@ -113,7 +139,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 = __malloc(len1 + len2);
/*
* Insert new string before already existing one. This is needed
* for options passed on the command line to override options from
@@ -192,12 +218,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 __malloc(*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 711fdb0..aebd010 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -962,8 +962,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_loader
