[PATCH 0/6] pci/dmar: small cleanup

[PATCH 0/6] pci/dmar: small cleanup

[Yinghai Lu]

About print out and some corner case with SRIOV

Thanks

Yinghai

[PATCH 1/6] pci/dmar: Don't complain that IOPAIC is not supported

[Yinghai Lu]

INTR_REMAP handle that type, so if that is used, we should not complain that

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/dmar.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
index 33ead97..3bd013f 100644
--- a/drivers/pci/dmar.c
+++ b/drivers/pci/dmar.c
@@ -131,9 +131,13 @@ static int __init dmar_parse_dev_scope(void *start, void *end, int *cnt,
 		if (scope->entry_type == ACPI_DMAR_SCOPE_TYPE_ENDPOINT ||
 		    scope->entry_type == ACPI_DMAR_SCOPE_TYPE_BRIDGE)
 			(*cnt)++;
-		else
-			printk(KERN_WARNING PREFIX
-				"Unsupported device scope\n");
+		else  {
+#ifdef CONFIG_INTR_REMAP
+			if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_IOAPIC)
+#endif
+				printk(KERN_WARNING PREFIX
+					"Unsupported device scope\n");
+		}
 		start += scope->length;
 	}
 	if (*cnt == 0)
-- 
1.6.4.2

[PATCH 2/6] pci/dmar: Print out iommu seq_id

[Yinghai Lu]

more info on system with more than one IOMMU

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/dmar.c           |    3 ++-
 drivers/pci/intel-iommu.c    |    9 ++++++---
 drivers/pci/intr_remapping.c |    6 +++---
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
index 3bd013f..d2f66a6 100644
--- a/drivers/pci/dmar.c
+++ b/drivers/pci/dmar.c
@@ -810,7 +810,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 	}
 
 	ver = readl(iommu->reg + DMAR_VER_REG);
-	pr_info("IOMMU %llx: ver %d:%d cap %llx ecap %llx\n",
+	pr_info("IOMMU %d: reg_base_addr %llx ver %d:%d cap %llx ecap %llx\n",
+		iommu->seq_id,
 		(unsigned long long)drhd->reg_base_addr,
 		DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
 		(unsigned long long)iommu->cap,
diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 4173125..83ac214 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -1154,7 +1154,8 @@ static int iommu_init_domains(struct intel_iommu *iommu)
 	unsigned long nlongs;
 
 	ndomains = cap_ndoms(iommu->cap);
-	pr_debug("Number of Domains supportd <%ld>\n", ndomains);
+	pr_debug("IOMMU %d: Number of Domains supportd <%ld>\n", iommu->seq_id,
+			ndomains);
 	nlongs = BITS_TO_LONGS(ndomains);
 
 	spin_lock_init(&iommu->lock);
@@ -2333,14 +2334,16 @@ int __init init_dmars(void)
 			 */
 			iommu->flush.flush_context = __iommu_flush_context;
 			iommu->flush.flush_iotlb = __iommu_flush_iotlb;
-			printk(KERN_INFO "IOMMU 0x%Lx: using Register based "
+			printk(KERN_INFO "IOMMU %d 0x%Lx: using Register based "
 			       "invalidation\n",
+				iommu->seq_id,
 			       (unsigned long long)drhd->reg_base_addr);
 		} else {
 			iommu->flush.flush_context = qi_flush_context;
 			iommu->flush.flush_iotlb = qi_flush_iotlb;
-			printk(KERN_INFO "IOMMU 0x%Lx: using Queued "
+			printk(KERN_INFO "IOMMU %d 0x%Lx: using Queued "
 			       "invalidation\n",
+				iommu->seq_id,
 			       (unsigned long long)drhd->reg_base_addr);
 		}
 	}
diff --git a/drivers/pci/intr_remapping.c b/drivers/pci/intr_remapping.c
index 6ee98a5..1315ac6 100644
--- a/drivers/pci/intr_remapping.c
+++ b/drivers/pci/intr_remapping.c
@@ -832,9 +832,9 @@ static int ir_parse_ioapic_hpet_scope(struct acpi_dmar_header *header,
 				return -1;
 			}
 
-			printk(KERN_INFO "IOAPIC id %d under DRHD base"
-			       " 0x%Lx\n", scope->enumeration_id,
-			       drhd->address);
+			printk(KERN_INFO "IOAPIC id %d under DRHD base "
+			       " 0x%Lx IOMMU %d\n", scope->enumeration_id,
+			       drhd->address, iommu->seq_id);
 
 			ir_parse_one_ioapic_scope(scope, iommu);
 		} else if (scope->entry_type == ACPI_DMAR_SCOPE_TYPE_HPET) {
-- 
1.6.4.2

[PATCH 3/6] pci/dmar: remove the noisy print out about unmapping

[Yinghai Lu]

it seems this one should be removed.
because map page does not have this annonyed print out.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/intel-iommu.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 83ac214..7dddc7c 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -2742,9 +2742,6 @@ static void intel_unmap_page(struct device *dev, dma_addr_t dev_addr,
 	start_pfn = mm_to_dma_pfn(iova->pfn_lo);
 	last_pfn = mm_to_dma_pfn(iova->pfn_hi + 1) - 1;
 
-	pr_debug("Device %s unmapping: pfn %lx-%lx\n",
-		 pci_name(pdev), start_pfn, last_pfn);
-
 	/*  clear the whole page */
 	dma_pte_clear_range(domain, start_pfn, last_pfn);
 
-- 
1.6.4.2

[PATCH 4/6] pci/dmar/sriov: use physfn to search drhd for VF

[Yinghai Lu]

When virtfn is used, we should use physfn to find correct drhd

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/dmar.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
index d2f66a6..550f0a9 100644
--- a/drivers/pci/dmar.c
+++ b/drivers/pci/dmar.c
@@ -534,6 +534,11 @@ dmar_find_matched_drhd_unit(struct pci_dev *dev)
 	struct dmar_drhd_unit *dmaru = NULL;
 	struct acpi_dmar_hardware_unit *drhd;
 
+#ifdef CONFIG_PCI_IOV
+	if (dev->is_virtfn)
+		dev = dev->physfn;
+#endif
+
 	list_for_each_entry(dmaru, &dmar_drhd_units, list) {
 		drhd = container_of(dmaru->hdr,
 				    struct acpi_dmar_hardware_unit,
-- 
1.6.4.2

[PATCH 5/6] pci/dmar: Fix link warning

[Yinghai Lu]

could make it to be __init to remove the linking section mismatch warning.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/dmar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
index 550f0a9..a9310ed 100644
--- a/drivers/pci/dmar.c
+++ b/drivers/pci/dmar.c
@@ -1467,7 +1467,7 @@ int dmar_reenable_qi(struct intel_iommu *iommu)
 /*
  * Check interrupt remapping support in DMAR table description.
  */
-int dmar_ir_support(void)
+int __init dmar_ir_support(void)
 {
 	struct acpi_table_dmar *dmar;
 	dmar = (struct acpi_table_dmar *)dmar_tbl;
-- 
1.6.4.2

[PATCH 6/6] intel-iommu: Don't call domain_exit if can not attach with iommu

[Yinghai Lu]

found when a lot of ixgbevf are used with intel iommu, will get crash

> >
> > eth304 ip=192.171.178.102 mac=56:95:16:88:4C:C1 pci=0000:0e:18.3 drv=ixgbevf
> > eth305 ip=192.171.179.102 mac=D6:41:8C:4A:87:B3 pci=0000:0e:18.5 drv=ixgbevf
> > [ 9534.886519] BUG: unable to handle kernel NULL pointer dereference at
> > 0000000000000008
> > [ 9534.889775] Call Trace:
> > [ 9534.889775]  [<ffffffff81406a3d>] domain_remove_dev_info+0x34/0xcf
> > [ 9534.889775]  [<ffffffff81408a1d>] domain_exit+0x23/0xc4
> > [ 9534.889775]  [<ffffffff81409c1c>] T.953+0x173/0x342
> > [ 9534.889775]  [<ffffffff81409fd0>] __intel_map_single+0x63/0x1b3
> > [ 9534.889775]  [<ffffffff8140a22a>] intel_alloc_coherent+0xc7/0xee
> > [ 9534.889775]  [<ffffffff816c3e7d>] ? ixgbevf_setup_tx_resources+0x2f/0x12c
> > [ 9534.889775]  [<ffffffff816c3f36>] ixgbevf_setup_tx_resources+0xe8/0x12c
> > [ 9534.889775]  [<ffffffff816c42d2>] ixgbevf_open+0x7a/0x160
> > [ 9534.889775]  [<ffffffff81adf21e>] __dev_open+0x8e/0xbc
> > [ 9534.889775]  [<ffffffff81adb716>] __dev_change_flags+0xad/0x130
> > [ 9534.889775]  [<ffffffff81adf15a>] dev_change_flags+0x21/0x57
> > [ 9534.889775]  [<ffffffff81b211e5>] devinet_ioctl+0x29d/0x541
> > [ 9534.889775]  [<ffffffff810a4b92>] ? trace_hardirqs_off_caller+0x1f/0xa9
> > [ 9534.889775]  [<ffffffff81b22801>] inet_ioctl+0x8f/0xa7
> > [ 9534.889775]  [<ffffffff81acc258>] sock_do_ioctl+0x29/0x48
> > [ 9534.889775]  [<ffffffff81acca64>] sock_ioctl+0x1fe/0x20d
> > [ 9534.889775]  [<ffffffff8113b86a>] vfs_ioctl+0x32/0xa6
> > [ 9534.889775]  [<ffffffff8113bd04>] do_vfs_ioctl+0x2b0/0x2cb
> > [ 9534.889775]  [<ffffffff81033c4c>] ? sysret_check+0x27/0x62
> > [ 9534.889775]  [<ffffffff8113bd66>] sys_ioctl+0x47/0x6a
> > [ 9534.889775]  [<ffffffff81033c1b>] system_call_fastpath+0x16/0x1b
> > [ 9534.889775] Code: cd 7f cc ff 41 54 9d 48 83 c4 20 5b 41 5c 41 5d 41
> > 5e c9 c3 55 48 89 e5 e8 11 ff ff ff c9 c3 55 48 89 e5 53 48 89 fb 48 83
> > ec 08 <48> 8b 47 08 4c 8b 00 49 39 f8 74 1d 48 89 f9 48 c7 c2 7f e9 18
> > [ 9534.889775] RIP  [<ffffffff813ddf94>] list_del+0xc/0x8b
> > [ 9534.889775]  RSP <ffff88c070373af8>
> > [ 9534.889775] CR2: 0000000000000008
> > [ 9534.889775] ---[ end trace 072bd8cdb08a760c ]---
> > xifconfig8_2x_vf.sh: line 10: 28555 Killed                  ifconfig
> > $DEV $IP
> >
when intel_iommu=off or iommu=pt is used, will work well.

root cause:
domain is initialized after trying attached it,

if it can not be attached, don't call domain_exit yet.
it will cause kernel crash in domain_exit()

after patch will will get error instead of crash.

[ 1781.910241] IOMMU: no free domain ids
[ 1781.910244] Allocating domain for 0000:d0:1f.5 failed
SIOCSIFFLAGS: Cannot allocate memory


Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/intel-iommu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 7dddc7c..c112e5d 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -1853,7 +1853,7 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 
 	ret = iommu_attach_domain(domain, iommu);
 	if (ret) {
-		domain_exit(domain);
+		free_domain_mem(domain);
 		goto error;
 	}
 
-- 
1.6.4.2

Re: [PATCH 4/6] pci/dmar/sriov: use physfn to search drhd for VF

[Roland Dreier]

 > +#ifdef CONFIG_PCI_IOV
 > +	if (dev->is_virtfn)
 > +		dev = dev->physfn;
 > +#endif

Seems we would prefer to avoid this ifdef... if we had a function like

	struct pci_dev *pci_physfn(struct pci_dev *virtfn)

as an inline in <linux/pci.h> that just returns NULL if PCI_IOV is not
enabled, then we could write this as

	if (dev->is_virtfn)
		dev = pci_physfn(dev)

without any #ifdef.
-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Re: [PATCH 5/6] pci/dmar: Fix link warning

[Chris Wright]

* Yinghai Lu (yinghai@kernel.org) wrote:
> could make it to be __init to remove the linking section mismatch warning.

Hey David, I just noticed this one is already in your tree, looks like
the only commit in there that didn't get pushed, accidentally stuck
perhaps?

thanks,
-chris

Re: [PATCH 4/6] pci/dmar/sriov: use physfn to search drhd for VF

[Yinghai]

please check

Subject: [PATCH] pci/dmar/sriov: use physfn to search drhd for VF

When virtfn is used, we should use physfn to find correct drhd

-v2: add pci_physfn() Suggested by Roland Dreier <rdreier@cisco.com>
     do can remove ifdef in dmar.c

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/dmar.c  |    2 ++
 include/linux/pci.h |   10 ++++++++++
 2 files changed, 12 insertions(+)

Index: linux-2.6/drivers/pci/dmar.c
===================================================================
--- linux-2.6.orig/drivers/pci/dmar.c
+++ linux-2.6/drivers/pci/dmar.c
@@ -534,6 +534,8 @@ dmar_find_matched_drhd_unit(struct pci_d
 	struct dmar_drhd_unit *dmaru = NULL;
 	struct acpi_dmar_hardware_unit *drhd;
 
+	dev = pci_physfn(dev);
+
 	list_for_each_entry(dmaru, &dmar_drhd_units, list) {
 		drhd = container_of(dmaru->hdr,
 				    struct acpi_dmar_hardware_unit,
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -334,6 +334,16 @@ struct pci_dev {
 #endif
 };
 
+static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCI_IOV
+	if (dev->is_virtfn)
+		dev = dev->physfn;
+#endif
+
+	return dev;
+}
+
 extern struct pci_dev *alloc_pci_dev(void);
 
 #define pci_dev_b(n) list_entry(n, struct pci_dev, bus_list)

Re: [PATCH 4/6] pci/dmar/sriov: use physfn to search drhd for VF

[Roland Dreier]

Looks good to me.
-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Re: [PATCH 4/6] pci/dmar/sriov: use physfn to search drhd for VF

[Chris Wright]

* Yinghai (yinghai.lu@oracle.com) wrote:
> --- linux-2.6.orig/drivers/pci/dmar.c
> +++ linux-2.6/drivers/pci/dmar.c
> @@ -534,6 +534,8 @@ dmar_find_matched_drhd_unit(struct pci_d
>  	struct dmar_drhd_unit *dmaru = NULL;
>  	struct acpi_dmar_hardware_unit *drhd;
>  
> +	dev = pci_physfn(dev);
> +

Yeah, we typically don't have enough VF's to wrap bus numbers, or we're
under a catchall IOMMU.  In the catchall case both vf->bus and vf->pf->bus
will have the same domain (segment) regardless of whether we have large
VF count or big offset/stride.  So, I suppose this could be done inside
of dmar_pci_device_match().

Otherwise, I think you'd want to add the same thing to
dmar_find_matched_atsr_unit() since it's the same device scopes there.

thanks,
-chris

Re: [PATCH 4/6] pci/dmar/sriov: use physfn to search drhd for VF

[Yinghai]

On 04/08/2010 04:24 PM, Chris Wright wrote:
> * Yinghai (yinghai.lu@oracle.com) wrote:
>> --- linux-2.6.orig/drivers/pci/dmar.c
>> +++ linux-2.6/drivers/pci/dmar.c
>> @@ -534,6 +534,8 @@ dmar_find_matched_drhd_unit(struct pci_d
>>  	struct dmar_drhd_unit *dmaru = NULL;
>>  	struct acpi_dmar_hardware_unit *drhd;
>>  
>> +	dev = pci_physfn(dev);
>> +
> 
> Yeah, we typically don't have enough VF's to wrap bus numbers, or we're
> under a catchall IOMMU.  In the catchall case both vf->bus and vf->pf->bus
> will have the same domain (segment) regardless of whether we have large
> VF count or big offset/stride.  So, I suppose this could be done inside
> of dmar_pci_device_match().
> 
> Otherwise, I think you'd want to add the same thing to
> dmar_find_matched_atsr_unit() since it's the same device scopes there.

please check

Subject: [PATCH] pci/dmar/sriov: use physfn to search drhd for VF

When virtfn is used, we should use physfn to find correct drhd

-v2: add pci_physfn() Suggested by Roland Dreier <rdreier@cisco.com>
     do can remove ifdef in dmar.c
-v3: Chris pointed out we need that for dma_find_matched_atsr_unit too
     also change dmar_pci_device_match() static

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Roland Dreier <rdreier@cisco.com>

---
 drivers/pci/dmar.c  |    6 +++++-
 include/linux/pci.h |   10 ++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/dmar.c
===================================================================
--- linux-2.6.orig/drivers/pci/dmar.c
+++ linux-2.6/drivers/pci/dmar.c
@@ -313,6 +313,8 @@ int dmar_find_matched_atsr_unit(struct p
 	struct acpi_dmar_atsr *atsr;
 	struct dmar_atsr_unit *atsru;
 
+	dev = pci_physfn(dev);
+
 	list_for_each_entry(atsru, &dmar_atsr_units, list) {
 		atsr = container_of(atsru->hdr, struct acpi_dmar_atsr, header);
 		if (atsr->segment == pci_domain_nr(dev->bus))
@@ -511,7 +513,7 @@ parse_dmar_table(void)
 	return ret;
 }
 
-int dmar_pci_device_match(struct pci_dev *devices[], int cnt,
+static int dmar_pci_device_match(struct pci_dev *devices[], int cnt,
 			  struct pci_dev *dev)
 {
 	int index;
@@ -534,6 +536,8 @@ dmar_find_matched_drhd_unit(struct pci_d
 	struct dmar_drhd_unit *dmaru = NULL;
 	struct acpi_dmar_hardware_unit *drhd;
 
+	dev = pci_physfn(dev);
+
 	list_for_each_entry(dmaru, &dmar_drhd_units, list) {
 		drhd = container_of(dmaru->hdr,
 				    struct acpi_dmar_hardware_unit,
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -334,6 +334,16 @@ struct pci_dev {
 #endif
 };
 
+static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCI_IOV
+	if (dev->is_virtfn)
+		dev = dev->physfn;
+#endif
+
+	return dev;
+}
+
 extern struct pci_dev *alloc_pci_dev(void);
 
 #define pci_dev_b(n) list_entry(n, struct pci_dev, bus_list)

Re: [PATCH 4/6] pci/dmar/sriov: use physfn to search drhd for VF

[Chris Wright]

* Yinghai (yinghai.lu@oracle.com) wrote:
> Subject: [PATCH] pci/dmar/sriov: use physfn to search drhd for VF
> 
> When virtfn is used, we should use physfn to find correct drhd
> 
> -v2: add pci_physfn() Suggested by Roland Dreier <rdreier@cisco.com>
>      do can remove ifdef in dmar.c
> -v3: Chris pointed out we need that for dma_find_matched_atsr_unit too
>      also change dmar_pci_device_match() static
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Acked-by: Roland Dreier <rdreier@cisco.com>

Looks good

Acked-by: Chris Wright <chrisw@sous-sol.org>

Re: [PATCH 1/6] pci/dmar: Don't complain that IOPAIC is not supported

[David Woodhouse]

On Thu, 2010-04-08 at 19:58 +0100, Yinghai Lu wrote:
> INTR_REMAP handle that type, so if that is used, we should not complain that
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/dmar.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
> index 33ead97..3bd013f 100644
> --- a/drivers/pci/dmar.c
> +++ b/drivers/pci/dmar.c
> @@ -131,9 +131,13 @@ static int __init dmar_parse_dev_scope(void *start, void *end, int *cnt,
>  		if (scope->entry_type == ACPI_DMAR_SCOPE_TYPE_ENDPOINT ||
>  		    scope->entry_type == ACPI_DMAR_SCOPE_TYPE_BRIDGE)
>  			(*cnt)++;
> -		else
> -			printk(KERN_WARNING PREFIX
> -				"Unsupported device scope\n");
> +		else  {
> +#ifdef CONFIG_INTR_REMAP
> +			if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_IOAPIC)
> +#endif
> +				printk(KERN_WARNING PREFIX
> +					"Unsupported device scope\n");
> +		}
>  		start += scope->length;
>  	}
>  	if (*cnt == 0)

Applied, without the ifdef and slightly cleaned up.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH 3/6] pci/dmar: remove the noisy print out about unmapping

[David Woodhouse]

On Thu, 2010-04-08 at 19:58 +0100, Yinghai Lu wrote:
> it seems this one should be removed.
> because map page does not have this annonyed print out.

It's debug output... I'd be more inclined to add it in map_page() than
remove it from here.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH 6/6] intel-iommu: Don't call domain_exit if can not attach with iommu

[David Woodhouse]

On Thu, 2010-04-08 at 19:58 +0100, Yinghai Lu wrote:
> 
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -1853,7 +1853,7 @@ static struct dmar_domain
> *get_domain_for_dev(struct pci_dev *pdev, int gaw)
>  
>         ret = iommu_attach_domain(domain, iommu);
>         if (ret) {
> -               domain_exit(domain);
> +               free_domain_mem(domain);
>                 goto error;
>         }
>   

Um, shouldn't the next error have the same change? If domain_init()
fails before it gets round to initialising the domain->devices list, the
same oops in domain_exit() will occur, won't it? It's because
domain->devices.{next,prev} are both NULL?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH 4/6] pci/dmar/sriov: use physfn to search drhd for VF

[Jesse Barnes]

On Thu, 08 Apr 2010 17:07:55 -0700
Yinghai <yinghai.lu@oracle.com> wrote:

> On 04/08/2010 04:24 PM, Chris Wright wrote:
> > * Yinghai (yinghai.lu@oracle.com) wrote:
> >> --- linux-2.6.orig/drivers/pci/dmar.c
> >> +++ linux-2.6/drivers/pci/dmar.c
> >> @@ -534,6 +534,8 @@ dmar_find_matched_drhd_unit(struct pci_d
> >>  	struct dmar_drhd_unit *dmaru = NULL;
> >>  	struct acpi_dmar_hardware_unit *drhd;
> >>  
> >> +	dev = pci_physfn(dev);
> >> +
> > 
> > Yeah, we typically don't have enough VF's to wrap bus numbers, or we're
> > under a catchall IOMMU.  In the catchall case both vf->bus and vf->pf->bus
> > will have the same domain (segment) regardless of whether we have large
> > VF count or big offset/stride.  So, I suppose this could be done inside
> > of dmar_pci_device_match().
> > 
> > Otherwise, I think you'd want to add the same thing to
> > dmar_find_matched_atsr_unit() since it's the same device scopes there.
> 
> please check
> 
> Subject: [PATCH] pci/dmar/sriov: use physfn to search drhd for VF
> 
> When virtfn is used, we should use physfn to find correct drhd
> 
> -v2: add pci_physfn() Suggested by Roland Dreier <rdreier@cisco.com>
>      do can remove ifdef in dmar.c
> -v3: Chris pointed out we need that for dma_find_matched_atsr_unit too
>      also change dmar_pci_device_match() static
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Acked-by: Roland Dreier <rdreier@cisco.com>

This one looks good:

Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 6/6] intel-iommu: Don't call domain_exit if can not attach with iommu

[Yinghai]

On 04/09/2010 08:43 AM, David Woodhouse wrote:
> On Thu, 2010-04-08 at 19:58 +0100, Yinghai Lu wrote:
>>
>> --- a/drivers/pci/intel-iommu.c
>> +++ b/drivers/pci/intel-iommu.c
>> @@ -1853,7 +1853,7 @@ static struct dmar_domain
>> *get_domain_for_dev(struct pci_dev *pdev, int gaw)
>>  
>>         ret = iommu_attach_domain(domain, iommu);
>>         if (ret) {
>> -               domain_exit(domain);
>> +               free_domain_mem(domain);
>>                 goto error;
>>         }
>>   
> 
> Um, shouldn't the next error have the same change? If domain_init()
> fails before it gets round to initialising the domain->devices list, the
> same oops in domain_exit() will occur, won't it? It's because
> domain->devices.{next,prev} are both NULL?
> 

thanks. please check

Subject: [PATCH] intel-iommu: Don't call domain_exit if can not attach with iommu

found when a lot of ixgbevf are used with intel iommu, will get crash

> > 
> > eth304 ip=192.171.178.102 mac=56:95:16:88:4C:C1 pci=0000:0e:18.3 drv=ixgbevf
> > eth305 ip=192.171.179.102 mac=D6:41:8C:4A:87:B3 pci=0000:0e:18.5 drv=ixgbevf
> > [ 9534.886519] BUG: unable to handle kernel NULL pointer dereference at
> > 0000000000000008
> > [ 9534.889775] Call Trace:
> > [ 9534.889775]  [<ffffffff81406a3d>] domain_remove_dev_info+0x34/0xcf
> > [ 9534.889775]  [<ffffffff81408a1d>] domain_exit+0x23/0xc4
> > [ 9534.889775]  [<ffffffff81409c1c>] T.953+0x173/0x342
> > [ 9534.889775]  [<ffffffff81409fd0>] __intel_map_single+0x63/0x1b3
> > [ 9534.889775]  [<ffffffff8140a22a>] intel_alloc_coherent+0xc7/0xee
> > [ 9534.889775]  [<ffffffff816c3e7d>] ? ixgbevf_setup_tx_resources+0x2f/0x12c
> > [ 9534.889775]  [<ffffffff816c3f36>] ixgbevf_setup_tx_resources+0xe8/0x12c
> > [ 9534.889775]  [<ffffffff816c42d2>] ixgbevf_open+0x7a/0x160
> > [ 9534.889775]  [<ffffffff81adf21e>] __dev_open+0x8e/0xbc
> > [ 9534.889775]  [<ffffffff81adb716>] __dev_change_flags+0xad/0x130
> > [ 9534.889775]  [<ffffffff81adf15a>] dev_change_flags+0x21/0x57
> > [ 9534.889775]  [<ffffffff81b211e5>] devinet_ioctl+0x29d/0x541
> > [ 9534.889775]  [<ffffffff810a4b92>] ? trace_hardirqs_off_caller+0x1f/0xa9
> > [ 9534.889775]  [<ffffffff81b22801>] inet_ioctl+0x8f/0xa7
> > [ 9534.889775]  [<ffffffff81acc258>] sock_do_ioctl+0x29/0x48
> > [ 9534.889775]  [<ffffffff81acca64>] sock_ioctl+0x1fe/0x20d
> > [ 9534.889775]  [<ffffffff8113b86a>] vfs_ioctl+0x32/0xa6
> > [ 9534.889775]  [<ffffffff8113bd04>] do_vfs_ioctl+0x2b0/0x2cb
> > [ 9534.889775]  [<ffffffff81033c4c>] ? sysret_check+0x27/0x62
> > [ 9534.889775]  [<ffffffff8113bd66>] sys_ioctl+0x47/0x6a
> > [ 9534.889775]  [<ffffffff81033c1b>] system_call_fastpath+0x16/0x1b
> > [ 9534.889775] Code: cd 7f cc ff 41 54 9d 48 83 c4 20 5b 41 5c 41 5d 41
> > 5e c9 c3 55 48 89 e5 e8 11 ff ff ff c9 c3 55 48 89 e5 53 48 89 fb 48 83
> > ec 08 <48> 8b 47 08 4c 8b 00 49 39 f8 74 1d 48 89 f9 48 c7 c2 7f e9 18
> > [ 9534.889775] RIP  [<ffffffff813ddf94>] list_del+0xc/0x8b
> > [ 9534.889775]  RSP <ffff88c070373af8>
> > [ 9534.889775] CR2: 0000000000000008
> > [ 9534.889775] ---[ end trace 072bd8cdb08a760c ]---
> > xifconfig8_2x_vf.sh: line 10: 28555 Killed                  ifconfig
> > $DEV $IP
> > 
when intel_iommu=off or iommu=pt is used, will work well.

root cause:
domain is initialized after trying attached it,

if it can not be attached, don't call domain_exit yet.
it will cause kernel crash in domain_exit()

after patch will will get error instead of crash.

[ 1781.910241] IOMMU: no free domain ids
[ 1781.910244] Allocating domain for 0000:d0:1f.5 failed
SIOCSIFFLAGS: Cannot allocate memory

-v2: DavidW pointed out next error with domain_init could have the same problem.
     So try to adjust sequence in domain_init() to make sure domain->devices get
     initialized properly

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/intel-iommu.c |   37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

Index: linux-2.6/drivers/pci/intel-iommu.c
===================================================================
--- linux-2.6.orig/drivers/pci/intel-iommu.c
+++ linux-2.6/drivers/pci/intel-iommu.c
@@ -1379,24 +1379,10 @@ static int domain_init(struct dmar_domai
 
 	domain_reserve_special_ranges(domain);
 
-	/* calculate AGAW */
-	iommu = domain_get_iommu(domain);
-	if (guest_width > cap_mgaw(iommu->cap))
-		guest_width = cap_mgaw(iommu->cap);
-	domain->gaw = guest_width;
-	adjust_width = guestwidth_to_adjustwidth(guest_width);
-	agaw = width_to_agaw(adjust_width);
-	sagaw = cap_sagaw(iommu->cap);
-	if (!test_bit(agaw, &sagaw)) {
-		/* hardware doesn't support it, choose a bigger one */
-		pr_debug("IOMMU: hardware doesn't support agaw %d\n", agaw);
-		agaw = find_next_bit(&sagaw, 5, agaw);
-		if (agaw >= 5)
-			return -ENODEV;
-	}
-	domain->agaw = agaw;
 	INIT_LIST_HEAD(&domain->devices);
 
+	iommu = domain_get_iommu(domain);
+
 	if (ecap_coherent(iommu->ecap))
 		domain->iommu_coherency = 1;
 	else
@@ -1415,6 +1401,23 @@ static int domain_init(struct dmar_domai
 	if (!domain->pgd)
 		return -ENOMEM;
 	__iommu_flush_cache(iommu, domain->pgd, PAGE_SIZE);
+
+	/* calculate AGAW */
+	if (guest_width > cap_mgaw(iommu->cap))
+		guest_width = cap_mgaw(iommu->cap);
+	domain->gaw = guest_width;
+	adjust_width = guestwidth_to_adjustwidth(guest_width);
+	agaw = width_to_agaw(adjust_width);
+	sagaw = cap_sagaw(iommu->cap);
+	if (!test_bit(agaw, &sagaw)) {
+		/* hardware doesn't support it, choose a bigger one */
+		pr_debug("IOMMU: hardware doesn't support agaw %d\n", agaw);
+		agaw = find_next_bit(&sagaw, 5, agaw);
+		if (agaw >= 5)
+			return -ENODEV;
+	}
+	domain->agaw = agaw;
+
 	return 0;
 }
 
@@ -1853,7 +1856,7 @@ static struct dmar_domain *get_domain_fo
 
 	ret = iommu_attach_domain(domain, iommu);
 	if (ret) {
-		domain_exit(domain);
+		free_domain_mem(domain);
 		goto error;
 	}