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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2026 16:01, Jan Beulich wrote:
> 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.

In fact, to add to this, originally I had a variable of this purpose. It
simply didn't work out nicely, in particular because of the page alignment
which may need enforcing. (Surely it could be done like you say, but I'd
really prefer not to.)

Jan