Re: [PATCH v2 1/1] iommu-api: Add map_range/unmap_range functions

[Olav Haugan <ohaugan@codeaurora.org>]

On 7/17/2014 1:21 AM, Thierry Reding wrote:
> On Wed, Jul 16, 2014 at 06:01:57PM -0700, Olav Haugan wrote:
>> Mapping and unmapping are more often than not in the critical path.
>> map_range and unmap_range allows SMMU driver implementations to optimize
> 
> s/SMMU/IOMMU/
>
>> the process of mapping and unmapping buffers into the SMMU page tables.
> 
> s/SMMU/IOMMU/
> 
>> Instead of mapping one physical address, do TLB operation (expensive),
>> mapping, do TLB operation, mapping, do TLB operation the driver can map
>> a scatter-gatherlist of physically contiguous pages into one virtual
>> address space and then at the end do one TLB operation.
> 
> I find the above hard to read. Maybe:
> 
> Instead of mapping a buffer one page at a time and requiring potentially
> expensive TLB operations for each page, this function allows the driver
> to map all pages in one go and defer TLB maintenance until after all
> pages have been mapped.

Yeah, all above is OK with me.

> 
>> Additionally, the mapping operation would be faster in general since
>> clients does not have to keep calling map API over and over again for
>> each physically contiguous chunk of memory that needs to be mapped to a
>> virtually contiguous region.
>>
>> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
>> ---
>>  drivers/iommu/iommu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/iommu.h | 25 +++++++++++++++++++++++++
>>  2 files changed, 73 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 1698360..a0eebb7 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1089,6 +1089,54 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
>>  EXPORT_SYMBOL_GPL(iommu_unmap);
>>  
>>  
>> +int iommu_map_range(struct iommu_domain *domain, unsigned int iova,
> 
> Maybe iova should be dma_addr_t? Or at least unsigned long? And perhaps
> iommu_map_sg() would be more consistent with the equivalent function in
> struct dma_ops?
> 
>> +		    struct scatterlist *sg, unsigned int len, int opt)
> 
> The length argument seems to be the size of the mapping. Again, the
> struct dma_ops function uses this argument to denote the number of
> entries in the scatterlist.
> 
> opt is somewhat opaque. Perhaps this should be turned into unsigned long
> flags? Although given that there aren't any users yet it's difficult to
> say what's best here. Perhaps the addition of this argument should be
> postponed until there are actual users?

I am thinking something like this:

int iommu_map_sg(struct iommu_domain *domain, struct scatterlist *sg,
unsigned int nents, int prot, unsigned long flags);
int iommu_unmap_sg(struct iommu_domain *domain, struct scatterlist *sg,
unsigned int nents, unsigned long flags);

The iova is contained within sg so we don't need that argument really
and I would like to keep the flags argument. I would prefer not to
change the API after it has been published which could potentially
affect a lot of call sites.

>> +{
>> +	s32 ret = 0;
> 
> Should be int to match the function's return type.
> 
>> +	u32 offset = 0;
>> +	u32 start_iova = iova;
> 
> These should match the type of iova. Also, what's the point of
> start_iova if we can simply keep iova constant and use offset where
> necessary?
> 
>> +	BUG_ON(iova & (~PAGE_MASK));
>> +
>> +	if (unlikely(domain->ops->map_range == NULL)) {
>> +		while (offset < len) {
> 
> Maybe this should use for_each_sg()?
> 
>> +			phys_addr_t phys = page_to_phys(sg_page(sg));
>> +			u32 page_len = PAGE_ALIGN(sg->offset + sg->length);
> 
> Shouldn't this alignment be left to iommu_map() to handle? It has code
> to deal with that already.

I don't see page alignment in the iommu_map function. I only see a check
whether the (iova | paddr | size) is aligned to the minimum page size
and then it errors out if it isn't....


>> +			ret = iommu_map(domain, iova, phys, page_len, opt);
> 
> This conflates the new opt argument with iommu_map()'s prot argument.
> Maybe those two should rather be split?
> 
>> +			if (ret)
>> +				goto fail;
>> +
>> +			iova += page_len;
>> +			offset += page_len;
>> +			if (offset < len)
>> +				sg = sg_next(sg);
>> +		}
>> +	} else {
>> +		ret = domain->ops->map_range(domain, iova, sg, len, opt);
>> +	}
> 
> Perhaps rather than check for a ->map_range implementation everytime a
> better option may be to export this generic implementation so that
> drivers can set it in their iommu_ops if they don't implement it? So the
> contents of the if () block could become a new function:
> 
> 	int iommu_map_range_generic(...)
> 	{
> 		...
> 	}
> 	EXPORT_SYMBOL(iommu_map_range_generic);
> 
> And drivers would do this:
> 
> 	static const struct iommu_ops driver_iommu_ops = {
> 		...
> 		.map_range = iommu_map_range_generic,
> 		...
> 	};

I'd like to keep the new API consistent with the rest of the API. Most
if not all of the other APIs always check if the operation is non-NULL.
A new driver could choose not to set the .map_range callback. I think it
is better to keep this consistent with the behavior of the other APIs.

>> +int iommu_unmap_range(struct iommu_domain *domain, unsigned int iova,
>> +		      unsigned int len, int opt)
> 
> Some comments regarding function name and argument types as for
> iommu_map_range().
> 
>> +static inline int iommu_map_range(struct iommu_domain *domain,
>> +				  unsigned int iova, struct scatterlist *sg,
>> +				  unsigned int len, int opt)
>> +{
>> +	return -ENODEV;
> 
> I know other IOMMU API dummies already use this error code, but I find
> it to be a little confusing. The dummies are used when the IOMMU API is
> disabled via Kconfig, so -ENOSYS (Function not implemented) seems like a
> more useful error.

Again, I would prefer to keep this consistent with the other APIs
already there. iommu_map/iommu_unmap both returns -ENODEV. If we want to
change this I think this should be done as a separate patch that changed
all of them to be consistent.

Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation