| Message ID | 1471646606-28519-13-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> wrote: > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -103,9 +103,56 @@ static void __init relocate_trampoline(unsigned long phys) > *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; > } > > +#define EBMALLOC_SIZE MB(1) > + > +static char __section(".bss.page_aligned") ebmalloc_mem[EBMALLOC_SIZE]; You need to specify the alignment of the object (using the relatively new __aligned() construct). > +static char __initdata *ebmalloc_free = NULL; > + > +/* EFI boot allocator. */ > +static void __init *ebmalloc(size_t size) > +{ > + void *ptr; > + > + /* > + * Init ebmalloc_free on runtime. Static initialization > + * will not work because it puts virtual address there. > + */ I don't understand this static allocation comment: We have this issue elsewhere (and use bootsym() as needed), and we do not have this issue at all in xen.efi (which this code also gets built for). So I think at the very least the comment needs improvement. And then, if static initialization indeed can't be used, then a static symbol's initializer of NULL is pointless and hence should be omitted. > + if ( ebmalloc_free == NULL ) > + ebmalloc_free = ebmalloc_mem; > + > + ptr = ebmalloc_free; > + > + ebmalloc_free += size; No minimal (at least pointer size) alignment getting enforced somewhere here? > +void __init free_ebmalloc_unused_mem(void) > +{ > + unsigned long start, end; > + > + if ( ebmalloc_free ) > + { > + start = (unsigned long)ebmalloc_free - xen_phys_start; > + start = PAGE_ALIGN(start + XEN_VIRT_START); > + } > + else > + start = (unsigned long)ebmalloc_mem; > + > + end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem); > + > + destroy_xen_mappings(start, end); > + init_xenheap_pages(__pa(start), __pa(end)); > + > + printk("Freed %lukB unused BSS memory\n", (end - start) >> 10); XENLOG_INFO And then - wouldn't this better go into xen/common/efi/boot.c, even if ARM64 does not have a use for it right away? The code certainly isn't really x86-specific. Jan
On Mon, Sep 05, 2016 at 06:33:57AM -0600, Jan Beulich wrote: > >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> wrote: [...] > > +static char __initdata *ebmalloc_free = NULL; > > + > > +/* EFI boot allocator. */ > > +static void __init *ebmalloc(size_t size) > > +{ > > + void *ptr; > > + > > + /* > > + * Init ebmalloc_free on runtime. Static initialization > > + * will not work because it puts virtual address there. > > + */ > > I don't understand this static allocation comment: We have this issue > elsewhere (and use bootsym() as needed), and we do not have this > issue at all in xen.efi (which this code also gets built for). So I think at > the very least the comment needs improvement. And then, if static > initialization indeed can't be used, then a static symbol's initializer of > NULL is pointless and hence should be omitted. You have to remember that xen/arch/x86/efi/efi-boot.h stuff is build into xen.efi and xen.gz. Of course xen.efi with static char __initdata *ebmalloc_free = ebmalloc_mem; works, however, xen.gz does not. Sadly, I have not found simpler solution for that issue, so, I do initialization during runtime. > > + if ( ebmalloc_free == NULL ) > > + ebmalloc_free = ebmalloc_mem; > > + > > + ptr = ebmalloc_free; > > + > > + ebmalloc_free += size; > > No minimal (at least pointer size) alignment getting enforced > somewhere here? For what? > > +void __init free_ebmalloc_unused_mem(void) > > +{ > > + unsigned long start, end; > > + > > + if ( ebmalloc_free ) > > + { > > + start = (unsigned long)ebmalloc_free - xen_phys_start; > > + start = PAGE_ALIGN(start + XEN_VIRT_START); > > + } > > + else > > + start = (unsigned long)ebmalloc_mem; > > + > > + end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem); > > + > > + destroy_xen_mappings(start, end); > > + init_xenheap_pages(__pa(start), __pa(end)); > > + > > + printk("Freed %lukB unused BSS memory\n", (end - start) >> 10); > > XENLOG_INFO OK. > And then - wouldn't this better go into xen/common/efi/boot.c, > even if ARM64 does not have a use for it right away? The code > certainly isn't really x86-specific. Sure thing. However, if it is not used by ARM64 then I think ebmalloc stuff should not be moved to xen/common/efi/boot.c. Daniel
>>> On 07.09.16 at 14:05, <daniel.kiper@oracle.com> wrote: > On Mon, Sep 05, 2016 at 06:33:57AM -0600, Jan Beulich wrote: >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> wrote: >> > +static char __initdata *ebmalloc_free = NULL; >> > + >> > +/* EFI boot allocator. */ >> > +static void __init *ebmalloc(size_t size) >> > +{ >> > + void *ptr; >> > + >> > + /* >> > + * Init ebmalloc_free on runtime. Static initialization >> > + * will not work because it puts virtual address there. >> > + */ >> >> I don't understand this static allocation comment: We have this issue >> elsewhere (and use bootsym() as needed), and we do not have this >> issue at all in xen.efi (which this code also gets built for). So I think at >> the very least the comment needs improvement. And then, if static >> initialization indeed can't be used, then a static symbol's initializer of >> NULL is pointless and hence should be omitted. > > You have to remember that xen/arch/x86/efi/efi-boot.h stuff is build > into xen.efi and xen.gz. Of course xen.efi with > > static char __initdata *ebmalloc_free = ebmalloc_mem; > > works, however, xen.gz does not. Sadly, I have not found simpler > solution for that issue, so, I do initialization during runtime. Which all is in line with my request of improving the comment. >> > + if ( ebmalloc_free == NULL ) >> > + ebmalloc_free = ebmalloc_mem; >> > + >> > + ptr = ebmalloc_free; >> > + >> > + ebmalloc_free += size; >> >> No minimal (at least pointer size) alignment getting enforced >> somewhere here? > > For what? To avoid the penalty unaligned accesses incur? And that's alongside the fact that it's simply bad practice to knowingly but without actual need cause unaligned accesses even if they work fine. >> And then - wouldn't this better go into xen/common/efi/boot.c, >> even if ARM64 does not have a use for it right away? The code >> certainly isn't really x86-specific. > > Sure thing. However, if it is not used by ARM64 then I think ebmalloc > stuff should not be moved to xen/common/efi/boot.c. Being architecture independent it has all reasons to be moved there. Agreed there may be compiler warnings for these then being unused static functions, but I'd rather see this code get #ifdef-ed out for ARM for the time being than it needing to be moved over later on. And of course a question to be asked first is whether in fact there is something in common or ARM specific code that could benefit from using this new allocator, if you already introduce it. Jan
On Wed, Sep 07, 2016 at 08:01:31AM -0600, Jan Beulich wrote: > >>> On 07.09.16 at 14:05, <daniel.kiper@oracle.com> wrote: > > On Mon, Sep 05, 2016 at 06:33:57AM -0600, Jan Beulich wrote: > >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> wrote: > >> > +static char __initdata *ebmalloc_free = NULL; > >> > + > >> > +/* EFI boot allocator. */ > >> > +static void __init *ebmalloc(size_t size) > >> > +{ > >> > + void *ptr; > >> > + > >> > + /* > >> > + * Init ebmalloc_free on runtime. Static initialization > >> > + * will not work because it puts virtual address there. > >> > + */ > >> > >> I don't understand this static allocation comment: We have this issue > >> elsewhere (and use bootsym() as needed), and we do not have this > >> issue at all in xen.efi (which this code also gets built for). So I think at > >> the very least the comment needs improvement. And then, if static > >> initialization indeed can't be used, then a static symbol's initializer of > >> NULL is pointless and hence should be omitted. > > > > You have to remember that xen/arch/x86/efi/efi-boot.h stuff is build > > into xen.efi and xen.gz. Of course xen.efi with > > > > static char __initdata *ebmalloc_free = ebmalloc_mem; > > > > works, however, xen.gz does not. Sadly, I have not found simpler > > solution for that issue, so, I do initialization during runtime. > > Which all is in line with my request of improving the comment. OK. > >> > + if ( ebmalloc_free == NULL ) > >> > + ebmalloc_free = ebmalloc_mem; > >> > + > >> > + ptr = ebmalloc_free; > >> > + > >> > + ebmalloc_free += size; > >> > >> No minimal (at least pointer size) alignment getting enforced > >> somewhere here? > > > > For what? > > To avoid the penalty unaligned accesses incur? And that's alongside > the fact that it's simply bad practice to knowingly but without actual > need cause unaligned accesses even if they work fine. I expected that but I do not think it is very important here. Anyway, I am still not sure why you say "at least pointer size". Because sizeof(void *) assures proper alignment on any architecture? Additionally, will this alignment sufficiently replace alignment provided by current efi_arch_allocate_mmap_buffer() implementation? > >> And then - wouldn't this better go into xen/common/efi/boot.c, > >> even if ARM64 does not have a use for it right away? The code > >> certainly isn't really x86-specific. > > > > Sure thing. However, if it is not used by ARM64 then I think ebmalloc > > stuff should not be moved to xen/common/efi/boot.c. > > Being architecture independent it has all reasons to be moved > there. Agreed there may be compiler warnings for these then > being unused static functions, but I'd rather see this code get > #ifdef-ed out for ARM for the time being than it needing to be OK. > moved over later on. And of course a question to be asked first > is whether in fact there is something in common or ARM specific > code that could benefit from using this new allocator, if you > already introduce it. I think that it is x86 specific stuff and should stay here as is. However, potentially it can be common allocator for both architectures. Though I do not see gains on ARM itself. Daniel
>>> On 08.09.16 at 10:29, <daniel.kiper@oracle.com> wrote: > On Wed, Sep 07, 2016 at 08:01:31AM -0600, Jan Beulich wrote: >> >>> On 07.09.16 at 14:05, <daniel.kiper@oracle.com> wrote: >> > On Mon, Sep 05, 2016 at 06:33:57AM -0600, Jan Beulich wrote: >> >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> wrote: >> >> > + if ( ebmalloc_free == NULL ) >> >> > + ebmalloc_free = ebmalloc_mem; >> >> > + >> >> > + ptr = ebmalloc_free; >> >> > + >> >> > + ebmalloc_free += size; >> >> >> >> No minimal (at least pointer size) alignment getting enforced >> >> somewhere here? >> > >> > For what? >> >> To avoid the penalty unaligned accesses incur? And that's alongside >> the fact that it's simply bad practice to knowingly but without actual >> need cause unaligned accesses even if they work fine. > > I expected that but I do not think it is very important here. Anyway, > I am still not sure why you say "at least pointer size". Because > sizeof(void *) assures proper alignment on any architecture? Yes, this gives (on "normal" architectures at least) machine word size alignment, which commonly is good enough for everything except SIMD data (or things similar to it). > Additionally, will this alignment sufficiently replace alignment > provided by current efi_arch_allocate_mmap_buffer() implementation? Just compare __alignof__(EFI_MEMORY_DESCRIPTOR) and __alignof__(void *). Jan
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 1fa9e47..3f87b7c 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -103,9 +103,56 @@ static void __init relocate_trampoline(unsigned long phys) *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; } +#define EBMALLOC_SIZE MB(1) + +static char __section(".bss.page_aligned") ebmalloc_mem[EBMALLOC_SIZE]; +static char __initdata *ebmalloc_free = NULL; + +/* EFI boot allocator. */ +static void __init *ebmalloc(size_t size) +{ + void *ptr; + + /* + * Init ebmalloc_free on runtime. Static initialization + * will not work because it puts virtual address there. + */ + if ( ebmalloc_free == NULL ) + ebmalloc_free = ebmalloc_mem; + + ptr = ebmalloc_free; + + ebmalloc_free += size; + + if ( ebmalloc_free - ebmalloc_mem > sizeof(ebmalloc_mem) ) + blexit(L"Out of static memory\r\n"); + + return ptr; +} + +void __init free_ebmalloc_unused_mem(void) +{ + unsigned long start, end; + + if ( ebmalloc_free ) + { + start = (unsigned long)ebmalloc_free - xen_phys_start; + start = PAGE_ALIGN(start + XEN_VIRT_START); + } + else + start = (unsigned long)ebmalloc_mem; + + end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem); + + destroy_xen_mappings(start, end); + init_xenheap_pages(__pa(start), __pa(end)); + + printk("Freed %lukB unused BSS memory\n", (end - start) >> 10); +} + static void __init place_string(u32 *addr, const char *s) { - static char *__initdata alloc = start; + char *alloc = NULL; if ( s && *s ) { @@ -113,7 +160,7 @@ static void __init place_string(u32 *addr, const char *s) const char *old = (char *)(long)*addr; size_t len2 = *addr ? strlen(old) + 1 : 0; - alloc -= len1 + len2; + alloc = ebmalloc(len1 + len2); /* * Insert new string before already existing one. This is needed * for options passed on the command line to override options from @@ -196,12 +243,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) { - place_string(&mbi.mem_upper, NULL); - mbi.mem_upper -= map_size; - mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR); - if ( mbi.mem_upper < xen_phys_start ) - return NULL; - return (void *)(long)mbi.mem_upper; + return ebmalloc(map_size); } static void __init efi_arch_pre_exit_boot(void) diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index f087023..f7bc042 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -9,6 +9,8 @@ bool_t efi_enabled(int feature) return false; } +void __init free_ebmalloc_unused_mem(void) { } + void __init efi_init_memory(void) { } void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { } diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 360ca59..bc48351 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -520,6 +520,8 @@ static void noinline init_done(void) system_state = SYS_STATE_active; + free_ebmalloc_unused_mem(); + /* MUST be done prior to removing .init data. */ unregister_init_virtual_region(); @@ -1080,8 +1082,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) if ( !xen_phys_start ) panic("Not enough memory to relocate Xen."); - reserve_e820_ram(&boot_e820, efi_enabled(EFI_BOOT) ? mbi->mem_upper : __pa(&_start), - __pa(&_end)); + reserve_e820_ram(&boot_e820, __pa(&_start), __pa(&_end)); /* Late kexec reservation (dynamic start address). */ kexec_reserve_area(&boot_e820); diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h index 8372ae2..147a0e8 100644 --- a/xen/include/xen/efi.h +++ b/xen/include/xen/efi.h @@ -33,6 +33,7 @@ struct xenpf_efi_runtime_call; struct compat_pf_efi_runtime_call; bool_t efi_enabled(int feature); +void free_ebmalloc_unused_mem(void); void efi_init_memory(void); bool_t efi_rs_using_pgtables(void); unsigned long efi_get_time(void);
There is a problem with place_string() which is used as early memory allocator. It gets memory chunks starting from start symbol and goes down. Sadly this does not work when Xen is loaded using multiboot2 protocol because then the start lives on 1 MiB address and we should not allocate a memory from below of it. So, I tried to use mem_lower address calculated by GRUB2. However, this solution works only on some machines. There are machines in the wild (e.g. Dell PowerEdge R820) which uses first ~640 KiB for boot services code or data... :-((( Hence, we need new memory allocator for Xen EFI boot code which is quite simple and generic and could be used by place_string() and efi_arch_allocate_mmap_buffer(). I think about following solutions: 1) We could use native EFI allocation functions (e.g. AllocatePool() or AllocatePages()) to get memory chunk. However, later (somewhere in __start_xen()) we must copy its contents to safe place or reserve it in e820 memory map and map it in Xen virtual address space. This means that the code referring to Xen command line, loaded modules and EFI memory map, mostly in __start_xen(), will be further complicated and diverge from legacy BIOS cases. Additionally, both former things have to be placed below 4 GiB because their addresses are stored in multiboot_info_t structure which has 32-bit relevant members. 2) We may allocate memory area statically somewhere in Xen code which could be used as memory pool for early dynamic allocations. Looks quite simple. Additionally, it would not depend on EFI at all and could be used on legacy BIOS platforms if we need it. However, we must carefully choose size of this pool. We do not want increase Xen binary size too much and waste too much memory but also we must fit at least memory map on x86 EFI platforms. As I saw on small machine, e.g. IBM System x3550 M2 with 8 GiB RAM, memory map may contain more than 200 entries. Every entry on x86-64 platform is 40 bytes in size. So, it means that we need more than 8 KiB for EFI memory map only. Additionally, if we use this memory pool for Xen and modules command line storage (it would be used when xen.efi is executed as EFI application) then we should add, I think, about 1 KiB. In this case, to be on safe side, we should assume at least 64 KiB pool for early memory allocations. Which is about 4 times of our earlier calculations. However, during discussion on Xen-devel Jan Beulich suggested that just in case we should use 1 MiB memory pool like it is in original place_string() implementation. So, let's use 1 MiB as it was proposed. If we think that we should not waste unallocated memory in the pool on running system then we can mark this region as __initdata and move all required data to dynamically allocated places somewhere in __start_xen(). 2a) We could put memory pool into .bss.page_aligned section. Then allocate memory chunks starting from the lowest address. After init phase we can free unused portion of the memory pool as in case of .init.text or .init.data sections. This way we do not need to allocate any space in image file and freeing of unused area in the memory pool is very simple. Now #2a solution is implemented because it is quite simple and requires limited number of changes, especially in __start_xen(). Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v4 - suggestions/fixes: - move from #2 solution to #2a solution, - improve commit message. --- xen/arch/x86/efi/efi-boot.h | 58 +++++++++++++++++++++++++++++++++++++------ xen/arch/x86/efi/stub.c | 2 ++ xen/arch/x86/setup.c | 5 ++-- xen/include/xen/efi.h | 1 + 4 files changed, 56 insertions(+), 10 deletions(-)