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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/07/2026 11:19 am, Jan Beulich wrote:
> --- 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

While brk.c has x86 specifics, I'm not sure it's worthy if being in
boot/, rather than simply in x86/.  It's used until mid way through
__start_xen().

> @@ -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;

This means one (or more) subsystem has brk_alloc()'d more than they
reserved.

While returning NULL is probably the best action, I think a
printk_once() is also warranted.

> +
> +    return ptr;
> +}
> +
> +unsigned long __init brk_get_unused_start(void)
> +{
> +    finished = true;

This doesn't get the unused start.  It also terminates the allocator,
and the name needs to reflect that.

But combined with brk_end in the next patch, it's really quite a mess. 
Integrating brk_end properly simplifies this patch too.

> +
> +    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) );
> +
> +    /* 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);

It's inevitable that brk is going to be used in other architectures, and
none of this is x86 specific.

This should be xen/include/brk.h right from the outset, even if that's
all we do in the way of making it generic.

Except, brk_end in patch 2 really needs to live in common/brk.c so it's
probably easier to go in arch-neutral right from the outset and use
"select ARCH_HAS_BRK" or so to allow the arches to opt into using it.

Furthermore, this header needs some commentary.  To do this nicely,
DEFINE_BRK() needs to be introduced here so the comment makes sense.

-----
Early Boot memory allocator.

Subsystems which conditionally need memory prior to the main heap being
set up should use DEFINE_BRK() to reserve BSS space in Xen.

During boot, brk_alloc() allocates memory from the reserved space.  Such
allocations are good for the lifetime of Xen.  Subsystems MUST NOT
brk_alloc() more memory than they reserved.

When the main heap is set up, brk allocations become unavailable. 
Reserved but unallocated space is handed to the main heap, so it doesn't
go to waste.
-----


With just a few sentences, it's now far clearer what brk is and how to
use it.


> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -321,7 +321,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);

This looks fine, but if it's becoming common then it wants to be a macro
in xen.lds.h

~Andrew