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

From: Oleksandr <olekstysh@gmail.com>
To: Julien Grall <julien@xen.org>, xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH] xen/arm: bootfdt: Always sort memory banks
Date: Mon, 5 Jul 2021 20:26:40 +0300	[thread overview]
Message-ID: <fb1526f2-d377-771f-7824-817891d883be@gmail.com> (raw)
In-Reply-To: <47cbf077-8681-7ac4-04e2-f31e1fa4c04f@xen.org>

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