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

[George Dunlap <dunlapg@xxxxxxxxx>]

On Wed, Oct 14, 2026 at 2:02 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> ... to replace (on x86) 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 be 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 on x86. That would happen if BRK space extends across
> a 2Mb boundary, and actual use stops before that boundary.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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.
>
> When linking xen.efi, GNU ld prior to 2.46 produces a base relocation for
> the reference to __subsystem__, which is wrong (that's an absolute symbol,
> after all). That does no harm for our purposes, though.
> ---
> v3: Move to common code and introduce BSS_WITH_BRK. Rename
>     brk_get_unused_start() to brk_cease(). Add header guard to brk.h. In
>     brk_alloc() don't calculate result too early. Add printk_once_init(),
>     for use in brk_alloc(). Move introduction of DEFINE_BRK() here; add
>     comment.
>
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -15,6 +15,7 @@ config X86
>         select FUNCTION_ALIGNMENT_16B
>         select GENERIC_BUG_FRAME
>         select HAS_ALTERNATIVE
> +       select HAS_BRK_ALLOCATOR
>         select HAS_COMPAT
>         select HAS_CPUFREQ
>         select HAS_DIT
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/brk.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef X86_BRK_H
> +#define X86_BRK_H
> +
> +static inline unsigned long brk_adjust_start(unsigned long start)
> +{
> +    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);

This seems a bit fragile: As it happens this corresponds to the clip
to brk_end in setup.c in patch 2, but if it didn't, then the boundary
in the resulting e820 reservation and what gets unmapped in
brk_free_unsed() wouldn't match.

Claude seems to think you could make using_2M_mapping() available
here; and then brk_adjust_start() becomes return using_2M_mapping() ?
PAGE_ALIGN_2M(start) : start.  That drops the ASM, and the comment
about the linker producing a bogus relocation.  2/3 could then do
brk_end = brk_adjust_start(brk_cease()), and guaranteeing that the two
are the same.

> --- /dev/null
> +++ b/xen/common/brk.c
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/brk.h>
> +#include <xen/efi.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +#include <xen/page-defs.h>
> +
> +#if __has_include(<asm/brk.h>)
> +# include <asm/brk.h>
> +#endif
> +
> +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;
> +
> +    if ( finished )
> +        return NULL;
> +
> +    /* Allocations PAGE_SIZE and up will be page-aligned. */
> +    if ( size >= PAGE_SIZE )
> +        allocated = ROUNDUP(allocated, PAGE_SIZE);
> +
> +    ptr = __brk_start + allocated;
> +
> +    allocated += ROUNDUP(size, sizeof(void *));
> +
> +    if ( allocated > __bss_end - __brk_start )
> +    {
> +        printk_once_init(
> +            "%pS: over-sized BRK request: %zu bytes\n",
> +            __builtin_extract_return_addr(__builtin_return_address(0)),
> +            size);
> +        return NULL;
> +    }

You bump the global `allocated` variable here before the check, and
don't reset it afterwards.   All future allocation attempts will
continue to bump this value, and
continue to fail.  So now allocated + brk_start is past _bbs_end,
brk_cease() will return something past bss_end, and (in patch 2) pass
that as reserve_e820_ram().  So now, so far from reducing the size
reserved to Xen by , we may be *increasing* the size reserved to Xen
with unused pages.  Worse, if it ends up overshooting the region Xen
is in, then reserve_e820_ram() will fail, and the host will panic.

This should probably use a local variable, then set the global
variable after we've determined that the check has succeeded.

The other thing Claude points out is that is_xen_fixed_mfn() isn't
updated, so if ever any of these pages want to be offlined for any
reason, it will be blocked, claiming that they're part of Xen, even if
they've already been assigned to a guest.  This is a pre-existing
situation that is extended by this patch.

 -George