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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 13/11/2024 17:41, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 13/11/2024 15:40, Michal Orzel wrote:
>>
>>
>> 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.
> 
> Sure we noticed that U-boot is doing some we didn't expect. But this
> really doesn't mean there are not other interesting behavior happening.
> 
>>
>>> 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.
> 
> There are no requirements to be 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
> 
> Yet this doesn't provide any information why this only has to be an
> exact region... It only tells me the current behavior.
> 
>>
>>>
>>>>
>>>> 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.
> 
> The DTB is not described but the kernel is. We also have XSM modules.
> All of which could in theory be in memreserve if for some reasons the
> bootloader wanted to preserve the modules for future use (think
> Live-Update)...
> 
> Anyway, to be honest, I don't understand why you are pushing back at a
> more generic solution... Yes this may be what we just notice today, but
> I haven't seen any evidence that it never happen.
> 
> So I would rather go with the generic solution.
My reluctance comes from the fact that it's difficult for me to later justify 
(if someone asks) a code like that
in the port we maintain because I can't answer the question about the rationale 
of other modules.
All you wrote is just a theory. Neither you nor anyone seen a code where a 
bootloader sets up /memreserve/
for sth else than initrd. That's it. I understand that generic solution solves 
the possible future problems
(the current u-boot behavior dates back to 2014 and nothing new happened since 
that time) but at least I find
it more difficult to reason about. I can see you might not see it the same way 
I do, therefore I am fine with
the generic solution.

~Michal