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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

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


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.

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.

Cheers
Bertrand


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