| Message ID | 1474667259-27290-8-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On 23/09/2016 22:47, Daniel Kiper wrote: > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 38eb888..2085f35 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -38,6 +38,7 @@ > #include <xen/vmap.h> > #include <xen/libfdt/libfdt.h> > #include <xen/acpi.h> > +#include <xen/efi.h> > #include <asm/alternative.h> > #include <asm/page.h> > #include <asm/current.h> > @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); > > static __used void init_done(void) > { > + free_ebmalloc_unused_mem(); I said no to this on the previous version. And I think Jan suggested a per-arch way to do it. So why is it here? As mentioned, the EFI stub is standalone and should stay as it for now. With this solution, you break this assumption. I explained why it should not be done, and the issue you will run into by doing that. So please don't call a function on ARM where it is known that it will not work. Also, the EFI is not mandatory on ARM, for instance this code will not build on ARM32. I much prefer a TODO comment (around the #ifdef CONFIG_ARM) to say what needs to be done for the time being. Regards,
>>> On 24.09.16 at 01:35, <julien.grall@arm.com> wrote: > Hi Daniel, > > On 23/09/2016 22:47, Daniel Kiper wrote: >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 38eb888..2085f35 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -38,6 +38,7 @@ >> #include <xen/vmap.h> >> #include <xen/libfdt/libfdt.h> >> #include <xen/acpi.h> >> +#include <xen/efi.h> >> #include <asm/alternative.h> >> #include <asm/page.h> >> #include <asm/current.h> >> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); >> >> static __used void init_done(void) >> { >> + free_ebmalloc_unused_mem(); > > I said no to this on the previous version. And I think Jan suggested a > per-arch way to do it. So why is it here? No, I specifically did not. I intended this to be universal, but then I wasn't really aware that on ARM the EFI loader is so much different from x86's. Before coming to a final conclusion I'd really like to understand how you would see dynamic memory allocation to work for pieces of data to be communicated from EFI loader to main Xen. That'll determine whether I'll have to grumblingly accept this code to be x86-specific. Jan
>>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote: > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -79,6 +79,10 @@ static size_t wstrlen(const CHAR16 * s); > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); > static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); > > +#ifndef CONFIG_ARM > +static void *ebmalloc(size_t size); > +#endif Leaving aside the ARM aspect (to be clarified by Julien), is there a reason you need to forward declare this here, rather than moving the whole addition from further down up immediately ahead of the inclusion point of efi-boot.h? Jan
Hi Jan, On 25/09/2016 23:53, Jan Beulich wrote: >>>> On 24.09.16 at 01:35, <julien.grall@arm.com> wrote: >> Hi Daniel, >> >> On 23/09/2016 22:47, Daniel Kiper wrote: >>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >>> index 38eb888..2085f35 100644 >>> --- a/xen/arch/arm/setup.c >>> +++ b/xen/arch/arm/setup.c >>> @@ -38,6 +38,7 @@ >>> #include <xen/vmap.h> >>> #include <xen/libfdt/libfdt.h> >>> #include <xen/acpi.h> >>> +#include <xen/efi.h> >>> #include <asm/alternative.h> >>> #include <asm/page.h> >>> #include <asm/current.h> >>> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); >>> >>> static __used void init_done(void) >>> { >>> + free_ebmalloc_unused_mem(); >> >> I said no to this on the previous version. And I think Jan suggested a >> per-arch way to do it. So why is it here? > > No, I specifically did not. I intended this to be universal, but then I > wasn't really aware that on ARM the EFI loader is so much different > from x86's. > > Before coming to a final conclusion I'd really like to understand how > you would see dynamic memory allocation to work for pieces of data > to be communicated from EFI loader to main Xen. That'll determine > whether I'll have to grumblingly accept this code to be x86-specific. In the current state, all the communication from EFI loader to main Xen should be done via the device-tree or the data have to be in init section (bss is zeroed when leaving the EFI stub and before entering to Xen). I am not against changing this behavior, however in the current state this will not work at all. The call to the function will be misleading, hence why I suggested a TODO for the time-being (for now the code is only compiled on x86, anyway). Regards,
>>> On 26.09.16 at 22:01, <julien.grall@arm.com> wrote: > On 25/09/2016 23:53, Jan Beulich wrote: >>>>> On 24.09.16 at 01:35, <julien.grall@arm.com> wrote: >>> On 23/09/2016 22:47, Daniel Kiper wrote: >>>> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); >>>> >>>> static __used void init_done(void) >>>> { >>>> + free_ebmalloc_unused_mem(); >>> >>> I said no to this on the previous version. And I think Jan suggested a >>> per-arch way to do it. So why is it here? >> >> No, I specifically did not. I intended this to be universal, but then I >> wasn't really aware that on ARM the EFI loader is so much different >> from x86's. >> >> Before coming to a final conclusion I'd really like to understand how >> you would see dynamic memory allocation to work for pieces of data >> to be communicated from EFI loader to main Xen. That'll determine >> whether I'll have to grumblingly accept this code to be x86-specific. > > In the current state, all the communication from EFI loader to main Xen > should be done via the device-tree or the data have to be in init > section (bss is zeroed when leaving the EFI stub and before entering to > Xen). This late .bss zeroing is something which, as Daniel had already suggested, could (and imo should) be avoided (just like he already needs to do for x86 in his series). > I am not against changing this behavior, however in the current state > this will not work at all. The call to the function will be misleading, > hence why I suggested a TODO for the time-being (for now the code is > only compiled on x86, anyway). This doesn't really help make progress with the patch here. The question isn't so much what current behavior should be, but what sane behavior would be going forward. Again - we're needing your input mainly to decide whether to put this allocator in common or x86-specific code (with the goal of not having to move it later if at all possible). Jan
On Mon, Sep 26, 2016 at 07:37:45AM -0600, Jan Beulich wrote: > >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote: > > --- a/xen/common/efi/boot.c > > +++ b/xen/common/efi/boot.c > > @@ -79,6 +79,10 @@ static size_t wstrlen(const CHAR16 * s); > > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); > > static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); > > > > +#ifndef CONFIG_ARM > > +static void *ebmalloc(size_t size); > > +#endif > > Leaving aside the ARM aspect (to be clarified by Julien), is there a > reason you need to forward declare this here, rather than moving > the whole addition from further down up immediately ahead of the > inclusion point of efi-boot.h? If you wish I can move ebmalloc code before efi-boot.h inclusion. There is a pretty good chance that it will work here. Daniel
Hi Jan, On 27/09/2016 01:06, Jan Beulich wrote: >>>> On 26.09.16 at 22:01, <julien.grall@arm.com> wrote: >> On 25/09/2016 23:53, Jan Beulich wrote: >>>>>> On 24.09.16 at 01:35, <julien.grall@arm.com> wrote: >>>> On 23/09/2016 22:47, Daniel Kiper wrote: >>>>> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); >>>>> >>>>> static __used void init_done(void) >>>>> { >>>>> + free_ebmalloc_unused_mem(); >>>> >>>> I said no to this on the previous version. And I think Jan suggested a >>>> per-arch way to do it. So why is it here? >>> >>> No, I specifically did not. I intended this to be universal, but then I >>> wasn't really aware that on ARM the EFI loader is so much different >>> from x86's. >>> >>> Before coming to a final conclusion I'd really like to understand how >>> you would see dynamic memory allocation to work for pieces of data >>> to be communicated from EFI loader to main Xen. That'll determine >>> whether I'll have to grumblingly accept this code to be x86-specific. >> >> In the current state, all the communication from EFI loader to main Xen >> should be done via the device-tree or the data have to be in init >> section (bss is zeroed when leaving the EFI stub and before entering to >> Xen). > > This late .bss zeroing is something which, as Daniel had already > suggested, could (and imo should) be avoided (just like he already > needs to do for x86 in his series). I am happy to see a such patch for ARM. > >> I am not against changing this behavior, however in the current state >> this will not work at all. The call to the function will be misleading, >> hence why I suggested a TODO for the time-being (for now the code is >> only compiled on x86, anyway). > > This doesn't really help make progress with the patch here. The > question isn't so much what current behavior should be, but what > sane behavior would be going forward. Again - we're needing your > input mainly to decide whether to put this allocator in common or > x86-specific code (with the goal of not having to move it later if at > all possible). Unifying the behavior between x86 and ARM would be the best. Regards,
>>> On 27.09.16 at 19:49, <daniel.kiper@oracle.com> wrote: > On Mon, Sep 26, 2016 at 07:37:45AM -0600, Jan Beulich wrote: >> >>> On 23.09.16 at 23:47, <daniel.kiper@oracle.com> wrote: >> > --- a/xen/common/efi/boot.c >> > +++ b/xen/common/efi/boot.c >> > @@ -79,6 +79,10 @@ static size_t wstrlen(const CHAR16 * s); >> > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); >> > static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); >> > >> > +#ifndef CONFIG_ARM >> > +static void *ebmalloc(size_t size); >> > +#endif >> >> Leaving aside the ARM aspect (to be clarified by Julien), is there a >> reason you need to forward declare this here, rather than moving >> the whole addition from further down up immediately ahead of the >> inclusion point of efi-boot.h? > > If you wish I can move ebmalloc code before efi-boot.h inclusion. > There is a pretty good chance that it will work here. Yes please, the more that Julien now confirmed that he's fine with the code staying in common/efi/boot.c. Jan
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 38eb888..2085f35 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -38,6 +38,7 @@ #include <xen/vmap.h> #include <xen/libfdt/libfdt.h> #include <xen/acpi.h> +#include <xen/efi.h> #include <asm/alternative.h> #include <asm/page.h> #include <asm/current.h> @@ -66,6 +67,7 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); static __used void init_done(void) { + free_ebmalloc_unused_mem(); free_init_memory(); startup_cpu_idle_loop(); } diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 388c4ea..62c010e 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -114,7 +114,7 @@ static void __init relocate_trampoline(unsigned long phys) static void __init place_string(u32 *addr, const char *s) { - static char *__initdata alloc = start; + char *alloc = NULL; if ( s && *s ) { @@ -122,7 +122,7 @@ static void __init place_string(u32 *addr, const char *s) const char *old = (char *)(long)*addr; size_t len2 = *addr ? strlen(old) + 1 : 0; - alloc -= len1 + len2; + alloc = ebmalloc(len1 + len2); /* * Insert new string before already existing one. This is needed * for options passed on the command line to override options from @@ -205,12 +205,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) { - place_string(&mbi.mem_upper, NULL); - mbi.mem_upper -= map_size; - mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR); - if ( mbi.mem_upper < xen_phys_start ) - return NULL; - return (void *)(long)mbi.mem_upper; + return ebmalloc(map_size); } static void __init efi_arch_pre_exit_boot(void) diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 4158124..bc9919c 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -9,6 +9,8 @@ bool efi_enabled(unsigned 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 06f3970..5b17783 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_LOADER) ? mbi->mem_upper : __pa(&_start), - __pa(&_end)); + reserve_e820_ram(&boot_e820, __pa(&_start), __pa(&_end)); /* Late kexec reservation (dynamic start address). */ kexec_reserve_area(&boot_e820); diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 1ef5d0b..91df9b5 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -79,6 +79,10 @@ static size_t wstrlen(const CHAR16 * s); static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); +#ifndef CONFIG_ARM +static void *ebmalloc(size_t size); +#endif + static const EFI_BOOT_SERVICES *__initdata efi_bs; static UINT32 __initdata efi_bs_revision; static EFI_HANDLE __initdata efi_ih; @@ -577,6 +581,40 @@ static char *__init get_value(const struct file *cfg, const char *section, return NULL; } +#define EBMALLOC_SIZE MB(1) + +static char __section(".bss.page_aligned") __aligned(PAGE_SIZE) + ebmalloc_mem[EBMALLOC_SIZE]; +static unsigned long __initdata ebmalloc_allocated; + +#ifndef CONFIG_ARM +/* EFI boot allocator. */ +static void __init *ebmalloc(size_t size) +{ + void *ptr = ebmalloc_mem + ebmalloc_allocated; + + ebmalloc_allocated += (size + sizeof(void *) - 1) & ~(sizeof(void *) - 1); + + if ( ebmalloc_allocated > sizeof(ebmalloc_mem) ) + blexit(L"Out of static memory\r\n"); + + return ptr; +} +#endif + +void __init free_ebmalloc_unused_mem(void) +{ + unsigned long start, end; + + start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated); + end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem); + + destroy_xen_mappings(start, end); + init_xenheap_pages(__pa(start), __pa(end)); + + printk(XENLOG_INFO "Freed %lukB unused BSS memory\n", (end - start) >> 10); +} + static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { efi_ih = ImageHandle; diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h index 68c68a8..6c7f601 100644 --- a/xen/include/xen/efi.h +++ b/xen/include/xen/efi.h @@ -32,6 +32,7 @@ struct xenpf_efi_runtime_call; struct compat_pf_efi_runtime_call; bool efi_enabled(unsigned 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(). New allocator is quite generic and can be used on ARM platforms too. So, let's enable most of its machinery on them. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v7 - suggestions/fixes: - enable most of ebmalloc machinery on ARM platforms (suggested by Jan Beulich), - remove unneeded cast (suggested by Jan Beulich), - wrap long line (suggested by Jan Beulich), - improve commit message. v6 - suggestions/fixes: - optimize ebmalloc allocator, - move ebmalloc machinery to xen/common/efi/boot.c (suggested by Jan Beulich), - enforce PAGE_SIZE ebmalloc_mem alignment (suggested by Jan Beulich), - ebmalloc() must allocate properly aligned memory regions (suggested by Jan Beulich), - printk() should use XENLOG_INFO (suggested by Jan Beulich). v4 - suggestions/fixes: - move from #2 solution to #2a solution, - improve commit message. --- xen/arch/arm/setup.c | 2 ++ xen/arch/x86/efi/efi-boot.h | 11 +++-------- xen/arch/x86/efi/stub.c | 2 ++ xen/arch/x86/setup.c | 5 +++-- xen/common/efi/boot.c | 38 ++++++++++++++++++++++++++++++++++++++ xen/include/xen/efi.h | 1 + 6 files changed, 49 insertions(+), 10 deletions(-)