[PATCH v3 03/15] efi: memory: use the lmb API's for allocating and freeing memory

[Simon Glass]

Hi Tom,

On Tue, 15 Oct 2024 at 13:05, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Oct 15, 2024 at 07:10:27AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 14 Oct 2024 at 17:07, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Mon, Oct 14, 2024 at 03:55:05PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 14 Oct 2024 at 15:48, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Mon, Oct 14, 2024 at 03:35:12PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Mon, 14 Oct 2024 at 15:27, Tom Rini <trini at konsulko.com> wrote:
> > > > > > >
> > > > > > > On Mon, Oct 14, 2024 at 03:13:01PM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Mon, 14 Oct 2024 at 15:04, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Oct 14, 2024 at 01:13:17PM -0600, Simon Glass wrote:
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Mon, 14 Oct 2024 at 09:56, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Oct 14, 2024 at 09:50:37AM -0600, Simon Glass wrote:
> > > > > > > > > > > > Hi Sughosh,
> > > > > > > > > > > >
> > > > > > > > > > > > On Sun, 13 Oct 2024 at 04:55, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Use the LMB API's for allocating and freeing up memory. With this, the
> > > > > > > > > > > > > LMB module becomes the common backend for managing non U-Boot image
> > > > > > > > > > > > > memory that might be requested by other modules.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > Changes since V2:
> > > > > > > > > > > > > * Use map_to_sysmem() to get the user-visible address to be shared
> > > > > > > > > > > > >   with the lmb API's for sandbox.
> > > > > > > > > > > > >
> > > > > > > > > > > > >  lib/efi_loader/Kconfig      |  1 +
> > > > > > > > > > > > >  lib/efi_loader/efi_memory.c | 77 +++++++++++--------------------------
> > > > > > > > > > > > >  2 files changed, 24 insertions(+), 54 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > When efi_init_obj() is called, it should be able to add the lmb memory
> > > > > > > > > > > > to its own tables. There is no need to worry about lmb after that,
> > > > > > >
> > > > > > > Saying "There is no need to worry about lmb after that" is not true.
> > > > > > > Invoking the "env" command for example will have efi_init_obj() be
> > > > > > > called, among the others that Heinrich listed. And to possibly refute
> > > > > > > a next issue, that is intentional so that efivars, the standard
> > > > > > > mechanism used by an OS to talk with the firmware can be available to
> > > > > > > U-Boot, if I recall things correctly.
> > > > > > >
> > > > > > > My understanding of your assumption is that you believe that once the
> > > > > > > EFI_LOADER subsystem has started work on a payload we're just a few call
> > > > > > > chains away from the OS being started and runtime services aside U-Boot
> > > > > > > being done.
> > > > > > >
> > > > > > > My understanding of how things are used today is that this is incorrect.
> > > > > >
> > > > > > What I am getting at is that once we have called that function we know
> > > > > > we are booting an EFI app or using an EFI feature in preparation for
> > > > > > doing so. Let's start there. Is that correct?
> > > > >
> > > > > No, it is not correct.
> > > >
> > > > Can you give me a code link?
> > >
> > > env_set() -> env_do_env_set() -> do_env_set_efi() -> efi_init_obj_list()
> >
> > That's with the '-e' option, though, so people doing that are
> > specifically choosing EFI.
>
> They're choosing a feature, yes and from there may not use any more, and
> can / will be using "load" to put things in to memory.
>
> > What I am saying is that, if we are not booting an EFI app or using an
> > EFI feature. Since 'env set -e' is an EFI feature, efi_init_obj_list()
> > is called, and EFI is in the picture.
> >
> > Even if efi_init_obj_list(), I don't like treating U-Boot's memory as
> > a stack to just grow into. But as a first step, I do want to ensure
> > that non-EFI boot can be more like U-Boot.
>
> I think you intended a negation somewhere in that last sentence.

It looks right to me.

>
> But among the points trying to be made here are that no, we didn't start
> an EFI app (a previous point of contention you had), even if we used an
> EFI feature. We may or may not use more.

Yes, we have ended up with what I feel is a dog's breakfast. I'd like
to tidy it up.

>  But saying "I guess now we can
> live with the EFI pool doing what it needs" is quite silly since we've
> potentially done almost nothing else.

Do you mean that it is inconsistent for me to say that when
efi_init_obj_list() is called, any memory can be used? Yes it is
inconsistent. In fact I may as well just say that we should use
U-Boot's region until we can't, e.g. we exhaust the reserved space. By
that time, the boot will be well-advanced, I suspect. I'll take a look
at some Ubuntu boots and see what happens in practice.

> And so why did we make this
> special case anyways?

Once an EFI feature is used, we know we are booting EFI, so it might
start using memory outside the U-Boot region. That's all.

> This is why I keep saying various versions of, you
> need to re-think what you consider "U-Boot memory". If you have concerns
> about our stack smashing in to things, that's the problem to address (it
> could just as easily smash an initrd placed high in memory by a non-EFI
> bootmeth).

I am not yet ready to re-think that, because my intuition tells me
that I have this right. I would like to get some of my ideas in, to
solve this problem. To do that, we need to be willing to let U-Boot
allocate memory (on EFI's behalf) as it wishes, within the spec, not
necessarily based on just what the code does today.

Regards,
SImon