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

Luca Fancellu posted 1 patch 2 years, 3 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20211206153730.49791-1-luca.fancellu@arm.com
xen/arch/arm/Kconfig        | 8 ++++++++
xen/include/asm-arm/setup.h | 2 +-
2 files changed, 9 insertions(+), 1 deletion(-)
[PATCH] xen/arm: Add Kconfig parameter for memory banks number
Posted by Luca Fancellu 2 years, 3 months ago
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(-)

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 */
 
-- 
2.17.1


Re: [PATCH] xen/arm: Add Kconfig parameter for memory banks number
Posted by Andrew Cooper 2 years, 3 months ago
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

Re: [PATCH] xen/arm: Add Kconfig parameter for memory banks number
Posted by Julien Grall 2 years, 3 months ago
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,

-- 
Julien Grall

Re: [PATCH] xen/arm: Add Kconfig parameter for memory banks number
Posted by Luca Fancellu 2 years, 3 months ago

> 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


Re: [PATCH] xen/arm: Add Kconfig parameter for memory banks number
Posted by Bertrand Marquis 2 years, 3 months ago
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

Re: [PATCH] xen/arm: Add Kconfig parameter for memory banks number
Posted by Julien Grall 2 years, 3 months ago
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,

-- 
Julien Grall