[PATCH 6/6] efi: Show the location of the bounce buffer

[Simon Glass]

Hi Heinrich,

On Thu, 25 Jul 2024 at 10:36, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 25.07.24 15:56, Simon Glass wrote:
> > The EFI_LOADER_BOUNCE_BUFFER feature was added many years ago. It is not
> > clear whether it is still needed, but 24 boards (lx2160ardb_tfa_stmm,
> > lx2162aqds_tfa_SECURE_BOOT and the like) use it.
> >
> > This feature uses EFI page allocation to create a 64MB buffer 'in space'
> > without any knowledge of where boards intend to load their images. This
> > may result in image corruption or other problems.
>
> This is what Sughosh's LMB series in addressing.

Not really...it will still be 'in space' so will be annoying for some
boards. This series is separate from Sughosh's.

>
> With CONFIG_SYS_MALLOC_LEN=0x202000 for lx2160ardb_tfa_stmm we cannot
> move the buffer to malloc(). I don't understand why
> CONFIG_SYS_MALLOC_LEN is so small.

Because it is designed for small allocations, e.g. the equivalent of
EFI's pool. We have never used malloc() to load images, not for
allocating bounce buffers.

>
> >
> > For example, if the feature is enabled on qemu_arm64 it puts the EFI
> > bounce buffer at 1045MB, with the kernel at 1028MB and the ramdisk at
> > 1088MB. The kernel is probably smaller than 27MB but the buffer does
> > overlap the ramdisk.
> >
> > The solution is probably to use BOUNCE_BUFFER instead, with the EFI
> > version being dropped. For now, show the address of the EFI bounce
> > buffer so people have a better chance to detect the problem.
> >
> > Note: I avoided converting this #ifdef to use IS_ENABLED() since I hope
> > that the feature may be removed.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >   lib/efi_loader/efi_bootbin.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
> > index 07c8fca68cc..53e5e429d2e 100644
> > --- a/lib/efi_loader/efi_bootbin.c
> > +++ b/lib/efi_loader/efi_bootbin.c
> > @@ -211,6 +211,14 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
> >               return -1;
> >       }
> >
> > +#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
> > +     /*
> > +      * Add a warning about this buffer, since it may conflict with other
> > +      * things
> > +      */
> > +     printf("EFI bounce buffer at %p\n", efi_bounce_buffer);
> > +#endif
>
> Does this sound like a warning to you?
>
> There is nothing that a user can reasonably do when seeing this message
> while the board is booting via one of the boot methods. So we should not
> write it.

We could add a 'warning:' prefix, perhaps? At the moment there is no
clue that the bounce buffer may overwrite an image. What do you
suggest?

> > +
> >       ret = efi_install_fdt(fdt);
> >       if (ret != EFI_SUCCESS)
> >               return ret;
>

Regards,
Simon