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
