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

[Julien Grall <julien@xxxxxxx>]

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.

Cheers,

--
Julien Grall