| Message ID | 20211206153730.49791-1-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Add Kconfig parameter for memory banks number | expand |
On 06/12/2021 15:37, Luca Fancellu wrote: > 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 $ git grep -wc NR_MEM_BANKS arch/arm/bootfdt.c:1 arch/arm/domain_build.c:5 arch/arm/efi/efi-boot.h:3 include/asm-arm/setup.h:2 For this few instances, it is probably better to use CONFIG_MEM_BANKS everywhere, rather than add the level of indirection. ~Andrew
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 Can you provide some information on the setup? Is it using UEFI? > 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. > > 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@arm.com> > --- > 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. > + > 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,
> On 6 Dec 2021, at 17:05, Julien Grall <julien@xen.org> 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@arm.com> >> --- >> 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
Hi, > On 7 Dec 2021, at 10:52, Luca Fancellu <Luca.Fancellu@arm.com> wrote: > > > >> On 6 Dec 2021, at 17:05, Julien Grall <julien@xen.org> 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@arm.com> >>> --- >>> 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
Hi Bertrand and Luca, On 07/12/2021 11:09, Bertrand Marquis wrote: >> On 7 Dec 2021, at 10:52, Luca Fancellu <Luca.Fancellu@arm.com> wrote: >> >> >> >>> On 6 Dec 2021, at 17:05, Julien Grall <julien@xen.org> 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@arm.com> >>>> --- >>>> 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,
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. + 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 */
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 and prevents Xen from booting. Create a Kconfig parameter to set the value, by default 128. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/Kconfig | 8 ++++++++ xen/include/asm-arm/setup.h | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-)