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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

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

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

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

I can do that.

Cheers,
Luca

> 
>> +
>>  config ACPI
>>      bool "ACPI (Advanced Configuration and Power Interface) Support 
>> (UNSUPPORTED)" if UNSUPPORTED
>>      depends on ARM_64
>> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
>> index 95da0b7ab9cd..785a8fe81450 100644
>> --- a/xen/include/asm-arm/setup.h
>> +++ b/xen/include/asm-arm/setup.h
>> @@ -6,7 +6,7 @@
>>  #define MIN_FDT_ALIGN 8
>>  #define MAX_FDT_SIZE SZ_2M
>>  -#define NR_MEM_BANKS 128
>> +#define NR_MEM_BANKS CONFIG_MEM_BANKS
>>    #define MAX_MODULES 32 /* Current maximum useful modules */
>>  
> 
> Cheers,
> 
> -- 
> Julien Grall