[PATCH] efi_loader: remove EFI_BOUNCE_BUFFER

[Ilias Apalodimas]

Hi Mark,
Thanks for taking a look

On Mon, 17 Mar 2025 at 18:18, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > From: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > Date: Mon, 17 Mar 2025 15:38:36 +0200
> >
> > The EFI subsystem defines its own bounce buffer for devices that
> > can't transfer data > 4GB. U-Boot already has a generic BOUNCE_BUFFER
> > which can be reused instead of defining another symbol.
> > The only limitation for EFI is that we don't know how big the file a user
> > chooses to transfer is and as a result we can't depend on allocating the
> > memory from the malloc area, which can prove too small.
> >
> > So allocate an EFI buffer of the correct size and pass it to the DM,
> > which already supports bounce buffering via bounce_buffer_start_extalign()
>
> The existing bounce buffer code servers a completely different purpose
> though.  It exists to make sure that hardware with cache-incoherent
> DMA can safely do the required cache flushes.
>
> This means that:
>
> * SoCs with cache-coherent DMA don't necessarily set BOUNCE_BUFFER.
>   Looks like you added that option to all the SoCs where you remove
>   EFI_LOADER_BOUNCE_BUFFER.

Yes, and that has a side effect I should have probably added to the
commit message. Using the existing bounce buffer will flush caches
even if it's pointless.

>
> * SoCs that (now) set BOUNCE_BUFFER may double bounce if the buffers
>   aren't properly aligned.  I suppose that this won't happen since
>   efi_disk_add_dev() sets medio.io_align to the device block size
>   which is typically larger than the cache line size.

I think it won't happen indeed but for a different reason. The EFI
memory we allocate and pass to bounce_buffer_start_extalign() is
page-aligned.
The DM subsystem will call that function with either
blk_buffer_aligned() which always returns 1 or whatever the device has
defined. The strictest one I found was the virtio one which requires
page alignment.

Allocating memory from EFI is needed, simply because the current
bounce buffer API will use the malloc area, which might not be enough.
Do you think the extra cache flush is a no-go and we should leave the
code as-is?

>
> Still the commit message is somewhat misleading; this code doesn't
> really make us use the DM bounce buffering code.
>
> I also spotted a few bugs in the implementation.  See below.

[...]

> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -105,6 +105,8 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
> >       int blksz;
> >       int blocks;
> >       unsigned long n;
> > +     u64 bb = 0xffffffff;
> > +     void *bb_ptr = buffer;
> >
> >       diskobj = container_of(this, struct efi_disk_obj, ops);
> >       blksz = diskobj->media.block_size;
> > @@ -113,27 +115,35 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
> >       EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n",
> >                 blocks, lba, blksz, direction);
> >
> > +     if (IS_ENABLED(CONFIG_BOUNCE_BUFFER) && (uintptr_t)buffer >= SZ_4G + buffer_size - 1) {
>
> Shouldn't that check be
>
>         if (IS_ENABLED(CONFIG_BOUNCE_BUFFER) && (uintptr_t)buffer + buffer_size - 1 >= SZ_4G) {
>
> ?

Yes...
I originally had (uintptr_t)buffer > SZ_4G - buffer_size - 1 and avoid
potential overflows, but then I started to think what happens if
buffer_size is 4GB and completely messed this up ...
I think it's (uintptr_t)buffer + buffer_size + 1 >= SZ_4G though,
because SZ_4G is 0x100000000. Anyway yes, you are right, I'll fix it
in v2, but using subtractions.

>
> > +             if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA,
> > +                                    buffer_size >> EFI_PAGE_SHIFT, &bb) != EFI_SUCCESS)
> > +                     return EFI_OUT_OF_RESOURCES;
> > +
> > +             bb_ptr = (void *)(uintptr_t)bb;
> > +     }
> >       /* We only support full block access */
> > -     if (buffer_size & (blksz - 1))
> > +     if (buffer_size & (blksz - 1)) {
> > +             if (buffer != bb_ptr)
> > +                     efi_free_pages(bb, buffer_size >> EFI_PAGE_SHIFT);
> >               return EFI_BAD_BUFFER_SIZE;
> > +     }
>
> Any reason why you don't check the buffer_size check before allocating
> the bounce buffer?  That way you don't have to worry about freeing it
> here.

Nop, that code was already there and I  didn't move it. I'll move it around.

>
> You're missing a memcpy() here in the case direction == EFI_DISK_WRITE.

ah thanks

>
> >       if (CONFIG_IS_ENABLED(PARTITIONS) &&
> >           device_get_uclass_id(diskobj->header.dev) == UCLASS_PARTITION) {
> >               if (direction == EFI_DISK_READ)

[...]

> > +                     efi_free_pages(bb, buffer_size >> EFI_PAGE_SHIFT);
> >               return EFI_DEVICE_ERROR;
> > +     }
> > +
> > +     if (buffer != bb_ptr) {
> > +             memcpy(buffer, bb_ptr, buffer_size);
>
> This memcpy() is only necessary if direction == EFI_DISK_READ.

Ok

[...]

Thanks
/Ilias