On 30.07.2025 13:11, Robin Murphy wrote: > On 2025-07-08 11:27 am, Marek Szyprowski wrote: >> On 30.06.2025 15:38, Christoph Hellwig wrote: >>> On Fri, Jun 27, 2025 at 08:02:13PM +0300, Leon Romanovsky wrote: >>>>> Thanks for this rework! I assume that the next step is to add >>>>> map_phys >>>>> callback also to the dma_map_ops and teach various dma-mapping >>>>> providers >>>>> to use it to avoid more phys-to-page-to-phys conversions. >>>> Probably Christoph will say yes, however I personally don't see any >>>> benefit in this. Maybe I wrong here, but all existing .map_page() >>>> implementation platforms don't support p2p anyway. They won't benefit >>>> from this such conversion. >>> I think that conversion should eventually happen, and rather sooner >>> than >>> later. >> >> Agreed. >> >> Applied patches 1-7 to my dma-mapping-next branch. Let me know if one >> needs a stable branch with it. > > As the maintainer of iommu-dma, please drop the iommu-dma patch > because it is broken. It does not in any way remove the struct page > dependency from iommu-dma, it merely hides it so things can crash more > easily in circumstances that clearly nobody's bothered to test. > >> Leon, it would be great if You could also prepare an incremental patch >> adding map_phys callback to the dma_maps_ops, so the individual >> arch-specific dma-mapping providers can be then converted (or simplified >> in many cases) too. > > Marek, I'm surprised that even you aren't seeing why that would at > best be pointless churn. The fundamental design of dma_map_page() > operating on struct page is that it sits in between alloc_pages() at > the caller and kmap_atomic() deep down in the DMA API implementation > (which also subsumes any dependencies on having a kernel virtual > address at the implementation end). The natural working unit for > whatever replaces dma_map_page() will be whatever the replacement for > alloc_pages() returns, and the replacement for kmap_atomic() operates > on. Until that exists (and I simply cannot believe it would be an > unadorned physical address) there cannot be any *meaningful* progress > made towards removing the struct page dependency from the DMA API. If > there is also a goal to kill off highmem before then, then logically > we should just wait for that to land, then revert back to > dma_map_single() being the first-class interface, and dma_map_page() > can turn into a trivial page_to_virt() wrapper for the long tail of > caller conversions. > > Simply obfuscating the struct page dependency today by dressing it up > as a phys_addr_t with implicit baggage is not not in any way helpful. > It only makes the code harder to understand and more bug-prone. > Despite the disingenuous claims, it is quite blatantly the opposite of > "efficient" for callers to do extra work to throw away useful > information with page_to_phys(), and the implementation then have to > re-derive that information with pfn_valid()/phys_to_page(). > > And by "bug-prone" I also include greater distractions like this > misguided idea that the same API could somehow work for non-memory > addresses too, so then everyone can move on bikeshedding VFIO while > overlooking the fundamental flaws in the whole premise. I mean, > besides all the issues I've already pointed out in that regard, not > least the glaring fact that it's literally just a worse version of *an > API we already have*, as DMA API maintainer do you *really* approve of > a design that depends on callers abusing DMA_ATTR_SKIP_CPU_SYNC, yet > will still readily blow up if they did then call a dma_sync op? > Robin, Your concerns are right. I missed the fact that making everything depend on phys_addr_t would make DMA-mapping API prone for various abuses. I need to think a bit more on this and try to understand more the PCI P2P case, what means that I will probably miss this merge window. I'm sorry for the lack of being active in the discussion, but I just got back from my holidays and I'm trying to catch up.
Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
