[PATCH 04/16] event: add event to notify lmb memory map changes

[Sughosh Ganu]

On Fri, 27 Sept 2024 at 16:12, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 26 Sept 2024 at 14:29, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > On Thu, 26 Sept 2024 at 16:38, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Thu, 26 Sept 2024 at 09:12, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > On Wed, 25 Sept 2024 at 18:23, Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > >
> > > > > > On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas
> > > > > > <ilias.apalodimas at linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > > > > > > >
> > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote:
> > > > > > > > > > Add an event which would be used for notifying changes in the
> > > > > > > > > > LMB modules' memory map. This is to be used for having a
> > > > > > > > > > synchronous view of the memory that is currently in use, and that is
> > > > > > > > > > available for allocations.
> > > > > > > > >
> > > > > > > > > The synchronous view problem only exists because we are duplicating
> > > > > > > > > data. Store the EFI memory type in LMB and the problem vanishes.
> > > > > > > >
> > > > > > > > The LMB module only concerns itself with RAM memory. If I understand
> > > > > > > > correctly, you are proposing maintaining the EFI memory map within the
> > > > > > > > LMB module ? That would mean handling memory types other than
> > > > > > > > conventional memory in LMB.
> > > > > > >
> > > > > > > I am pretty sure I've asked this before, but do these *always* need to
> > > > > > > be in sync?
> > > > > > >
> > > > > > > The efi allocators will call LMB now. So when we allocate something
> > > > > > > gtom EFI, even if any potential changes from LMB haven't been
> > > > > > > published to EFI, we won't have any memory corruptions. Can't we just
> > > > > > > opportunistically update the memory map once someone requests it?
> > > > > >
> > > > > > I have given this a thought. Because what you mention, Simon has a
> > > > > > similar comment. But to achieve this, it would require generating a
> > > > > > new efi memory map afresh every time such a requirement comes up. This
> > > > > > would mean, create a new memory map, put in the conventional memory,
> > > > > > and add other memory types that were part of the existing memory map.
> > > > > > And then remove the older memory map. This would need to be done every
> > > > > > time a memory map is needed to be generated. And that would also
> > > > > > include instances when a user enters a command to get the current
> > > > > > memory map. I think notifying any changes to the lmb memory map to the
> > > > > > efi memory module is easier, and less error prone.
> > > > >
> > > > > We need to get some agreement on my memory-allocation patches first. I
> > > > > don't believe we are on the same page on those, despite some weeks of
> > > > > discussion. We need to resolve that issue first. I did try right from
> > > > > the start to first agree on the problem to be solved. We skipped that,
> > > > > so now we are having to do it now...
> > > >
> > > > These patches are currently under review. But fwiw, I think you are
> > > > aware that these patches are not related to what your patch series is
> > > > attempting. Your patches are related to the efi_allocate_pool()
> > > > function, whereas this series is trying to use LMB as the backend for
> > > > allocating pages, as requested from efi_allocate_pages(). So these are
> > > > not related. But like I said, you are aware of these details :)
> > >
> > > The thing is, if we actually tidy up the EFI allocations then there
> > > will be no use of allocate-pages before the app starts. So we won't
> > > need to 'sync' the two different sets of memory tables. There will
> > > just be one.
>
> The issue is, again, that we never actually identified the problem to
> be solved at the beginning. So we are suffering somewhat now from a
> lack of clarity. But in any case, let me try to respond to the points
> you raise.
>
> >
> > How do we get rid of using efi_allocate_pages() in U-Boot ? This
> > function is used, among other things, to allocate memory used for
> > loading images to memory. This can be a significant amount of memory.
> > Are you proposing using malloc() even for loading EFI images ?
>
> We actually don't use efi_allocate_pages() to load things into memory,
> or at least I hope we don't. We should be using lmb. No, we cannot use
> malloc() for loading images as the region is quite small on many
> boards. Heinrich mentioned it can be as small as 1-2MB on some. Also,
> it isn't really what malloc() is designed for.

So you are proposing using lmb for loading images. And that is
precisely what this series is doing -- changing the
efi_allocate_pages() function so that it calls the lmb API's to get
the memory. So then I wonder why are we not on the same page ? Yes, I
understand that you have a differing view when it comes to
efi_allocate_pool(), but the changes to the efi_allocate_pages() done
in this series are exactly what you refer to above.

>
> >
> > Moreover, the syncing of the memory maps is currently needed primarily
> > as we support printing the EFI memory map through a command. And I
> > mentioned in one of my earlier replies that not syncing the changes in
> > the LMB memory map with the EFI module would mean that the EFI memory
> > map would have to be generated afresh whenever there is a need for the
> > EFI memory map -- this would be either when requested from an EFI app,
> > or when a user asks through a command. Re-generating the EFI memory
> > map is not a trivial task, and syncing the changes in the LMB map as
> > and when they happen is much easier.
>
> Yes I have seen you make this point and I am sorry I didn't reply
> directly. The thing is, we don't care about the EFI memory until we
> boot the app. After all, it is only the app that actually needs it.
>
> I see that 'efi_init_obj_list()' is called by the efidebug command, so
> yes, it should show the memory map.
>
> But in my view of the world the memory map (before an app boots and
> does what it likes in terms of allocations, etc.) consists solely of:
>
> 1. U-Boot's memory (malloc() space, code, bloblist, stack,
> arch-specific stuff, etc.)
> 2. Images that were loaded (which are in lmb)
>
> Since we need to handle (1) by manually adding records to the EFI map,
> it isn't a big stretch to handle (2) with a for-loop through the lmb
> records.
>
>
> > > If I were confident that we could apply your series and then clean it
> > > up later, I would be fine with it. But given that my two series have
> > > sat here for a considerable period of time and are still not really
> > > making progress, I lack confidence.

So the difference here is, unlike the way the LMB memory map gets
stored and shown, i.e. a separate entry for the total available
memory, and then a separate list of currently allocated entries, the
EFI memory map storage is designed in a different way. The
conventional memory, which is the DRAM memory gets added first, and
that is then followed by carving out the different memory regions
which are being used, e.g. the U-Boot's memory, and other memory
regions that have been requested. So if you try out the 'efidebug
memmap' command, you will see a single list of memory regions, which
includes the conventional memory, and other reserved memory regions.
Although I did not come up with this design, I suspect that it is the
way it is because the EFI memory map is not dealing only with
DRAM/conventional memory, but other possible memory regions as well.

What this means is that syncing the EFI memory map in the manner that
you propose would mean that the entire EFI memory map would have to be
re-generated, as the carve-out's (reserved memory regions) might have
changed. Please also note that, apart from the added complexity, we
are also going to be worse off from the size impact point-of-view.
Especially if we consider Ilias's review comment of doing away with
the event framework, and directly calling the efi_add_memory_map_pg()
directly.

-sughosh

>
> Regards,
> Simon