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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 14/11/2024 11:31, Julien Grall wrote:
> 
> 
> Hi Stefano,
> 
> On 13/11/2024 22:41, Stefano Stabellini wrote:
>> On Wed, 13 Nov 2024, Julien Grall wrote:
>>> On 13/11/2024 15:40, Michal Orzel wrote:
>>>> On 13/11/2024 15:40, Julien Grall wrote:
>>>>> On 13/11/2024 14:19, Michal Orzel wrote:
>>>>>> On 13/11/2024 14:50, Julien Grall wrote:
>>>>>>> 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.
>>
>> I looked into the question: "Is this an observation or part of a
>> specification?"
>>
>> Looking at the device tree specification
>> source/chapter5-flattened-format.rst:"Memory Reservation Block"
>>
>> It says:
>>
>> "It is used to protect vital data structures from being overwritten by
>> the client program." [...] "More specifically, a client program shall
>> not access memory in a reserved region unless other information provided
>> by the boot program explicitly indicates that it shall do so."
>>
>>
>> I think it is better to stay on the safe side and implement in Xen a
>> more generic behavior to support /memreserve/. It is possible that in a
>> future board more information could be residing in a /memreserve/
>> region. For instance, I could imagine EFI runtime services residing in a
>> /memreserve/ region.
> 
> I am not 100% sure about this one. The specification implies that if a
> region is reserved, then it would need to be marked as
> EfiReservedMemoryType in the EFI memory map. But for EFI runtime
> services, they should be using EfiRuntimeServicesCode or
> EfiRuntimeServicesData.
> 
>>
>> I am a bit confused by ranges that are both in /memreserve/ and
>> /reserved-memory. Ranges under /memreserve/ should not be accessed at
>> all (unless otherwise specified), ranges under /reserved-memory are
>> reserved for specific drivers.
> 
> IIUC /memreserve/ is the legacy approach for describing reserved regions.
> 
>> I guess ranges that are both in /memreserve/ and /reserved-memory are
>> exactly the type of ranges that fall under this statement in the spec:
>> "unless other information provided by the boot program explicitly
>> indicates that it shall do so".
> 
> Yes. The OS would be able to use the range based what /reserved-memory
> says. Note that you can also the describe a region from /memreserve/
> outside or /reserved-memory (such as the CPU spin table).
> 
>>
>> The way I see it from the device tree spec, I think Xen should not map
>> /memreserve/ ranges to Dom0, and it should avoid accessing them itself.
> 
> See above, Xen should be able to access the regions in /memreserve/. But
> it should map them in the directmap.
> 
>> But if a range is both in /memreserve/ and also in /reserved-memory,
>> then basically /reserved-memory takes precedence, so Xen should map it
>> to Dom0.
> 
> Unless Xen needs to use some of them. At which point this will need to
> be excluded from Dom0.
> 
> Looking at the code, I think /memreserve/ and /reserved-memory are not
> mapped in Xen and everything in /reserved-memory is mapped to dom0.
Why do we forward /reserved-memory to dom0 fdt but /memreserve/ not? From the 
discussion
we're having it seems like we should treat them equally. Also, looking at Luca 
patch,
we seem to special case /memreserve/ and only allow for overlap /memresrve/ 
with boot modules
and not /reserved-memory/ with boot modules. If we are going to claim that all 
the boot modules
can be marked as reserved by the bootloader, then I think we should treat them 
equally providing
the same experience to dom0.

Last thing I wanted to ask (for my clarity) is that if bootloader loads initrd 
at region A and marks
it as reserved (either in /memreserve/ or /reserved-memory/), and later on Xen 
copies initrd from region
A to B, shouldn't the reserved memory region be updated to cover new region for 
initrd?

~Michal