Re: [PATCH] xen/arm/efi: merge neighboring banks

[Julien Grall <julien@xxxxxxx>]

On 21/03/2025 22:22, Julien Grall wrote:
> Hi Stefano,
>
> On 21/03/2025 21:14, Stefano Stabellini wrote:
> > When booting from U-Boot bootefi, there can be a high number of
> > neighboring RAM banks. See for example:
> >
> > (XEN) RAM: 0000000000000000 - 0000000000bfffff
> > (XEN) RAM: 0000000000c00000 - 0000000000c00fff
> > (XEN) RAM: 0000000000c01000 - 0000000000dfffff
> > (XEN) RAM: 0000000000e00000 - 000000000279dfff
> > (XEN) RAM: 000000000279e000 - 00000000029fffff
> > (XEN) RAM: 0000000002a00000 - 0000000008379fff
> > (XEN) RAM: 000000000837a000 - 00000000083fffff
> > (XEN) RAM: 0000000008400000 - 0000000008518fff
> > (XEN) RAM: 0000000008519000 - 00000000085fffff
> > (XEN) RAM: 0000000008600000 - 0000000008613fff
> > (XEN) RAM: 0000000008614000 - 00000000097fffff
> > (XEN) RAM: 0000000009800000 - 00000000098a7fff
> > (XEN) RAM: 00000000098a8000 - 0000000009dfffff
> > (XEN) RAM: 0000000009e00000 - 0000000009ea7fff
> > (XEN) RAM: 0000000009ea8000 - 000000001fffffff
> > (XEN) RAM: 0000000020000000 - 000000002007ffff
> > (XEN) RAM: 0000000020080000 - 0000000077b17fff
> > (XEN) RAM: 0000000077b19000 - 0000000077b2bfff
> > (XEN) RAM: 0000000077b2c000 - 0000000077c8dfff
> > (XEN) RAM: 0000000077c8e000 - 0000000077c91fff
> > (XEN) RAM: 0000000077ca7000 - 0000000077caafff
> > (XEN) RAM: 0000000077cac000 - 0000000077caefff
> > (XEN) RAM: 0000000077cd0000 - 0000000077cd2fff
> > (XEN) RAM: 0000000077cd4000 - 0000000077cd7fff
> > (XEN) RAM: 0000000077cd8000 - 000000007bd07fff
> > (XEN) RAM: 000000007bd09000 - 000000007fd5ffff
> > (XEN) RAM: 000000007fd70000 - 000000007fefffff
> > (XEN) RAM: 0000000800000000 - 000000087fffffff
> >
> > It is undesirable to keep them separate, as this approach consumes more
> > resources.
> What resources? This is pre-allocated static array in the binary. They 
> are also dropped after init. The more interesting argument is...
> > Additionally, Xen does not currently support boot modules that span
> > multiple banks: at least one of the regions get freed twice. The first
> > time from setup_mm->populate_boot_allocator, then again from
> > discard_initial_modules->fw_unreserved_regions. With a high number of
> > banks, it can be difficult to arrange the boot modules in a way that
> > avoids spanning across multiple banks.
> ... this one. Although, I find weird that U-boot would create tons of 
> banks. Have you considered to ask them what's going on?
> Also, what about the cases where U-boot is not booting Xen without UEFI? 
> Why is this not a problem? Asking just in case the merge should happen 
> in code common rather than in UEFI.
> > This small patch merges neighboring regions, to make dealing with them
> > more efficient, and to make it easier to load boot modules.
> While I understand the value for this, I ...
>
> > The patch
> > also takes the opportunity to check and discard duplicates.
> don't understand why we need to check for duplicates. This also doesn't 
> properly work for a few reasons:
>    * This doesn't cover partial overlap
>    * This would not work if an entry was merged with another previously
>
> So I think the code to check duplicates should be removed. If you are 
> concerned about overlap, then it would be better to enable 
> check_reserved  just drop the code. If you are concerned about to detect 
> and warn/panic.
What I wrote is a bit mangled. I meant that if you are concern about 
duplicates then it would be better to check if there is an overlap and
either warn or panic.

Cheers,

--
Julien Grall