[PATCH v5 0/7] RMRR related fixes and enhancements

[PATCH v5 0/7] RMRR related fixes and enhancements

[Eric Auger]

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().

Last, the VFIO subsystem would need to compute the usable IOVA range
by querying the iommu_get_group_resv_regions() API. This would allow,
for instance, to report potential conflicts between the guest physical
address space and host reserved regions.

However iommu_get_group_resv_regions() currently fails to differentiate
RMRRs that are known safe for device assignment and RMRRs that must be
enforced. So we introduce a new reserved memory region type (relaxable),
reported when associated to an USB or GFX device. The last 2 patches aim
at unblocking [1] which is stuck since 4.18.

[1-5] are fixes
[6-7] are enhancements

The two parts can be considered separately if needed.

References:
[1] [PATCH v6 0/7] vfio/type1: Add support for valid iova list management
    https://patchwork.kernel.org/patch/10425309/

Branch: This series is available at:
https://github.com/eauger/linux/tree/v5.2-rc2-rmrr-v5

History:

v4 -> v5:
- remove iommu: Pass a GFP flag parameter to iommu_alloc_resv_region()
- use dmar_global_lock instead of rcu-lock in intel_iommu_get_resv_regions

v3 -> v4:
- added "iommu: Fix a leak in iommu_insert_resv_region"
- introduced device_rmrr_is_relaxable and fixed to_pci_dev call
  without checking dev_is_pci
- Despite Robin suggested to hide direct relaxable behind direct
  ones, I think this would lead to a very complex implementation
  of iommu_insert_resv_region while in general the relaxable
  regions are going to be ignored by the caller. By the way I
  found a leak in this function, hence the new first patch

v2 -> v3:
s/||/&& in iommu_group_create_direct_mappings

v1 -> v2:
- introduce is_downstream_to_pci_bridge() in a separate patch, change param
  names and add kerneldoc comment
- add 6,7


Eric Auger (7):
  iommu: Fix a leak in iommu_insert_resv_region
  iommu/vt-d: Duplicate iommu_resv_region objects per device list
  iommu/vt-d: Introduce is_downstream_to_pci_bridge helper
  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
  iommu: Introduce IOMMU_RESV_DIRECT_RELAXABLE reserved memory regions
  iommu/vt-d: Differentiate relaxable and non relaxable RMRRs

 .../ABI/testing/sysfs-kernel-iommu_groups     |   9 ++
 drivers/iommu/intel-iommu.c                   | 128 ++++++++++++------
 drivers/iommu/iommu.c                         |  20 +--
 include/linux/iommu.h                         |   6 +
 4 files changed, 115 insertions(+), 48 deletions(-)

-- 
2.20.1

[PATCH v5 1/7] iommu: Fix a leak in iommu_insert_resv_region

[Eric Auger]

In case we expand an existing region, we unlink
this latter and insert the larger one. In
that case we should free the original region after
the insertion. Also we can immediately return.

Fixes: 6c65fb318e8b ("iommu: iommu_get_group_resv_regions")

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 67ee6623f9b2..f961f71e4ff8 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -237,18 +237,21 @@ static int iommu_insert_resv_region(struct iommu_resv_region *new,
 			pos = pos->next;
 		} else if ((start >= a) && (end <= b)) {
 			if (new->type == type)
-				goto done;
+				return 0;
 			else
 				pos = pos->next;
 		} else {
 			if (new->type == type) {
 				phys_addr_t new_start = min(a, start);
 				phys_addr_t new_end = max(b, end);
+				int ret;
 
 				list_del(&entry->list);
 				entry->start = new_start;
 				entry->length = new_end - new_start + 1;
-				iommu_insert_resv_region(entry, regions);
+				ret = iommu_insert_resv_region(entry, regions);
+				kfree(entry);
+				return ret;
 			} else {
 				pos = pos->next;
 			}
@@ -261,7 +264,6 @@ static int iommu_insert_resv_region(struct iommu_resv_region *new,
 		return -ENOMEM;
 
 	list_add_tail(&region->list, pos);
-done:
 	return 0;
 }
 
-- 
2.20.1

[PATCH v5 2/7] iommu/vt-d: Duplicate iommu_resv_region objects per device list

[Eric Auger]

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(). We hold the dmar_global_lock instead
of the rcu-lock to allow sleeping.

Fixes: 0659b8dc45a6 ("iommu/vt-d: Implement reserved region get/put callbacks")
Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v4 -> v5
- replace rcu-lock by the dmar_global_lock
---
 drivers/iommu/intel-iommu.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index a209199f3af6..5ec8b5bd308f 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 {
@@ -4205,7 +4204,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;
 
@@ -4219,22 +4217,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);
-	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:
@@ -4452,7 +4444,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);
 	}
 
@@ -5470,22 +5461,33 @@ 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;
 	int i;
 
-	rcu_read_lock();
+	down_write(&dmar_global_lock);
 	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);
+			if (!resv)
+				break;
+
+			list_add_tail(&resv->list, head);
 		}
 	}
-	rcu_read_unlock();
+	up_write(&dmar_global_lock);
 
 	reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
 				      IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
@@ -5500,10 +5502,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);
 }
 
 int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev)
-- 
2.20.1

[PATCH v5 3/7] iommu/vt-d: Introduce is_downstream_to_pci_bridge helper

[Eric Auger]

Several call sites are about to check whether a device belongs
to the PCI sub-hierarchy of a candidate PCI-PCI bridge.
Introduce an helper to perform that check.

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

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 5ec8b5bd308f..879f11c82b05 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -736,12 +736,39 @@ static int iommu_dummy(struct device *dev)
 	return dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
 }
 
+/* is_downstream_to_pci_bridge - test if a device belongs to the
+ * PCI sub-hierarchy of a candidate PCI-PCI bridge
+ *
+ * @dev: candidate PCI device belonging to @bridge PCI sub-hierarchy
+ * @bridge: the candidate PCI-PCI bridge
+ *
+ * Return: true if @dev belongs to @bridge PCI sub-hierarchy
+ */
+static bool
+is_downstream_to_pci_bridge(struct device *dev, struct device *bridge)
+{
+	struct pci_dev *pdev, *pbridge;
+
+	if (!dev_is_pci(dev) || !dev_is_pci(bridge))
+		return false;
+
+	pdev = to_pci_dev(dev);
+	pbridge = to_pci_dev(bridge);
+
+	if (pbridge->subordinate &&
+	    pbridge->subordinate->number <= pdev->bus->number &&
+	    pbridge->subordinate->busn_res.end >= pdev->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 +814,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;
 		}
 
-- 
2.20.1

[PATCH v5 4/7] iommu/vt-d: Handle RMRR with PCI bridge device scopes

[Eric Auger]

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>

---

v1 -> v2:
- is_downstream_to_pci_bridge helper introduced in a separate patch
---
 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 879f11c82b05..35508687f178 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2910,7 +2910,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

[PATCH v5 5/7] iommu/vt-d: Handle PCI bridge RMRR device scopes in intel_iommu_get_resv_regions

[Eric Auger]

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 35508687f178..9302351818ab 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5496,7 +5496,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

[PATCH v5 6/7] iommu: Introduce IOMMU_RESV_DIRECT_RELAXABLE reserved memory regions

[Eric Auger]

Introduce a new type for reserved region. This corresponds
to directly mapped regions which are known to be relaxable
in some specific conditions, such as device assignment use
case. Well known examples are those used by USB controllers
providing PS/2 keyboard emulation for pre-boot BIOS and
early BOOT or RMRRs associated to IGD working in legacy mode.

Since commit c875d2c1b808 ("iommu/vt-d: Exclude devices using RMRRs
from IOMMU API domains") and commit 18436afdc11a ("iommu/vt-d: Allow
RMRR on graphics devices too"), those regions are currently
considered "safe" with respect to device assignment use case
which requires a non direct mapping at IOMMU physical level
(RAM GPA -> HPA mapping).

Those RMRRs currently exist and sometimes the device is
attempting to access it but this has not been considered
an issue until now.

However at the moment, iommu_get_group_resv_regions() is
not able to make any difference between directly mapped
regions: those which must be absolutely enforced and those
like above ones which are known as relaxable.

This is a blocker for reporting severe conflicts between
non relaxable RMRRs (like MSI doorbells) and guest GPA space.

With this new reserved region type we will be able to use
iommu_get_group_resv_regions() to enumerate the IOVA space
that is usable through the IOMMU API without introducing
regressions with respect to existing device assignment
use cases (USB and IGD).

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v3 -> v4:
- expose the relaxable regions as "direct-relaxable" in the sysfs
- update ABI documentation

v2 -> v3:
- fix direct type check
---
 Documentation/ABI/testing/sysfs-kernel-iommu_groups |  9 +++++++++
 drivers/iommu/iommu.c                               | 12 +++++++-----
 include/linux/iommu.h                               |  6 ++++++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
index 35c64e00b35c..017f5bc3920c 100644
--- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
+++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
@@ -24,3 +24,12 @@ Description:    /sys/kernel/iommu_groups/reserved_regions list IOVA
 		region is described on a single line: the 1st field is
 		the base IOVA, the second is the end IOVA and the third
 		field describes the type of the region.
+
+What:		/sys/kernel/iommu_groups/reserved_regions
+Date: 		June 2019
+KernelVersion:  v5.3
+Contact: 	Eric Auger <eric.auger@redhat.com>
+Description:    In case an RMRR is used only by graphics or USB devices
+		it is now exposed as "direct-relaxable" instead of "direct".
+		In device assignment use case, for instance, those RMRR
+		are considered to be relaxable and safe.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f961f71e4ff8..276eae9822f2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -73,10 +73,11 @@ struct iommu_group_attribute {
 };
 
 static const char * const iommu_group_resv_type_string[] = {
-	[IOMMU_RESV_DIRECT]	= "direct",
-	[IOMMU_RESV_RESERVED]	= "reserved",
-	[IOMMU_RESV_MSI]	= "msi",
-	[IOMMU_RESV_SW_MSI]	= "msi",
+	[IOMMU_RESV_DIRECT]			= "direct",
+	[IOMMU_RESV_DIRECT_RELAXABLE]		= "direct-relaxable",
+	[IOMMU_RESV_RESERVED]			= "reserved",
+	[IOMMU_RESV_MSI]			= "msi",
+	[IOMMU_RESV_SW_MSI]			= "msi",
 };
 
 #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
@@ -575,7 +576,8 @@ static int iommu_group_create_direct_mappings(struct iommu_group *group,
 		start = ALIGN(entry->start, pg_size);
 		end   = ALIGN(entry->start + entry->length, pg_size);
 
-		if (entry->type != IOMMU_RESV_DIRECT)
+		if (entry->type != IOMMU_RESV_DIRECT &&
+		    entry->type != IOMMU_RESV_DIRECT_RELAXABLE)
 			continue;
 
 		for (addr = start; addr < end; addr += pg_size) {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a815cf6f6f47..d7d1c8de9bbc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -135,6 +135,12 @@ enum iommu_attr {
 enum iommu_resv_type {
 	/* Memory regions which must be mapped 1:1 at all times */
 	IOMMU_RESV_DIRECT,
+	/*
+	 * Memory regions which are advertised to be 1:1 but are
+	 * commonly considered relaxable in some conditions,
+	 * for instance in device assignment use case (USB, Graphics)
+	 */
+	IOMMU_RESV_DIRECT_RELAXABLE,
 	/* Arbitrary "never map this or give it to a device" address ranges */
 	IOMMU_RESV_RESERVED,
 	/* Hardware MSI region (untranslated) */
-- 
2.20.1

[PATCH v5 7/7] iommu/vt-d: Differentiate relaxable and non relaxable RMRRs

[Eric Auger]

Now we have a new IOMMU_RESV_DIRECT_RELAXABLE reserved memory
region type, let's report USB and GFX RMRRs as relaxable ones.

We introduce a new device_rmrr_is_relaxable() helper to check
whether the rmrr belongs to the relaxable category.

This allows to have a finer reporting at IOMMU API level of
reserved memory regions. This will be exploitable by VFIO to
define the usable IOVA range and detect potential conflicts
between the guest physical address space and host reserved
regions.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v3 -> v4:
- introduce device_rmrr_is_relaxable and reshuffle the comments
---
 drivers/iommu/intel-iommu.c | 55 +++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 9302351818ab..01c82f848470 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2920,6 +2920,36 @@ static bool device_has_rmrr(struct device *dev)
 	return false;
 }
 
+/*
+ * device_rmrr_is_relaxable - Test whether the RMRR of this device
+ * is relaxable (ie. is allowed to be not enforced under some conditions)
+ *
+ * @dev: device handle
+ *
+ * We assume that PCI USB devices with RMRRs have them largely
+ * for historical reasons and that the RMRR space is not actively used post
+ * boot.  This exclusion may change if vendors begin to abuse it.
+ *
+ * The same exception is made for graphics devices, with the requirement that
+ * any use of the RMRR regions will be torn down before assigning the device
+ * to a guest.
+ *
+ * Return: true if the RMRR is relaxable
+ */
+static bool device_rmrr_is_relaxable(struct device *dev)
+{
+	struct pci_dev *pdev;
+
+	if (!dev_is_pci(dev))
+		return false;
+
+	pdev = to_pci_dev(dev);
+	if (IS_USB_DEVICE(pdev) || IS_GFX_DEVICE(pdev))
+		return true;
+	else
+		return false;
+}
+
 /*
  * There are a couple cases where we need to restrict the functionality of
  * devices associated with RMRRs.  The first is when evaluating a device for
@@ -2934,25 +2964,16 @@ static bool device_has_rmrr(struct device *dev)
  * We therefore prevent devices associated with an RMRR from participating in
  * the IOMMU API, which eliminates them from device assignment.
  *
- * In both cases we assume that PCI USB devices with RMRRs have them largely
- * for historical reasons and that the RMRR space is not actively used post
- * boot.  This exclusion may change if vendors begin to abuse it.
- *
- * The same exception is made for graphics devices, with the requirement that
- * any use of the RMRR regions will be torn down before assigning the device
- * to a guest.
+ * In both cases, devices which have relaxable RMRRs are not concerned by this
+ * restriction. See device_rmrr_is_relaxable comment.
  */
 static bool device_is_rmrr_locked(struct device *dev)
 {
 	if (!device_has_rmrr(dev))
 		return false;
 
-	if (dev_is_pci(dev)) {
-		struct pci_dev *pdev = to_pci_dev(dev);
-
-		if (IS_USB_DEVICE(pdev) || IS_GFX_DEVICE(pdev))
-			return false;
-	}
+	if (device_rmrr_is_relaxable(dev))
+		return false;
 
 	return true;
 }
@@ -5494,6 +5515,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
 		for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
 					  i, i_dev) {
 			struct iommu_resv_region *resv;
+			enum iommu_resv_type type;
 			size_t length;
 
 			if (i_dev != device &&
@@ -5501,9 +5523,12 @@ static void intel_iommu_get_resv_regions(struct device *device,
 				continue;
 
 			length = rmrr->end_address - rmrr->base_address + 1;
+
+			type = device_rmrr_is_relaxable(device) ?
+				IOMMU_RESV_DIRECT_RELAXABLE : IOMMU_RESV_DIRECT;
+
 			resv = iommu_alloc_resv_region(rmrr->base_address,
-						       length, prot,
-						       IOMMU_RESV_DIRECT);
+						       length, prot, type);
 			if (!resv)
 				break;
 
-- 
2.20.1

Re: [PATCH v5 2/7] iommu/vt-d: Duplicate iommu_resv_region objects per device list

[Lu Baolu]

Hi Eric,

On 5/28/19 7:50 PM, Eric Auger wrote:
> 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(). We hold the dmar_global_lock instead
> of the rcu-lock to allow sleeping.
> 
> Fixes: 0659b8dc45a6 ("iommu/vt-d: Implement reserved region get/put callbacks")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v4 -> v5
> - replace rcu-lock by the dmar_global_lock
> ---
>   drivers/iommu/intel-iommu.c | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index a209199f3af6..5ec8b5bd308f 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 {
> @@ -4205,7 +4204,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;
>   
> @@ -4219,22 +4217,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);
> -	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:
> @@ -4452,7 +4444,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);
>   	}
>   
> @@ -5470,22 +5461,33 @@ 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;

I know this is moved from above. How about adding spaces around the '|'?

>   	struct iommu_resv_region *reg;
>   	struct dmar_rmrr_unit *rmrr;
>   	struct device *i_dev;
>   	int i;
>   
> -	rcu_read_lock();
> +	down_write(&dmar_global_lock);

Just out of curiosity, why not down_read()? We don't change the rmrr
list here, right?

>   	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);
> +			if (!resv)
> +				break;
> +
> +			list_add_tail(&resv->list, head);
>   		}
>   	}
> -	rcu_read_unlock();
> +	up_write(&dmar_global_lock);
>   
>   	reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
>   				      IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
> @@ -5500,10 +5502,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);
>   }
>   
>   int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev)
> 

Other looks good to me.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
Baolu

Re: [PATCH v5 3/7] iommu/vt-d: Introduce is_downstream_to_pci_bridge helper

[Lu Baolu]

Hi,

On 5/28/19 7:50 PM, Eric Auger wrote:
> Several call sites are about to check whether a device belongs
> to the PCI sub-hierarchy of a candidate PCI-PCI bridge.
> Introduce an helper to perform that check.
> 

This looks good to me.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
Baolu


> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++++--------
>   1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 5ec8b5bd308f..879f11c82b05 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -736,12 +736,39 @@ static int iommu_dummy(struct device *dev)
>   	return dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>   }
>   
> +/* is_downstream_to_pci_bridge - test if a device belongs to the
> + * PCI sub-hierarchy of a candidate PCI-PCI bridge
> + *
> + * @dev: candidate PCI device belonging to @bridge PCI sub-hierarchy
> + * @bridge: the candidate PCI-PCI bridge
> + *
> + * Return: true if @dev belongs to @bridge PCI sub-hierarchy
> + */
> +static bool
> +is_downstream_to_pci_bridge(struct device *dev, struct device *bridge)
> +{
> +	struct pci_dev *pdev, *pbridge;
> +
> +	if (!dev_is_pci(dev) || !dev_is_pci(bridge))
> +		return false;
> +
> +	pdev = to_pci_dev(dev);
> +	pbridge = to_pci_dev(bridge);
> +
> +	if (pbridge->subordinate &&
> +	    pbridge->subordinate->number <= pdev->bus->number &&
> +	    pbridge->subordinate->busn_res.end >= pdev->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 +814,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;
>   		}
>   
> 

Re: [PATCH v5 4/7] iommu/vt-d: Handle RMRR with PCI bridge device scopes

[Lu Baolu]

Hi,

On 5/28/19 7:50 PM, Eric Auger 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.


This looks good to me.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
Baolu

> 
> Fixes: ea2447f700ca ("intel-iommu: Prevent devices with RMRRs from being placed into SI Domain")
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - is_downstream_to_pci_bridge helper introduced in a separate patch
> ---
>   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 879f11c82b05..35508687f178 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2910,7 +2910,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;
>   			}
> 

Re: [PATCH v5 5/7] iommu/vt-d: Handle PCI bridge RMRR device scopes in intel_iommu_get_resv_regions

[Lu Baolu]

Hi,

On 5/28/19 7:50 PM, Eric Auger wrote:
> In the case the RMRR device scope is a PCI-PCI bridge, let's check
> the device belongs to the PCI sub-hierarchy.


This looks good to me.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
Baolu

> 
> 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 35508687f178..9302351818ab 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -5496,7 +5496,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;
> 

Re: [PATCH v5 7/7] iommu/vt-d: Differentiate relaxable and non relaxable RMRRs

[Lu Baolu]

Hi,

On 5/28/19 7:50 PM, Eric Auger wrote:
> Now we have a new IOMMU_RESV_DIRECT_RELAXABLE reserved memory
> region type, let's report USB and GFX RMRRs as relaxable ones.
> 
> We introduce a new device_rmrr_is_relaxable() helper to check
> whether the rmrr belongs to the relaxable category.
> 
> This allows to have a finer reporting at IOMMU API level of
> reserved memory regions. This will be exploitable by VFIO to
> define the usable IOVA range and detect potential conflicts
> between the guest physical address space and host reserved
> regions.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v3 -> v4:
> - introduce device_rmrr_is_relaxable and reshuffle the comments
> ---
>   drivers/iommu/intel-iommu.c | 55 +++++++++++++++++++++++++++----------
>   1 file changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 9302351818ab..01c82f848470 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2920,6 +2920,36 @@ static bool device_has_rmrr(struct device *dev)
>   	return false;
>   }
>   
> +/*
> + * device_rmrr_is_relaxable - Test whether the RMRR of this device
> + * is relaxable (ie. is allowed to be not enforced under some conditions)
> + *
> + * @dev: device handle
> + *
> + * We assume that PCI USB devices with RMRRs have them largely
> + * for historical reasons and that the RMRR space is not actively used post
> + * boot.  This exclusion may change if vendors begin to abuse it.
> + *
> + * The same exception is made for graphics devices, with the requirement that
> + * any use of the RMRR regions will be torn down before assigning the device
> + * to a guest.
> + *
> + * Return: true if the RMRR is relaxable
> + */
> +static bool device_rmrr_is_relaxable(struct device *dev)
> +{
> +	struct pci_dev *pdev;
> +
> +	if (!dev_is_pci(dev))
> +		return false;
> +
> +	pdev = to_pci_dev(dev);
> +	if (IS_USB_DEVICE(pdev) || IS_GFX_DEVICE(pdev))
> +		return true;
> +	else
> +		return false;
> +}

I know this is only code refactoring. But strictly speaking, the rmrr of
any USB host device is ignorable only if quirk_usb_early_handoff() has
been called. There, the control of USB host controller will be handed
over from BIOS to OS and the corresponding SMI are disabled.

This function is registered in drivers/usb/host/pci-quirks.c

DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
                         PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);

and only get compiled if CONFIG_USB_PCI is enabled.

Hence, it's safer to say:

+#ifdef CONFIG_USB_PCI
+	if (IS_USB_DEVICE(pdev))
+		return true;
+#endif /* CONFIG_USB_PCI */

I am okay if we keep this untouched and make this change within a
separated patch.

> +
>   /*
>    * There are a couple cases where we need to restrict the functionality of
>    * devices associated with RMRRs.  The first is when evaluating a device for
> @@ -2934,25 +2964,16 @@ static bool device_has_rmrr(struct device *dev)
>    * We therefore prevent devices associated with an RMRR from participating in
>    * the IOMMU API, which eliminates them from device assignment.
>    *
> - * In both cases we assume that PCI USB devices with RMRRs have them largely
> - * for historical reasons and that the RMRR space is not actively used post
> - * boot.  This exclusion may change if vendors begin to abuse it.
> - *
> - * The same exception is made for graphics devices, with the requirement that
> - * any use of the RMRR regions will be torn down before assigning the device
> - * to a guest.
> + * In both cases, devices which have relaxable RMRRs are not concerned by this
> + * restriction. See device_rmrr_is_relaxable comment.
>    */
>   static bool device_is_rmrr_locked(struct device *dev)
>   {
>   	if (!device_has_rmrr(dev))
>   		return false;
>   
> -	if (dev_is_pci(dev)) {
> -		struct pci_dev *pdev = to_pci_dev(dev);
> -
> -		if (IS_USB_DEVICE(pdev) || IS_GFX_DEVICE(pdev))
> -			return false;
> -	}
> +	if (device_rmrr_is_relaxable(dev))
> +		return false;
>   
>   	return true;
>   }
> @@ -5494,6 +5515,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
>   		for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
>   					  i, i_dev) {
>   			struct iommu_resv_region *resv;
> +			enum iommu_resv_type type;
>   			size_t length;
>   
>   			if (i_dev != device &&
> @@ -5501,9 +5523,12 @@ static void intel_iommu_get_resv_regions(struct device *device,
>   				continue;
>   
>   			length = rmrr->end_address - rmrr->base_address + 1;
> +
> +			type = device_rmrr_is_relaxable(device) ?
> +				IOMMU_RESV_DIRECT_RELAXABLE : IOMMU_RESV_DIRECT;
> +
>   			resv = iommu_alloc_resv_region(rmrr->base_address,
> -						       length, prot,
> -						       IOMMU_RESV_DIRECT);
> +						       length, prot, type);
>   			if (!resv)
>   				break;
>   
> 

Other looks good to me.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
Baolu

Re: [PATCH v5 1/7] iommu: Fix a leak in iommu_insert_resv_region

[Christoph Hellwig]

>  		} else if ((start >= a) && (end <= b)) {
>  			if (new->type == type)
> -				goto done;
> +				return 0;
>  			else
>  				pos = pos->next;

Please remove the pointless else after the return statement.

>  		} else {
>  			if (new->type == type) {
>  				phys_addr_t new_start = min(a, start);
>  				phys_addr_t new_end = max(b, end);
> +				int ret;
>  
>  				list_del(&entry->list);
>  				entry->start = new_start;
>  				entry->length = new_end - new_start + 1;
> -				iommu_insert_resv_region(entry, regions);
> +				ret = iommu_insert_resv_region(entry, regions);
> +				kfree(entry);
> +				return ret;
>  			} else {
>  				pos = pos->next;
>  			}

Same here.

Re: [PATCH v5 3/7] iommu/vt-d: Introduce is_downstream_to_pci_bridge helper

[Christoph Hellwig]

> +/* is_downstream_to_pci_bridge - test if a device belongs to the
> + * PCI sub-hierarchy of a candidate PCI-PCI bridge
> + *
> + * @dev: candidate PCI device belonging to @bridge PCI sub-hierarchy
> + * @bridge: the candidate PCI-PCI bridge
> + *
> + * Return: true if @dev belongs to @bridge PCI sub-hierarchy
> + */

This is not valid kerneldoc comment.  Try something like this:

/**
 * is_downstream_to_pci_bridge - test if a device belongs to the PCI
 *				 sub-hierarchy of a candidate PCI-PCI bridge
 * @dev: candidate PCI device belonging to @bridge PCI sub-hierarchy
 * @bridge: the candidate PCI-PCI bridge
 *
 * Returns true if @dev belongs to @bridge PCI sub-hierarchy, else false.
 */

Re: [PATCH v5 1/7] iommu: Fix a leak in iommu_insert_resv_region

[Auger Eric]

Hi Christoph,

On 5/29/19 8:17 AM, Christoph Hellwig wrote:
>>  		} else if ((start >= a) && (end <= b)) {
>>  			if (new->type == type)
>> -				goto done;
>> +				return 0;
>>  			else
>>  				pos = pos->next;
> 
> Please remove the pointless else after the return statement.
Sorry I don't get your point here. For both cases, we only return if
both types match. Otherwise we continue parsing the list as there may be
a region of the same type as @new following the current interval that
may need to be merged with @new. Let's consider the insertion of those
resv regions:

add_resv(head, 0xa001000, 0xa001FFF, IOMMU_RESV_TYPE1);
head -> 0x000000000a001000 0x000000000a001fff type1

add_resv(head, 0xa003000, 0xa003FFF, IOMMU_RESV_TYPE1);
head ->
0x000000000a001000 0x000000000a001fff type1 ->
0x000000000a003000 0x000000000a003fff type1

add_resv(head, 0xa002000, 0xa004FFF, IOMMU_RESV_TYPE2);
head ->
0x000000000a001000 0x000000000a001fff type1 ->
0x000000000a003000 0x000000000a003fff type1 ->
0x000000000a002000 0x000000000a004fff type2 ->

add_resv(head, 0xa0010FF, 0xa003000, IOMMU_RESV_TYPE2);

head ->
0x000000000a001000 0x000000000a001fff type1 ->
0x000000000a003000 0x000000000a003fff type1 ->
0x000000000a0010ff 0x000000000a004fff type2 <- merge

Or maybe I simply missed your point. Please let me know.

Thanks

Eric

> 
>>  		} else {
>>  			if (new->type == type) {
>>  				phys_addr_t new_start = min(a, start);
>>  				phys_addr_t new_end = max(b, end);
>> +				int ret;
>>  
>>  				list_del(&entry->list);
>>  				entry->start = new_start;
>>  				entry->length = new_end - new_start + 1;
>> -				iommu_insert_resv_region(entry, regions);
>> +				ret = iommu_insert_resv_region(entry, regions);
>> +				kfree(entry);
>> +				return ret;
>>  			} else {
>>  				pos = pos->next;
>>  			}
> 
> Same here.
> 

Re: [PATCH v5 2/7] iommu/vt-d: Duplicate iommu_resv_region objects per device list

[Auger Eric]

Hi Lu,

On 5/29/19 4:04 AM, Lu Baolu wrote:
> Hi Eric,
> 
> On 5/28/19 7:50 PM, Eric Auger wrote:
>> 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(). We hold the dmar_global_lock instead
>> of the rcu-lock to allow sleeping.
>>
>> Fixes: 0659b8dc45a6 ("iommu/vt-d: Implement reserved region get/put
>> callbacks")
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v4 -> v5
>> - replace rcu-lock by the dmar_global_lock
>> ---
>>   drivers/iommu/intel-iommu.c | 34 +++++++++++++++++-----------------
>>   1 file changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index a209199f3af6..5ec8b5bd308f 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 {
>> @@ -4205,7 +4204,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;
>>   @@ -4219,22 +4217,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);
>> -    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:
>> @@ -4452,7 +4444,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);
>>       }
>>   @@ -5470,22 +5461,33 @@ 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;
> 
> I know this is moved from above. How about adding spaces around the '|'?
sure
> 
>>       struct iommu_resv_region *reg;
>>       struct dmar_rmrr_unit *rmrr;
>>       struct device *i_dev;
>>       int i;
>>   -    rcu_read_lock();
>> +    down_write(&dmar_global_lock);
> 
> Just out of curiosity, why not down_read()? We don't change the rmrr
> list here, right?
you're right, my mistake.
> 
>>       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);
>> +            if (!resv)
>> +                break;
>> +
>> +            list_add_tail(&resv->list, head);
>>           }
>>       }
>> -    rcu_read_unlock();
>> +    up_write(&dmar_global_lock);
>>         reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
>>                         IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
>> @@ -5500,10 +5502,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);
>>   }
>>     int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct
>> device *dev)
>>
> 
> Other looks good to me.
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Thanks!

Eric
> 
> Best regards,
> Baolu

Re: [PATCH v5 3/7] iommu/vt-d: Introduce is_downstream_to_pci_bridge helper

[Auger Eric]

Hi Christoph,

On 5/29/19 8:21 AM, Christoph Hellwig wrote:
>> +/* is_downstream_to_pci_bridge - test if a device belongs to the
>> + * PCI sub-hierarchy of a candidate PCI-PCI bridge
>> + *
>> + * @dev: candidate PCI device belonging to @bridge PCI sub-hierarchy
>> + * @bridge: the candidate PCI-PCI bridge
>> + *
>> + * Return: true if @dev belongs to @bridge PCI sub-hierarchy
>> + */
> 
> This is not valid kerneldoc comment.  Try something like this:
> 
> /**
>  * is_downstream_to_pci_bridge - test if a device belongs to the PCI
>  *				 sub-hierarchy of a candidate PCI-PCI bridge
>  * @dev: candidate PCI device belonging to @bridge PCI sub-hierarchy
>  * @bridge: the candidate PCI-PCI bridge
>  *
>  * Returns true if @dev belongs to @bridge PCI sub-hierarchy, else false.
>  */
> 
Sure,

just replaced Returns by Return:

Thanks

Eric

Re: [PATCH v5 7/7] iommu/vt-d: Differentiate relaxable and non relaxable RMRRs

[Auger Eric]

Hi Lu,

On 5/29/19 4:34 AM, Lu Baolu wrote:
> Hi,
> 
> On 5/28/19 7:50 PM, Eric Auger wrote:
>> Now we have a new IOMMU_RESV_DIRECT_RELAXABLE reserved memory
>> region type, let's report USB and GFX RMRRs as relaxable ones.
>>
>> We introduce a new device_rmrr_is_relaxable() helper to check
>> whether the rmrr belongs to the relaxable category.
>>
>> This allows to have a finer reporting at IOMMU API level of
>> reserved memory regions. This will be exploitable by VFIO to
>> define the usable IOVA range and detect potential conflicts
>> between the guest physical address space and host reserved
>> regions.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v3 -> v4:
>> - introduce device_rmrr_is_relaxable and reshuffle the comments
>> ---
>>   drivers/iommu/intel-iommu.c | 55 +++++++++++++++++++++++++++----------
>>   1 file changed, 40 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 9302351818ab..01c82f848470 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -2920,6 +2920,36 @@ static bool device_has_rmrr(struct device *dev)
>>       return false;
>>   }
>>   +/*
>> + * device_rmrr_is_relaxable - Test whether the RMRR of this device
>> + * is relaxable (ie. is allowed to be not enforced under some
>> conditions)
>> + *
>> + * @dev: device handle
>> + *
>> + * We assume that PCI USB devices with RMRRs have them largely
>> + * for historical reasons and that the RMRR space is not actively
>> used post
>> + * boot.  This exclusion may change if vendors begin to abuse it.
>> + *
>> + * The same exception is made for graphics devices, with the
>> requirement that
>> + * any use of the RMRR regions will be torn down before assigning the
>> device
>> + * to a guest.
>> + *
>> + * Return: true if the RMRR is relaxable
>> + */
>> +static bool device_rmrr_is_relaxable(struct device *dev)
>> +{
>> +    struct pci_dev *pdev;
>> +
>> +    if (!dev_is_pci(dev))
>> +        return false;
>> +
>> +    pdev = to_pci_dev(dev);
>> +    if (IS_USB_DEVICE(pdev) || IS_GFX_DEVICE(pdev))
>> +        return true;
>> +    else
>> +        return false;
>> +}
> 
> I know this is only code refactoring. But strictly speaking, the rmrr of
> any USB host device is ignorable only if quirk_usb_early_handoff() has
> been called. There, the control of USB host controller will be handed
> over from BIOS to OS and the corresponding SMI are disabled.
> 
> This function is registered in drivers/usb/host/pci-quirks.c
> 
> DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
>                         PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
> 
> and only get compiled if CONFIG_USB_PCI is enabled.
> 
> Hence, it's safer to say:
> 
> +#ifdef CONFIG_USB_PCI
> +    if (IS_USB_DEVICE(pdev))
> +        return true;
> +#endif /* CONFIG_USB_PCI */
> 
> I am okay if we keep this untouched and make this change within a
> separated patch.

As we first checked whether the device was a pci device, isn't it
sufficient to guarantee the quirk is setup?

As you suggested, I am inclined to keep it as a separate patch anyway.

Thank you for the review!

Best Regards

Eric
> 
>> +
>>   /*
>>    * There are a couple cases where we need to restrict the
>> functionality of
>>    * devices associated with RMRRs.  The first is when evaluating a
>> device for
>> @@ -2934,25 +2964,16 @@ static bool device_has_rmrr(struct device *dev)
>>    * We therefore prevent devices associated with an RMRR from
>> participating in
>>    * the IOMMU API, which eliminates them from device assignment.
>>    *
>> - * In both cases we assume that PCI USB devices with RMRRs have them
>> largely
>> - * for historical reasons and that the RMRR space is not actively
>> used post
>> - * boot.  This exclusion may change if vendors begin to abuse it.
>> - *
>> - * The same exception is made for graphics devices, with the
>> requirement that
>> - * any use of the RMRR regions will be torn down before assigning the
>> device
>> - * to a guest.
>> + * In both cases, devices which have relaxable RMRRs are not
>> concerned by this
>> + * restriction. See device_rmrr_is_relaxable comment.
>>    */
>>   static bool device_is_rmrr_locked(struct device *dev)
>>   {
>>       if (!device_has_rmrr(dev))
>>           return false;
>>   -    if (dev_is_pci(dev)) {
>> -        struct pci_dev *pdev = to_pci_dev(dev);
>> -
>> -        if (IS_USB_DEVICE(pdev) || IS_GFX_DEVICE(pdev))
>> -            return false;
>> -    }
>> +    if (device_rmrr_is_relaxable(dev))
>> +        return false;
>>         return true;
>>   }
>> @@ -5494,6 +5515,7 @@ static void intel_iommu_get_resv_regions(struct
>> device *device,
>>           for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
>>                         i, i_dev) {
>>               struct iommu_resv_region *resv;
>> +            enum iommu_resv_type type;
>>               size_t length;
>>                 if (i_dev != device &&
>> @@ -5501,9 +5523,12 @@ static void intel_iommu_get_resv_regions(struct
>> device *device,
>>                   continue;
>>                 length = rmrr->end_address - rmrr->base_address + 1;
>> +
>> +            type = device_rmrr_is_relaxable(device) ?
>> +                IOMMU_RESV_DIRECT_RELAXABLE : IOMMU_RESV_DIRECT;
>> +
>>               resv = iommu_alloc_resv_region(rmrr->base_address,
>> -                               length, prot,
>> -                               IOMMU_RESV_DIRECT);
>> +                               length, prot, type);
>>               if (!resv)
>>                   break;
>>  
> 
> Other looks good to me.
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Best regards,
> Baolu