Re: [PATCH 03/12] intel-ipu3: Add DMA API implementation
On 09/06/17 07:20, Tomasz Figa wrote:
> On Fri, Jun 9, 2017 at 3:07 AM, Robin Murphy wrote:
>> On 08/06/17 15:35, Tomasz Figa wrote:
>>> On Thu, Jun 8, 2017 at 10:22 PM, Robin Murphy wrote:
On 07/06/17 10:47, Tomasz Figa wrote:
> Hi Yong,
>
> +Robin, Joerg, IOMMU ML
>
> Please see my comments inline.
>
> On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi wrote:
>>> [snip]
>> +
>> +/* End of things adapted from arch/arm/mm/dma-mapping.c */
>> +static void ipu3_dmamap_sync_single_for_cpu(struct device *dev,
>> + dma_addr_t dma_handle,
>> size_t size,
>> + enum dma_data_direction dir)
>> +{
>> + struct ipu3_mmu *mmu = to_ipu3_mmu(dev);
>> + dma_addr_t daddr = iommu_iova_to_phys(mmu->domain, dma_handle);
>> +
>> + clflush_cache_range(phys_to_virt(daddr), size);
>
> You might need to consider another IOMMU on the way here. Generally,
> given that daddr is your MMU DMA address (not necessarily CPU physical
> address), you should be able to call
>
> dma_sync_single_for_cpu(, daddr, size, dir)
I'd hope that this IPU complex is some kind of embedded endpoint thing
that bypasses the VT-d IOMMU or is always using its own local RAM,
because it would be pretty much unworkable otherwise.
>>>
>>> It uses system RAM and, as far as my understanding goes, by default it
>>> operates without the VT-d IOMMU and that's how it's used right now.
>>
>> OK, if it *is* behind a DMAR unit then booting with "iommu=force" (or
>> whatever the exact incantation for intel-iommu is) should be fun...
>>
>>> I'm suggesting VT-d IOMMU as a way to further strengthen the security
>>> and error resilience in future (due to the IPU complex being
>>> non-coherent and also running a closed source firmware).
>>
>> TBH, doing DMA remapping through *two* IOMMUS will add horrible hardware
>> overhead,
>
> Not necessarily, if done right and with right hardware (I lack the
> details about Intel hardware unfortunately). One can for example
> notice the fact that the IOVA ranges from the parent IOMMU are going
> to be contiguous for the child IOMMU, so one could use huge pages in
> the child IOMMU and essentially make a selective 1:1 mapping.
Note that many IOMMUs don't actually implement TLBs for larger page
sizes, even if they support them. And that still doesn't really help the
main issue of the way in which nested table walks blow up exponentially:
For a n-level pagetable at the first level and an m-level table at the
second level, a single access by the device can become m * (n + 1)
memory accesses (and that's generously ignoring additional things like
source-ID-to-context lookups).
>> increase the scope for kernel-side bugs, and not much more. If
>> we don't trust this IOMMU to behave, why are we trying to drive it in
>> the first place? If we do, then a second IOMMU behind it won't protect
>> anything that the first one doesn't already.
>
> That's a valid point, right. But on the other hand, I lack the
> hardware details on whether we can just disable the internal IOMMU and
> use DMAR alone instead.
>
>>
The whole
infrastructure isn't really capable of dealing with nested IOMMUs, and
nested DMA ops would be an equally horrible idea.
>>>
>>> Could you elaborate a bit more on this? I think we should be able to
>>> deal with this in a way I suggested before:
>>>
>>> a) the PCI device would use the system DMA ops,
>>> b) the PCI device would implement a secondary bus for which it would
>>> provide its own DMA and IOMMU ops.
>>> c) a secondary device would be registered on the secondary bus,
>>> d) all memory for the IPU would be managed on behalf of the secondary
>>> device.
>>>
>>> In fact, the driver already is designed in a way that all the points
>>> above are true. If I'm not missing something, the only significant
>>> missing point is calling into system DMA ops from IPU DMA ops.
>>
>> I don't believe x86 has any non-coherent DMA ops, therefore the IPU DMA
>> ops would still probably have to do all their own cache maintenance.
>
> I'd argue that it means that we need to add non-coherent DMA ops on
> x86, as we have on other archs, which can have both coherent and
> non-coherent devices in the same system.
I'd argue that that's what this patch *is* doing - x86 already has DMA
ops all over the place for special cases, and this is really just
another special case. I think trying to introduce some notion of
non-coherent devices to the arch code, plus some generic way to identify
them, plus some way to make sure everywhere else that overrides DMA ops
still does the right thing, would be a lot more work for very little gain.
>> Allocation/mapping, though, would have to be done with the parent DMA
>> ops first (in case DMA address != physical address), *then* mapped at
>> the IPU MMU, which is the real killer - if the
Re: [PATCH 03/12] intel-ipu3: Add DMA API implementation
On Fri, Jun 9, 2017 at 3:07 AM, Robin Murphy wrote:
> On 08/06/17 15:35, Tomasz Figa wrote:
>> On Thu, Jun 8, 2017 at 10:22 PM, Robin Murphy wrote:
>>> On 07/06/17 10:47, Tomasz Figa wrote:
Hi Yong,
+Robin, Joerg, IOMMU ML
Please see my comments inline.
On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi wrote:
>> [snip]
> +
> +/* End of things adapted from arch/arm/mm/dma-mapping.c */
> +static void ipu3_dmamap_sync_single_for_cpu(struct device *dev,
> + dma_addr_t dma_handle, size_t
> size,
> + enum dma_data_direction dir)
> +{
> + struct ipu3_mmu *mmu = to_ipu3_mmu(dev);
> + dma_addr_t daddr = iommu_iova_to_phys(mmu->domain, dma_handle);
> +
> + clflush_cache_range(phys_to_virt(daddr), size);
You might need to consider another IOMMU on the way here. Generally,
given that daddr is your MMU DMA address (not necessarily CPU physical
address), you should be able to call
dma_sync_single_for_cpu(, daddr, size, dir)
>>>
>>> I'd hope that this IPU complex is some kind of embedded endpoint thing
>>> that bypasses the VT-d IOMMU or is always using its own local RAM,
>>> because it would be pretty much unworkable otherwise.
>>
>> It uses system RAM and, as far as my understanding goes, by default it
>> operates without the VT-d IOMMU and that's how it's used right now.
>
> OK, if it *is* behind a DMAR unit then booting with "iommu=force" (or
> whatever the exact incantation for intel-iommu is) should be fun...
>
>> I'm suggesting VT-d IOMMU as a way to further strengthen the security
>> and error resilience in future (due to the IPU complex being
>> non-coherent and also running a closed source firmware).
>
> TBH, doing DMA remapping through *two* IOMMUS will add horrible hardware
> overhead,
Not necessarily, if done right and with right hardware (I lack the
details about Intel hardware unfortunately). One can for example
notice the fact that the IOVA ranges from the parent IOMMU are going
to be contiguous for the child IOMMU, so one could use huge pages in
the child IOMMU and essentially make a selective 1:1 mapping.
> increase the scope for kernel-side bugs, and not much more. If
> we don't trust this IOMMU to behave, why are we trying to drive it in
> the first place? If we do, then a second IOMMU behind it won't protect
> anything that the first one doesn't already.
That's a valid point, right. But on the other hand, I lack the
hardware details on whether we can just disable the internal IOMMU and
use DMAR alone instead.
>
>>> The whole
>>> infrastructure isn't really capable of dealing with nested IOMMUs, and
>>> nested DMA ops would be an equally horrible idea.
>>
>> Could you elaborate a bit more on this? I think we should be able to
>> deal with this in a way I suggested before:
>>
>> a) the PCI device would use the system DMA ops,
>> b) the PCI device would implement a secondary bus for which it would
>> provide its own DMA and IOMMU ops.
>> c) a secondary device would be registered on the secondary bus,
>> d) all memory for the IPU would be managed on behalf of the secondary device.
>>
>> In fact, the driver already is designed in a way that all the points
>> above are true. If I'm not missing something, the only significant
>> missing point is calling into system DMA ops from IPU DMA ops.
>
> I don't believe x86 has any non-coherent DMA ops, therefore the IPU DMA
> ops would still probably have to do all their own cache maintenance.
I'd argue that it means that we need to add non-coherent DMA ops on
x86, as we have on other archs, which can have both coherent and
non-coherent devices in the same system.
> Allocation/mapping, though, would have to be done with the parent DMA
> ops first (in case DMA address != physical address), *then* mapped at
> the IPU MMU, which is the real killer - if the PCI DMA ops are from
> intel-iommu, then there's little need for the IPU MMU mapping to be
> anything other than 1:1, so you may as well not bother.
Okay, I think I can agree with you on this. It indeed makes little
sense to use both MMUs at the same time, if there is a way to disable
one of them.
Let's just keep this unaware of DMAR at this point of time, as a
starter, and get back to it later whenever someone wants to use DMAR
instead. I guess the way to proceed then would be either disabling the
internal MMU, if possible, or making it use a 1:1 (huge page, if
possible) mapping, if not.
> If the PCI DMA
> ops are from SWIOTLB, then the constraints of having to go through that
> first eliminate all the scatter-gather benefit of the IPU MMU.
Does the SWIOTLB give you a physically contiguous memory? If not, you
still need the IPU MMU to actually be able to access the memory.
>
> The IOMMU API ops would have to be handled similarly, by checking for
> ops on the parent bus, calling those first if present, t
Re: [PATCH 03/12] intel-ipu3: Add DMA API implementation
On 08/06/17 15:35, Tomasz Figa wrote:
> On Thu, Jun 8, 2017 at 10:22 PM, Robin Murphy wrote:
>> On 07/06/17 10:47, Tomasz Figa wrote:
>>> Hi Yong,
>>>
>>> +Robin, Joerg, IOMMU ML
>>>
>>> Please see my comments inline.
>>>
>>> On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi wrote:
> [snip]
+
+/* End of things adapted from arch/arm/mm/dma-mapping.c */
+static void ipu3_dmamap_sync_single_for_cpu(struct device *dev,
+ dma_addr_t dma_handle, size_t
size,
+ enum dma_data_direction dir)
+{
+ struct ipu3_mmu *mmu = to_ipu3_mmu(dev);
+ dma_addr_t daddr = iommu_iova_to_phys(mmu->domain, dma_handle);
+
+ clflush_cache_range(phys_to_virt(daddr), size);
>>>
>>> You might need to consider another IOMMU on the way here. Generally,
>>> given that daddr is your MMU DMA address (not necessarily CPU physical
>>> address), you should be able to call
>>>
>>> dma_sync_single_for_cpu(, daddr, size, dir)
>>
>> I'd hope that this IPU complex is some kind of embedded endpoint thing
>> that bypasses the VT-d IOMMU or is always using its own local RAM,
>> because it would be pretty much unworkable otherwise.
>
> It uses system RAM and, as far as my understanding goes, by default it
> operates without the VT-d IOMMU and that's how it's used right now.
OK, if it *is* behind a DMAR unit then booting with "iommu=force" (or
whatever the exact incantation for intel-iommu is) should be fun...
> I'm suggesting VT-d IOMMU as a way to further strengthen the security
> and error resilience in future (due to the IPU complex being
> non-coherent and also running a closed source firmware).
TBH, doing DMA remapping through *two* IOMMUS will add horrible hardware
overhead, increase the scope for kernel-side bugs, and not much more. If
we don't trust this IOMMU to behave, why are we trying to drive it in
the first place? If we do, then a second IOMMU behind it won't protect
anything that the first one doesn't already.
>> The whole
>> infrastructure isn't really capable of dealing with nested IOMMUs, and
>> nested DMA ops would be an equally horrible idea.
>
> Could you elaborate a bit more on this? I think we should be able to
> deal with this in a way I suggested before:
>
> a) the PCI device would use the system DMA ops,
> b) the PCI device would implement a secondary bus for which it would
> provide its own DMA and IOMMU ops.
> c) a secondary device would be registered on the secondary bus,
> d) all memory for the IPU would be managed on behalf of the secondary device.
>
> In fact, the driver already is designed in a way that all the points
> above are true. If I'm not missing something, the only significant
> missing point is calling into system DMA ops from IPU DMA ops.
I don't believe x86 has any non-coherent DMA ops, therefore the IPU DMA
ops would still probably have to do all their own cache maintenance.
Allocation/mapping, though, would have to be done with the parent DMA
ops first (in case DMA address != physical address), *then* mapped at
the IPU MMU, which is the real killer - if the PCI DMA ops are from
intel-iommu, then there's little need for the IPU MMU mapping to be
anything other than 1:1, so you may as well not bother. If the PCI DMA
ops are from SWIOTLB, then the constraints of having to go through that
first eliminate all the scatter-gather benefit of the IPU MMU.
The IOMMU API ops would have to be handled similarly, by checking for
ops on the parent bus, calling those first if present, then running the
intermediate results through the IPU MMU's own functions. Sure, it's not
impossible, but it's really really grim. Not to mention that all the IPU
MMU's page tables/control structures/etc. would also have to be
DMA-allocated/mapped because it may or may not be operating in physical
address space.
The reasonable option - assuming the topology really is this way - would
seem to be special-casing the IPU in intel-iommu in a similar manner to
integrated graphics, to make sure it gets a passthrough domain for DMA
ops, but still allowing the whole PCI device to be passed through to a
guest VM via VFIO if desired (which is really the only case where nested
translation does start to make sense).
Robin.
>
> Best regards,
> Tomasz
>
Re: [PATCH 03/12] intel-ipu3: Add DMA API implementation
Hi Tomasz and Alan, On Thu, Jun 08, 2017 at 11:55:18AM +0900, Tomasz Figa wrote: > Hi Alan, > > On Thu, Jun 8, 2017 at 2:45 AM, Alan Cox wrote: > >> > + struct ipu3_mmu *mmu = to_ipu3_mmu(dev); > >> > + dma_addr_t daddr = iommu_iova_to_phys(mmu->domain, dma_handle); > >> > + > >> > + clflush_cache_range(phys_to_virt(daddr), size); > >> > >> You might need to consider another IOMMU on the way here. Generally, > >> given that daddr is your MMU DMA address (not necessarily CPU physical > >> address), you should be able to call > >> > >> dma_sync_single_for_cpu(, daddr, size, dir) > > > > Te system IOMMU (if enabled) may be cache coherent - and on x86 would be, > > so it doesn't think it needs to do anything for cache synchronization > > and the dma_sync won't actually do any work. > > I'm not very familiar with x86, but typically I found coherency to be > an attribute of the DMA master (i.e. if it is connected to a coherent > memory port). > > Looking at all the IPU3 code, it looks like the whole PCI device is > non-coherent for some reason (e.g. you can see implicit cache flushes > for page tables). So I would have expected that a non-coherent variant > of x86 dma_ops is used for the PCI struct device, which would do cache > maintenance in its dma_sync_* ops. It can actually do both --- in most cases. The MMU page tables are an exception so they will still need an explicit flush. -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
Re: [PATCH 03/12] intel-ipu3: Add DMA API implementation
On Thu, Jun 8, 2017 at 10:22 PM, Robin Murphy wrote:
> On 07/06/17 10:47, Tomasz Figa wrote:
>> Hi Yong,
>>
>> +Robin, Joerg, IOMMU ML
>>
>> Please see my comments inline.
>>
>> On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi wrote:
[snip]
>>> +
>>> +/* End of things adapted from arch/arm/mm/dma-mapping.c */
>>> +static void ipu3_dmamap_sync_single_for_cpu(struct device *dev,
>>> + dma_addr_t dma_handle, size_t
>>> size,
>>> + enum dma_data_direction dir)
>>> +{
>>> + struct ipu3_mmu *mmu = to_ipu3_mmu(dev);
>>> + dma_addr_t daddr = iommu_iova_to_phys(mmu->domain, dma_handle);
>>> +
>>> + clflush_cache_range(phys_to_virt(daddr), size);
>>
>> You might need to consider another IOMMU on the way here. Generally,
>> given that daddr is your MMU DMA address (not necessarily CPU physical
>> address), you should be able to call
>>
>> dma_sync_single_for_cpu(, daddr, size, dir)
>
> I'd hope that this IPU complex is some kind of embedded endpoint thing
> that bypasses the VT-d IOMMU or is always using its own local RAM,
> because it would be pretty much unworkable otherwise.
It uses system RAM and, as far as my understanding goes, by default it
operates without the VT-d IOMMU and that's how it's used right now.
I'm suggesting VT-d IOMMU as a way to further strengthen the security
and error resilience in future (due to the IPU complex being
non-coherent and also running a closed source firmware).
> The whole
> infrastructure isn't really capable of dealing with nested IOMMUs, and
> nested DMA ops would be an equally horrible idea.
Could you elaborate a bit more on this? I think we should be able to
deal with this in a way I suggested before:
a) the PCI device would use the system DMA ops,
b) the PCI device would implement a secondary bus for which it would
provide its own DMA and IOMMU ops.
c) a secondary device would be registered on the secondary bus,
d) all memory for the IPU would be managed on behalf of the secondary device.
In fact, the driver already is designed in a way that all the points
above are true. If I'm not missing something, the only significant
missing point is calling into system DMA ops from IPU DMA ops.
Best regards,
Tomasz
Re: [PATCH 03/12] intel-ipu3: Add DMA API implementation
On 07/06/17 10:47, Tomasz Figa wrote:
> Hi Yong,
>
> +Robin, Joerg, IOMMU ML
>
> Please see my comments inline.
>
> On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi wrote:
>> IPU3 mmu based DMA mapping driver
>>
>> Signed-off-by: Yong Zhi
>> ---
>> drivers/media/pci/intel/ipu3/Kconfig | 6 +
>> drivers/media/pci/intel/ipu3/Makefile | 1 +
>> drivers/media/pci/intel/ipu3/ipu3-dmamap.c | 408
>> +
>> drivers/media/pci/intel/ipu3/ipu3-dmamap.h | 20 ++
>> 4 files changed, 435 insertions(+)
>> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.c
>> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.h
>>
>> diff --git a/drivers/media/pci/intel/ipu3/Kconfig
>> b/drivers/media/pci/intel/ipu3/Kconfig
>> index ab2edcb..2030be7 100644
>> --- a/drivers/media/pci/intel/ipu3/Kconfig
>> +++ b/drivers/media/pci/intel/ipu3/Kconfig
>> @@ -26,3 +26,9 @@ config INTEL_IPU3_MMU
>>
>> Say Y here if you have Skylake/Kaby Lake SoC with IPU3.
>> Say N if un-sure.
>> +
>> +config INTEL_IPU3_DMAMAP
>> + bool "Intel ipu3 DMA mapping driver"
>> + select IOMMU_IOVA
>> + ---help---
>> + This is IPU3 IOMMU domain specific DMA driver.
>> diff --git a/drivers/media/pci/intel/ipu3/Makefile
>> b/drivers/media/pci/intel/ipu3/Makefile
>> index 2b669df..2c2a035 100644
>> --- a/drivers/media/pci/intel/ipu3/Makefile
>> +++ b/drivers/media/pci/intel/ipu3/Makefile
>> @@ -1,2 +1,3 @@
>> obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
>> obj-$(CONFIG_INTEL_IPU3_MMU) += ipu3-mmu.o
>> +obj-$(CONFIG_INTEL_IPU3_DMAMAP) += ipu3-dmamap.o
>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
>> b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
>> new file mode 100644
>> index 000..74704d9
>> --- /dev/null
>> +++ b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
>> @@ -0,0 +1,408 @@
>> +/*
>> + * Copyright (c) 2017 Intel Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License version
>> + * 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include "ipu3-mmu.h"
>> +
>> +/* Begin of things adapted from arch/arm/mm/dma-mapping.c */
>
> ARM's DMA ops are not a good example of today's coding standards.
> There are already generic DMA mapping helpers available in
> drivers/iommu/dma-iommu.c and drivers/base/dma-*. (Hmm, I remember
> writing this already, déjà vu maybe...)
Yes, dma-iommu exists for precisely this purpose -
arch/arm64/mm/dma-mapping.c would have been a better point of reference.
>> +static void ipu3_dmamap_clear_buffer(struct page *page, size_t size,
>> +unsigned long attrs)
>> +{
>> + /*
>> +* Ensure that the allocated pages are zeroed, and that any data
>> +* lurking in the kernel direct-mapped region is invalidated.
>> +*/
>> + if (PageHighMem(page)) {
>> + while (size > 0) {
>> + void *ptr = kmap_atomic(page);
>> +
>> + memset(ptr, 0, PAGE_SIZE);
>> + if ((attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
>> + clflush_cache_range(ptr, PAGE_SIZE);
>> + kunmap_atomic(ptr);
>> + page++;
>> + size -= PAGE_SIZE;
>> + }
>> + } else {
>> + void *ptr = page_address(page);
>> +
>> + memset(ptr, 0, size);
>> + if ((attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
>> + clflush_cache_range(ptr, size);
>> + }
>> +}
>> +
>> +/**
>> + * ipu3_dmamap_alloc_buffer - allocate buffer based on attributes
>> + * @dev: struct device pointer
>> + * @size: size of buffer in bytes
>> + * @gfp: specify the free page type
>> + * @attrs: defined in linux/dma-attrs.h
>> + *
>> + * This is a helper function for physical page allocation
>> + *
>> + * Return array representing buffer from alloc_pages() on success
>> + * or NULL on failure
>> + *
>> + * Must be freed with ipu3_dmamap_free_buffer.
>> + */
>> +static struct page **ipu3_dmamap_alloc_buffer(struct device *dev, size_t
>> size,
>> + gfp_t gfp, unsigned long attrs)
>> +{
>> + struct page **pages;
>> + int count = size >> PAGE_SHIFT;
>> + int array_size = count * sizeof(struct page *);
>> + int i = 0;
>> +
>> + /* Allocate mem for array of page ptrs */
>> + if (array_size <= PAGE_SIZE)
>> + pages = kzalloc(array_size, GFP_KERNEL);
>> + else
>> + pages =
Re: [PATCH 03/12] intel-ipu3: Add DMA API implementation
Hi Alan, On Thu, Jun 8, 2017 at 2:45 AM, Alan Cox wrote: >> > + struct ipu3_mmu *mmu = to_ipu3_mmu(dev); >> > + dma_addr_t daddr = iommu_iova_to_phys(mmu->domain, dma_handle); >> > + >> > + clflush_cache_range(phys_to_virt(daddr), size); >> >> You might need to consider another IOMMU on the way here. Generally, >> given that daddr is your MMU DMA address (not necessarily CPU physical >> address), you should be able to call >> >> dma_sync_single_for_cpu(, daddr, size, dir) > > Te system IOMMU (if enabled) may be cache coherent - and on x86 would be, > so it doesn't think it needs to do anything for cache synchronization > and the dma_sync won't actually do any work. I'm not very familiar with x86, but typically I found coherency to be an attribute of the DMA master (i.e. if it is connected to a coherent memory port). Looking at all the IPU3 code, it looks like the whole PCI device is non-coherent for some reason (e.g. you can see implicit cache flushes for page tables). So I would have expected that a non-coherent variant of x86 dma_ops is used for the PCI struct device, which would do cache maintenance in its dma_sync_* ops. Best regards, Tomasz
Re: [PATCH 03/12] intel-ipu3: Add DMA API implementation
> > + struct ipu3_mmu *mmu = to_ipu3_mmu(dev); > > + dma_addr_t daddr = iommu_iova_to_phys(mmu->domain, dma_handle); > > + > > + clflush_cache_range(phys_to_virt(daddr), size); > > You might need to consider another IOMMU on the way here. Generally, > given that daddr is your MMU DMA address (not necessarily CPU physical > address), you should be able to call > > dma_sync_single_for_cpu(, daddr, size, dir) Te system IOMMU (if enabled) may be cache coherent - and on x86 would be, so it doesn't think it needs to do anything for cache synchronization and the dma_sync won't actually do any work. Alan
Re: [PATCH 03/12] intel-ipu3: Add DMA API implementation
Hi Yong,
+Robin, Joerg, IOMMU ML
Please see my comments inline.
On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi wrote:
> IPU3 mmu based DMA mapping driver
>
> Signed-off-by: Yong Zhi
> ---
> drivers/media/pci/intel/ipu3/Kconfig | 6 +
> drivers/media/pci/intel/ipu3/Makefile | 1 +
> drivers/media/pci/intel/ipu3/ipu3-dmamap.c | 408
> +
> drivers/media/pci/intel/ipu3/ipu3-dmamap.h | 20 ++
> 4 files changed, 435 insertions(+)
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.h
>
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig
> b/drivers/media/pci/intel/ipu3/Kconfig
> index ab2edcb..2030be7 100644
> --- a/drivers/media/pci/intel/ipu3/Kconfig
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -26,3 +26,9 @@ config INTEL_IPU3_MMU
>
> Say Y here if you have Skylake/Kaby Lake SoC with IPU3.
> Say N if un-sure.
> +
> +config INTEL_IPU3_DMAMAP
> + bool "Intel ipu3 DMA mapping driver"
> + select IOMMU_IOVA
> + ---help---
> + This is IPU3 IOMMU domain specific DMA driver.
> diff --git a/drivers/media/pci/intel/ipu3/Makefile
> b/drivers/media/pci/intel/ipu3/Makefile
> index 2b669df..2c2a035 100644
> --- a/drivers/media/pci/intel/ipu3/Makefile
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -1,2 +1,3 @@
> obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> obj-$(CONFIG_INTEL_IPU3_MMU) += ipu3-mmu.o
> +obj-$(CONFIG_INTEL_IPU3_DMAMAP) += ipu3-dmamap.o
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> new file mode 100644
> index 000..74704d9
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> @@ -0,0 +1,408 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include "ipu3-mmu.h"
> +
> +/* Begin of things adapted from arch/arm/mm/dma-mapping.c */
ARM's DMA ops are not a good example of today's coding standards.
There are already generic DMA mapping helpers available in
drivers/iommu/dma-iommu.c and drivers/base/dma-*. (Hmm, I remember
writing this already, déjà vu maybe...)
> +static void ipu3_dmamap_clear_buffer(struct page *page, size_t size,
> +unsigned long attrs)
> +{
> + /*
> +* Ensure that the allocated pages are zeroed, and that any data
> +* lurking in the kernel direct-mapped region is invalidated.
> +*/
> + if (PageHighMem(page)) {
> + while (size > 0) {
> + void *ptr = kmap_atomic(page);
> +
> + memset(ptr, 0, PAGE_SIZE);
> + if ((attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
> + clflush_cache_range(ptr, PAGE_SIZE);
> + kunmap_atomic(ptr);
> + page++;
> + size -= PAGE_SIZE;
> + }
> + } else {
> + void *ptr = page_address(page);
> +
> + memset(ptr, 0, size);
> + if ((attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
> + clflush_cache_range(ptr, size);
> + }
> +}
> +
> +/**
> + * ipu3_dmamap_alloc_buffer - allocate buffer based on attributes
> + * @dev: struct device pointer
> + * @size: size of buffer in bytes
> + * @gfp: specify the free page type
> + * @attrs: defined in linux/dma-attrs.h
> + *
> + * This is a helper function for physical page allocation
> + *
> + * Return array representing buffer from alloc_pages() on success
> + * or NULL on failure
> + *
> + * Must be freed with ipu3_dmamap_free_buffer.
> + */
> +static struct page **ipu3_dmamap_alloc_buffer(struct device *dev, size_t
> size,
> + gfp_t gfp, unsigned long attrs)
> +{
> + struct page **pages;
> + int count = size >> PAGE_SHIFT;
> + int array_size = count * sizeof(struct page *);
> + int i = 0;
> +
> + /* Allocate mem for array of page ptrs */
> + if (array_size <= PAGE_SIZE)
> + pages = kzalloc(array_size, GFP_KERNEL);
> + else
> + pages = vzalloc(array_size);
> + if (!pages)
> + return NULL;
> +
> + gfp |= __GFP_NOWARN;
> +
> + while (count) {
> + int j, order = __fls(count);
> +
> + pages[i] = alloc_pages(gfp, order);
> + while (!pages[i] && order)
> +
