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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.10.2026 22:40, George Dunlap wrote:
> On Wed, Oct 14, 2026 at 2:02 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> --- /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.

I don't think exposing using_2M_mapping() globally would be a good idea.

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

Could be done, but did Claude also realize that failure to set aside
enough space so allocations can't fail is, effectively, a fatal bug?
The printk_once_init() isn't a panic() here merely because panic()
isn't going to work very well when still in EFI boot services: We
shouldn't halt the system there, we should only yield control back to
EFI (such that another boot attempt can be made). See patch 2.

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

This indeed needs taking care of, albeit in patch 2. It's not clear
to me though why you (they?) say this would be a pre-existing
situation. The use of __2M_rwdata_end there looks correct to me prior
to this series.

Jan