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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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 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.

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".

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.
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.

Please have a look at the spec, and let me know if you come to the same
conclusion.