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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2025 18:41, Marek Marczykowski-Górecki wrote:
> On Thu, Nov 13, 2025 at 12:08:18PM +0100, Jan Beulich wrote:
>> --- /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() ?

Getting too long for my taste, and a caller obtaining this value kind of
needs to understand that either what it gets back is stale the moment it
uses the result, or (for it to not be stale) no further changes (i.e.
allocations) are permitted afterwards.

>> +{
>> +    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?

Well, it took me a while to figure _some_ reasonably reliable way. I'm
all ears towards better approaches.

Jan