[PATCH 0/4] Fix IOVA allocation in Apple dart iommu after global LMB mem map changes

[Janne Grunau]

On Sun, Nov 03, 2024 at 07:53:36PM +0530, Sughosh Ganu wrote:
> On Sat, 2 Nov 2024 at 16:00, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> >
> > > From: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > Date: Fri, 1 Nov 2024 17:17:22 +0530
> > >
> > > On Fri, 1 Nov 2024 at 15:47, Janne Grunau <j at jannau.net> wrote:
> > > >
> > > > The changes in "Make LMB memory map global and persistent" [1] break
> > > > mapping DMA memory in the USB xHCI driver when using the apple_dart
> > > > iommu present on Apple silicon systems.
> > > >
> > > > The IOVA space used by the u-boot driver (low 4GB) and physical memory
> > > > do not overlap. The physical memory on this systems starts depending on
> > > > the SoC either at 0x10_0000_0000 or 0x100_0000_0000. It make no sense to
> > > > manage these distinct regions in a single LMB map. In addition every
> > > > device has its own iommu and IO address space so sharing a single memory
> > > > map between all iommu instances is not necessary.
> > > >
> > > > To fix this issue restore the used subset (add, alloc and free) of the
> > > > previous pointer based LMB interface with "io_" as prefix.
> > > >
> > > > To ensure that low level lmb functions do not use the global LMB
> > > > variable reorder lib/lmb.c so that the variable is not visible.
> > > >
> > > > Tested with patches from my "Fix device removal order for Apple dart
> > > > iommu" series [2] to fix a separate issue.
> > > >
> > > > The cosmetic commit has two checkpatch warnings in existing code which I
> > > > ignored.
> > > >
> > > > [1] https://lore.kernel.org/u-boot/20240826115940.3233167-1-sughosh.ganu@linaro.org/
> > > > [2] https://lore.kernel.org/u-boot/20241031-iommu_apple_dart_ordering-v1-0-8a6877946d6b@jannau.net/
> > > >
> > > > Signed-off-by: Janne Grunau <j at jannau.net>
> > > > ---
> > > > Janne Grunau (4):
> > > >       lmb: Do not use global LMB variable in _lmb_free()
> > > >       lmb: cosmetic: reorder functions and global LMB variable
> > > >       lmb: Add basic io_lmb functionality
> > > >       iommu: apple: Manage IOVA separately from global LMB mem map
> > >
> > > Thanks for these changes. I am currently on leave, but will take a
> > > look at this in detail next week. But on a cursory glance, I have to
> > > say that I am not entirely satisfied with this solution as well. With
> > > these patches, we now have a disparity in terms on how we view the LMB
> > > memory map -- we have a global map for RAM address range, but a per
> > > device one for IOVA's. I understand that IOVA's are supposed to be
> > > per-device,
> >
> > The IVOA's are per-stream.  Multiple devices might share a single
> > stream.
> >
> > > but then I think that perhaps LMB is not the right way to
> > > solve this. This was fine earlier, when the entire LMB implementation
> > > in U-Boot was local, but that is not the case now. I wonder if there
> > > can be a different, non-LMB way to implement this.
> >
> > Of course there can be a different implementation.  But why add a
> > separate implementation for what is essentially the same problem:
> > allocate blocks out of an address space.
> 
> My primary concern here is to prevent setting a precedent that it is
> okay to have a local, per caller lmb instance.

I think the issue is thinking this is per caller. It is not (primarily)
by caller. It is one LMB instance per managed independent address space
block. Apple silicon SoCs have multiple independent iommus. Each iommu
can handle multiple address spaces (streams, only partially implemented
in the u-boot driver.

> If it is allowed for
> one caller, it becomes difficult to say no to someone proposing a
> similar approach later. In any case, based on how the allocation logic
> works, with all the addresses being allocated from the IOMMU driver's
> map function callback, why can there not be a single instance of an
> iova lmb structure. That would at least ensure some kind of parity on
> how the lmb maps are maintained.

There are multiple fully independent IOVA spaces. I doesn't make sense
to allocate them from a single pool.

> > Before you changed the goal posts, lmb did exactly that.  The only
> > problem was that there were multiple lmb instances covering the same
> > "main memory" address space.
> 
> I had provided an explanation of the situation that existed prior to
> these changes in a writeup [1]. And that writeup had clearly provided
> the two possible solutions to fix the issue with EFI allocated memory
> working with LMB. And it was decided through discussion on that
> writeup as well as on the mailing list that this was the approach to
> be taken.

That document missed due to unfortunate circumstances one special case
in the LMB usage. Consolidating all struct lmb referring main memory
into a single opaque object is useful. Please let's move on to fix the
single usage which broke due to the change.

>From my point of view it's clear that "io_lmb*" is separate from the
main memory lmb. It manages the IO virtual address space of devices and
it is fine that there as many (io_)lmb instances as there are IO virtual
address spaces. These do not have to be managed via struct lmb and
looking at the minimal io_lmb* API a simpler implementation would work.
An alternative implementation would be duplicated code and reusing the
existing tested lmb code is desireable.
If we want a stronger distinction between lmb and io_lmb we could hide
struct lmb from io_lmb.

Janne