[PATCH] iommu/amd: flush IOTLB for specific domains only

[Michel Dänzer]

On 07/04/17 07:20 PM, Joerg Roedel wrote:
> On Mon, Mar 27, 2017 at 11:47:07AM +0530, arindam.nath at amd.com wrote:
>> From: Arindam Nath <arindam.nath at amd.com>
>>
>> The idea behind flush queues is to defer the IOTLB flushing
>> for domains for which the mappings are no longer valid. We
>> add such domains in queue_add(), and when the queue size
>> reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>>
>> Since we have already taken lock before __queue_flush()
>> is called, we need to make sure the IOTLB flushing is
>> performed as quickly as possible.
>>
>> In the current implementation, we perform IOTLB flushing
>> for all domains irrespective of which ones were actually
>> added in the flush queue initially. This can be quite
>> expensive especially for domains for which unmapping is
>> not required at this point of time.
>>
>> This patch makes use of domain information in
>> 'struct flush_queue_entry' to make sure we only flush
>> IOTLBs for domains who need it, skipping others.
>>
>> Signed-off-by: Arindam Nath <arindam.nath at amd.com>
>> ---
>>  drivers/iommu/amd_iommu.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 98940d1..6a9a048 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -2227,15 +2227,16 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
>>  
>>  static void __queue_flush(struct flush_queue *queue)
>>  {
>> -	struct protection_domain *domain;
>> -	unsigned long flags;
>>  	int idx;
>>  
>> -	/* First flush TLB of all known domains */
>> -	spin_lock_irqsave(&amd_iommu_pd_lock, flags);
>> -	list_for_each_entry(domain, &amd_iommu_pd_list, list)
>> -		domain_flush_tlb(domain);
>> -	spin_unlock_irqrestore(&amd_iommu_pd_lock, flags);
>> +	/* First flush TLB of all domains which were added to flush queue */
>> +	for (idx = 0; idx < queue->next; ++idx) {
>> +		struct flush_queue_entry *entry;
>> +
>> +		entry = queue->entries + idx;
>> +
>> +		domain_flush_tlb(&entry->dma_dom->domain);
>> +	}
> 
> With this we will flush a domain every time we find one of its
> iova-addresses in the flush queue, so potentially we flush a domain
> multiple times per __queue_flush() call.
> 
> Its better to either add a flush-flag to the domains and evaluate that
> in __queue_flush or keep a list of domains to flush to make the flushing
> really more efficient.

Arindam, can you incorporate Joerg's feedback?

FWIW, looks like Carrizo systems are affected by this as well (see e.g.
https://bugs.freedesktop.org/show_bug.cgi?id=101029#c21), so it would be
good to land this fix in some form ASAP.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer