[PATCH] xen/arm: bootfdt: Always sort memory banks

[PATCH] xen/arm: bootfdt: Always sort memory banks

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

At the moment, Xen expects the memory banks to be ordered.
Unfortunately, there may be a case when updated by firmware
device tree contains unordered banks. This means Xen will panic
when setting xenheap mappings for the subsequent bank with start
address being less than xenheap_mfn_start (start address of
the first bank).

As there is no clear requirment regarding ordering in the device
tree, update code to be able to deal with by sorting memory
banks if we have more than one.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---

The proposed commit fixes the booting Xen on R-Car M3-W+ SoC:

Starting kernel ...
- UART enabled -
- Boot CPU booting -
- Current EL 00000008 -
- Initialize CPU -
- Turning on paging -
- Zero BSS -
- Ready -
(XEN) Checking for initrd in /chosen
(XEN) Initrd 0000000084000040-0000000085dbc32a
(XEN) RAM: 0000000480000000 - 00000004ffffffff
(XEN) RAM: 0000000048000000 - 00000000bfffffff
(XEN) RAM: 0000000600000000 - 00000006ffffffff

...

(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) cannot add xenheap mapping at 48000 below heap start 480000
(XEN) ****************************************
(XEN) 
(XEN) Reboot in five seconds...
---
 xen/arch/arm/bootfdt.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index dcff512..3ef63b3 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -13,6 +13,7 @@
 #include <xen/init.h>
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
+#include <xen/sort.h>
 #include <xsm/xsm.h>
 #include <asm/setup.h>
 
@@ -395,6 +396,21 @@ static void __init early_print_info(void)
     printk("\n");
 }
 
+/* This function assumes that memory regions are not overlapped */
+static int __init cmp_memory_node(const void *key, const void *elem)
+{
+    const struct membank *handler0 = key;
+    const struct membank *handler1 = elem;
+
+    if ( handler0->start < handler1->start )
+        return -1;
+
+    if ( handler0->start >= (handler1->start + handler1->size) )
+        return 1;
+
+    return 0;
+}
+
 /**
  * boot_fdt_info - initialize bootinfo from a DTB
  * @fdt: flattened device tree binary
@@ -412,6 +428,12 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
     add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
 
     device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
+    if ( bootinfo.mem.nr_banks > 1 )
+    {
+        /* Some DT may describe unordered banks, sort them in ascending order */
+        sort(bootinfo.mem.bank, bootinfo.mem.nr_banks, sizeof(struct membank),
+             cmp_memory_node, NULL);
+    }
     early_print_info();
 
     return fdt_totalsize(fdt);
-- 
2.7.4

Re: [PATCH] xen/arm: bootfdt: Always sort memory banks

[Stefano Stabellini]

On Mon, 14 Jun 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> At the moment, Xen expects the memory banks to be ordered.
> Unfortunately, there may be a case when updated by firmware
> device tree contains unordered banks. This means Xen will panic
> when setting xenheap mappings for the subsequent bank with start
> address being less than xenheap_mfn_start (start address of
> the first bank).
> 
> As there is no clear requirment regarding ordering in the device
> tree, update code to be able to deal with by sorting memory
> banks if we have more than one.
> 
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> The proposed commit fixes the booting Xen on R-Car M3-W+ SoC:
> 
> Starting kernel ...
> - UART enabled -
> - Boot CPU booting -
> - Current EL 00000008 -
> - Initialize CPU -
> - Turning on paging -
> - Zero BSS -
> - Ready -
> (XEN) Checking for initrd in /chosen
> (XEN) Initrd 0000000084000040-0000000085dbc32a
> (XEN) RAM: 0000000480000000 - 00000004ffffffff
> (XEN) RAM: 0000000048000000 - 00000000bfffffff
> (XEN) RAM: 0000000600000000 - 00000006ffffffff
> 
> ...
> 
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) cannot add xenheap mapping at 48000 below heap start 480000
> (XEN) ****************************************
> (XEN) 
> (XEN) Reboot in five seconds...
> ---
>  xen/arch/arm/bootfdt.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index dcff512..3ef63b3 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -13,6 +13,7 @@
>  #include <xen/init.h>
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
> +#include <xen/sort.h>
>  #include <xsm/xsm.h>
>  #include <asm/setup.h>
>  
> @@ -395,6 +396,21 @@ static void __init early_print_info(void)
>      printk("\n");
>  }
>  
> +/* This function assumes that memory regions are not overlapped */
> +static int __init cmp_memory_node(const void *key, const void *elem)
> +{
> +    const struct membank *handler0 = key;
> +    const struct membank *handler1 = elem;
> +
> +    if ( handler0->start < handler1->start )
> +        return -1;
> +
> +    if ( handler0->start >= (handler1->start + handler1->size) )
> +        return 1;
> +
> +    return 0;
> +}
> +
>  /**
>   * boot_fdt_info - initialize bootinfo from a DTB
>   * @fdt: flattened device tree binary
> @@ -412,6 +428,12 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
>      add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
>  
>      device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
> +    if ( bootinfo.mem.nr_banks > 1 )
> +    {
> +        /* Some DT may describe unordered banks, sort them in ascending order */
> +        sort(bootinfo.mem.bank, bootinfo.mem.nr_banks, sizeof(struct membank),
> +             cmp_memory_node, NULL);
> +    }
>      early_print_info();
>  
>      return fdt_totalsize(fdt);
> -- 
> 2.7.4
> 

Re: [PATCH] xen/arm: bootfdt: Always sort memory banks

[Julien Grall]

Hi Oleksandr,

On 14/06/2021 20:34, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> At the moment, Xen expects the memory banks to be ordered.
> Unfortunately, there may be a case when updated by firmware
> device tree contains unordered banks. This means Xen will panic
> when setting xenheap mappings for the subsequent bank with start
> address being less than xenheap_mfn_start (start address of
> the first bank).

Please clarify in the commit message that the behavior you are 
describing is for arm64. For arm32, there is only one heap region.

That said, I think the sorting is fine to be done in common code.

> 
> As there is no clear requirment regarding ordering in the device

s/requirment/requirement/

> tree, update code to be able to deal with by sorting memory
> banks if we have more than one.
> 
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> 
> The proposed commit fixes the booting Xen on R-Car M3-W+ SoC:
> 
> Starting kernel ...
> - UART enabled -
> - Boot CPU booting -
> - Current EL 00000008 -
> - Initialize CPU -
> - Turning on paging -
> - Zero BSS -
> - Ready -
> (XEN) Checking for initrd in /chosen
> (XEN) Initrd 0000000084000040-0000000085dbc32a
> (XEN) RAM: 0000000480000000 - 00000004ffffffff
> (XEN) RAM: 0000000048000000 - 00000000bfffffff
> (XEN) RAM: 0000000600000000 - 00000006ffffffff
> 
> ...
> 
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) cannot add xenheap mapping at 48000 below heap start 480000
> (XEN) ****************************************
> (XEN)
> (XEN) Reboot in five seconds...
> ---
>   xen/arch/arm/bootfdt.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index dcff512..3ef63b3 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -13,6 +13,7 @@
>   #include <xen/init.h>
>   #include <xen/device_tree.h>
>   #include <xen/libfdt/libfdt.h>
> +#include <xen/sort.h>
>   #include <xsm/xsm.h>
>   #include <asm/setup.h>
>   
> @@ -395,6 +396,21 @@ static void __init early_print_info(void)
>       printk("\n");
>   }
>   
> +/* This function assumes that memory regions are not overlapped */
> +static int __init cmp_memory_node(const void *key, const void *elem)
> +{
> +    const struct membank *handler0 = key;
> +    const struct membank *handler1 = elem;
> +
> +    if ( handler0->start < handler1->start )
> +        return -1;
> +
> +    if ( handler0->start >= (handler1->start + handler1->size) )
> +        return 1;
> +
> +    return 0;
> +}
> +
>   /**
>    * boot_fdt_info - initialize bootinfo from a DTB
>    * @fdt: flattened device tree binary
> @@ -412,6 +428,12 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
>       add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
>   
>       device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
> +    if ( bootinfo.mem.nr_banks > 1 )

NIT: sort() is already be able to deal with one element array. So this 
checks looks a bit pointless.

> +    {
> +        /* Some DT may describe unordered banks, sort them in ascending order */

It would be good to explain in the comment *why* this is necessary. Some 
along the line:

On Arm64, setup_xenheap_pages() expects to be called with the lowest 
bank in memory first. There is no requirement that the DT will provide 
the banks sorted in ascending order. So sort them through.

> +        sort(bootinfo.mem.bank, bootinfo.mem.nr_banks, sizeof(struct membank),
> +             cmp_memory_node, NULL);
> +    }
>       early_print_info();
>   
>       return fdt_totalsize(fdt);
> 

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: bootfdt: Always sort memory banks

[Oleksandr]

On 30.06.21 11:56, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


Sorry for the late response.


>
> On 14/06/2021 20:34, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> At the moment, Xen expects the memory banks to be ordered.
>> Unfortunately, there may be a case when updated by firmware
>> device tree contains unordered banks. This means Xen will panic
>> when setting xenheap mappings for the subsequent bank with start
>> address being less than xenheap_mfn_start (start address of
>> the first bank).
>
> Please clarify in the commit message that the behavior you are 
> describing is for arm64. For arm32, there is only one heap region.
>
> That said, I think the sorting is fine to be done in common code.

ok


>
>
>>
>> As there is no clear requirment regarding ordering in the device
>
> s/requirment/requirement/

ok


>
>> tree, update code to be able to deal with by sorting memory
>> banks if we have more than one.
>>
>> Suggested-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>
>> The proposed commit fixes the booting Xen on R-Car M3-W+ SoC:
>>
>> Starting kernel ...
>> - UART enabled -
>> - Boot CPU booting -
>> - Current EL 00000008 -
>> - Initialize CPU -
>> - Turning on paging -
>> - Zero BSS -
>> - Ready -
>> (XEN) Checking for initrd in /chosen
>> (XEN) Initrd 0000000084000040-0000000085dbc32a
>> (XEN) RAM: 0000000480000000 - 00000004ffffffff
>> (XEN) RAM: 0000000048000000 - 00000000bfffffff
>> (XEN) RAM: 0000000600000000 - 00000006ffffffff
>>
>> ...
>>
>> (XEN) ****************************************
>> (XEN) Panic on CPU 0:
>> (XEN) cannot add xenheap mapping at 48000 below heap start 480000
>> (XEN) ****************************************
>> (XEN)
>> (XEN) Reboot in five seconds...
>> ---
>>   xen/arch/arm/bootfdt.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>> index dcff512..3ef63b3 100644
>> --- a/xen/arch/arm/bootfdt.c
>> +++ b/xen/arch/arm/bootfdt.c
>> @@ -13,6 +13,7 @@
>>   #include <xen/init.h>
>>   #include <xen/device_tree.h>
>>   #include <xen/libfdt/libfdt.h>
>> +#include <xen/sort.h>
>>   #include <xsm/xsm.h>
>>   #include <asm/setup.h>
>>   @@ -395,6 +396,21 @@ static void __init early_print_info(void)
>>       printk("\n");
>>   }
>>   +/* This function assumes that memory regions are not overlapped */
>> +static int __init cmp_memory_node(const void *key, const void *elem)
>> +{
>> +    const struct membank *handler0 = key;
>> +    const struct membank *handler1 = elem;
>> +
>> +    if ( handler0->start < handler1->start )
>> +        return -1;
>> +
>> +    if ( handler0->start >= (handler1->start + handler1->size) )
>> +        return 1;
>> +
>> +    return 0;
>> +}
>> +
>>   /**
>>    * boot_fdt_info - initialize bootinfo from a DTB
>>    * @fdt: flattened device tree binary
>> @@ -412,6 +428,12 @@ size_t __init boot_fdt_info(const void *fdt, 
>> paddr_t paddr)
>>       add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
>>         device_tree_for_each_node((void *)fdt, 0, early_scan_node, 
>> NULL);
>> +    if ( bootinfo.mem.nr_banks > 1 )
>
> NIT: sort() is already be able to deal with one element array. So this 
> checks looks a bit pointless.

Agree, will drop.


>
>
>> +    {
>> +        /* Some DT may describe unordered banks, sort them in 
>> ascending order */
>
> It would be good to explain in the comment *why* this is necessary. 
> Some along the line:
>
> On Arm64, setup_xenheap_pages() expects to be called with the lowest 
> bank in memory first. There is no requirement that the DT will provide 
> the banks sorted in ascending order. So sort them through.

ok


>
>
>> +        sort(bootinfo.mem.bank, bootinfo.mem.nr_banks, sizeof(struct 
>> membank),
>> +             cmp_memory_node, NULL);
>> +    }
>>       early_print_info();
>>         return fdt_totalsize(fdt);
>>
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko