Re: [PATCH] efi_loader: remove EFI_BOUNCE_BUFFER

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5322 bytes --]

On Fri, Mar 28, 2025 at 05:17:36PM +0100, Heinrich Schuchardt wrote:
> On 28.03.25 17:04, Tom Rini wrote:
> > On Fri, Mar 28, 2025 at 02:26:39PM +0200, Ilias Apalodimas wrote:
> > > On Fri, 28 Mar 2025 at 13:34, Simon Glass <sjg@chromium.org> wrote:
> > > > 
> > > > Hi Ilias,
> > > > 
> > > > On Thu, 27 Mar 2025 at 15:19, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > 
> > > > > Hi Simon
> > > > > 
> > > > > On Thu, 27 Mar 2025 at 15:33, Simon Glass <sjg@chromium.org> wrote:
> > > > > > 
> > > > > > Hi Ilias,
> > > > > > 
> > > > > > On Wed, 26 Mar 2025 at 02:37, Ilias Apalodimas
> > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > > 
> > > > > > > Hi Heinrich,
> > > > > > > 
> > > > > > > On Mon, 24 Mar 2025 at 19:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > 
> > > > > > > > On 17.03.25 14:38, Ilias Apalodimas wrote:
> > > > > > > > 
> > > > > > > > %s/EFI_BOUNCE_BUFFER/CONFIG_EFI_LOADER_BOUNCE_BUFFER/
> > > > > > > > 
> > > > > > > > > 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()
> > > > > > > > 
> > > > > > > > Looking at
> > > > > > > > 
> > > > > > > >       if (IS_ENABLED(CONFIG_BOUNCE_BUFFER) && desc->bb) {
> > > > > > > > 
> > > > > > > > in drivers/block/blk-uclass.c the bounce buffer has to be explicitly
> > > > > > > > enabled by the device driver. Only the scsi drivers sets bb = true.
> > > > > > > > 
> > > > > > > > Cf. 81bd22e935dc ("rockchip: block: blk-uclass: add bounce buffer flag
> > > > > > > > to blk_desc")
> > > > > > > > 
> > > > > > > > Which device-drivers of the boards mentioned below do actually need
> > > > > > > > bounce buffering?
> > > > > > > 
> > > > > > > Unfortunately, I don't have any of the hardware to test and I havent
> > > > > > > worked with that platform much.
> > > > > > > That 'bb' variable and the fact that EFI needs bigger allocations is
> > > > > > > why I ended up allocationg properly aligned memory from the EFI
> > > > > > > subsystem. But as Mark pointed out, the cache flush is a no go for
> > > > > > > now, so I'll drop this and see if I find time to rework the bounce
> > > > > > > buffer logic overall
> > > > > > 
> > > > > > There was quite a bit of discussion about all this in the context of
> > > > > > my attempt to just add a message to warn the user[1]
> > > > > > 
> > > > > > We might consider adding an event to reserve memory before relocation,
> > > > > > along with a way to discover (in board_r) where the memory was
> > > > > > allocated. That would make the solution more generic.
> > > > > 
> > > > > I am not sure what you are trying to solve here. The EFI bounce buffer
> > > > > after the LMB patches can't overwrite memory, nor can it be
> > > > > overwritten.
> > > > 
> > > > I am thinking of we can create a single implementation of the
> > > > bouncebuf logic which also works for EFI.
> > > > 
> > > > I think the two sane things to do are:
> > > > - restrict U-Boot to using memory below 4GB for platforms which have
> > > > the DMA limitation
> > > 
> > > You don't need that. The bounce buf code has a callback you can use to
> > > define the limitations
> > > 
> > > > - create (in board_f) a special region below 4GB for use with the
> > > > bouncebuf logic
> > > 
> > > The only problem with EFI is that you don't know how much memory it
> > > needs and we can't use the existing memalign calls. So if we replace
> > > that memalign in the bounce buffer code, with an lmb reservation we
> > > have everything we need.
> > 
> > It's not even an EFI problem is it? You could hit the same problem
> > reading a file from a filesystem outside of EFI too. These specific
> > SoCs just aren't heavily exercised is one of the challenges I think and
> > so it's possible that we have a few things to yet improve in the
> > bounce buffer code (which was added for other SoCs and done as generic
> > enough starting point for others).
> > 
> 
> EFI or LMB allocate memory top down. So EFI applications have a good
> chance of loading files into high memory. Other file-system operations
> typically use predefined addresses line $loadaddr. This is why the
> problem became more visible in EFI.
> 
> It is evident that the bounce-buffer functionality is not EFI specific
> per se.

Right. But it's not the destination address we're talking about but
rather that for "fun" system design reasons we need to have the storage
IP block read from device to one address and then bounce it over to the
final address. Or have I missed understood which kind of bounce buffer
we're needing something here for?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]