iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] RMRR related fixes
@ 2019-05-13  7:12 Eric Auger
  2019-05-13  7:12 ` [PATCH 1/4] iommu: Pass a GFP flag parameter to iommu_alloc_resv_region() Eric Auger
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Eric Auger @ 2019-05-13  7:12 UTC (permalink / raw)
  To: eric.auger.pro, eric.auger, joro, iommu, linux-kernel, dwmw2,
	lorenzo.pieralisi, robin.murphy, will.deacon, hanjun.guo,
	sudeep.holla
  Cc: alex.williamson

Currently the Intel reserved region is attached to the
RMRR unit and when building the list of RMRR seen by a device
we link this unique reserved region without taking care of
potential multiple usage of this reserved region by several devices.

Also while reading the vtd spec it is unclear to me whether
the RMRR device scope referenced by an RMRR ACPI struct could
be a PCI-PCI bridge, in which case I think we also need to
check the device belongs to the PCI sub-hierarchy of the device
referenced in the scope. This would be true for device_has_rmrr()
and intel_iommu_get_resv_regions().

Eric Auger (4):
  iommu: Pass a GFP flag parameter to iommu_alloc_resv_region()
  iommu/vt-d: Duplicate iommu_resv_region objects per device list
  iommu/vt-d: Handle RMRR with PCI bridge device scopes
  iommu/vt-d: Handle PCI bridge RMRR device scopes in
    intel_iommu_get_resv_regions

 drivers/acpi/arm64/iort.c   |  3 +-
 drivers/iommu/amd_iommu.c   |  7 ++--
 drivers/iommu/arm-smmu-v3.c |  2 +-
 drivers/iommu/arm-smmu.c    |  2 +-
 drivers/iommu/intel-iommu.c | 68 +++++++++++++++++++++++--------------
 drivers/iommu/iommu.c       |  7 ++--
 include/linux/iommu.h       |  2 +-
 7 files changed, 55 insertions(+), 36 deletions(-)

-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/4] iommu: Pass a GFP flag parameter to iommu_alloc_resv_region()
  2019-05-13  7:12 [PATCH 0/4] RMRR related fixes Eric Auger
@ 2019-05-13  7:12 ` Eric Auger
  2019-05-13  7:13 ` [PATCH 2/4] iommu/vt-d: Duplicate iommu_resv_region objects per device list Eric Auger
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Eric Auger @ 2019-05-13  7:12 UTC (permalink / raw)
  To: eric.auger.pro, eric.auger, joro, iommu, linux-kernel, dwmw2,
	lorenzo.pieralisi, robin.murphy, will.deacon, hanjun.guo,
	sudeep.holla
  Cc: alex.williamson

We plan to call iommu_alloc_resv_region in a non preemptible section.
Pass a GFP flag to the function and update all the call sites.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/acpi/arm64/iort.c   | 3 ++-
 drivers/iommu/amd_iommu.c   | 7 ++++---
 drivers/iommu/arm-smmu-v3.c | 2 +-
 drivers/iommu/arm-smmu.c    | 2 +-
 drivers/iommu/intel-iommu.c | 4 ++--
 drivers/iommu/iommu.c       | 7 ++++---
 include/linux/iommu.h       | 2 +-
 7 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index adbf7cbedf80..20b56ae91513 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -868,7 +868,8 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
 			struct iommu_resv_region *region;
 
 			region = iommu_alloc_resv_region(base + SZ_64K, SZ_64K,
-							 prot, IOMMU_RESV_MSI);
+							 prot, IOMMU_RESV_MSI,
+							 GFP_KERNEL);
 			if (region) {
 				list_add_tail(&region->list, head);
 				resv++;
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index f7cdd2ab7f11..a9aab13a9487 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3186,7 +3186,8 @@ static void amd_iommu_get_resv_regions(struct device *dev,
 			type = IOMMU_RESV_RESERVED;
 
 		region = iommu_alloc_resv_region(entry->address_start,
-						 length, prot, type);
+						 length, prot, type,
+						 GFP_KERNEL);
 		if (!region) {
 			dev_err(dev, "Out of memory allocating dm-regions\n");
 			return;
@@ -3196,14 +3197,14 @@ static void amd_iommu_get_resv_regions(struct device *dev,
 
 	region = iommu_alloc_resv_region(MSI_RANGE_START,
 					 MSI_RANGE_END - MSI_RANGE_START + 1,
-					 0, IOMMU_RESV_MSI);
+					 0, IOMMU_RESV_MSI, GFP_KERNEL);
 	if (!region)
 		return;
 	list_add_tail(&region->list, head);
 
 	region = iommu_alloc_resv_region(HT_RANGE_START,
 					 HT_RANGE_END - HT_RANGE_START + 1,
-					 0, IOMMU_RESV_RESERVED);
+					 0, IOMMU_RESV_RESERVED, GFP_KERNEL);
 	if (!region)
 		return;
 	list_add_tail(&region->list, head);
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index d3880010c6cf..5aae50c811b3 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2031,7 +2031,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
 
 	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-					 prot, IOMMU_RESV_SW_MSI);
+					 prot, IOMMU_RESV_SW_MSI, GFP_KERNEL);
 	if (!region)
 		return;
 
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 045d93884164..3c28bc0555d4 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1667,7 +1667,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
 
 	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-					 prot, IOMMU_RESV_SW_MSI);
+					 prot, IOMMU_RESV_SW_MSI, GFP_KERNEL);
 	if (!region)
 		return;
 
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 28cb713d728c..2075abdb174d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4221,7 +4221,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 
 	length = rmrr->end_address - rmrr->base_address + 1;
 	rmrru->resv = iommu_alloc_resv_region(rmrr->base_address, length, prot,
-					      IOMMU_RESV_DIRECT);
+					      IOMMU_RESV_DIRECT, GFP_KERNEL);
 	if (!rmrru->resv)
 		goto free_rmrru;
 
@@ -5290,7 +5290,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
 
 	reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
 				      IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
-				      0, IOMMU_RESV_MSI);
+				      0, IOMMU_RESV_MSI, GFP_KERNEL);
 	if (!reg)
 		return;
 	list_add_tail(&reg->list, head);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 109de67d5d72..b0598c202e11 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -260,7 +260,7 @@ static int iommu_insert_resv_region(struct iommu_resv_region *new,
 	}
 insert:
 	region = iommu_alloc_resv_region(new->start, new->length,
-					 new->prot, new->type);
+					 new->prot, new->type, GFP_KERNEL);
 	if (!region)
 		return -ENOMEM;
 
@@ -1898,11 +1898,12 @@ void iommu_put_resv_regions(struct device *dev, struct list_head *list)
 
 struct iommu_resv_region *iommu_alloc_resv_region(phys_addr_t start,
 						  size_t length, int prot,
-						  enum iommu_resv_type type)
+						  enum iommu_resv_type type,
+						  gfp_t flags)
 {
 	struct iommu_resv_region *region;
 
-	region = kzalloc(sizeof(*region), GFP_KERNEL);
+	region = kzalloc(sizeof(*region), flags);
 	if (!region)
 		return NULL;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ffbbc7e39cee..c9cc8be6840b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -310,7 +310,7 @@ extern void iommu_put_resv_regions(struct device *dev, struct list_head *list);
 extern int iommu_request_dm_for_dev(struct device *dev);
 extern struct iommu_resv_region *
 iommu_alloc_resv_region(phys_addr_t start, size_t length, int prot,
-			enum iommu_resv_type type);
+			enum iommu_resv_type type, gfp_t flags);
 extern int iommu_get_group_resv_regions(struct iommu_group *group,
 					struct list_head *head);
 
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/4] iommu/vt-d: Duplicate iommu_resv_region objects per device list
  2019-05-13  7:12 [PATCH 0/4] RMRR related fixes Eric Auger
  2019-05-13  7:12 ` [PATCH 1/4] iommu: Pass a GFP flag parameter to iommu_alloc_resv_region() Eric Auger
@ 2019-05-13  7:13 ` Eric Auger
  2019-05-13  7:13 ` [PATCH 3/4] iommu/vt-d: Handle RMRR with PCI bridge device scopes Eric Auger
  2019-05-13  7:13 ` [PATCH 4/4] iommu/vt-d: Handle PCI bridge RMRR device scopes in intel_iommu_get_resv_regions Eric Auger
  3 siblings, 0 replies; 7+ messages in thread
From: Eric Auger @ 2019-05-13  7:13 UTC (permalink / raw)
  To: eric.auger.pro, eric.auger, joro, iommu, linux-kernel, dwmw2,
	lorenzo.pieralisi, robin.murphy, will.deacon, hanjun.guo,
	sudeep.holla
  Cc: alex.williamson

intel_iommu_get_resv_regions() aims to return the list of
reserved regions accessible by a given @device. However several
devices can access the same reserved memory region and when
building the list it is not safe to use a single iommu_resv_region
object, whose container is the RMRR. This iommu_resv_region must
be duplicated per device reserved region list.

Let's remove the struct iommu_resv_region from the RMRR unit
and allocate the iommu_resv_region directly in
intel_iommu_get_resv_regions().

Fixes: 0659b8dc45a6 ("iommu/vt-d: Implement reserved region get/put callbacks")
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/iommu/intel-iommu.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 2075abdb174d..e2134b13c9ae 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -322,7 +322,6 @@ struct dmar_rmrr_unit {
 	u64	end_address;		/* reserved end address */
 	struct dmar_dev_scope *devices;	/* target devices */
 	int	devices_cnt;		/* target device count */
-	struct iommu_resv_region *resv; /* reserved region handle */
 };
 
 struct dmar_atsr_unit {
@@ -4206,7 +4205,6 @@ static inline void init_iommu_pm_ops(void) {}
 int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 {
 	struct acpi_dmar_reserved_memory *rmrr;
-	int prot = DMA_PTE_READ|DMA_PTE_WRITE;
 	struct dmar_rmrr_unit *rmrru;
 	size_t length;
 
@@ -4220,22 +4218,16 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 	rmrru->end_address = rmrr->end_address;
 
 	length = rmrr->end_address - rmrr->base_address + 1;
-	rmrru->resv = iommu_alloc_resv_region(rmrr->base_address, length, prot,
-					      IOMMU_RESV_DIRECT, GFP_KERNEL);
-	if (!rmrru->resv)
-		goto free_rmrru;
 
 	rmrru->devices = dmar_alloc_dev_scope((void *)(rmrr + 1),
 				((void *)rmrr) + rmrr->header.length,
 				&rmrru->devices_cnt);
 	if (rmrru->devices_cnt && rmrru->devices == NULL)
-		goto free_all;
+		goto free_rmrru;
 
 	list_add(&rmrru->list, &dmar_rmrr_units);
 
 	return 0;
-free_all:
-	kfree(rmrru->resv);
 free_rmrru:
 	kfree(rmrru);
 out:
@@ -4453,7 +4445,6 @@ static void intel_iommu_free_dmars(void)
 	list_for_each_entry_safe(rmrru, rmrr_n, &dmar_rmrr_units, list) {
 		list_del(&rmrru->list);
 		dmar_free_dev_scope(&rmrru->devices, &rmrru->devices_cnt);
-		kfree(rmrru->resv);
 		kfree(rmrru);
 	}
 
@@ -5271,6 +5262,7 @@ static void intel_iommu_remove_device(struct device *dev)
 static void intel_iommu_get_resv_regions(struct device *device,
 					 struct list_head *head)
 {
+	int prot = DMA_PTE_READ|DMA_PTE_WRITE;
 	struct iommu_resv_region *reg;
 	struct dmar_rmrr_unit *rmrr;
 	struct device *i_dev;
@@ -5280,10 +5272,21 @@ static void intel_iommu_get_resv_regions(struct device *device,
 	for_each_rmrr_units(rmrr) {
 		for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
 					  i, i_dev) {
+			struct iommu_resv_region *resv;
+			size_t length;
+
 			if (i_dev != device)
 				continue;
 
-			list_add_tail(&rmrr->resv->list, head);
+			length = rmrr->end_address - rmrr->base_address + 1;
+			resv = iommu_alloc_resv_region(rmrr->base_address,
+						       length, prot,
+						       IOMMU_RESV_DIRECT,
+						       GFP_ATOMIC);
+			if (!resv)
+				break;
+
+			list_add_tail(&resv->list, head);
 		}
 	}
 	rcu_read_unlock();
@@ -5301,10 +5304,8 @@ static void intel_iommu_put_resv_regions(struct device *dev,
 {
 	struct iommu_resv_region *entry, *next;
 
-	list_for_each_entry_safe(entry, next, head, list) {
-		if (entry->type == IOMMU_RESV_MSI)
-			kfree(entry);
-	}
+	list_for_each_entry_safe(entry, next, head, list)
+		kfree(entry);
 }
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/4] iommu/vt-d: Handle RMRR with PCI bridge device scopes
  2019-05-13  7:12 [PATCH 0/4] RMRR related fixes Eric Auger
  2019-05-13  7:12 ` [PATCH 1/4] iommu: Pass a GFP flag parameter to iommu_alloc_resv_region() Eric Auger
  2019-05-13  7:13 ` [PATCH 2/4] iommu/vt-d: Duplicate iommu_resv_region objects per device list Eric Auger
@ 2019-05-13  7:13 ` Eric Auger
  2019-05-13 16:41   ` Jacob Pan
  2019-05-13  7:13 ` [PATCH 4/4] iommu/vt-d: Handle PCI bridge RMRR device scopes in intel_iommu_get_resv_regions Eric Auger
  3 siblings, 1 reply; 7+ messages in thread
From: Eric Auger @ 2019-05-13  7:13 UTC (permalink / raw)
  To: eric.auger.pro, eric.auger, joro, iommu, linux-kernel, dwmw2,
	lorenzo.pieralisi, robin.murphy, will.deacon, hanjun.guo,
	sudeep.holla
  Cc: alex.williamson

When reading the vtd specification and especially the
Reserved Memory Region Reporting Structure chapter,
it is not obvious a device scope element cannot be a
PCI-PCI bridge, in which case all downstream ports are
likely to access the reserved memory region. Let's handle
this case in device_has_rmrr.

Fixes: ea2447f700ca ("intel-iommu: Prevent devices with RMRRs from being placed into SI Domain")

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/iommu/intel-iommu.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index e2134b13c9ae..89d82a1d50b1 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -736,12 +736,31 @@ static int iommu_dummy(struct device *dev)
 	return dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
 }
 
+static bool
+is_downstream_to_pci_bridge(struct device *deva, struct device *devb)
+{
+	struct pci_dev *pdeva, *pdevb;
+
+	if (!dev_is_pci(deva) || !dev_is_pci(devb))
+		return false;
+
+	pdeva = to_pci_dev(deva);
+	pdevb = to_pci_dev(devb);
+
+	if (pdevb->subordinate &&
+	    pdevb->subordinate->number <= pdeva->bus->number &&
+	    pdevb->subordinate->busn_res.end >= pdeva->bus->number)
+		return true;
+
+	return false;
+}
+
 static struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn)
 {
 	struct dmar_drhd_unit *drhd = NULL;
 	struct intel_iommu *iommu;
 	struct device *tmp;
-	struct pci_dev *ptmp, *pdev = NULL;
+	struct pci_dev *pdev = NULL;
 	u16 segment = 0;
 	int i;
 
@@ -787,13 +806,7 @@ static struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devf
 				goto out;
 			}
 
-			if (!pdev || !dev_is_pci(tmp))
-				continue;
-
-			ptmp = to_pci_dev(tmp);
-			if (ptmp->subordinate &&
-			    ptmp->subordinate->number <= pdev->bus->number &&
-			    ptmp->subordinate->busn_res.end >= pdev->bus->number)
+			if (is_downstream_to_pci_bridge(dev, tmp))
 				goto got_pdev;
 		}
 
@@ -2886,7 +2899,8 @@ static bool device_has_rmrr(struct device *dev)
 		 */
 		for_each_active_dev_scope(rmrr->devices,
 					  rmrr->devices_cnt, i, tmp)
-			if (tmp == dev) {
+			if (tmp == dev ||
+			    is_downstream_to_pci_bridge(dev, tmp)) {
 				rcu_read_unlock();
 				return true;
 			}
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 4/4] iommu/vt-d: Handle PCI bridge RMRR device scopes in intel_iommu_get_resv_regions
  2019-05-13  7:12 [PATCH 0/4] RMRR related fixes Eric Auger
                   ` (2 preceding siblings ...)
  2019-05-13  7:13 ` [PATCH 3/4] iommu/vt-d: Handle RMRR with PCI bridge device scopes Eric Auger
@ 2019-05-13  7:13 ` Eric Auger
  3 siblings, 0 replies; 7+ messages in thread
From: Eric Auger @ 2019-05-13  7:13 UTC (permalink / raw)
  To: eric.auger.pro, eric.auger, joro, iommu, linux-kernel, dwmw2,
	lorenzo.pieralisi, robin.murphy, will.deacon, hanjun.guo,
	sudeep.holla
  Cc: alex.williamson

In the case the RMRR device scope is a PCI-PCI bridge, let's check
the device belongs to the PCI sub-hierarchy.

Fixes: 0659b8dc45a6 ("iommu/vt-d: Implement reserved region get/put callbacks")

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/iommu/intel-iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 89d82a1d50b1..9c1a765eca8b 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5289,7 +5289,8 @@ static void intel_iommu_get_resv_regions(struct device *device,
 			struct iommu_resv_region *resv;
 			size_t length;
 
-			if (i_dev != device)
+			if (i_dev != device &&
+			    !is_downstream_to_pci_bridge(device, i_dev))
 				continue;
 
 			length = rmrr->end_address - rmrr->base_address + 1;
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/4] iommu/vt-d: Handle RMRR with PCI bridge device scopes
  2019-05-13  7:13 ` [PATCH 3/4] iommu/vt-d: Handle RMRR with PCI bridge device scopes Eric Auger
@ 2019-05-13 16:41   ` Jacob Pan
  2019-05-13 16:58     ` Auger Eric
  0 siblings, 1 reply; 7+ messages in thread
From: Jacob Pan @ 2019-05-13 16:41 UTC (permalink / raw)
  To: Eric Auger
  Cc: alex.williamson, robin.murphy, will.deacon, linux-kernel, iommu,
	sudeep.holla, dwmw2, eric.auger.pro

On Mon, 13 May 2019 09:13:01 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> When reading the vtd specification and especially the
> Reserved Memory Region Reporting Structure chapter,
> it is not obvious a device scope element cannot be a
> PCI-PCI bridge, in which case all downstream ports are
> likely to access the reserved memory region. Let's handle
> this case in device_has_rmrr.
> 
> Fixes: ea2447f700ca ("intel-iommu: Prevent devices with RMRRs from
> being placed into SI Domain")
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  drivers/iommu/intel-iommu.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index e2134b13c9ae..89d82a1d50b1 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -736,12 +736,31 @@ static int iommu_dummy(struct device *dev)
>  	return dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>  }
>  
> +static bool
> +is_downstream_to_pci_bridge(struct device *deva, struct device *devb)
> +{
> +	struct pci_dev *pdeva, *pdevb;
> +
is there a more illustrative name for these. i guess deva is is the
bridge dev?
> +	if (!dev_is_pci(deva) || !dev_is_pci(devb))
> +		return false;
> +
> +	pdeva = to_pci_dev(deva);
> +	pdevb = to_pci_dev(devb);
> +
> +	if (pdevb->subordinate &&
> +	    pdevb->subordinate->number <= pdeva->bus->number &&
> +	    pdevb->subordinate->busn_res.end >= pdeva->bus->number)
> +		return true;
> +
> +	return false;
> +}
> +
this seems to be a separate cleanup patch.
>  static struct intel_iommu *device_to_iommu(struct device *dev, u8
> *bus, u8 *devfn) {
>  	struct dmar_drhd_unit *drhd = NULL;
>  	struct intel_iommu *iommu;
>  	struct device *tmp;
> -	struct pci_dev *ptmp, *pdev = NULL;
> +	struct pci_dev *pdev = NULL;
>  	u16 segment = 0;
>  	int i;
>  
> @@ -787,13 +806,7 @@ static struct intel_iommu
> *device_to_iommu(struct device *dev, u8 *bus, u8 *devf goto out;
>  			}
>  
> -			if (!pdev || !dev_is_pci(tmp))
> -				continue;
> -
> -			ptmp = to_pci_dev(tmp);
> -			if (ptmp->subordinate &&
> -			    ptmp->subordinate->number <=
> pdev->bus->number &&
> -			    ptmp->subordinate->busn_res.end >=
> pdev->bus->number)
> +			if (is_downstream_to_pci_bridge(dev, tmp))
>  				goto got_pdev;
>  		}
>  
> @@ -2886,7 +2899,8 @@ static bool device_has_rmrr(struct device *dev)
>  		 */
>  		for_each_active_dev_scope(rmrr->devices,
>  					  rmrr->devices_cnt, i, tmp)
> -			if (tmp == dev) {
> +			if (tmp == dev ||
> +			    is_downstream_to_pci_bridge(dev, tmp)) {
>  				rcu_read_unlock();
>  				return true;
>  			}

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/4] iommu/vt-d: Handle RMRR with PCI bridge device scopes
  2019-05-13 16:41   ` Jacob Pan
@ 2019-05-13 16:58     ` Auger Eric
  0 siblings, 0 replies; 7+ messages in thread
From: Auger Eric @ 2019-05-13 16:58 UTC (permalink / raw)
  To: Jacob Pan
  Cc: alex.williamson, robin.murphy, will.deacon, linux-kernel, iommu,
	sudeep.holla, dwmw2, eric.auger.pro

Hi Jacob,

On 5/13/19 6:41 PM, Jacob Pan wrote:
> On Mon, 13 May 2019 09:13:01 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> When reading the vtd specification and especially the
>> Reserved Memory Region Reporting Structure chapter,
>> it is not obvious a device scope element cannot be a
>> PCI-PCI bridge, in which case all downstream ports are
>> likely to access the reserved memory region. Let's handle
>> this case in device_has_rmrr.
>>
>> Fixes: ea2447f700ca ("intel-iommu: Prevent devices with RMRRs from
>> being placed into SI Domain")
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  drivers/iommu/intel-iommu.c | 32 +++++++++++++++++++++++---------
>>  1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index e2134b13c9ae..89d82a1d50b1 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -736,12 +736,31 @@ static int iommu_dummy(struct device *dev)
>>  	return dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>>  }
>>  
>> +static bool
>> +is_downstream_to_pci_bridge(struct device *deva, struct device *devb)
>> +{
>> +	struct pci_dev *pdeva, *pdevb;
>> +
> is there a more illustrative name for these. i guess deva is is the
> bridge dev?
deva is the candidate PCI device likely to belong to the PCI
sub-hierarchy of devb (the candidate bridge).

My concern about the naming was that they are not necessarily a pci
device or a bridge. But at least I can add a comment or rename according
to your suggestion ;-)


>> +	if (!dev_is_pci(deva) || !dev_is_pci(devb))
>> +		return false;
>> +
>> +	pdeva = to_pci_dev(deva);
>> +	pdevb = to_pci_dev(devb);
>> +
>> +	if (pdevb->subordinate &&
>> +	    pdevb->subordinate->number <= pdeva->bus->number &&
>> +	    pdevb->subordinate->busn_res.end >= pdeva->bus->number)
>> +		return true;
>> +
>> +	return false;
>> +>> +
> this seems to be a separate cleanup patch.
I can split into 2 patches: introduction of this helper and its usage in
device_to_iommu and second patch using it in iommu_has_rmrr.

>>  static struct intel_iommu *device_to_iommu(struct device *dev, u8
>> *bus, u8 *devfn) {
>>  	struct dmar_drhd_unit *drhd = NULL;
>>  	struct intel_iommu *iommu;
>>  	struct device *tmp;
>> -	struct pci_dev *ptmp, *pdev = NULL;
>> +	struct pci_dev *pdev = NULL;
>>  	u16 segment = 0;
>>  	int i;
>>  
>> @@ -787,13 +806,7 @@ static struct intel_iommu
>> *device_to_iommu(struct device *dev, u8 *bus, u8 *devf goto out;
>>  			}
>>  
>> -			if (!pdev || !dev_is_pci(tmp))
>> -				continue;
>> -
>> -			ptmp = to_pci_dev(tmp);
>> -			if (ptmp->subordinate &&
>> -			    ptmp->subordinate->number <=
>> pdev->bus->number &&
>> -			    ptmp->subordinate->busn_res.end >=
>> pdev->bus->number)
>> +			if (is_downstream_to_pci_bridge(dev, tmp))
>>  				goto got_pdev;
>>  		}
>>  
>> @@ -2886,7 +2899,8 @@ static bool device_has_rmrr(struct device *dev)
>>  		 */
>>  		for_each_active_dev_scope(rmrr->devices,
>>  					  rmrr->devices_cnt, i, tmp)
>> -			if (tmp == dev) {
>> +			if (tmp == dev ||
>> +			    is_downstream_to_pci_bridge(dev, tmp)) {
>>  				rcu_read_unlock();
>>  				return true;
>>  			}
> 
> [Jacob Pan]
> 
Thanks

Eric
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-05-13 16:58 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13  7:12 [PATCH 0/4] RMRR related fixes Eric Auger
2019-05-13  7:12 ` [PATCH 1/4] iommu: Pass a GFP flag parameter to iommu_alloc_resv_region() Eric Auger
2019-05-13  7:13 ` [PATCH 2/4] iommu/vt-d: Duplicate iommu_resv_region objects per device list Eric Auger
2019-05-13  7:13 ` [PATCH 3/4] iommu/vt-d: Handle RMRR with PCI bridge device scopes Eric Auger
2019-05-13 16:41   ` Jacob Pan
2019-05-13 16:58     ` Auger Eric
2019-05-13  7:13 ` [PATCH 4/4] iommu/vt-d: Handle PCI bridge RMRR device scopes in intel_iommu_get_resv_regions Eric Auger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).