Re: [PATCH] xen/arm: Add Kconfig parameter for memory banks number

[Julien Grall <julien@xxxxxxx>]

Hi Bertrand and Luca,

On 07/12/2021 11:09, Bertrand Marquis wrote:
> > On 7 Dec 2021, at 10:52, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
> >
> >
> >
> > > On 6 Dec 2021, at 17:05, Julien Grall <julien@xxxxxxx> wrote:
> > >
> > > Hi Luca,
> > >
> > > On 06/12/2021 15:37, Luca Fancellu wrote:
> > > > Currently the maximum number of memory banks is fixed to
> > > > 128, but on some new platforms that have a large amount
> > > > of memory, this value is not enough
> > Hi Julien,
> >
> > > Can you provide some information on the setup? Is it using UEFI?
> > Yes it is a platform with 32gb of ram, I’ve built Xen with ACPI support and 
> > it’s starting using UEFI+ACPI.
Thanks! That makes more sense now. Although...

> > > > and prevents Xen
> > > > from booting.
> > > AFAIK, the restriction should only prevent Xen to use all the memory. If that's 
> > > not the case, then this should be fixed.
> > The code that it’s failing is this, inside efi_process_memory_map_bootinfo(…) 
> > in the arch/arm/efi/efi-boot.h:
> >
> > #ifdef CONFIG_ACPI
> >         else if ( desc_ptr->Type == EfiACPIReclaimMemory )
> >         {
> >             if ( !meminfo_add_bank(&bootinfo.acpi, desc_ptr) )
> >             {
> >                 PrintStr(L"Error: All " __stringify(NR_MEM_BANKS)
> >                           " acpi meminfo mem banks exhausted.\r\n");
> >                 return EFI_LOAD_ERROR;
> >             }
> >         }
> > #endif
... I was expecting bootinfo.mem to be filled rather than bootinfo.acpi:

            if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
            {
                PrintStr(L"Warning: All " __stringify(NR_MEM_BANKS)
                          " bootinfo mem banks exhausted.\r\n");
                break;
            }

It is actually quite surprising that you end up with more than 128 
regions here.
> > > > Create a Kconfig parameter to set the value, by default
> > > > 128.
> > > I think Xen should be able to boot on any platform with the default 
> > > configuration. So the value should at least be bumped.
> > >
> > > > Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > > > ---
> > > > xen/arch/arm/Kconfig        | 8 ++++++++
> > > > xen/include/asm-arm/setup.h | 2 +-
> > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > > index ecfa6822e4d3..805e3c417e89 100644
> > > > --- a/xen/arch/arm/Kconfig
> > > > +++ b/xen/arch/arm/Kconfig
> > > > @@ -25,6 +25,14 @@ menu "Architecture Features"
> > > >    source "arch/Kconfig"
> > > > +config MEM_BANKS
> > > > +       int "Maximum number of memory banks."
> > > > +       default "128"
> > > > +       help
> > > > +         Controls the build-time size memory bank array.
> > > > +         It is the upper bound of the number of logical entities describing
> > > > +         the memory.
> > > NR_MEM_BANKS is going to be used by multiple internal structure in Xen (e.g. 
> > > static memory, reserved memory, normal memory). So how could an admin decide 
> > > the correct value?
> > >
> > > In particular for UEFI, we are at the mercy of the firmware that can expose any 
> > > kind of memory map (that's why we had to increase the original number of banks).
> > >
> > > So maybe it is time for us to move out from a static array and re-think how we 
> > > discover the memory.
> > >
> > > That this is probably going to take some time to get it properly, so
> > > I would be OK with bumping the value + a config gated UNSUPPORTED.
>
> Looking at what we have, the memory is actually fragmented by ACPI but a long 
> term solution could be to make the code a little bit more smart and try to 
> merge together adjacent banks.
That could work, but I think we could get rid of bootinfo.acpi completely.

If I am not mistaken, bootinfo.acpi is only used by Xen to figure out 
how to create the EFI memory map for dom0. So this could be replaced 
with a walk of the UEFI memory map.
Looking at the code, we have already some boiler plate for x86 to pass 
the UEFI memory map from the stub to Xen. They would need need to be 
adjusted for Arm to:
   * Enable ebmalloc() (see TODOs in common/efi/ebmalloc.c)
   * Switch efi_arch_allocate_mmap_buffer() to use ebmalloc()
   * Adjust the pointers (see end of the efi_exit_boot()). I think we 
would want to pass the physical and let the actual Xen to translate to a 
virtual address
> I would suggest to just increase the existing define to 256 to fix the current 
> issue (which might be encountered by anybody using ACPI) and put a comment in 
> the code for now with a TODO explaining why we currently need such a high value 
> and what should be done to fix this.
I am fine with simply bumping the value (the actual array doesn't take a 
lot of space and will be freed after boot).
Long term, I think it would be better if we start to reduce the number 
of bootinfo.mem* and removing any hardcoded size.
When using UEFI, we could replace with the UEFI memory map. For non-UEFI 
DT boot, we would still need to create the memory map by hand to avoid 
walking the DT every time.
Cheers,

--
Julien Grall