iommu/amd: flush IOTLB for specific domains only

Submitted by Nath, Arindam on March 27, 2017, 6:17 a.m.

Details

Message ID 1490595427-11979-1-git-send-email-arindam.nath@amd.com
State New
Headers show
Series "amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out" ( rev: 2 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Nath, Arindam March 27, 2017, 6:17 a.m.
From: Arindam Nath <arindam.nath@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@amd.com>
---
 drivers/iommu/amd_iommu.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

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);
+	}
 
 	/* Wait until flushes have completed */
 	domain_flush_complete(NULL);

Comments

Hi Arindam,

You CC'd me on this - does this mean that it is a fix for the issue
described in the thread "amd-iommu: can't boot with amdgpu, AMD-Vi:
Completion-Wait loop timed out" ?

Thanks
Daniel


On Mon, Mar 27, 2017 at 12:17 AM,  <arindam.nath@amd.com> wrote:
> From: Arindam Nath <arindam.nath@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@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);
> +       }
>
>         /* Wait until flushes have completed */
>         domain_flush_complete(NULL);
> --
> 1.9.1
>
>-----Original Message-----

>From: Daniel Drake [mailto:drake@endlessm.com]

>Sent: Monday, March 27, 2017 5:56 PM

>To: Nath, Arindam

>Cc: joro@8bytes.org; Deucher, Alexander; Bridgman, John; amd-

>gfx@lists.freedesktop.org; iommu@lists.linux-foundation.org; Suthikulpanit,

>Suravee; Linux Upstreaming Team

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

>

>Hi Arindam,

>

>You CC'd me on this - does this mean that it is a fix for the issue

>described in the thread "amd-iommu: can't boot with amdgpu, AMD-Vi:

>Completion-Wait loop timed out" ?


Yes Daniel, please test this patch to confirm if the issue gets resolved.

Thanks,
Arindam

>

>Thanks

>Daniel

>

>

>On Mon, Mar 27, 2017 at 12:17 AM,  <arindam.nath@amd.com> wrote:

>> From: Arindam Nath <arindam.nath@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@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);

>> +       }

>>

>>         /* Wait until flushes have completed */

>>         domain_flush_complete(NULL);

>> --

>> 1.9.1

>>
>-----Original Message-----

>From: Nath, Arindam

>Sent: Monday, March 27, 2017 5:57 PM

>To: 'Daniel Drake'

>Cc: joro@8bytes.org; Deucher, Alexander; Bridgman, John; amd-

>gfx@lists.freedesktop.org; iommu@lists.linux-foundation.org; Suthikulpanit,

>Suravee; Linux Upstreaming Team

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

>

>>-----Original Message-----

>>From: Daniel Drake [mailto:drake@endlessm.com]

>>Sent: Monday, March 27, 2017 5:56 PM

>>To: Nath, Arindam

>>Cc: joro@8bytes.org; Deucher, Alexander; Bridgman, John; amd-

>>gfx@lists.freedesktop.org; iommu@lists.linux-foundation.org; Suthikulpanit,

>>Suravee; Linux Upstreaming Team

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

>>

>>Hi Arindam,

>>

>>You CC'd me on this - does this mean that it is a fix for the issue

>>described in the thread "amd-iommu: can't boot with amdgpu, AMD-Vi:

>>Completion-Wait loop timed out" ?

>

>Yes Daniel, please test this patch to confirm if the issue gets resolved.


Daniel, did you get chance to test this patch?

Thanks,
Arindam

>

>Thanks,

>Arindam

>

>>

>>Thanks

>>Daniel

>>

>>

>>On Mon, Mar 27, 2017 at 12:17 AM,  <arindam.nath@amd.com> wrote:

>>> From: Arindam Nath <arindam.nath@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@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);

>>> +       }

>>>

>>>         /* Wait until flushes have completed */

>>>         domain_flush_complete(NULL);

>>> --

>>> 1.9.1

>>>
On Thu, Mar 30, 2017 at 12:23 AM, Nath, Arindam <Arindam.Nath@amd.com> wrote:
> Daniel, did you get chance to test this patch?

Not yet. Should we test it alone or alongside "PCI: Blacklist AMD
Stoney GPU devices for ATS"?

Thanks
Daniel
>-----Original Message-----

>From: Daniel Drake [mailto:drake@endlessm.com]

>Sent: Thursday, March 30, 2017 7:15 PM

>To: Nath, Arindam

>Cc: joro@8bytes.org; Deucher, Alexander; Bridgman, John; amd-

>gfx@lists.freedesktop.org; iommu@lists.linux-foundation.org; Suthikulpanit,

>Suravee; Linux Upstreaming Team

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

>

>On Thu, Mar 30, 2017 at 12:23 AM, Nath, Arindam <Arindam.Nath@amd.com>

>wrote:

>> Daniel, did you get chance to test this patch?

>

>Not yet. Should we test it alone or alongside "PCI: Blacklist AMD

>Stoney GPU devices for ATS"?


You should test this patch alone.

Thanks,
Arindam

>

>Thanks

>Daniel
>-----Original Message-----

>From: Daniel Drake [mailto:drake@endlessm.com]

>Sent: Thursday, March 30, 2017 7:15 PM

>To: Nath, Arindam

>Cc: joro@8bytes.org; Deucher, Alexander; Bridgman, John; amd-

>gfx@lists.freedesktop.org; iommu@lists.linux-foundation.org; Suthikulpanit,

>Suravee; Linux Upstreaming Team

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

>

>On Thu, Mar 30, 2017 at 12:23 AM, Nath, Arindam <Arindam.Nath@amd.com>

>wrote:

>> Daniel, did you get chance to test this patch?

>

>Not yet. Should we test it alone or alongside "PCI: Blacklist AMD

>Stoney GPU devices for ATS"?


Daniel, any luck with this patch?

Thanks,
Arindam

>

>Thanks

>Daniel
On Mon, Mar 27, 2017 at 11:47:07AM +0530, arindam.nath@amd.com wrote:
> From: Arindam Nath <arindam.nath@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@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.


	Joerg
On Wed, Apr 5, 2017 at 9:01 AM, Nath, Arindam <Arindam.Nath@amd.com> wrote:
>
> >-----Original Message-----
> >From: Daniel Drake [mailto:drake@endlessm.com]
> >Sent: Thursday, March 30, 2017 7:15 PM
> >To: Nath, Arindam
> >Cc: joro@8bytes.org; Deucher, Alexander; Bridgman, John; amd-
> >gfx@lists.freedesktop.org; iommu@lists.linux-foundation.org; Suthikulpanit,
> >Suravee; Linux Upstreaming Team
> >Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only
> >
> >On Thu, Mar 30, 2017 at 12:23 AM, Nath, Arindam <Arindam.Nath@amd.com>
> >wrote:
> >> Daniel, did you get chance to test this patch?
> >
> >Not yet. Should we test it alone or alongside "PCI: Blacklist AMD
> >Stoney GPU devices for ATS"?
>
> Daniel, any luck with this patch?

Sorry for the delay. The patch appears to be working fine.

Daniel
On 07/04/17 07:20 PM, Joerg Roedel wrote:
> On Mon, Mar 27, 2017 at 11:47:07AM +0530, arindam.nath@amd.com wrote:
>> From: Arindam Nath <arindam.nath@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@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.