Patchworkβ [4/5] intel-iommu: Reinstate RMRRs if a device is removed from passthrough domain

login
register
about
Submitter Alex Williamson
Date 2009-10-26 23:25:14
Message ID <20091026232514.9646.58322.stgit@nehalem.aw>
Download mbox | patch
Permalink /patch/56008/
State Not Applicable
Headers show

Comments

Alex Williamson - 2009-10-26 23:25:14
When a device is setup for passthrough it has full access to memory
so processing the RMRRs is unnecessary.  However, if we remove the device
from the si_domain, we need to reinstate the associated RMRRs.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

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


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Woodhouse - 2009-10-27 08:15:24
On Mon, 2009-10-26 at 17:25 -0600, Alex Williamson wrote:
> When a device is setup for passthrough it has full access to memory
> so processing the RMRRs is unnecessary.  However, if we remove the device
> from the si_domain, we need to reinstate the associated RMRRs.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

If your device is doing DMA to host memory autonomously, you may still
have problems with this patch -- you take it out of the si_domain and
then there's a period of time before you reapply the RMRRs, during which
its DMA may be prevented.

You want to set up the new domain first, then switch the device over to
it atomically.
Alex Williamson - 2009-10-27 15:50:32
On Tue, 2009-10-27 at 08:15 +0000, David Woodhouse wrote:
> On Mon, 2009-10-26 at 17:25 -0600, Alex Williamson wrote:
> > When a device is setup for passthrough it has full access to memory
> > so processing the RMRRs is unnecessary.  However, if we remove the device
> > from the si_domain, we need to reinstate the associated RMRRs.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> 
> If your device is doing DMA to host memory autonomously, you may still
> have problems with this patch -- you take it out of the si_domain and
> then there's a period of time before you reapply the RMRRs, during which
> its DMA may be prevented.
> 
> You want to set up the new domain first, then switch the device over to
> it atomically.

Yes, good point.  I'm not seeing any convenient ways to setup a new
domain for a device while it's still a member of the si_domain.  It
looks like I'd need to extract parts of the get_valid_domain_for_dev()
path and ignore any bits about using the already existing domain.  Is
there an easier way?  Thanks,

Alex


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Woodhouse - 2009-10-28 14:36:59
On Tue, 2009-10-27 at 09:50 -0600, Alex Williamson wrote:
> On Tue, 2009-10-27 at 08:15 +0000, David Woodhouse wrote:
> > On Mon, 2009-10-26 at 17:25 -0600, Alex Williamson wrote:
> > > When a device is setup for passthrough it has full access to memory
> > > so processing the RMRRs is unnecessary.  However, if we remove the device
> > > from the si_domain, we need to reinstate the associated RMRRs.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> > 
> > If your device is doing DMA to host memory autonomously, you may still
> > have problems with this patch -- you take it out of the si_domain and
> > then there's a period of time before you reapply the RMRRs, during which
> > its DMA may be prevented.
> > 
> > You want to set up the new domain first, then switch the device over to
> > it atomically.
> 
> Yes, good point.  I'm not seeing any convenient ways to setup a new
> domain for a device while it's still a member of the si_domain.  It
> looks like I'd need to extract parts of the get_valid_domain_for_dev()
> path and ignore any bits about using the already existing domain.

That seems like a sane plan. That code wants cleaning up anyway, and
factoring out the 'make new domain' part of get_valid_domain_for_dev()
should be simple enough.
Alex Williamson - 2009-10-28 16:06:55
On Wed, 2009-10-28 at 14:36 +0000, David Woodhouse wrote:
> On Tue, 2009-10-27 at 09:50 -0600, Alex Williamson wrote:
> > 
> > Yes, good point.  I'm not seeing any convenient ways to setup a new
> > domain for a device while it's still a member of the si_domain.  It
> > looks like I'd need to extract parts of the get_valid_domain_for_dev()
> > path and ignore any bits about using the already existing domain.
> 
> That seems like a sane plan. That code wants cleaning up anyway, and
> factoring out the 'make new domain' part of get_valid_domain_for_dev()
> should be simple enough.

Ok, I'll work on something along those lines.  FWIW, even though I sent
these as a series, each one stands on it's own.  While I think
reinstating RMRRs is the right thing to do, I don't know of any devices
that break with the current model (the device I know of with the
interesting RMRR usage shouldn't be demoted from the si_domain).  So
please consider pushing patches 2, 3 & 5 upstream (particularly 2).
Thanks,

Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 19f10ae..a7f4476 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -2538,6 +2538,10 @@  static int iommu_no_mapping(struct device *dev)
 		if (iommu_should_identity_map(pdev, 0))
 			return 1;
 		else {
+			struct dmar_rmrr_unit *rmrr;
+			struct dmar_domain *domain;
+			int i, ret;
+
 			/*
 			 * Devices that cannot support identity mapping
 			 * are removed from si_domain and fall back to
@@ -2546,6 +2550,35 @@  static int iommu_no_mapping(struct device *dev)
 			domain_remove_one_dev_info(si_domain, pdev);
 			printk(KERN_INFO "%s uses non-identity mapping\n",
 			       pci_name(pdev));
+
+			domain = get_valid_domain_for_dev(pdev);
+			if (!domain) {
+				printk(KERN_ERR
+				       "Allocating domain for %s failed",
+				       pci_name(pdev));
+				return 0;
+			}
+
+			ret = domain_add_dev_info(domain, pdev,
+						  CONTEXT_TT_MULTI_LEVEL);
+			if (ret) {
+				printk(KERN_ERR
+				       "Attaching %s to domain failed",
+				       pci_name(pdev));
+				return 0;
+			}
+
+			for_each_rmrr_units(rmrr) {
+				for (i = 0; i < rmrr->devices_cnt; i++) {
+					if (pdev != rmrr->devices[i])
+						continue;
+					ret = iommu_prepare_rmrr_dev(rmrr,
+								     pdev);
+					if (ret)
+						printk(KERN_ERR
+						       "IOMMU: mapping reserved region failed\n");
+				}
+			}
 			return 0;
 		}
 	} else {