RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

2017-09-12 Thread Nath, Arindam 
Hi Daniel,

>-Original Message-
>From: Daniel Drake [mailto:dr...@endlessm.com]
>Sent: Tuesday, September 12, 2017 12:20 PM
>To: Nath, Arindam <arindam.n...@amd.com>
>Cc: Joerg Roedel <j...@8bytes.org>; Lendacky, Thomas
><thomas.lenda...@amd.com>; io...@lists.linux-foundation.org; amd-
>g...@lists.freedesktop.org; Deucher, Alexander
><alexander.deuc...@amd.com>; Bridgman, John
><john.bridg...@amd.com>; Suthikulpanit, Suravee
><suravee.suthikulpa...@amd.com>; li...@endlessm.com; Craig Stein
><stein...@gmail.com>; mic...@daenzer.net; Kuehling, Felix
><felix.kuehl...@amd.com>; sta...@vger.kernel.org
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
>
>Hi,
>
>On Tue, May 30, 2017 at 3:38 PM, Nath, Arindam <arindam.n...@amd.com>
>wrote:
>>>-Original Message-
>>>From: Joerg Roedel [mailto:j...@8bytes.org]
>>>Sent: Monday, May 29, 2017 8:09 PM
>>>To: Nath, Arindam <arindam.n...@amd.com>; Lendacky, Thomas
>>><thomas.lenda...@amd.com>
>>>Cc: io...@lists.linux-foundation.org; amd-gfx@lists.freedesktop.org;
>>>Deucher, Alexander <alexander.deuc...@amd.com>; Bridgman, John
>>><john.bridg...@amd.com>; dr...@endlessm.com; Suthikulpanit,
>Suravee
>>><suravee.suthikulpa...@amd.com>; li...@endlessm.com; Craig Stein
>>><stein...@gmail.com>; mic...@daenzer.net; Kuehling, Felix
>>><felix.kuehl...@amd.com>; sta...@vger.kernel.org
>>>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only
>(v3)
>>>
>>>Hi Arindam,
>>>
>>>I met Tom Lendacky last week in Nuremberg last week and he told me he is
>>>working on the same area of the code that this patch is for. His reason
>>>for touching this code was to solve some locking problems. Maybe you two
>>>can work together on a joint approach to improve this?
>>
>> Sure Joerg, I will work with Tom.
>
>What was the end result here? I see that the code has been reworked in
>4.13 so your original patch no longer applies. Is the reworked version
>also expected to solve the original issue?

Yes, if the reworked patch resolves the issue, my patch won't be required 
anymore.

Thanks,
Arindam

>
>Thanks
>Daniel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

2017-09-12 Thread Daniel Drake 
Hi,

On Tue, May 30, 2017 at 3:38 PM, Nath, Arindam <arindam.n...@amd.com> wrote:
>>-Original Message-
>>From: Joerg Roedel [mailto:j...@8bytes.org]
>>Sent: Monday, May 29, 2017 8:09 PM
>>To: Nath, Arindam <arindam.n...@amd.com>; Lendacky, Thomas
>><thomas.lenda...@amd.com>
>>Cc: io...@lists.linux-foundation.org; amd-gfx@lists.freedesktop.org;
>>Deucher, Alexander <alexander.deuc...@amd.com>; Bridgman, John
>><john.bridg...@amd.com>; dr...@endlessm.com; Suthikulpanit, Suravee
>><suravee.suthikulpa...@amd.com>; li...@endlessm.com; Craig Stein
>><stein...@gmail.com>; mic...@daenzer.net; Kuehling, Felix
>><felix.kuehl...@amd.com>; sta...@vger.kernel.org
>>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
>>
>>Hi Arindam,
>>
>>I met Tom Lendacky last week in Nuremberg last week and he told me he is
>>working on the same area of the code that this patch is for. His reason
>>for touching this code was to solve some locking problems. Maybe you two
>>can work together on a joint approach to improve this?
>
> Sure Joerg, I will work with Tom.

What was the end result here? I see that the code has been reworked in
4.13 so your original patch no longer applies. Is the reworked version
also expected to solve the original issue?

Thanks
Daniel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-06-03 Thread Jan Ziak 
Hi

I am getting "AMD-Vi: Completion-Wait loop timed out" error
approximately once per several days with R9 390.

Is there a reason why this patch isn't in the mainstream linux-git yet?

-Jan
[18107.021297] AMD-Vi: Completion-Wait loop timed out
[18107.168770] AMD-Vi: Completion-Wait loop timed out
[18107.444225] AMD-Vi: Completion-Wait loop timed out
[18107.714855] AMD-Vi: Completion-Wait loop timed out
[18107.846366] AMD-Vi: Completion-Wait loop timed out
[18108.047749] AMD-Vi: Event logged [
[18108.047752] IOTLB_INV_TIMEOUT device=23:00.0 address=0x00042bb129e0]
[18108.179774] AMD-Vi: Completion-Wait loop timed out
[18108.580262] AMD-Vi: Completion-Wait loop timed out
[18108.710005] AMD-Vi: Completion-Wait loop timed out
[18108.911565] AMD-Vi: Event logged [
[18108.911567] IOTLB_INV_TIMEOUT device=23:00.0 address=0x00042bb12a80]
[18109.041945] AMD-Vi: Completion-Wait loop timed out
[18109.243115] AMD-Vi: Completion-Wait loop timed out
[18109.646030] AMD-Vi: Completion-Wait loop timed out
[18109.847087] AMD-Vi: Completion-Wait loop timed out
[18109.918406] clocksource: timekeeping watchdog on CPU5: Marking clocksource 
'tsc' as unstable because the skew is too large:
[18109.918408] clocksource:   'hpet' wd_now: 5f6204a6 
wd_last: 5e860c6b mask: 
[18109.918408] clocksource:   'tsc' cs_now: 34ca9e6f0220 
cs_last: 34c9ebff5f00 mask: 
[18109.918410] sched_clock: Marking unstable (18109972341058, 
-53936107)<-(18110036284612, -117879661)
[18109.918411] tsc: Marking TSC unstable due to clocksource watchdog
[18109.918421] AMD-Vi: Event logged [
[18109.918423] IOTLB_INV_TIMEOUT device=23:00.0 address=0x00042bb12b20]
[18110.119761] sched: RT throttling activated
[18110.249804] AMD-Vi: Completion-Wait loop timed out
[18110.453355] AMD-Vi: Completion-Wait loop timed out
[18110.652707] AMD-Vi: Completion-Wait loop timed out
[18110.854045] AMD-Vi: Completion-Wait loop timed out
[18111.055387] AMD-Vi: Completion-Wait loop timed out
[18111.126714] AMD-Vi: Event logged [
[18111.126717] IOTLB_INV_TIMEOUT device=23:00.0 address=0x00042bb12bc0]
[18111.127250] clocksource: Switched to clocksource hpet
[18111.144384] AMD-Vi: Completion-Wait loop timed out
[18111.274097] AMD-Vi: Completion-Wait loop timed out
[18111.427604] AMD-Vi: Completion-Wait loop timed out
[18111.533317] AMD-Vi: Completion-Wait loop timed out
[18111.663103] AMD-Vi: Completion-Wait loop timed out
[18111.891058] AMD-Vi: Event logged [
[18111.891061] IOTLB_INV_TIMEOUT device=23:00.0 address=0x00042bb12c60]
[18111.894334] AMD-Vi: Completion-Wait loop timed out
[18112.024375] AMD-Vi: Completion-Wait loop timed out
[18112.167603] AMD-Vi: Completion-Wait loop timed out
[18112.297630] AMD-Vi: Completion-Wait loop timed out
[18112.427562] AMD-Vi: Completion-Wait loop timed out
[18112.557171] AMD-Vi: Completion-Wait loop timed out
[18112.689019] AMD-Vi: Completion-Wait loop timed out
[18112.891054] AMD-Vi: Event logged [
[18112.891056] IOTLB_INV_TIMEOUT device=23:00.0 address=0x00042bb12d00]
[18112.927055] AMD-Vi: Completion-Wait loop timed out
[18113.039385] AMD-Vi: Completion-Wait loop timed out
[18113.187600] AMD-Vi: Completion-Wait loop timed out
[18113.315955] AMD-Vi: Completion-Wait loop timed out
[18113.445461] AMD-Vi: Completion-Wait loop timed out
[18113.600945] AMD-Vi: Completion-Wait loop timed out
[18113.724978] AMD-Vi: Completion-Wait loop timed out
[18113.891067] AMD-Vi: Event logged [
[18113.891069] IOTLB_INV_TIMEOUT device=23:00.0 address=0x00042bb12da0]
[18113.854379] AMD-Vi: Completion-Wait loop timed out
[18114.024385] AMD-Vi: Completion-Wait loop timed out
[18114.173507] AMD-Vi: Completion-Wait loop timed out
[18114.310944] AMD-Vi: Completion-Wait loop timed out
[18114.573226] AMD-Vi: Completion-Wait loop timed out
[18114.621895] AMD-Vi: Completion-Wait loop timed out
[18114.734704] AMD-Vi: Completion-Wait loop timed out
[18114.891088] AMD-Vi: Event logged [
[18114.891090] IOTLB_INV_TIMEOUT device=23:00.0 address=0x00042bb12e40]
[18114.910947] AMD-Vi: Completion-Wait loop timed out
[18115.039803] AMD-Vi: Completion-Wait loop timed out
[18115.184277] AMD-Vi: Completion-Wait loop timed out
[18115.324281] AMD-Vi: Completion-Wait loop timed out
[18115.464271] AMD-Vi: Completion-Wait loop timed out
[18115.677841] AMD-Vi: Completion-Wait loop timed out
[18115.747713] AMD-Vi: Completion-Wait loop timed out
[18115.891107] AMD-Vi: Event logged [
[18115.891109] IOTLB_INV_TIMEOUT device=23:00.0 address=0x00042bb12ee0]
[18115.884118] AMD-Vi: Completion-Wait loop timed out
[18116.020404] AMD-Vi: Completion-Wait loop timed out
[18116.157621] AMD-Vi: Completion-Wait loop timed out
[18116.291560] AMD-Vi: Completion-Wait loop timed out
[18116.420952] AMD-Vi: Completion-Wait loop timed out
[18116.420952] [ cut here ]
[18116.420952] WARNING: CPU: 4 PID: 0 at drivers/iommu/amd_iommu.c:1256 
__domain_flush_pages+0x1d0/0x1f0
[18116.420952]

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

2017-05-30 Thread Nath, Arindam 
>-Original Message-
>From: Joerg Roedel [mailto:j...@8bytes.org]
>Sent: Monday, May 29, 2017 8:09 PM
>To: Nath, Arindam <arindam.n...@amd.com>; Lendacky, Thomas
><thomas.lenda...@amd.com>
>Cc: io...@lists.linux-foundation.org; amd-gfx@lists.freedesktop.org;
>Deucher, Alexander <alexander.deuc...@amd.com>; Bridgman, John
><john.bridg...@amd.com>; dr...@endlessm.com; Suthikulpanit, Suravee
><suravee.suthikulpa...@amd.com>; li...@endlessm.com; Craig Stein
><stein...@gmail.com>; mic...@daenzer.net; Kuehling, Felix
><felix.kuehl...@amd.com>; sta...@vger.kernel.org
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
>
>Hi Arindam,
>
>I met Tom Lendacky last week in Nuremberg last week and he told me he is
>working on the same area of the code that this patch is for. His reason
>for touching this code was to solve some locking problems. Maybe you two
>can work together on a joint approach to improve this?

Sure Joerg, I will work with Tom.

Thanks.

>
>On Mon, May 22, 2017 at 01:18:01PM +0530, arindam.n...@amd.com wrote:
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 63cacf5..1edeebec 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -2227,15 +2227,26 @@ 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(_iommu_pd_lock, flags);
>> -list_for_each_entry(domain, _iommu_pd_list, list)
>> -domain_flush_tlb(domain);
>> -spin_unlock_irqrestore(_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;
>> +
>> +/*
>> + * There might be cases where multiple IOVA entries for the
>> + * same domain are queued in the flush queue. To avoid
>> + * flushing the same domain again, we check whether the
>> + * flag is set or not. This improves the efficiency of
>> + * flush operation.
>> + */
>> +if (!entry->dma_dom->domain.already_flushed) {
>> +entry->dma_dom->domain.already_flushed = true;
>> +domain_flush_tlb(>dma_dom->domain);
>> +}
>> +}
>
>There is also a race condition here I have to look into. It is not
>introduced by your patch, but needs fixing anyway. I'll look into this
>too.
>
>
>Regards,
>
>   Joerg

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

2017-05-29 Thread Joerg Roedel 
Hi Arindam,

I met Tom Lendacky last week in Nuremberg last week and he told me he is
working on the same area of the code that this patch is for. His reason
for touching this code was to solve some locking problems. Maybe you two
can work together on a joint approach to improve this?

On Mon, May 22, 2017 at 01:18:01PM +0530, arindam.n...@amd.com wrote:
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 63cacf5..1edeebec 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2227,15 +2227,26 @@ 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(_iommu_pd_lock, flags);
> - list_for_each_entry(domain, _iommu_pd_list, list)
> - domain_flush_tlb(domain);
> - spin_unlock_irqrestore(_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;
> +
> + /*
> +  * There might be cases where multiple IOVA entries for the
> +  * same domain are queued in the flush queue. To avoid
> +  * flushing the same domain again, we check whether the
> +  * flag is set or not. This improves the efficiency of
> +  * flush operation.
> +  */
> + if (!entry->dma_dom->domain.already_flushed) {
> + entry->dma_dom->domain.already_flushed = true;
> + domain_flush_tlb(>dma_dom->domain);
> + }
> + }

There is also a race condition here I have to look into. It is not
introduced by your patch, but needs fixing anyway. I'll look into this
too.


Regards,

Joerg

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)

2017-05-23 Thread Deucher, Alexander 
> -Original Message-
> From: Arindam Nath [mailto:anath@gmail.com] On Behalf Of
> arindam.n...@amd.com
> Sent: Monday, May 22, 2017 3:48 AM
> To: io...@lists.linux-foundation.org
> Cc: amd-gfx@lists.freedesktop.org; Joerg Roedel; Deucher, Alexander;
> Bridgman, John; dr...@endlessm.com; Suthikulpanit, Suravee;
> li...@endlessm.com; Craig Stein; mic...@daenzer.net; Kuehling, Felix;
> sta...@vger.kernel.org; Nath, Arindam
> Subject: [PATCH] iommu/amd: flush IOTLB for specific domains only (v3)
> 
> From: Arindam Nath <arindam.n...@amd.com>
> 
> Change History
> --
> 
> v3:
> - add Fixes and CC tags
> - add link to Bugzilla
> 
> v2: changes suggested by Joerg
> - add flush flag to improve efficiency of flush operation
> 
> v1:
> - 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.
> 
> Bugzilla: https://bugs.freedesktop.org/101029
> Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
> Cc: sta...@vger.kernel.org
> Suggested-by: Joerg Roedel <j...@8bytes.org>
> Signed-off-by: Arindam Nath <arindam.n...@amd.com>

Acked-by: Alex Deucher <alexander.deuc...@amd.com>

> ---
>  drivers/iommu/amd_iommu.c   | 27 ---
>  drivers/iommu/amd_iommu_types.h |  2 ++
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 63cacf5..1edeebec 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2227,15 +2227,26 @@ 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(_iommu_pd_lock, flags);
> - list_for_each_entry(domain, _iommu_pd_list, list)
> - domain_flush_tlb(domain);
> - spin_unlock_irqrestore(_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;
> +
> + /*
> +  * There might be cases where multiple IOVA entries for the
> +  * same domain are queued in the flush queue. To avoid
> +  * flushing the same domain again, we check whether the
> +  * flag is set or not. This improves the efficiency of
> +  * flush operation.
> +  */
> + if (!entry->dma_dom->domain.already_flushed) {
> + entry->dma_dom->domain.already_flushed = true;
> + domain_flush_tlb(>dma_dom->domain);
> + }
> + }
> 
>   /* Wait until flushes have completed */
>   domain_flush_complete(NULL);
> @@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain
> *dma_dom,
>   pages = __roundup_pow_of_two(pages);
>   address >>= PAGE_SHIFT;
> 
> + dma_dom->domain.already_flushed = false;
> +
>   queue = get_cpu_ptr(_queue);
>   spin_lock_irqsave(>lock, flags);
> 
> diff --git a/drivers/iommu/amd_iommu_types.h
> b/drivers/iommu/amd_iommu_types.h
> index 4de8f41..4f5519d 100644
> --- a/drivers/iommu/amd_iommu_types.h
> +++ b/drivers/iommu/amd_iommu_types.h
> @@ -454,6 +454,8 @@ struct protection_domain {
>   bool updated;   /* complete domain flush required */
>   unsigned dev_cnt;   /* devices assigned to this domain */
>   unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference
> count */
> + bool already_flushed;   /* flag to avoid flushing the same domain
> again
> +in a single invocation of __queue_flush() */
>  };
> 
>  /*
> --
> 2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-05-22 Thread arindam . nath 
From: Arindam Nath 

Change History
--

v3:
- add Fixes and CC tags
- add link to Bugzilla

v2: changes suggested by Joerg
- add flush flag to improve efficiency of flush operation

v1:
- 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.

Bugzilla: https://bugs.freedesktop.org/101029
Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
Cc: sta...@vger.kernel.org
Suggested-by: Joerg Roedel 
Signed-off-by: Arindam Nath 
---
 drivers/iommu/amd_iommu.c   | 27 ---
 drivers/iommu/amd_iommu_types.h |  2 ++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 63cacf5..1edeebec 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2227,15 +2227,26 @@ 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(_iommu_pd_lock, flags);
-   list_for_each_entry(domain, _iommu_pd_list, list)
-   domain_flush_tlb(domain);
-   spin_unlock_irqrestore(_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;
+
+   /*
+* There might be cases where multiple IOVA entries for the
+* same domain are queued in the flush queue. To avoid
+* flushing the same domain again, we check whether the
+* flag is set or not. This improves the efficiency of
+* flush operation.
+*/
+   if (!entry->dma_dom->domain.already_flushed) {
+   entry->dma_dom->domain.already_flushed = true;
+   domain_flush_tlb(>dma_dom->domain);
+   }
+   }
 
/* Wait until flushes have completed */
domain_flush_complete(NULL);
@@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain *dma_dom,
pages = __roundup_pow_of_two(pages);
address >>= PAGE_SHIFT;
 
+   dma_dom->domain.already_flushed = false;
+
queue = get_cpu_ptr(_queue);
spin_lock_irqsave(>lock, flags);
 
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 4de8f41..4f5519d 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -454,6 +454,8 @@ struct protection_domain {
bool updated;   /* complete domain flush required */
unsigned dev_cnt;   /* devices assigned to this domain */
unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
+   bool already_flushed;   /* flag to avoid flushing the same domain again
+  in a single invocation of __queue_flush() */
 };
 
 /*
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

2017-05-21 Thread Michel Dänzer 
On 22/05/17 10:08 AM, Michel Dänzer wrote:
> On 19/05/17 07:02 PM, arindam.n...@amd.com wrote:
>> From: Arindam Nath 
>>
>> Change History
>> --
>>
>> v2: changes suggested by Joerg
>> - add flush flag to improve efficiency of flush operation
>>
>> v1:
>> - 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.
>>
>> Suggested-by: Joerg Roedel 
>> Signed-off-by: Arindam Nath 
> 
> Please add these tags:
> 
> Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
> Cc: sta...@vger.kernel.org

Also

Bugzilla: https://bugs.freedesktop.org/101029


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

2017-05-21 Thread Michel Dänzer 
On 19/05/17 07:02 PM, arindam.n...@amd.com wrote:
> From: Arindam Nath 
> 
> Change History
> --
> 
> v2: changes suggested by Joerg
> - add flush flag to improve efficiency of flush operation
> 
> v1:
> - 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.
> 
> Suggested-by: Joerg Roedel 
> Signed-off-by: Arindam Nath 

Please add these tags:

Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
Cc: sta...@vger.kernel.org


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

2017-05-21 Thread Nath, Arindam 
>-Original Message-
>From: Jan Vesely [mailto:jv...@scarletmail.rutgers.edu] On Behalf Of Jan
>Vesely
>Sent: Friday, May 19, 2017 7:06 PM
>To: Nath, Arindam; io...@lists.linux-foundation.org
>Cc: Bridgman, John; Joerg Roedel; amd-gfx@lists.freedesktop.org;
>dr...@endlessm.com; Craig Stein; Suthikulpanit, Suravee; Deucher,
>Alexander; Kuehling, Felix; li...@endlessm.com; mic...@daenzer.net
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)
>
>On Fri, 2017-05-19 at 15:32 +0530, arindam.n...@amd.com wrote:
>> From: Arindam Nath <arindam.n...@amd.com>
>>
>> Change History
>> --
>>
>> v2: changes suggested by Joerg
>> - add flush flag to improve efficiency of flush operation

Joerg, do you have any comments on the patch?

Thanks,
Arindam

>>
>> v1:
>> - 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.
>
>Hi,
>
>just a note, the patch also fixes "AMD-Vi: Completion-Wait loop timed
>out" boot hang on my system (carrizo + iceland) [0,1,2]. Presumably the
>old loop also tried to flush domains that included powered-off devices.
>
>regards,
>Jan
>
>[0] https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver/issues/20
>[1] https://bugs.freedesktop.org/show_bug.cgi?id=101029
>[2] https://bugzilla.redhat.com/show_bug.cgi?id=1448121
>
>>
>> Suggested-by: Joerg Roedel <j...@8bytes.org>
>> Signed-off-by: Arindam Nath <arindam.n...@amd.com>
>> ---
>>  drivers/iommu/amd_iommu.c   | 27 ---
>>  drivers/iommu/amd_iommu_types.h |  2 ++
>>  2 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 63cacf5..1edeebec 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -2227,15 +2227,26 @@ 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(_iommu_pd_lock, flags);
>> -list_for_each_entry(domain, _iommu_pd_list, list)
>> -domain_flush_tlb(domain);
>> -spin_unlock_irqrestore(_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;
>> +
>> +/*
>> + * There might be cases where multiple IOVA entries for the
>> + * same domain are queued in the flush queue. To avoid
>> + * flushing the same domain again, we check whether the
>> + * flag is set or not. This improves the efficiency of
>> + * flush operation.
>> + */
>> +if (!entry->dma_dom->domain.already_flushed) {
>> +entry->dma_dom->domain.already_flushed = true;
>> +domain_flush_tlb(>dma_dom->domain);
>> +}
>> +}
>>
>>  /* Wait until flushes have completed */
>>  domain_flush_complete(NULL);
>> @@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain
>*dma_dom,
>>  pages = __roundup_pow_of_two(pages);
>>  address >>= PAGE_SHIFT;
>>
>> +dma_dom->domain.already_flushed = false;
>> +
>>  queue = get_cpu_ptr(_queue);
>>  spin_lock_irqsave(>lock, flags);
>>
>> diff --git a/drivers/iommu/amd

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

2017-05-19 Thread Jan Vesely 
On Fri, 2017-05-19 at 15:32 +0530, arindam.n...@amd.com wrote:
> From: Arindam Nath 
> 
> Change History
> --
> 
> v2: changes suggested by Joerg
> - add flush flag to improve efficiency of flush operation
> 
> v1:
> - 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.

Hi,

just a note, the patch also fixes "AMD-Vi: Completion-Wait loop timed
out" boot hang on my system (carrizo + iceland) [0,1,2]. Presumably the
old loop also tried to flush domains that included powered-off devices.

regards,
Jan

[0] https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver/issues/20
[1] https://bugs.freedesktop.org/show_bug.cgi?id=101029
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1448121

> 
> Suggested-by: Joerg Roedel 
> Signed-off-by: Arindam Nath 
> ---
>  drivers/iommu/amd_iommu.c   | 27 ---
>  drivers/iommu/amd_iommu_types.h |  2 ++
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 63cacf5..1edeebec 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2227,15 +2227,26 @@ 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(_iommu_pd_lock, flags);
> - list_for_each_entry(domain, _iommu_pd_list, list)
> - domain_flush_tlb(domain);
> - spin_unlock_irqrestore(_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;
> +
> + /*
> +  * There might be cases where multiple IOVA entries for the
> +  * same domain are queued in the flush queue. To avoid
> +  * flushing the same domain again, we check whether the
> +  * flag is set or not. This improves the efficiency of
> +  * flush operation.
> +  */
> + if (!entry->dma_dom->domain.already_flushed) {
> + entry->dma_dom->domain.already_flushed = true;
> + domain_flush_tlb(>dma_dom->domain);
> + }
> + }
>  
>   /* Wait until flushes have completed */
>   domain_flush_complete(NULL);
> @@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain *dma_dom,
>   pages = __roundup_pow_of_two(pages);
>   address >>= PAGE_SHIFT;
>  
> + dma_dom->domain.already_flushed = false;
> +
>   queue = get_cpu_ptr(_queue);
>   spin_lock_irqsave(>lock, flags);
>  
> diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
> index 4de8f41..4f5519d 100644
> --- a/drivers/iommu/amd_iommu_types.h
> +++ b/drivers/iommu/amd_iommu_types.h
> @@ -454,6 +454,8 @@ struct protection_domain {
>   bool updated;   /* complete domain flush required */
>   unsigned dev_cnt;   /* devices assigned to this domain */
>   unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
> + bool already_flushed;   /* flag to avoid flushing the same domain again
> +in a single invocation of __queue_flush() */
>  };
>  
>  /*

-- 
Jan Vesely 

signature.asc
Description: This is a digitally signed message part
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-05-19 Thread arindam . nath 
From: Arindam Nath 

Change History
--

v2: changes suggested by Joerg
- add flush flag to improve efficiency of flush operation

v1:
- 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.

Suggested-by: Joerg Roedel 
Signed-off-by: Arindam Nath 
---
 drivers/iommu/amd_iommu.c   | 27 ---
 drivers/iommu/amd_iommu_types.h |  2 ++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 63cacf5..1edeebec 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2227,15 +2227,26 @@ 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(_iommu_pd_lock, flags);
-   list_for_each_entry(domain, _iommu_pd_list, list)
-   domain_flush_tlb(domain);
-   spin_unlock_irqrestore(_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;
+
+   /*
+* There might be cases where multiple IOVA entries for the
+* same domain are queued in the flush queue. To avoid
+* flushing the same domain again, we check whether the
+* flag is set or not. This improves the efficiency of
+* flush operation.
+*/
+   if (!entry->dma_dom->domain.already_flushed) {
+   entry->dma_dom->domain.already_flushed = true;
+   domain_flush_tlb(>dma_dom->domain);
+   }
+   }
 
/* Wait until flushes have completed */
domain_flush_complete(NULL);
@@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain *dma_dom,
pages = __roundup_pow_of_two(pages);
address >>= PAGE_SHIFT;
 
+   dma_dom->domain.already_flushed = false;
+
queue = get_cpu_ptr(_queue);
spin_lock_irqsave(>lock, flags);
 
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 4de8f41..4f5519d 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -454,6 +454,8 @@ struct protection_domain {
bool updated;   /* complete domain flush required */
unsigned dev_cnt;   /* devices assigned to this domain */
unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
+   bool already_flushed;   /* flag to avoid flushing the same domain again
+  in a single invocation of __queue_flush() */
 };
 
 /*
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-05-18 Thread Michel Dänzer 
On 07/04/17 07:20 PM, Joerg Roedel wrote:
> On Mon, Mar 27, 2017 at 11:47:07AM +0530, arindam.n...@amd.com wrote:
>> From: Arindam Nath 
>>
>> 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 
>> ---
>>  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(_iommu_pd_lock, flags);
>> -list_for_each_entry(domain, _iommu_pd_list, list)
>> -domain_flush_tlb(domain);
>> -spin_unlock_irqrestore(_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(>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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-05-08 Thread Daniel Drake 
On Wed, Apr 5, 2017 at 9:01 AM, Nath, Arindam <arindam.n...@amd.com> wrote:
>
> >-Original Message-
> >From: Daniel Drake [mailto:dr...@endlessm.com]
> >Sent: Thursday, March 30, 2017 7:15 PM
> >To: Nath, Arindam
> >Cc: j...@8bytes.org; Deucher, Alexander; Bridgman, John; amd-
> >g...@lists.freedesktop.org; io...@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.n...@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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-04-05 Thread Nath, Arindam 
>-Original Message-
>From: Daniel Drake [mailto:dr...@endlessm.com]
>Sent: Thursday, March 30, 2017 7:15 PM
>To: Nath, Arindam
>Cc: j...@8bytes.org; Deucher, Alexander; Bridgman, John; amd-
>g...@lists.freedesktop.org; io...@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.n...@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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-03-30 Thread Nath, Arindam 
>-Original Message-
>From: Daniel Drake [mailto:dr...@endlessm.com]
>Sent: Thursday, March 30, 2017 7:15 PM
>To: Nath, Arindam
>Cc: j...@8bytes.org; Deucher, Alexander; Bridgman, John; amd-
>g...@lists.freedesktop.org; io...@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.n...@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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-03-30 Thread Nath, Arindam 
>-Original Message-
>From: Nath, Arindam
>Sent: Monday, March 27, 2017 5:57 PM
>To: 'Daniel Drake'
>Cc: j...@8bytes.org; Deucher, Alexander; Bridgman, John; amd-
>g...@lists.freedesktop.org; io...@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:dr...@endlessm.com]
>>Sent: Monday, March 27, 2017 5:56 PM
>>To: Nath, Arindam
>>Cc: j...@8bytes.org; Deucher, Alexander; Bridgman, John; amd-
>>g...@lists.freedesktop.org; io...@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.n...@amd.com> wrote:
>>> From: Arindam Nath <arindam.n...@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.n...@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(_iommu_pd_lock, flags);
>>> -   list_for_each_entry(domain, _iommu_pd_list, list)
>>> -   domain_flush_tlb(domain);
>>> -   spin_unlock_irqrestore(_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(>dma_dom->domain);
>>> +   }
>>>
>>> /* Wait until flushes have completed */
>>> domain_flush_complete(NULL);
>>> --
>>> 1.9.1
>>>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-03-27 Thread Nath, Arindam 
>-Original Message-
>From: Daniel Drake [mailto:dr...@endlessm.com]
>Sent: Monday, March 27, 2017 5:56 PM
>To: Nath, Arindam
>Cc: j...@8bytes.org; Deucher, Alexander; Bridgman, John; amd-
>g...@lists.freedesktop.org; io...@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.n...@amd.com> wrote:
>> From: Arindam Nath <arindam.n...@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.n...@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(_iommu_pd_lock, flags);
>> -   list_for_each_entry(domain, _iommu_pd_list, list)
>> -   domain_flush_tlb(domain);
>> -   spin_unlock_irqrestore(_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(>dma_dom->domain);
>> +   }
>>
>> /* Wait until flushes have completed */
>> domain_flush_complete(NULL);
>> --
>> 1.9.1
>>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-03-27 Thread Daniel Drake 
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,   wrote:
> From: Arindam Nath 
>
> 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 
> ---
>  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(_iommu_pd_lock, flags);
> -   list_for_each_entry(domain, _iommu_pd_list, list)
> -   domain_flush_tlb(domain);
> -   spin_unlock_irqrestore(_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(>dma_dom->domain);
> +   }
>
> /* Wait until flushes have completed */
> domain_flush_complete(NULL);
> --
> 1.9.1
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-03-27 Thread arindam . nath 
From: Arindam Nath 

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 
---
 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(_iommu_pd_lock, flags);
-   list_for_each_entry(domain, _iommu_pd_list, list)
-   domain_flush_tlb(domain);
-   spin_unlock_irqrestore(_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(>dma_dom->domain);
+   }
 
/* Wait until flushes have completed */
domain_flush_complete(NULL);
-- 
1.9.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx