Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions

[Michal Orzel <michal.orzel@xxxxxxx>]

On 13/11/2024 15:40, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 13/11/2024 14:19, Michal Orzel wrote:
>>
>>
>> On 13/11/2024 14:50, Julien Grall wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>> On 06/11/2024 15:07, Michal Orzel wrote:
>>>>
>>>>
>>>> On 06/11/2024 14:41, Luca Fancellu wrote:
>>>>>
>>>>>
>>>>> There are some cases where the device tree exposes a memory range
>>>>> in both /memreserve/ and reserved-memory node, in this case the
>>>>> current code will stop Xen to boot since it will find that the
>>>>> latter range is clashing with the already recorded /memreserve/
>>>>> ranges.
>>>>>
>>>>> Furthermore, u-boot lists boot modules ranges, such as ramdisk,
>>>>> in the /memreserve/ part and even in this case this will prevent
>>>>> Xen to boot since it will see that the module memory range that
>>>>> it is going to add in 'add_boot_module' clashes with a /memreserve/
>>>>> range.
>>>>>
>>>>> When Xen populate the data structure that tracks the memory ranges,
>>>>> it also adds a memory type described in 'enum membank_type', so
>>>>> in order to fix this behavior, allow the 'check_reserved_regions_overlap'
>>>>> function to check for exact memory range match given a specific memory
>>>>> type; allowing reserved-memory node ranges and boot modules to have an
>>>>> exact match with ranges from /memreserve/.
>>>>>
>>>>> While there, set a type for the memory recorded during ACPI boot.
>>>>>
>>>>> Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to 
>>>>> bootinfo.reserved_mem")
>>>>> Reported-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>>>>> Reported-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>>>>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>>>>> ---
>>>>> I tested this patch adding the same range in a /memreserve/ entry and
>>>>> /reserved-memory node, and by letting u-boot pass a ramdisk.
>>>>> I've also tested that a configuration running static shared memory still 
>>>>> works
>>>>> fine.
>>>>> ---
>>>> So we have 2 separate issues. I don't particularly like the concept of 
>>>> introducing MEMBANK_NONE
>>>> and the changes below look a bit too much for me, given that for boot 
>>>> modules we can only have
>>>> /memreserve/ matching initrd.
>>>
>>> How so? Is this an observation or part of a specification?
>> Not sure what specification you would want to see.
> 
> Anything that you bake your observation. My concern with observation is ...
> 
>   It's all part of U-Boot and Linux behavior that is not documented
> (except for code comments).
>> My statement is based on the U-Boot and Linux behavior. U-Boot part only 
>> present for initrd:
>> https://github.com/u-boot/u-boot/blob/master/boot/fdt_support.c#L249
> 
> ... a user is not forced to use U-boot. So this is not a good reason to
I thought that this behavior is solely down to u-boot playing tricks with 
memreserve.

> rely on it. If Linux starts to rely on it, then it is probably a better
> argument, but first I would need to see the code. Can you paste a link?
Not sure how I would do that given that it is all scattered. But if it means 
sth, here is kexec code
to create fdt. It is clear they do the same trick as u-boot.
https://github.com/torvalds/linux/blob/master/drivers/of/kexec.c#L355

> 
>>
>> For things that Xen can be interested in, only region for ramdisk for dom0 
>> can match the /memreserve/ region.
>> Providing a generic solution (like Luca did) would want providing an example 
>> of sth else that can match which I'm not aware of.
> 
> I would argue this is the other way around. If we are not certain that
> /memreserve/ will not be used for any other boot module, then we should
> have a generic solution. Otherwise, we will end up with similar weird
> issue in the future.
We have 3 possible modules for bootloader->kernel workflow: kernel, dtb and 
ramdisk. The first 2 are not described in DT so I'm not sure
what are your examples of bootmodules for which you want kernel know about 
memory reservation other than ramdisk.

~Michal