Thanks all this is helpful
On Fri, 28 Mar 2025 at 19:15, Mark Kettenis <mark.kette...@xs4all.nl> wrote:
>
> > Date: Fri, 28 Mar 2025 10:04:19 -0600
> > From: Tom Rini <tr...@konsulko.com>
> >
> > On Fri, Mar 28, 2025 at 02:26:39PM +0200, Ilias Apalodimas wrote:
> > > On Fri, 28 Mar 2025 at 13:34, Simon Glass <s...@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Thu, 27 Mar 2025 at 15:19, Ilias Apalodimas
> > > > <ilias.apalodi...@linaro.org> wrote:
> > > > >
> > > > > Hi Simon
> > > > >
> > > > > On Thu, 27 Mar 2025 at 15:33, Simon Glass <s...@chromium.org> wrote:
> > > > > >
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Wed, 26 Mar 2025 at 02:37, Ilias Apalodimas
> > > > > > <ilias.apalodi...@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Heinrich,
> > > > > > >
> > > > > > > On Mon, 24 Mar 2025 at 19:50, Heinrich Schuchardt
> > > > > > > <xypron.g...@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?
>
> Fundamentall, yes, this isn't an EFI problem.
+1 it's not. As Heinrich explained EFI just makes it a lot easier to trigger
>
> > You could hit the same problem reading a file from a filesystem
> > outside of EFI too.
>
> Yes, but we tend to choose the addresses in the env variables that are
> used in the more traditional boot methods to prevent this.
>
> > 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).
>
> The existing bounce buffer code was written to solve a completely
> different problem. But it could indeed be generalized to solve this
> problem as well. That requires somebody willing to work on a larger
> set of actual hardware that includes SoC with cache-coherency
> challanges (which is what the current bounce buffer implementation is
> there for) and SoCs with DMA addressing challenges.
So, I think if we
- Use LMB instead of memalign for the buffer allocation
- Add an explicit flag on the API to cache flush or not. We could add
this to the existing DM code
- Revisit the 'bb' variable usage and set it correctly on
drivers/platforms that need the bounce buffer
We should be covered, no?
Thanks
/Ilias
