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

[Simon Glass]

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.

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

Regards,
Simon