Re: [PATCH 2/3] x86/EFI: replace ebmalloc()

[Marek Marczykowski <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 3063 bytes --]

On Thu, Nov 13, 2025 at 12:09:37PM +0100, Jan Beulich wrote:
> Use the new brk_alloc() instead, with ebmalloc() merely being a thin
> wrapper.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I'm not quite certain whether we ought to permit non-page-granular
> reservations. The in-memory image being somewhat larger due to possibly
> excessive padding isn't really a big problem, I think.

My grep says ebmalloc is used in just two places:
1. For efi_memmap (via efi_arch_allocate_mmap_buffer())
2. For various cmdline options and module names (via aplace_string())

The second one is probably undesirable to allocate full page for each
one. On the other hand, the current approach (putting small allocations
at the same page as an earlier page-aligned one) also has its issues -
see comments on 3/3 patch.

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -262,7 +262,7 @@ quiet_cmd_obj_init_o = INIT_O  $@
>  define cmd_obj_init_o
>      $(OBJDUMP) -h $< | while read idx name sz rest; do \
>          case "$$name" in \
> -        .*.local) ;; \
> +        .*.local|.bss..brk*) ;; \
>          .text|.text.*|.data|.data.*|.bss|.bss.*) \
>              test $$(echo $$sz | sed 's,00*,0,') != 0 || continue; \
>              echo "Error: size of $<:$$name is 0x$$sz" >&2; \
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -10,6 +10,7 @@
>  #include <xen/vga.h>
>  
>  #include <asm/boot-helpers.h>
> +#include <asm/brk.h>
>  #include <asm/e820.h>
>  #include <asm/edd.h>
>  #include <asm/microcode.h>
> @@ -119,6 +120,18 @@ static void __init relocate_trampoline(u
>      reloc_trampoline64();
>  }
>  
> +DEFINE_BRK(efi, MB(1));
> +
> +static void *__init ebmalloc(size_t size)
> +{
> +    void *ptr = brk_alloc(size);
> +
> +    if ( !ptr )
> +        blexit(L"Out of BRK memory\r\n");
> +
> +    return ptr;
> +}
> +
>  static void __init place_string(u32 *addr, const char *s)
>  {
>      char *alloc = NULL;
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -41,12 +41,4 @@ void __init noreturn efi_multiboot2(EFI_
>  
>  void __init efi_init_memory(void) { }
>  
> -bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
> -{
> -    /* FIXME: Simplify once the call here with two NULLs goes away. */
> -    if ( start || end )
> -        *start = *end = (unsigned long)_end;
> -    return false;
> -}
> -
>  void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { }
> --- a/xen/arch/x86/include/asm/brk.h
> +++ b/xen/arch/x86/include/asm/brk.h
> @@ -2,6 +2,10 @@
>  
>  #include <xen/types.h>
>  
> +#define DEFINE_BRK(var, size) \
> +    static char __section(".bss..brk.page_aligned") __aligned(PAGE_SIZE) \
> +        __used var ## _brk_[size]

This chunk belongs to the previous patch I think.

> +
>  void *brk_alloc(size_t size);
>  unsigned long brk_get_unused_start(void);
>  void brk_free_unused(void);

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]