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

[Robin Murphy <robin.murphy@arm.com>]

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@nvidia.com>
>>>>>
>>>>> 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.