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

[Simon Glass]

Hi Sughosh,

On Fri, 27 Sept 2024 at 05:01, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> 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.

Look, I certainly understand your frustration and I share it.

I would like to see progress on the series i sent, which moves pool
allocations to use malloc(). I would also like to see the EFI test
applied, which has been held up for a long time (perhaps the latest
version will be OK) I did discuss this with Tom and was OK with your
first series on the basis that we can figure out the next step after
that. But I never believed that the problem to be solved was that lmb
and EFI were not deeply integrated and I was expecting to have made
some progress on the malloc() issue by now.

I should also point out that no one else dug into this to understand
what was really going on (that EFI was allocating stuff where it
shouldn't, i.e. in space that is supposed to be used by lmb). I had an
inkling of it but had never spent the time to really understand it.

My current view on EFI memory is that, just before launching the app
(or firmware update, or whatever), we should be able to call
efi_memory_init(), which adds the known memory, the U-Boot regions and
the images it knows about. Only the last piece is missing today. As to
that, we need to clean up the EFI hack and maintain a list of images
and which device they came from, so that the EFI loader can find that
out. It isn't just about lmb.

So I hope you can understand that, coming from that point of view, all
this stuff about keeping things in sync doesn't seem right.

Regards,
Simon