Re: [PATCH 1/3] x86: introduce "brk" allocator

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

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

On Thu, Nov 13, 2025 at 12:08:18PM +0100, Jan Beulich wrote:
> ... to replace ebmalloc(), and then to find further use(s) to allow
> recovering memory which is needed very early (and hence needs setting up
> statically), but may not fully be used (or not used at all).
> 
> Note that unlike free_ebmalloc_unused_mem(), brk_free_unused() (once
> other code is converted) will be able to free part of the BRK space even
> in the xen.efi case. That would happen if BRK space extends across a 2Mb
> boundary, and actual use stops before that boundary.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changing setup.c's reserve_e820_ram() uses would be cumbersome when done
> right here. That'll be done when ebmalloc() is replaced, and hence
> what's there can also simply be replaced.
> 
> The xen.efi detection may want separating out into a helper.
> 
> When linking xen.efi, ld produces a base relocation for the reference to
> __subsystem__, which is wrong (that's an absolute symbol, after all).
> While that will need fixing there, it does no harm for our purposes.
> 
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,4 +1,5 @@
>  obj-bin-y += head.o
> +obj-bin-y += brk.init.o
>  obj-bin-y += built-in-32.o
>  obj-bin-y += $(obj64)
>  
> --- /dev/null
> +++ b/xen/arch/x86/boot/brk.c
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/efi.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +#include <xen/page-defs.h>
> +
> +#include <asm/brk.h>
> +
> +extern char __brk_start[];
> +extern const char __bss_end[];
> +
> +static unsigned long __initdata allocated;
> +static bool __initdata finished;
> +
> +void *__init brk_alloc(size_t size)
> +{
> +    void *ptr = __brk_start + allocated;
> +
> +    if ( finished )
> +        return NULL;
> +
> +    /* Allocations PAGE_SIZE and up will be page-aligned. */
> +    if ( size >= PAGE_SIZE )
> +        allocated = ROUNDUP(allocated, PAGE_SIZE);
> +
> +    allocated += ROUNDUP(size, sizeof(void *));
> +
> +    if ( allocated > __bss_end - __brk_start )
> +        return NULL;
> +
> +    return ptr;
> +}
> +
> +unsigned long __init brk_get_unused_start(void)

It's a bit unintuitive for brk_get_* to have this significant side
effect. Maybe name it brk_finalize_get_unused_start() ?

> +{
> +    finished = true;
> +
> +    allocated = ROUNDUP(allocated, PAGE_SIZE);
> +
> +    return (unsigned long)__brk_start + allocated;
> +}
> +
> +void __init brk_free_unused(void)
> +{
> +    unsigned long start = brk_get_unused_start(),
> +                  end = (unsigned long)__bss_end;
> +    unsigned int subsys;
> +
> +    /*
> +     * Only xen.efi will have the symbol __subsystem__ available, and it'll
> +     * be non-zero (10) there.  In ELF the symbol will be undefined, and
> +     * hence zero will be loaded into the register.
> +     */
> +    asm ( ".weak __subsystem__; mov $__subsystem__, %0" : "=r" (subsys) );

Is this really the best way to detect xen.efi?

> +
> +    /* using_2M_mapping() isn't available here. */
> +    if ( IS_ENABLED(CONFIG_XEN_ALIGN_2M) || subsys )
> +        start = PAGE_ALIGN_2M(start);
> +
> +    if ( start >= end )
> +        return;
> +
> +    destroy_xen_mappings(start, PAGE_ALIGN_2M(end));
> +
> +    /*
> +     * By reserving needed space early in the E820 map, excess space gets freed
> +     * way before we make it here. Don't free the range a 2nd time.
> +     */
> +
> +    printk(XENLOG_INFO "Freed %lukB unused BRK memory\n", (end - start) >> 10);
> +}
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/brk.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/types.h>
> +
> +void *brk_alloc(size_t size);
> +unsigned long brk_get_unused_start(void);
> +void brk_free_unused(void);
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -331,7 +331,11 @@ SECTIONS
>         __bss_start = .;
>         *(.bss.page_aligned*)
>         PERCPU_BSS
> -       *(.bss .bss.*)
> +       *(.bss .bss.[a-zA-Z0-9_]*)
> +       . = ALIGN(PAGE_SIZE);
> +       __brk_start = .;
> +       *(.bss..brk.page_aligned*)
> +       *(.bss..brk*)
>         . = ALIGN(POINTER_ALIGN);
>         __bss_end = .;
>    } PHDR(text)
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -112,6 +112,7 @@
>  #include <xen/vmap.h>
>  #include <xen/xmalloc.h>
>  
> +#include <asm/brk.h>
>  #include <asm/e820.h>
>  #include <asm/fixmap.h>
>  #include <asm/flushtlb.h>
> @@ -337,6 +338,8 @@ void __init arch_init_memory(void)
>  
>      efi_init_memory();
>  
> +    brk_free_unused();
> +
>  #ifndef NDEBUG
>      if ( highmem_start )
>      {
> 
> 

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

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