[RFC PATCH v2 41/48] efi_memory: add an event handler to update memory map

[Simon Glass]

Hi Sughosh,

On Mon, 15 Jul 2024 at 10:39, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > There are events that would be used to notify other interested modules
> > > of any changes in available and occupied memory. This would happen
> > > when a module allocates or reserves memory, or frees up memory. These
> > > changes in memory map should be notified to other interested modules
> > > so that the allocated memory does not get overwritten. Add an event
> > > handler in the EFI memory module to update the EFI memory map
> > > accordingly when such changes happen. As a consequence, any subsequent
> > > memory request would honour the updated memory map and only available
> > > memory would be allocated from.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > ---
> > > Changes since V1:
> > > * Handle the addition of memory to the LMB memory map.
> > > * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
> > >   based on the type of operation.
> > >
> > >  lib/efi_loader/Kconfig      |  1 +
> > >  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
> > >  2 files changed, 35 insertions(+)
> > >
> >
> > This is getting complicated and I don't believe it is needed.
> >
> > EFI should not be allocating memory 'in free space' until it starts
> > up. For the very few (if any) cases where it does, it can do an lmb
> > allocation.
>
> EFI memory module is not allocating memory at all now. This patch is
> adding an event handler for updating the EFI memory map, whenever the
> LMB memory map changes. All the EFI allocations are now being routed
> through the LMB API's.

OK

>
> >
> > As to the lmb allocations themselves, EFI can simply call look through
> > the lmb list and call efi_add_memory_map_pg() for each entry, when it
> > is ready to boot. There is no need to do it earlier.
>
> So in this case, I believe that rather than adding code in multiple
> places where the EFI memory module would have to get the LMB map and
> then update it's own, I think it is easier to update the EFI memory
> map as and when the LMB map gets updated. Else, we have a scenario
> where the EFI memory map would have to be updated as part of the EFI
> memory map dump function, as well as before the EFI boot. Any new code
> that would be subsequently introduced that might have a similar
> requirement would then be needed to keep this point in mind(to get the
> memory map from LMB).

That doesn't hold water in my eyes. I actually like the idea of the
EFI memory map being set up before booting. It should be done in a
single function called from one place, just before booting. Well, I
suppose it could be called from the memory-map-dump function too. But
it should be pretty simple...just add some pre-defined things and then
add the lmb records. You can even write a unit test for it.

>
> This is not an OS file-system kind of an operation where performance
> is critical, nor is this event(LMB memory map update) going to happen
> very frequently. So I believe that it would be better to keep the EFI
> memory map updated along with the LMB one.

I really don't like that idea at all. One table is enough for use by
U-Boot. The EFI one is needed for booting. Keeping them in sync as
U-Boot is running is not necessary, just invites bugs and makes the
whole thing harder to test.

Regards,
Simon