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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2026 19:59, Andrew Cooper wrote:
> On 27/07/2026 11:19 am, Jan Beulich wrote:
>> --- /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().

As discussed, I'll move this to common/brk.c right away, abstracting out
the x86 specifics.

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

Hmm, yes, just that there's a quirk to work around: The static that
printk_once() uses gets in the way of the file getting compiled into
brk.init.o.

>> +
>> +    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.

brk_cease() then.

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

No, brk_end really is an x86-specific helper variable. I'd like to keep
that as it is.

>> --- /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.

Sure, I can move introduction of the macro here.

Jan

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