[PATCH 0/4] iommu/amd: Clean up patches

[PATCH 0/4] iommu/amd: Clean up patches

[Baoquan He]

These were found out when I tried to fix the kdump failure on system
with AMD iommu. Pack them into this patchset since they are not related
to the kdump issue and each other.

Baoquan He (4):
  iommu/amd: clean up the cmpxchg64 invocation
  iommu/amd: Use standard bitmap operation to set bitmap
  iommu/amd: Free domain id when free a domain of struct dma_ops_domain
  iommu/amd: No need to wait iommu completion if no dte irq entry change

 drivers/iommu/amd_iommu.c      | 12 ++++++++----
 drivers/iommu/amd_iommu_init.c |  3 ++-
 2 files changed, 10 insertions(+), 5 deletions(-)

-- 
2.5.5

[PATCH 1/4] iommu/amd: clean up the cmpxchg64 invocation

[Baoquan He]

Change it as it's designed for and keep it consistent with other
places.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 96de97a..160fc6a 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1274,7 +1274,8 @@ static u64 *alloc_pte(struct protection_domain *domain,
 
 			__npte = PM_LEVEL_PDE(level, virt_to_phys(page));
 
-			if (cmpxchg64(pte, __pte, __npte)) {
+			/* pte could have been changed somewhere. */
+			if (cmpxchg64(pte, __pte, __npte) != __pte) {
 				free_page((unsigned long)page);
 				continue;
 			}
-- 
2.5.5

[PATCH 1/4] iommu/amd: clean up the cmpxchg64 invocation

[Baoquan He]

Change it as it's designed for and keep it consistent with other
places.

Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/amd_iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 96de97a..160fc6a 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1274,7 +1274,8 @@ static u64 *alloc_pte(struct protection_domain *domain,
 
 			__npte = PM_LEVEL_PDE(level, virt_to_phys(page));
 
-			if (cmpxchg64(pte, __pte, __npte)) {
+			/* pte could have been changed somewhere. */
+			if (cmpxchg64(pte, __pte, __npte) != __pte) {
 				free_page((unsigned long)page);
 				continue;
 			}
-- 
2.5.5

[PATCH 2/4] iommu/amd: Use standard bitmap operation to set bitmap

[Baoquan He]

It will be more readable and safer than the old setting.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu_init.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 59741ea..3e810c6 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -20,6 +20,7 @@
 #include <linux/pci.h>
 #include <linux/acpi.h>
 #include <linux/list.h>
+#include <linux/bitmap.h>
 #include <linux/slab.h>
 #include <linux/syscore_ops.h>
 #include <linux/interrupt.h>
@@ -2136,7 +2137,7 @@ static int __init early_amd_iommu_init(void)
 	 * never allocate domain 0 because its used as the non-allocated and
 	 * error value placeholder
 	 */
-	amd_iommu_pd_alloc_bitmap[0] = 1;
+	__set_bit(0, amd_iommu_pd_alloc_bitmap);
 
 	spin_lock_init(&amd_iommu_pd_lock);
 
-- 
2.5.5

[PATCH 2/4] iommu/amd: Use standard bitmap operation to set bitmap

[Baoquan He]

It will be more readable and safer than the old setting.

Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/amd_iommu_init.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 59741ea..3e810c6 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -20,6 +20,7 @@
 #include <linux/pci.h>
 #include <linux/acpi.h>
 #include <linux/list.h>
+#include <linux/bitmap.h>
 #include <linux/slab.h>
 #include <linux/syscore_ops.h>
 #include <linux/interrupt.h>
@@ -2136,7 +2137,7 @@ static int __init early_amd_iommu_init(void)
 	 * never allocate domain 0 because its used as the non-allocated and
 	 * error value placeholder
 	 */
-	amd_iommu_pd_alloc_bitmap[0] = 1;
+	__set_bit(0, amd_iommu_pd_alloc_bitmap);
 
 	spin_lock_init(&amd_iommu_pd_lock);
 
-- 
2.5.5

[PATCH 3/4] iommu/amd: Free domain id when free a domain of struct dma_ops_domain

[Baoquan He]

The current code missed freeing domain id when free a domain of
struct dma_ops_domain.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 160fc6a..a9f78c2 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1655,6 +1655,9 @@ static void dma_ops_domain_free(struct dma_ops_domain *dom)
 
 	free_pagetable(&dom->domain);
 
+	if (dom->domain.id)
+		domain_id_free(dom->domain.id);
+
 	kfree(dom);
 }
 
-- 
2.5.5

[PATCH 3/4] iommu/amd: Free domain id when free a domain of struct dma_ops_domain

[Baoquan He]

The current code missed freeing domain id when free a domain of
struct dma_ops_domain.

Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/amd_iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 160fc6a..a9f78c2 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1655,6 +1655,9 @@ static void dma_ops_domain_free(struct dma_ops_domain *dom)
 
 	free_pagetable(&dom->domain);
 
+	if (dom->domain.id)
+		domain_id_free(dom->domain.id);
+
 	kfree(dom);
 }
 
-- 
2.5.5

[PATCH 4/4] iommu/amd: No need to wait iommu completion if no dte irq entry change

[Baoquan He]

This is a clean up. In get_irq_table() only if DTE entry is changed
iommu_completion_wait() need be called. Otherwise no need to do it.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index a9f78c2..461c2fe 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3581,7 +3581,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 
 	table = irq_lookup_table[devid];
 	if (table)
-		goto out;
+		goto out_unlock;
 
 	alias = amd_iommu_alias_table[devid];
 	table = irq_lookup_table[alias];
@@ -3595,7 +3595,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	/* Nothing there yet, allocate new irq remapping table */
 	table = kzalloc(sizeof(*table), GFP_ATOMIC);
 	if (!table)
-		goto out;
+		goto out_unlock;
 
 	/* Initialize table spin-lock */
 	spin_lock_init(&table->lock);
@@ -3608,7 +3608,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	if (!table->table) {
 		kfree(table);
 		table = NULL;
-		goto out;
+		goto out_unlock;
 	}
 
 	memset(table->table, 0, MAX_IRQS_PER_TABLE * sizeof(u32));
-- 
2.5.5

[PATCH 4/4] iommu/amd: No need to wait iommu completion if no dte irq entry change

[Baoquan He]

This is a clean up. In get_irq_table() only if DTE entry is changed
iommu_completion_wait() need be called. Otherwise no need to do it.

Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/amd_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index a9f78c2..461c2fe 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3581,7 +3581,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 
 	table = irq_lookup_table[devid];
 	if (table)
-		goto out;
+		goto out_unlock;
 
 	alias = amd_iommu_alias_table[devid];
 	table = irq_lookup_table[alias];
@@ -3595,7 +3595,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	/* Nothing there yet, allocate new irq remapping table */
 	table = kzalloc(sizeof(*table), GFP_ATOMIC);
 	if (!table)
-		goto out;
+		goto out_unlock;
 
 	/* Initialize table spin-lock */
 	spin_lock_init(&table->lock);
@@ -3608,7 +3608,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	if (!table->table) {
 		kfree(table);
 		table = NULL;
-		goto out;
+		goto out_unlock;
 	}
 
 	memset(table->table, 0, MAX_IRQS_PER_TABLE * sizeof(u32));
-- 
2.5.5

Re: [PATCH 0/4] iommu/amd: Clean up patches

[Joerg Roedel]

Hi Baoquan,

On Thu, Sep 15, 2016 at 04:50:49PM +0800, Baoquan He wrote:
> These were found out when I tried to fix the kdump failure on system
> with AMD iommu. Pack them into this patchset since they are not related
> to the kdump issue and each other.
> 
> Baoquan He (4):
>   iommu/amd: clean up the cmpxchg64 invocation
>   iommu/amd: Use standard bitmap operation to set bitmap
>   iommu/amd: Free domain id when free a domain of struct dma_ops_domain
>   iommu/amd: No need to wait iommu completion if no dte irq entry change

I applied patches 1-3, but patch 4 had some conflicts. Can you please
rebase this patch to my x86/amd branch and resend?


Thanks,

	Joerg

[PATCH RESEND 4/4] iommu/amd: No need to wait iommu completion if no dte irq entry change

[Baoquan He]

This is a clean up. In get_irq_table() only if DTE entry is changed
iommu_completion_wait() need be called. Otherwise no need to do it.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index b5b117b..a2479d0 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3634,7 +3634,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 
 	table = irq_lookup_table[devid];
 	if (table)
-		goto out;
+		goto out_unlock;
 
 	alias = amd_iommu_alias_table[devid];
 	table = irq_lookup_table[alias];
@@ -3648,7 +3648,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	/* Nothing there yet, allocate new irq remapping table */
 	table = kzalloc(sizeof(*table), GFP_ATOMIC);
 	if (!table)
-		goto out;
+		goto out_unlock;
 
 	/* Initialize table spin-lock */
 	spin_lock_init(&table->lock);
@@ -3661,7 +3661,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	if (!table->table) {
 		kfree(table);
 		table = NULL;
-		goto out;
+		goto out_unlock;
 	}
 
 	if (!AMD_IOMMU_GUEST_IR_GA(amd_iommu_guest_ir))
-- 
2.5.5

[PATCH RESEND 4/4] iommu/amd: No need to wait iommu completion if no dte irq entry change

[Baoquan He]

This is a clean up. In get_irq_table() only if DTE entry is changed
iommu_completion_wait() need be called. Otherwise no need to do it.

Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/amd_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index b5b117b..a2479d0 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3634,7 +3634,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 
 	table = irq_lookup_table[devid];
 	if (table)
-		goto out;
+		goto out_unlock;
 
 	alias = amd_iommu_alias_table[devid];
 	table = irq_lookup_table[alias];
@@ -3648,7 +3648,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	/* Nothing there yet, allocate new irq remapping table */
 	table = kzalloc(sizeof(*table), GFP_ATOMIC);
 	if (!table)
-		goto out;
+		goto out_unlock;
 
 	/* Initialize table spin-lock */
 	spin_lock_init(&table->lock);
@@ -3661,7 +3661,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	if (!table->table) {
 		kfree(table);
 		table = NULL;
-		goto out;
+		goto out_unlock;
 	}
 
 	if (!AMD_IOMMU_GUEST_IR_GA(amd_iommu_guest_ir))
-- 
2.5.5

Re: [PATCH 0/4] iommu/amd: Clean up patches

[Baoquan He]

On 09/19/16 at 04:20pm, Joerg Roedel wrote:
> Hi Baoquan,
> 
> On Thu, Sep 15, 2016 at 04:50:49PM +0800, Baoquan He wrote:
> > These were found out when I tried to fix the kdump failure on system
> > with AMD iommu. Pack them into this patchset since they are not related
> > to the kdump issue and each other.
> > 
> > Baoquan He (4):
> >   iommu/amd: clean up the cmpxchg64 invocation
> >   iommu/amd: Use standard bitmap operation to set bitmap
> >   iommu/amd: Free domain id when free a domain of struct dma_ops_domain
> >   iommu/amd: No need to wait iommu completion if no dte irq entry change
> 
> I applied patches 1-3, but patch 4 had some conflicts. Can you please
> rebase this patch to my x86/amd branch and resend?

Finished, please try the resent post.

Thanks
Baoquan

Re: [PATCH 0/4] iommu/amd: Clean up patches

[Baoquan He]

On 09/19/16 at 04:20pm, Joerg Roedel wrote:
> Hi Baoquan,
> 
> On Thu, Sep 15, 2016 at 04:50:49PM +0800, Baoquan He wrote:
> > These were found out when I tried to fix the kdump failure on system
> > with AMD iommu. Pack them into this patchset since they are not related
> > to the kdump issue and each other.
> > 
> > Baoquan He (4):
> >   iommu/amd: clean up the cmpxchg64 invocation
> >   iommu/amd: Use standard bitmap operation to set bitmap
> >   iommu/amd: Free domain id when free a domain of struct dma_ops_domain
> >   iommu/amd: No need to wait iommu completion if no dte irq entry change
> 
> I applied patches 1-3, but patch 4 had some conflicts. Can you please
> rebase this patch to my x86/amd branch and resend?

Finished, please try the resent post.

Thanks
Baoquan

Re: [PATCH RESEND 4/4] iommu/amd: No need to wait iommu completion if no dte irq entry change

[Joerg Roedel]

On Tue, Sep 20, 2016 at 09:05:34AM +0800, Baoquan He wrote:
> This is a clean up. In get_irq_table() only if DTE entry is changed
> iommu_completion_wait() need be called. Otherwise no need to do it.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  drivers/iommu/amd_iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Applied, thanks.

Re: [PATCH RESEND 4/4] iommu/amd: No need to wait iommu completion if no dte irq entry change

[Baoquan He]

On 09/20/16 at 11:56am, Joerg Roedel wrote:
> On Tue, Sep 20, 2016 at 09:05:34AM +0800, Baoquan He wrote:
> > This is a clean up. In get_irq_table() only if DTE entry is changed
> > iommu_completion_wait() need be called. Otherwise no need to do it.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  drivers/iommu/amd_iommu.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Applied, thanks.

Thanks!