| Message ID | 20240725135629.3505072-7-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilias Apalodimas |
| Headers | show |
| Series | efi: Start tidying up memory management | expand |
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. 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. > > 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@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. Best regards Heinrich > + > ret = efi_install_fdt(fdt); > if (ret != EFI_SUCCESS) > return ret;
Hi Heinrich, On Thu, 25 Jul 2024 at 10:36, Heinrich Schuchardt <xypron.glpk@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@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
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 + ret = efi_install_fdt(fdt); if (ret != EFI_SUCCESS) return ret;
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. 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@chromium.org> --- lib/efi_loader/efi_bootbin.c | 8 ++++++++ 1 file changed, 8 insertions(+)