Re: [RFC PATCH 3/5] iommu: implement common IOMMU ops for DMA mapping

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

Hi Laura,

Thanks for looking

On 23/01/15 17:42, Laura Abbott wrote:
> On 1/12/2015 12:48 PM, Robin Murphy wrote:
>> Taking inspiration from the existing arch/arm code, break out some
>> generic functions to interface the DMA-API to the IOMMU-API. This will
>> do the bulk of the heavy lifting for IOMMU-backed dma-mapping.
>>
>> Whilst the target is arm64, rather than introduce yet another private
>> implementation, place this in common code as the first step towards
>> consolidating the numerous versions spread around between architecture
>> code and IOMMU drivers.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>    include/linux/dma-iommu.h |  78 ++++++++
>>    lib/Kconfig               |   8 +
>>    lib/Makefile              |   1 +
>>    lib/dma-iommu.c           | 455 ++++++++++++++++++++++++++++++++++++++++++++++
>>    4 files changed, 542 insertions(+)
>>    create mode 100644 include/linux/dma-iommu.h
>>    create mode 100644 lib/dma-iommu.c
>>
>> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
>> new file mode 100644
>> index 0000000..4515407
>> --- /dev/null
>> +++ b/include/linux/dma-iommu.h
> <snip>
>> +
>> +struct iommu_dma_mapping *iommu_dma_create_mapping(struct iommu_ops *ops,
>> +		dma_addr_t base, size_t size)
>> +{
>> +	struct iommu_dma_mapping *mapping;
>> +	struct iommu_domain *domain;
>> +	struct iova_domain *iovad;
>> +	struct iommu_domain_geometry *dg;
>> +	unsigned long order, base_pfn, end_pfn;
>> +
>> +	pr_debug("base=%pad\tsize=0x%zx\n", &base, size);
>> +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
>> +	if (!mapping)
>> +		return NULL;
>> +
>> +	/*
>> +	 * HACK: We'd like to ask the relevant IOMMU in ops for a suitable
>> +	 * domain, but until that happens, bypass the bus nonsense and create
>> +	 * one directly for this specific device/IOMMU combination...
>> +	 */
>> +	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>> +
>> +	if (!domain)
>> +		goto out_free_mapping;
>> +	domain->ops = ops;
>> +
>> +	if (ops->domain_init(domain))
>> +		goto out_free_mapping;
>> +	/*
>> +	 * ...and do the bare minimum to sanity-check that the domain allows
>> +	 * at least some access to the device...
>> +	 */
>> +	dg = &domain->geometry;
>> +	if (!(base < dg->aperture_end && base + size > dg->aperture_start)) {
>> +		pr_warn("DMA range outside IOMMU capability; is DT correct?\n");
>> +		goto out_free_mapping;
>> +	}
>
> This check seems to always fail with base = 0 because dg->aperture_end is
> always going to be 0 as well. base <= dg->aperature_end ?
>

Strictly you're right, since aperture_end should be the highest mappable 
address thus you could create a valid, if a little silly, 1-byte domain 
if both were zero. However, it shouldn't really be zero.

I put this bit in since the majority of IOMMU drivers set up the 
geometry in domain_init and it seemed worth making use of. The ARM SMMU 
driver is currently one of the odd ones out that don't, however one of 
my as-yet-unpublished patches porting it to the new of_iommu_configure 
framework involves fixing domain initialisation and adds geometry setup 
as well - that's what I've been testing with. I'm hoping to get that 
series out soon, but among other things it needs a messy merge with 
Will's pagetable changes.

Robin.