Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Robin Murphy]

On 12/07/2024 6:50 am, Leon Romanovsky wrote:
> On Thu, Jul 11, 2024 at 09:08:49PM +0100, Robin Murphy wrote:
> > On 2024-07-11 7:57 pm, Leon Romanovsky wrote:
> > > On Thu, Jul 11, 2024 at 07:23:20PM +0100, Robin Murphy wrote:
> > > > On 11/07/2024 11:38 am, Leon Romanovsky wrote:
> > > > > From: Leon Romanovsky <leonro@xxxxxxxxxx>
> > > > >
> > > > > Most of the IOMMU drivers are using the same DMA operations, which are
> > > > > default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
> > > > > to properly set them as a default with direct call without need to
> > > > > perform function pointer dereference.
> > > > >
> > > > > During system initialization, the IOMMU driver can set its own DMA and
> > > > > in such case, the default DMA operations will be overridden.
> > > >
> > > > I'm going to guess you don't actually mean "IOMMU drivers" in the usual
> > > > sense of drivers/iommu/, but rather "arch DMA ops (which often, but not
> > > > always, involve some sort of IOMMU)."
> > >
> > > Yes, you are right. I used word "drivers" as a general term to describe
> > > everything that implements their own ->map_page() callback.
> > >
> > > > If so, I'd much rather see this done properly, i.e. hook everything up
> > > > similarly to dma-direct and be able to drop CONFIG_DMA_OPS for "modern"
> > > > dma-direct/iommu-dma architectures entirely. Furthermore the implementation
> > > > here isn't right - not only is it not conceptually appropriate to make
> > > > iommu-dma responsibile for proxying random arch DMA ops, but in practial
> > > > terms it's just plain broken, since the architectures which still have their
> > > > own DMA ops also don't use iommu-dma, so this is essentially disabling the
> > > > entire streaming DMA API on ARM/PowerPC/etc.
> > >
> > > Sorry but I'm not sure that I understood your reply. Can you please clarify
> > > what does it mean broken in this context?
> > >
> > > All archs have one of the following options:
> > > 1. No DMA ops -> for them dma_map_direct() will return True and they
> > > never will enter into IOMMU path.
> > > 2. Have DMA ops but without .map_page() -> they will use default IOMMU
> > > 3. Have DMA ops with .map_page() -> they will use their own implementation
> >
> > Urgh, sorry, let that instead be a lesson in not adding needless layers of
> > indirection that are named as confusingly as possible, then. Seems I saw
> > stubs plus static inline wrappers, managed to misread dma_iommu_* vs.
> > iommu_dma_*, and jump to the wrong conclusion of what was stubbed out, not
> > least since that was the only interpretation in which adding an extra layer
> > of inline wrappers would seem necessary in the first place. If these
> > dma_iommu_* functions serve no purpose other than to make the code even more
> > of a maze of twisty little passages, all alike, then please just feed them
> > to a grue instead.
>
> This is done to keep layering similar to existing in DMA subsystem. We
> have special files and calls to dma-direct, it looks natural to have
> special files and call to dma-iommu. It is not nice to call to drivers/iommu
> from kernel/dma/mapping.c

That's where I firmly disagree. In the DMA API aspect, iommu-dma is 
exactly a peer of dma-direct, however it lives in drivers/iommu for 
practical reasons because it's more closely coupled to IOMMU API 
internals in all other aspects of its implementation. TBH what I'd 
really like is a private interface between mapping.c and dma-iommu.c 
which doesn't expose internal details via include/linux/ at all, but 
*that's* a level that I think people may rightfully start to object to.

Thanks,
Robin.