Re: [PATCH 2/2] iommu: Introduce Interface for IOMMU TLB Flushing

["Leizhen (ThunderTown)" <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>]

On 2017/8/29 19:19, Robin Murphy wrote:
> On 29/08/17 03:53, Leizhen (ThunderTown) wrote:
> [...]
>>> -size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
>>> +static size_t __iommu_unmap(struct iommu_domain *domain,
>>> +			    unsigned long iova, size_t size,
>>> +			    bool sync)
>>>  {
>>> +	const struct iommu_ops *ops = domain->ops;
>>>  	size_t unmapped_page, unmapped = 0;
>>> -	unsigned int min_pagesz;
>>>  	unsigned long orig_iova = iova;
>>> +	unsigned int min_pagesz;
>>>  
>>> -	if (unlikely(domain->ops->unmap == NULL ||
>>> +	if (unlikely(ops->unmap == NULL ||
>>>  		     domain->pgsize_bitmap == 0UL))
>>>  		return -ENODEV;
>>>  
>>> @@ -1592,10 +1597,13 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
>>>  	while (unmapped < size) {
>>>  		size_t pgsize = iommu_pgsize(domain, iova, size - unmapped);
>>>  
>>> -		unmapped_page = domain->ops->unmap(domain, iova, pgsize);
>>> +		unmapped_page = ops->unmap(domain, iova, pgsize);
>>>  		if (!unmapped_page)
>>>  			break;
>>>  
>>> +		if (sync && ops->iotlb_range_add)
>>> +			ops->iotlb_range_add(domain, iova, pgsize);
>>> +
>>>  		pr_debug("unmapped: iova 0x%lx size 0x%zx\n",
>>>  			 iova, unmapped_page);
>>>  
>>> @@ -1603,11 +1611,27 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
>>>  		unmapped += unmapped_page;
>>>  	}
>>>  
>>> +	if (sync && ops->iotlb_sync)
>>> +		ops->iotlb_sync(domain);
>>> +
>>>  	trace_unmap(orig_iova, size, unmapped);
>>>  	return unmapped;
>>>  }
>>> +
>>> +size_t iommu_unmap(struct iommu_domain *domain,
>>> +		   unsigned long iova, size_t size)
>>> +{
>>> +	return __iommu_unmap(domain, iova, size, true);
>>> +}
>>>  EXPORT_SYMBOL_GPL(iommu_unmap);
>>>  
>>> +size_t iommu_unmap_fast(struct iommu_domain *domain,
>>> +			unsigned long iova, size_t size)
>>> +{
>> Do we need to add a check "if (!domain->ops->iotlb_sync)". Suppose the new added three hooks are not
>> registered, we should fallback to iommu_unmap.
> 
> If those callbacks don't exist, then iommu_unmap() isn't going to be
> able to call them either, so I don't see what difference that makes :/
Yes, you're right, I see.

> 
>>> +	return __iommu_unmap(domain, iova, size, false);
>>> +}
>>> +EXPORT_SYMBOL_GPL(iommu_unmap_fast);
>>> +
>>>  size_t default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
>>>  			 struct scatterlist *sg, unsigned int nents, int prot)
>>>  {
>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>> index 2cb54ad..67fa954 100644
>>> --- a/include/linux/iommu.h
>>> +++ b/include/linux/iommu.h
>>> @@ -167,6 +167,10 @@ struct iommu_resv_region {
>>>   * @map: map a physically contiguous memory region to an iommu domain
>>>   * @unmap: unmap a physically contiguous memory region from an iommu domain
>>>   * @map_sg: map a scatter-gather list of physically contiguous memory chunks
>>> + * @flush_tlb_all: Synchronously flush all hardware TLBs for this domain
>>> + * @tlb_range_add: Add a given iova range to the flush queue for this domain
>>> + * @tlb_sync: Flush all queued ranges from the hardware TLBs and empty flush
>>> + *            queue
>>>   * to an iommu domain
>>>   * @iova_to_phys: translate iova to physical address
>>>   * @add_device: add device to iommu grouping
>>> @@ -199,6 +203,10 @@ struct iommu_ops {
>>>  		     size_t size);
>>>  	size_t (*map_sg)(struct iommu_domain *domain, unsigned long iova,
>>>  			 struct scatterlist *sg, unsigned int nents, int prot);
>>> +	void (*flush_iotlb_all)(struct iommu_domain *domain);
>>> +	void (*iotlb_range_add)(struct iommu_domain *domain,
>>> +				unsigned long iova, size_t size);
>>> +	void (*iotlb_sync)(struct iommu_domain *domain);
>> I think we'd better to make sure all these three hooks are registered or all are not, in
>> function __iommu_domain_alloc or some other suitable place.
> 
> I'd prefer for them to be individually optional than for drivers to have
> to implement no-op callbacks - e.g. for SMMUv2 where issuing TLBIs is
> relatively cheap, but latency-sensitive, we're probably better off not
> bothering with with .iotlb_range_add (leaving the TLBIs implicit in
> .unmap) only implementing .iotlb_sync.
OK, so that the arch iommu can free to do so.

> 
> Robin.
> 
> .
> 

-- 
Thanks!
BestRegards