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

[Leon Romanovsky]

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

Thanks

> 
> Thanks,
> Robin.