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

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

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

At the moment, Xen on Arm64 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 requirement regarding ordering in the device
tree, update code to be able to deal with by sorting memory
banks. There is only one heap region on Arm32, so the sorting
is fine to be done in the common code.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
   V2:
   - add Stefano's R-b
   - clarify patch description
   - clarify comment in code
   - drop "if (bootinfo.mem.nr_banks > 1)" check
---
 xen/arch/arm/bootfdt.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index dcff512..476e32e 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,15 @@ 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);
+
+    /*
+     * On Arm64 setup_xenheap_mappings() 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);
-- 
2.7.4

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

[Bertrand Marquis]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

Hi Oleksandr,

On 5 Jul 2021, at 18:48, Oleksandr Tyshchenko <olekstysh@gmail.com<mailto:olekstysh@gmail.com>> wrote:

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com<mailto:oleksandr_tyshchenko@epam.com>>

At the moment, Xen on Arm64 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 requirement regarding ordering in the device
tree, update code to be able to deal with by sorting memory
banks. There is only one heap region on Arm32, so the sorting
is fine to be done in the common code.

Suggested-by: Julien Grall <jgrall@amazon.com<mailto:jgrall@amazon.com>>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com<mailto:oleksandr_tyshchenko@epam.com>>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org<mailto:sstabellini@kernel.org>>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com<mailto:bertrand.marquis@arm.com>>

Cheers
Bertrand


---
  V2:
  - add Stefano's R-b
  - clarify patch description
  - clarify comment in code
  - drop "if (bootinfo.mem.nr_banks > 1)" check
---
xen/arch/arm/bootfdt.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index dcff512..476e32e 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,15 @@ 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);
+
+    /*
+     * On Arm64 setup_xenheap_mappings() 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);
--
2.7.4




[-- Attachment #2: Type: text/html, Size: 5384 bytes --]

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

[Julien Grall]

Hi Oleksandr,

On 05/07/2021 18:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> At the moment, Xen on Arm64 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 requirement regarding ordering in the device
> tree, update code to be able to deal with by sorting memory
> banks. There is only one heap region on Arm32, so the sorting
> is fine to be done in the common code.
> 
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> 
> ---
>     V2:
>     - add Stefano's R-b
>     - clarify patch description
>     - clarify comment in code
>     - drop "if (bootinfo.mem.nr_banks > 1)" check
> ---
>   xen/arch/arm/bootfdt.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index dcff512..476e32e 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,15 @@ 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);
> +
> +    /*
> +     * On Arm64 setup_xenheap_mappings() 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);
> 

-- 
Julien Grall

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

[Julien Grall]

On 06/07/2021 09:52, Julien Grall wrote:
> Hi Oleksandr,
> 
> On 05/07/2021 18:48, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> At the moment, Xen on Arm64 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 requirement regarding ordering in the device
>> tree, update code to be able to deal with by sorting memory
>> banks. There is only one heap region on Arm32, so the sorting
>> is fine to be done in the common code.
>>
>> Suggested-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

And committed.

Cheers,

-- 
Julien Grall