linux-kernel - Re: [RFC PATCH 4/5] iommu/dma: Export non-static functions to use in modules

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Jul 2017 12:09:45 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     "open list:IOMMU DRIVERS" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Joerg Roedel <joro@...tes.org>,
        Will Deacon <will.deacon@....com>,
        Vineet Gupta <vgupta@...opsys.com>,
        Hans-Christian Noren Egtvedt <egtvedt@...fundet.no>,
        Mitchel Humpherys <mitchelh@...eaurora.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [RFC PATCH 4/5] iommu/dma: Export non-static functions to use in
 modules

On 06/07/17 03:25, Tomasz Figa wrote:
> On Thu, Jul 6, 2017 at 1:22 AM, Robin Murphy <robin.murphy@....com> wrote:
>> On 05/07/17 08:12, Tomasz Figa wrote:
>>> There is nothing wrong in having a loadable module implementing DMA API,
>>> for example to be used for sub-devices registered by the module. However,
>>> most of the functions from dma-iommu do not have their symbols exported,
>>> making it impossible to use them from loadable modules.
>>>
>>> Export all the non-static functions in the file, so that loadable modules
>>> can benefit from them. Use EXPORT_SYMBOL() for consistency with other
>>> exports in the file.
>>
>> To echo what Christoph said, everything not already exported here
>> shouldn't in any way be considered a driver-facing API in the general
>> sense, it's horrible glue code to sit behind an arch-specific DMA
>> mapping implementation (and frankly I'd consider even the current
>> exports more of an unfortunate abstraction leakage).
> 
> Well, if I remember correctly, we agreed that the IPU3 driver would
> benefit from using all the iommu_dma_*() helpers in its DMA ops,
> similarly to ARM64. This is IMHO much better than re-implementing them
> again internally just for this driver. However almost none of
> necessary helpers are currently exported...

Oh, for sure - I don't personally have much objection to arch code being
modular (even as part of a driver subsystem), I just don't want anyone
to get the impression that this layer is something that any old driver
can dip into as it fancies.

>>> Signed-off-by: Tomasz Figa <tfiga@...omium.org>
>>> ---
>>
>> [...]
>>
>>> @@ -829,17 +838,20 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
>>>       return __iommu_dma_map(dev, phys, size,
>>>                       dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
>>>  }
>>> +EXPORT_SYMBOL(iommu_dma_map_resource);
>>>
>>>  void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>>>               size_t size, enum dma_data_direction dir, unsigned long attrs)
>>>  {
>>>       __iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle, size);
>>>  }
>>> +EXPORT_SYMBOL(iommu_dma_unmap_resource);
>>
>> Do you need these two? Unless your custom DMA ops really have to support
>> slave DMA or other peer-to-peer traffic through their IOMMU, I'd be more
>> inclined to implement dma_map_resource as "return 0;" and ignore
>> dma_unmap_resource.
> 
> I don't need them. Getting an idea what is desirable to export and
> what not is actually one of the goals of this RFC.
> 
>>
>>> @@ -913,3 +925,4 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>>               msg->address_lo += lower_32_bits(msi_page->iova);
>>>       }
>>>  }
>>> +EXPORT_SYMBOL(iommu_dma_map_msi_msg);
>>
>> Given the nature of the kind of irqchip drivers this exists for, the
>> chances of one ever being modular seem vanishingly small.
> 
> Agreed. The IPU3 driver does not need it either.
> 
> Let me list the (not yet exported) helpers it requires:
> 
> dma-iommu.c
>  - iommu_dma_init,
>  - dma_info_to_prot,
>  - iommu_dma_free,
>  - iommu_dma_alloc,
>  - iommu_dma_mmap,
>  - iommu_dma_map_page,
>  - iommu_dma_unmap_page,
>  - iommu_dma_map_sg,
>  - iommu_dma_unmap_sg,
>  - iommu_dma_mapping_error,
> (added by my patch) iommu_dma_cleanup,
> 
> iommu.c
>  - iommu_group_get_for_dev,
> 
> base/dma-mapping.c
>  - dma_common_pages_remap,
>  - dma_common_free_remap,
> (added by my patch) dma_common_get_mapped_pages (OR find_vm_area),

I suppose another option is to just make the IOMMU and DMA ops a
self-contained non-modular driver mirroring the VT-d/AMD-Vi IOMMUs -
AFAICS it shouldn't have to be all that tightly coupled to the IPU bus
code, the latter more or less just needs to create the appropriate IOMMU
device for the driver to find.

Robin.

> 
> Best regards,
> Tomasz
>