Re: [RFC PATCH 4/5] arm64: add IOMMU dma_ops

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

Hi Joseph,

Thanks for giving it a spin,

On 26/01/15 03:25, Joseph Lo wrote:
> On 01/13/2015 04:48 AM, Robin Murphy wrote:
>> Taking some inspiration from the arch/arm code, implement the
>> arch-specific side of the DMA mapping ops using the new IOMMU-DMA layer.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>    arch/arm64/include/asm/device.h      |   3 +
>>    arch/arm64/include/asm/dma-mapping.h |  12 ++
>>    arch/arm64/mm/dma-mapping.c          | 297 +++++++++++++++++++++++++++++++++++
>>    3 files changed, 312 insertions(+)
>>
> [snip]
>> +static struct dma_map_ops iommu_dma_ops = {
>> +	.alloc = __iommu_alloc_attrs,
>> +	.free = __iommu_free_attrs,
>> +	.mmap = __swiotlb_mmap,
>
> Hi Robin,
>
> Thanks for posting this series. I spent some time to re-write Tegra-smmu
> driver to work with the new DT-based iommu initialization procedure and
> do some modification in Tegra/DRM driver to make it work with
> IOMMU-backed DMA mapping API.
>
> Found two issues, one is the mmap function here.
> The mmap function is not reliable, the userspace application and kernel
> easily leads to crash after calling this. Can we mmap the CPU VA to the
> IOVA?
> This issue was gone after replacing it with the same function
> (arm_iommu_mmap_attrs) in the ARM32 kernel. And everything just works fine.
>

Hmm, it looks like __dma_common_mmap might rely on pages being 
contiguous, which would be a big problem I hadn't noticed. I'll have a 
dig into it and see if it's feasible to extend the existing 
implementation, otherwise we'll have to just pull in the arm_iommu 
version and have two.

>> +	.map_page = __iommu_map_page,
>> +	.unmap_page = __iommu_unmap_page,
>> +	.map_sg = __iommu_map_sg_attrs,
>> +	.unmap_sg = __iommu_unmap_sg_attrs,
>> +	.sync_single_for_cpu = __iommu_sync_single_for_cpu,
>> +	.sync_single_for_device = __iommu_sync_single_for_device,
>> +	.sync_sg_for_cpu = __iommu_sync_sg_for_cpu,
>> +	.sync_sg_for_device = __iommu_sync_sg_for_device,
>> +	.dma_supported = iommu_dma_supported,
>> +	.mapping_error = iommu_dma_mapping_error,
>> +};
>> +
>> +static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>> +				  struct iommu_ops *ops)
>> +{
>> +	struct iommu_dma_mapping *mapping;
>> +
>> +	if (!ops)
>> +		return;
>> +
>> +	mapping = iommu_dma_create_mapping(ops, dma_base, size);
>
> The other issue is here. We create a DMA range here, but the
> __iova_alloc not really working with that, if the end of the range was
> under 4G limitation. The allocation address always starts from
> 0xfffxxxxx, because we set the .dma_mask to DMA_BIT_MASK(32) by default.
> If we don't expect that, we need one more function call like
> dma_coerce_mask_and_coherent(dev, 0x80000000) for the range of 0 to 2G
> for example.
>
> Does this the expectation when we use this framework?
>
> (Except the issue of __alloc_iova, I am ok with this. We could set up a
> default range from 0 to 4G for every device by default, and then isolate
> the virtual range by reset the .dma_mask of the device later.)
>

Yes, I failed to consider dma-ranges in this initial version, so it 
allocates based on the DMA mask alone. I hacked up an unsatisfactory 
workaround in preparation for v2, but it turns out the fix as per your 
suggestion is already in progress elsewhere[1], so I'll rework based on 
that idea.

Robin.

[1]:https://lkml.org/lkml/2015/1/23/694

> Thanks,
> -Joseph
>
>> +	if (!mapping) {
>> +		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
>> +				size, dev_name(dev));
>> +		return;
>> +	}
>> +
>> +	if (iommu_dma_attach_device(dev, mapping))
>> +		pr_warn("Failed to attach device %s to IOMMU mapping\n",
>> +				dev_name(dev));
>> +	else
>> +		dev->archdata.dma_ops = &iommu_dma_ops;
>> +
>> +	/* drop the initial mapping refcount */
>> +	iommu_dma_release_mapping(mapping);
>> +}
>> +
>> +#else
>> +
>> +static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>> +				  struct iommu_ops *iommu)
>> +{ }
>> +
>> +#endif  /* CONFIG_IOMMU_DMA */
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>