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

[Julien Grall <julien@xxxxxxx>]

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 
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?
> 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.
Cheers,

--
Julien Grall