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

[Julien Grall <julien@xxxxxxx>]

On 14/11/2024 12:22, Michal Orzel wrote:
> On 14/11/2024 13:04, Julien Grall wrote:
> > Hi Michal,
> >
> > On 14/11/2024 11:48, Michal Orzel wrote:
> > > On 14/11/2024 11:31, Julien Grall wrote:
> > > > 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?
> > I was wondering the same. The main issue I can think of with
> > /memreserve/ is some of the regions will likely be for Xen own usage. So
> Can you give example of regions defined as reserved for Xen usage (other than 
> static-mem)?
The spin table to bring-up CPUs.

> > we would need to have a way to exclude them from dom0.
> >
> >   >  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.
> > In my mind, /memreserved/ and /reserved-memory/ are different. The
> > former doesn't say what the region is for, but the latter will indicate it.
> In the context of this patch, I don't agree. We're discussing overlap, and if a 
> region A
> from /memreserve/ overlaps fully with a module A, we know what is the purpose 
> of it.
> Today it's initrd, but as you say we cannot rule out other modules as 
well.
To give a concrete example, the /reserved-region/ can be used to reserve 
space for the VGA buffer. It would be odd that someone would put the 
boot module in the middle of the VGA buffer... If Xen ends up to use the 
VGA buffer (not the case today), then it would be a problem. Xen would 
need to be reworked to move all boot modules outside of the memory 
because it can access the VGA (or any other reserved regions).
The problem is slightly different for /memreserve/ because we don't 
exactly know what the regions are for until we parse the rest of the DT. 
Yes technically, a user could put the initrd in the wrong place. So the 
problem is the same. But this is a relexation I am more willing to 
accept over /reserved-region/ right now.
> > So I am not 100% sure how the bootmodule could end up in
> > /reserved-memory/ because they are described as part of the multiboot
> > modules. Do you have a scenario?
> I don't same as I don't have scenario for /memreserve/ overlapping with sth 
> else than initrd.
> All of these comes from my validation of u-boot, grub, barebox code. I have a 
> feeling that due to
> U-Boot trick that is not present in any other *known* bootloader, we are trying 
> to over-engineer the problem :)
> But as Stefano and you wrote, we should follow the spec and for me we should 
> therefore treat them equally.
See above why I don't think we should treat them equally today. We might 
be able in the future if we can categorize all the regions in /memreserve/.
[...]

> > > 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?
> > If we mark the region has a reserved, then we are telling the OS it
> > can't use the region. But I am not sure why it would be needed as Xen
> Well, in the context of initrd, kernel uses it even though it is reserved. This 
> is because
> of the second part of the spec where other bindings come into play.
>
> > doesn't care how the regions is going to be used by the domain. From a
> > domain side, do you see any reason why we would want to mark again the
> > region as reserved?
> TBH I don't same as I still don't know why U-Boot does that trick. It comes 
> from a very
> old code and my initial understanding is that it is done purely for U-boot 
> bookkeeping.
/memreserve/ (and now) /reserved-regions/ are an easy way to find the 
regions that should be excluded from the RAM. Without that, you will 
need to have special case (such as for initrd, and the various xen boot 
moudles). I suspect that Linux have a special case and hence why it 
hasn't been a problem that Xen doesn't reserve the region.
Also, AFAIU, the Image protocol doesn't seem require the region to be 
reserved. It just says:
"""
If an initrd/initramfs is passed to the kernel at boot, it must reside
entirely within a 1 GB aligned physical memory window of up to 32 GB in
size that fully covers the kernel Image as well.

Any memory described to the kernel (even that below the start of the
image) which is not marked as reserved from the kernel (e.g., with a
memreserve region in the device tree) will be considered as available to
the kernel.
"""

Regarding the idea to mark the initird as reserved. My main concern is 
you now double the amount of memory that will be unusuable (AFAIU 
neither Xen nor Linux will be able to use the memory for allocations). I 
feel this is quite a steep price. A potential solution would to be map 
the initrd region rather than copying. Not sure how much code it will 
result, so someone will need to evaluate whether it is worth it.
Cheers,

--
Julien Grall