Re: [PATCH v2 2/3] x86/EFI: replace ebmalloc()

On 30.07.2026 20:08, Andrew Cooper wrote:
> On 27/07/2026 11:20 am, Jan Beulich wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -31,6 +31,7 @@
>>  #include <asm/alternative.h>
>>  #include <asm/apic.h>
>>  #include <asm/bootinfo.h>
>> +#include <asm/brk.h>
>>  #include <asm/bzimage.h>
>>  #include <asm/cpu-policy.h>
>>  #include <asm/e820.h>
>> @@ -164,6 +165,8 @@ cpumask_t __read_mostly cpu_present_map;
>>  
>>  unsigned long __read_mostly xen_phys_start;
>>  
>> +unsigned long __ro_after_init brk_end;
>> +
>>  /* Only used in asm code and within this source file */
>>  char asmlinkage __section(".init.bss.stack_aligned") __aligned(STACK_SIZE)
>>      cpu0_stack[STACK_SIZE];
>> @@ -1141,7 +1144,6 @@ void asmlinkage __init noreturn __start_
>>      struct boot_info *bi;
>>      unsigned long nr_pages, raw_max_page;
>>      int i, j, bytes = 0;
>> -    unsigned long eb_start, eb_end;
>>      bool acpi_boot_table_init_done = false, relocated = false;
>>      bool vm_init_done = false;
>>      int ret;
>> @@ -1511,7 +1513,7 @@ void asmlinkage __init noreturn __start_
>>          /*
>>           * This needs to remain in sync with remove_xen_ranges() and the
>>           * respective reserve_e820_ram() invocation below. No need to
>> -         * query efi_boot_mem_unused() here, though.
>> +         * query brk_get_unused_start() here, though.
>>           */
>>          xen->start = virt_to_maddr(_stext);
>>          xen->size  = __2M_rwdata_end - _stext;
>> @@ -1654,18 +1656,11 @@ void asmlinkage __init noreturn __start_
>>      if ( !xen_phys_start )
>>          panic("Not enough memory to relocate Xen\n");
>>  
>> -    /* FIXME: Putting a hole in .bss would shatter the large page mapping. */
>> -    if ( using_2M_mapping() )
>> -        efi_boot_mem_unused(NULL, NULL);
>> -
>>      /* This needs to remain in sync with remove_xen_ranges(). */
>> -    if ( efi_boot_mem_unused(&eb_start, &eb_end) )
>> -    {
>> -        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start));
>> -        reserve_e820_ram(&boot_e820, __pa(eb_end), __pa(__2M_rwdata_end));
>> -    }
>> -    else
>> -        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
>> +    brk_end = brk_get_unused_start();
>> +    if ( using_2M_mapping() )
>> +        brk_end = PAGE_ALIGN_2M(brk_end);
>> +    reserve_e820_ram(&boot_e820, __pa(_stext), __pa(brk_end));
> 
> Hiding brk_end in setup.c like this is quite rude.  I guess it's because
> you want to have brk.c be brk.init.o, but it really does live with the
> other brk functions.

As said in reply to you comments on patch 1 - brk_end is purely an x86
helper variable. I don't want it to move to common code.

> Furthermore, having brk_end right from the outset fixes the fact that
> brk_get_unused_start() is doing things beyond retrieving a value.
> 
> With brk_end being the real bump pointer the allocator uses,

Hmm, no - I don't view the variable as fulfilling that purpose.

Jan

> then the
> only function you need is brk_finish() (name subject to improvement)
> which is now very clear about the point at which brk allocations cease
> working.
> 
> The rest, dropping EFI's current ebmalloc() all looks fine now.
> 
> ~Andrew