pci, dmar: flush IOTLB before exit domain

PATCH In review Yinghai Lu 6 participants 9 messages May 6, 2011 – May 24, 2011 PCI

lore

Share & tools

lore

Yinghai Lu

author

2011-05-06 01:13 UTC #0 lore raw

PATCH 1/1 pci, dmar: flush IOTLB before exit domain

during one hotplug testing on system that support iommu/dmar. got memory corruption.

[  578.279327] pci 0000:c4:00.0: no hotplug settings from platform
[  578.299240] scsi11 : Fusion MPT SAS Host
[  578.301033] mpt2sas2: mpt2sas_base_attach
[  578.302643] mpt2sas2: mpt2sas_base_map_resources
[  578.302797] BUG: Bad page state in process udevd  pfn:dffe23e
[  578.302801] page:ffffea030ff97d90 count:0 mapcount:-27 mapping:          (null) index:0xffff88dffe23eec0
[  578.302803] page flags: 0x1a00000000000000()
[  578.302807] Pid: 18215, comm: udevd Not tainted 2.6.39-rc5-tip-yh-03961-gced6a85-dirty #898
[  578.302809] Call Trace:
[  578.302825]  [<ffffffff81101528>] ? dump_page+0xbb/0xc0
[  578.302831]  [<ffffffff8110160a>] bad_page+0xdd/0xf2
[  578.302838]  [<ffffffff811023e6>] prep_new_page+0x70/0x141
[  578.302844]  [<ffffffff811028da>] get_page_from_freelist+0x423/0x59f
[  578.302851]  [<ffffffff81102c0a>] __alloc_pages_nodemask+0x1b4/0x7fe
[  578.302864]  [<ffffffff810a276a>] ? local_clock+0x2b/0x3c
[  578.302879]  [<ffffffff8111929e>] ? __pud_alloc+0x73/0x84
[  578.302885]  [<ffffffff810a276a>] ? local_clock+0x2b/0x3c
[  578.302896]  [<ffffffff8112d5a1>] alloc_pages_current+0xba/0xdd
[  578.302903]  [<ffffffff810ff774>] __get_free_pages+0xe/0x4b
[  578.302909]  [<ffffffff810ff7c7>] get_zeroed_page+0x16/0x18
[  578.302915]  [<ffffffff811192d1>] __pmd_alloc+0x22/0x85
[  578.302922]  [<ffffffff8111a6ad>] copy_page_range+0x238/0x3d8
[  578.302938]  [<ffffffff8107dd1b>] dup_mmap+0x2b9/0x375
[  578.302944]  [<ffffffff8107e3c5>] dup_mm+0xab/0x171
[  578.302951]  [<ffffffff8107eb99>] copy_process+0x6ea/0xd8e
[  578.302959]  [<ffffffff810b1a87>] ? __lock_release+0x166/0x16f
[  578.302965]  [<ffffffff8107f396>] do_fork+0x130/0x2dd
[  578.302976]  [<ffffffff811541c2>] ? mntput_no_expire+0x27/0xc8
[  578.302982]  [<ffffffff81154289>] ? mntput+0x26/0x28
[  578.302994]  [<ffffffff8113c429>] ? __fput+0x1b9/0x1c8
[  578.303004]  [<ffffffff81c2f69c>] ? sysret_check+0x27/0x62
[  578.303015]  [<ffffffff81040f41>] sys_clone+0x28/0x2a
[  578.303021]  [<ffffffff81c2f953>] stub_clone+0x13/0x20
[  578.303027]  [<ffffffff81c2f66b>] ? system_call_fastpath+0x16/0x1b

the bug is uncoverred by

| commit a97590e56d0d58e1dd262353f7cbd84e81d8e600
| Author: Alex Williamson <[email protected]>
| Date:   Fri Mar 4 14:52:16 2011 -0700
|
|    intel-iommu: Unlink domain from iommu
|    
|    When we remove a device, we unlink the iommu from the domain, but
|    we never do the reverse unlinking of the domain from the iommu.
|    This means that we never clear iommu->domain_ids, eventually leading
|    to resource exhaustion if we repeatedly bind and unbind a device
|    to a driver.  Also free empty domains to avoid a resource leak.

that will remove domain really...
It exposes the problem that defer flushing is not handled properly during hot removing.

Try to flush unmaps before exit.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/intel-iommu.c |    3 +++
 1 file changed, 3 insertions(+)

drivers/pci/intel-iommu.c +3 -0

Index: linux-2.6/drivers/pci/intel-iommu.c
===================================================================
@@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
 		return 0;
 
 	if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
+		/* before we remove dev with domain, flush IOTLB */
+		flush_unmaps();
+
 		domain_remove_one_dev_info(domain, pdev);
 
 		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&

David Woodhouse

2011-05-09 14:49 UTC #1 lore raw

drivers/pci/intel-iommu.c static int device_notifier(struct notifi · line 3260

Quoted 10 lines · Yinghai Lu

drivers/pci/intel-iommu.c +3 -0

@@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
                return 0;
 
        if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
+               /* before we remove dev with domain, flush IOTLB */
+               flush_unmaps();
+
                domain_remove_one_dev_info(domain, pdev);
 
                if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&

That calls flush_unmaps() without the async_umap_flush_lock held, doesn't it? A few days ago I asked someone else to test this candidate patch for a similar issue:

http://david.woodhou.se/flush-unmaps-on-unbind.patch

Signature · 2 lines

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

Yinghai Lu

2011-05-09 20:57 UTC #2 lore raw

Quoted 10 lines · David Woodhouse

Quoted 9 lines · Yinghai Lu

drivers/pci/intel-iommu.c +3 -0

@@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
                return 0;
 
        if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
+               /* before we remove dev with domain, flush IOTLB */
+               flush_unmaps();
+
                domain_remove_one_dev_info(domain, pdev);

Quoted 8 lines

drivers/pci/intel-iommu.c static int device_notifier(struct notifi · line 3260
Quoted 1 line
drivers/pci/intel-iommu.c +0 -0
                if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
That calls flush_unmaps() without the async_umap_flush_lock held, doesn't it? A few days ago I asked someone else to test this candidate patch for a similar issue:
http://david.woodhou.se/flush-unmaps-on-unbind.patch

Your patch works.

Thanks

Yinghai

Alex Williamson

2011-05-16 15:13 UTC #3 lore raw

In reply to David Woodhouse

Quoted 10 lines · David Woodhouse

Quoted 9 lines · Yinghai Lu

drivers/pci/intel-iommu.c +3 -0

@@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
                return 0;
 
        if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
+               /* before we remove dev with domain, flush IOTLB */
+               flush_unmaps();
+
                domain_remove_one_dev_info(domain, pdev);

Quoted 7 lines

drivers/pci/intel-iommu.c static int device_notifier(struct notifi · line 3260
Quoted 1 line
drivers/pci/intel-iommu.c +0 -0
                if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
That calls flush_unmaps() without the async_umap_flush_lock held, doesn't it? A few days ago I asked someone else to test this candidate patch for a similar issue:
http://david.woodhou.se/flush-unmaps-on-unbind.patch

Copying here:

Quoted 17 lines

drivers/pci/intel-iommu.c +3 -1

index d552d2c..7e606d6 100644
@@ -3256,8 +3259,10 @@ static int device_notifier(struct notifier_block *nb,
 
 		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
 		    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
-		    list_empty(&domain->devices))
+		    list_empty(&domain->devices)) {
+			flush_unmaps_timeout(0);
 			domain_exit(domain);
+		}
 	}
 
 	return 0;

drivers/pci/intel-iommu.c static void intel_iommu_domain_destroy(struct iommu_domain *domain) · line 3597

Quoted 6 lines

drivers/pci/intel-iommu.c +1 -0

@@ -3587,6 +3592,7 @@ static void intel_iommu_domain_destroy(struct iommu_domain *domain)
 	struct dmar_domain *dmar_domain = domain->priv;
 
 	domain->priv = NULL;
+	flush_unmaps_timeout(0);
 	vm_domain_exit(dmar_domain);
 }

David, would it be worthwhile to push the unmaps into the {vm_}domain_exit() functions to avoid races like this in the future? I can verify the above resolves a panic after unbinding a device from snd_hda_intel that I hit recently. Do you plan to push this for .39? Thanks,

Alex

Alex Williamson

2011-05-17 19:41 UTC #4 lore raw

Quoted 40 lines · Alex Williamson

Quoted 10 lines · David Woodhouse

Quoted 9 lines · Yinghai Lu

drivers/pci/intel-iommu.c +3 -0

@@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
                return 0;
 
        if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
+               /* before we remove dev with domain, flush IOTLB */
+               flush_unmaps();
+
                domain_remove_one_dev_info(domain, pdev);

Quoted 7 lines

drivers/pci/intel-iommu.c static int device_notifier(struct notifi · line 3260
Quoted 1 line
drivers/pci/intel-iommu.c +0 -0
                if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
That calls flush_unmaps() without the async_umap_flush_lock held, doesn't it? A few days ago I asked someone else to test this candidate patch for a similar issue:
http://david.woodhou.se/flush-unmaps-on-unbind.patch

Copying here:

Quoted 19 lines

drivers/pci/intel-iommu.c +3 -1

index d552d2c..7e606d6 100644
@@ -3256,8 +3259,10 @@ static int device_notifier(struct notifier_block *nb,
 
 		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
 		    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
-		    list_empty(&domain->devices))
+		    list_empty(&domain->devices)) {
+			flush_unmaps_timeout(0);
 			domain_exit(domain);
+		}
 	}
 
 	return 0;

@@ -3587,6 +3592,7 @@ static void intel_iommu_domain_destroy(struct iommu_domain *domain)
 	struct dmar_domain *dmar_domain = domain->priv;

Quoted 9 lines

drivers/pci/intel-iommu.c static void intel_iommu_domain_destroy(struct iommu_domain *domain) · line 3597
Quoted 4 lines
drivers/pci/intel-iommu.c +1 -0
 	domain->priv = NULL;
+	flush_unmaps_timeout(0);
 	vm_domain_exit(dmar_domain);
 }
David, would it be worthwhile to push the unmaps into the {vm_}domain_exit() functions to avoid races like this in the future? I can verify the above resolves a panic after unbinding a device from snd_hda_intel that I hit recently. Do you plan to push this for .39?

BTW, is this second chunk really needed? VM iommu mappings don't seem to use the lazy unmap path. Thanks,

Alex

Alex Williamson

2011-05-18 01:57 UTC #5 lore raw

Quoted 51 lines · Alex Williamson

Quoted 40 lines · Alex Williamson
Quoted 10 lines · David Woodhouse
Quoted 9 lines · Yinghai Lu
(quoted diff) +3 -0
@@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
                return 0;
 
        if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
+               /* before we remove dev with domain, flush IOTLB */
+               flush_unmaps();
+
                domain_remove_one_dev_info(domain, pdev);
 
Quoted 7 lines
Review static int device_notifier(struct notifi · line 3260
Quoted 1 line
(quoted diff) +0 -0
                if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
That calls flush_unmaps() without the async_umap_flush_lock held, doesn't it? A few days ago I asked someone else to test this candidate patch for a similar issue:
http://david.woodhou.se/flush-unmaps-on-unbind.patch
Copying here:
Quoted 19 lines
drivers/pci/intel-iommu.c +3 -1
index d552d2c..7e606d6 100644
@@ -3256,8 +3259,10 @@ static int device_notifier(struct notifier_block *nb,
 
 		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
 		    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
-		    list_empty(&domain->devices))
+		    list_empty(&domain->devices)) {
+			flush_unmaps_timeout(0);
 			domain_exit(domain);
+		}
 	}
 
 	return 0;
@@ -3587,6 +3592,7 @@ static void intel_iommu_domain_destroy(struct iommu_domain *domain)
 	struct dmar_domain *dmar_domain = domain->priv;
 
Quoted 9 lines
drivers/pci/intel-iommu.c static void intel_iommu_domain_destroy(struct iommu_domain *domain) · line 3597
Quoted 4 lines
drivers/pci/intel-iommu.c +1 -0
 	domain->priv = NULL;
+	flush_unmaps_timeout(0);
 	vm_domain_exit(dmar_domain);
 }
David, would it be worthwhile to push the unmaps into the {vm_}domain_exit() functions to avoid races like this in the future? I can verify the above resolves a panic after unbinding a device from snd_hda_intel that I hit recently. Do you plan to push this for .39?

Quoted 2 lines

BTW, is this second chunk really needed? VM iommu mappings don't seem to use the lazy unmap path. Thanks,

David, what do you think of this instead? Thanks,

Alex

intel-iommu: Flush unmaps at domain_exit

From: Alex Williamson <[email protected]>

We typically batch unmaps to be lazily flushed out at regular intervals. When we destroy a domain, we need to force a flush of these lazy unmaps to be sure none reference the domain we're about to free.

Signed-off-by Alex Williamson <[email protected]>

---

 drivers/pci/intel-iommu.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/pci/intel-iommu.c +4 -0

index d552d2c..b04f84e 100644
@@ -1416,6 +1416,10 @@ static void domain_exit(struct dmar_domain *domain)
 	if (!domain)
 		return;
 
+	/* Flush any lazy unmaps that may reference this domain */
+	if (!intel_iommu_strict)
+		flush_unmaps_timeout(0);
+
 	domain_remove_dev_info(domain);
 	/* destroy iovas */
 	put_iova_domain(&domain->iovad);

Jan Kiszka

2011-05-21 14:38 UTC #6 lore raw

Quoted 82 lines · Alex Williamson

Quoted 51 lines · Alex Williamson
Quoted 40 lines · Alex Williamson
Quoted 10 lines · David Woodhouse
Quoted 9 lines · Yinghai Lu
(quoted diff) +3 -0
@@ -3252,6 +3252,9 @@ static int device_notifier(struct notifi
                return 0;
 
        if (action == BUS_NOTIFY_UNBOUND_DRIVER && !iommu_pass_through) {
+               /* before we remove dev with domain, flush IOTLB */
+               flush_unmaps();
+
                domain_remove_one_dev_info(domain, pdev);
 
Quoted 7 lines
Review static int device_notifier(struct notifi · line 3260
Quoted 1 line
(quoted diff) +0 -0
                if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) && 
That calls flush_unmaps() without the async_umap_flush_lock held, doesn't it? A few days ago I asked someone else to test this candidate patch for a similar issue:
http://david.woodhou.se/flush-unmaps-on-unbind.patch
Copying here:
Quoted 19 lines
drivers/pci/intel-iommu.c +3 -1
index d552d2c..7e606d6 100644
@@ -3256,8 +3259,10 @@ static int device_notifier(struct notifier_block *nb,
 
 		if (!(domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE) &&
 		    !(domain->flags & DOMAIN_FLAG_STATIC_IDENTITY) &&
-		    list_empty(&domain->devices))
+		    list_empty(&domain->devices)) {
+			flush_unmaps_timeout(0);
 			domain_exit(domain);
+		}
 	}
 
 	return 0;
@@ -3587,6 +3592,7 @@ static void intel_iommu_domain_destroy(struct iommu_domain *domain)
 	struct dmar_domain *dmar_domain = domain->priv;
 
Quoted 9 lines
drivers/pci/intel-iommu.c static void intel_iommu_domain_destroy(struct iommu_domain *domain) · line 3597
Quoted 4 lines
drivers/pci/intel-iommu.c +1 -0
 	domain->priv = NULL;
+	flush_unmaps_timeout(0);
 	vm_domain_exit(dmar_domain);
 }
David, would it be worthwhile to push the unmaps into the {vm_}domain_exit() functions to avoid races like this in the future? I can verify the above resolves a panic after unbinding a device from snd_hda_intel that I hit recently. Do you plan to push this for .39?
Quoted 2 lines
BTW, is this second chunk really needed? VM iommu mappings don't seem to use the lazy unmap path. Thanks,
David, what do you think of this instead? Thanks,
Alex
intel-iommu: Flush unmaps at domain_exit
From: Alex Williamson <[email protected]>
We typically batch unmaps to be lazily flushed out at regular intervals. When we destroy a domain, we need to force a flush of these lazy unmaps to be sure none reference the domain we're about to free.
Signed-off-by Alex Williamson <[email protected]>
---
 drivers/pci/intel-iommu.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/pci/intel-iommu.c +0 -0
index d552d2c..b04f84e 100644

drivers/pci/intel-iommu.c static void domain_exit(struct dmar_domain *domain) · line 1425

Quoted 10 lines

drivers/pci/intel-iommu.c +4 -0

@@ -1416,6 +1416,10 @@ static void domain_exit(struct dmar_domain *domain)
 	if (!domain)
 		return;
 
+	/* Flush any lazy unmaps that may reference this domain */
+	if (!intel_iommu_strict)
+		flush_unmaps_timeout(0);
+
 	domain_remove_dev_info(domain);
 	/* destroy iovas */
 	put_iova_domain(&domain->iovad);

Can we get this resolved for 2.6.39.1? Via David's patch, or Alex's (which works for me), or whatever. It's a really annoying regression of .39 when you want to run with IOMMU enabled.

Thanks,
Jan

Woodhouse, David

2011-05-24 11:05 UTC #7 lore raw

In reply to Alex Williamson

Quoted 2 lines · Alex Williamson

David, what do you think of this instead? Thanks,

Yes, that makes more sense. Thanks.

Sorry for not getting this in before the 2.6.39 release. I'm merging it now (with Cc: stable) and will push to Linus in a couple of days.

Signature · 2 lines

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

Alex Williamson

2011-05-24 16:30 UTC #8 lore raw

Quoted 8 lines · David

Quoted 2 lines · Alex Williamson
David, what do you think of this instead? Thanks,
Yes, that makes more sense. Thanks.
Sorry for not getting this in before the 2.6.39 release. I'm merging it now (with Cc: stable) and will push to Linus in a couple of days.

Thanks David. Please also review and include the domain unlinking patch I just sent. It should go in stable too. Thanks,

Alex