On Sun, Nov 03, 2024 at 10:56:00PM +0100, Mark Kettenis wrote: > > Date: Sun, 3 Nov 2024 18:36:33 +0100 > > From: Janne Grunau <j...@jannau.net> > > > > On Sun, Nov 03, 2024 at 07:53:36PM +0530, Sughosh Ganu wrote: > > > On Sat, 2 Nov 2024 at 16:00, Mark Kettenis <mark.kette...@xs4all.nl> > > > wrote: > > > > > > > > > From: Sughosh Ganu <sughosh.g...@linaro.org> > > > > > Date: Fri, 1 Nov 2024 17:17:22 +0530 > > > > > > > > > > On Fri, 1 Nov 2024 at 15:47, Janne Grunau <j...@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.g...@linaro.org/ > > > > > > [2] > > > > > > https://lore.kernel.org/u-boot/20241031-iommu_apple_dart_ordering-v1-0-8a6877946...@jannau.net/ > > > > > > > > > > > > Signed-off-by: Janne Grunau <j...@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. > > Thanks Janne, you explained that very well and I 100% agree.
Yes, thanks, that's a helpful explanation. I believe this series makes sense in that regard, and I've reviewed 1/2. I'd like Sughosh at least to look at 3, in light of the above. -- Tom
signature.asc
Description: PGP signature
