Re: [PATCH] efi_loader: remove EFI_BOUNCE_BUFFER

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Date: Mon, 17 Mar 2025 21:06:26 +0200

Hi Ilias,

> Hi Mark,
> Thanks for taking a look
> 
> On Mon, 17 Mar 2025 at 18:18, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > From: Ilias Apalodimas <ilias.apalodimas@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?

I think it is undesirable.  I believe it will measurably slow down
loading large files.

So probably best to leave the code as-is.  If somebody really cares,
they should probably separate out the cache flushing code from
CONFIG_BOUNCE_BUFFER first.  And maybe fix the DM bounce buffer code
such that it supports large buffers.

> > 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
>