> From: Ilias Apalodimas <ilias.apalodi...@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.kette...@xs4all.nl> wrote:
> >
> > > From: Ilias Apalodimas <ilias.apalodi...@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
>
