[PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Baptiste Reynal]

This patch series makes the VFIO_IOMMU_TYPE1 driver buildable on ARM, so it
may be used with ARM SMMUs. It also adds support for the IOMMU_NOEXEC flag
supported by SMMUs adhering to the ARM SMMU specification so the VFIO user can
specify whether the target memory can be executed by the device behind the
SMMU.

Changes from v4:
 - Remove domain->caps, use iommu capacity flags instead
Changes from v3:
 - Rebased on linux v4.0-rc1
 - Use bit shifting for domain->caps
 - Baptiste Reynal is the new maintainer of this serie
Changes from v2:
 - Rebased on latest iommu/next branch by Joerg Roedel
Changes from v1:
 - Bugfixes and corrected some typos
 - Use enum for VFIO IOMMU driver capabilities

Antonios Motakis (4):
  vfio: implement iommu driver capabilities with an enum
  vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
  vfio: type1: replace vfio_domains_have_iommu_cache with generic
    function
  vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag

 drivers/vfio/vfio_iommu_type1.c | 60 ++++++++++++++++++++++++++++-------------
 include/uapi/linux/vfio.h       | 30 ++++++++++++---------
 2 files changed, 58 insertions(+), 32 deletions(-)

-- 
2.3.1

[PATCH v5 1/4] vfio: implement iommu driver capabilities with an enum

[Baptiste Reynal]

From: Antonios Motakis <a.motakis@virtualopensystems.com>

Currently a VFIO driver's IOMMU capabilities are encoded as a series of
numerical defines. Replace this with an enum for future maintainability.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 include/uapi/linux/vfio.h | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 82889c3..5fb3d46 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -19,22 +19,20 @@
 
 /* Kernel & User level defines for VFIO IOCTLs. */
 
-/* Extensions */
-
-#define VFIO_TYPE1_IOMMU		1
-#define VFIO_SPAPR_TCE_IOMMU		2
-#define VFIO_TYPE1v2_IOMMU		3
 /*
- * IOMMU enforces DMA cache coherence (ex. PCIe NoSnoop stripping).  This
- * capability is subject to change as groups are added or removed.
+ * Capabilities exposed by the VFIO IOMMU driver. Some capabilities are subject
+ * to change as groups are added or removed.
  */
-#define VFIO_DMA_CC_IOMMU		4
-
-/* Check if EEH is supported */
-#define VFIO_EEH			5
+enum vfio_iommu_cap {
+	VFIO_TYPE1_IOMMU = 1,
+	VFIO_SPAPR_TCE_IOMMU = 2,
+	VFIO_TYPE1v2_IOMMU = 3,
+	VFIO_DMA_CC_IOMMU = 4,		/* IOMMU enforces DMA cache coherence
+					   (ex. PCIe NoSnoop stripping) */
+	VFIO_EEH = 5,			/* Check if EEH is supported */
+	VFIO_TYPE1_NESTING_IOMMU = 6,	/* Two-stage IOMMU, implies v2  */
+};
 
-/* Two-stage IOMMU */
-#define VFIO_TYPE1_NESTING_IOMMU	6	/* Implies v2 */
 
 /*
  * The IOCTL interface is designed for extensibility by embedding the
-- 
2.3.1

[PATCH v5 1/4] vfio: implement iommu driver capabilities with an enum

[Baptiste Reynal]

From: Antonios Motakis <a.motakis@virtualopensystems.com>

Currently a VFIO driver's IOMMU capabilities are encoded as a series of
numerical defines. Replace this with an enum for future maintainability.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 include/uapi/linux/vfio.h | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 82889c3..5fb3d46 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -19,22 +19,20 @@
 
 /* Kernel & User level defines for VFIO IOCTLs. */
 
-/* Extensions */
-
-#define VFIO_TYPE1_IOMMU		1
-#define VFIO_SPAPR_TCE_IOMMU		2
-#define VFIO_TYPE1v2_IOMMU		3
 /*
- * IOMMU enforces DMA cache coherence (ex. PCIe NoSnoop stripping).  This
- * capability is subject to change as groups are added or removed.
+ * Capabilities exposed by the VFIO IOMMU driver. Some capabilities are subject
+ * to change as groups are added or removed.
  */
-#define VFIO_DMA_CC_IOMMU		4
-
-/* Check if EEH is supported */
-#define VFIO_EEH			5
+enum vfio_iommu_cap {
+	VFIO_TYPE1_IOMMU = 1,
+	VFIO_SPAPR_TCE_IOMMU = 2,
+	VFIO_TYPE1v2_IOMMU = 3,
+	VFIO_DMA_CC_IOMMU = 4,		/* IOMMU enforces DMA cache coherence
+					   (ex. PCIe NoSnoop stripping) */
+	VFIO_EEH = 5,			/* Check if EEH is supported */
+	VFIO_TYPE1_NESTING_IOMMU = 6,	/* Two-stage IOMMU, implies v2  */
+};
 
-/* Two-stage IOMMU */
-#define VFIO_TYPE1_NESTING_IOMMU	6	/* Implies v2 */
 
 /*
  * The IOCTL interface is designed for extensibility by embedding the
-- 
2.3.1

[PATCH v5 2/4] vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Baptiste Reynal]

From: Antonios Motakis <a.motakis@virtualopensystems.com>

We introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag to the VFIO dma map call,
and expose its availability via the capability VFIO_DMA_NOEXEC_IOMMU.
This way the user can control whether the XN flag will be set on the
requested mappings. The IOMMU_NOEXEC flag needs to be available for all
the IOMMUs of the container used.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 include/uapi/linux/vfio.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 5fb3d46..30801a7 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -31,6 +31,7 @@ enum vfio_iommu_cap {
 					   (ex. PCIe NoSnoop stripping) */
 	VFIO_EEH = 5,			/* Check if EEH is supported */
 	VFIO_TYPE1_NESTING_IOMMU = 6,	/* Two-stage IOMMU, implies v2  */
+	VFIO_DMA_NOEXEC_IOMMU = 7,
 };
 
 
@@ -397,12 +398,17 @@ struct vfio_iommu_type1_info {
  *
  * Map process virtual addresses to IO virtual addresses using the
  * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
+ *
+ * To use the VFIO_DMA_MAP_FLAG_NOEXEC flag, the container must support the
+ * VFIO_DMA_NOEXEC_IOMMU capability. If mappings are created using this flag,
+ * any groups subsequently added to the container must support this capability.
  */
 struct vfio_iommu_type1_dma_map {
 	__u32	argsz;
 	__u32	flags;
 #define VFIO_DMA_MAP_FLAG_READ (1 << 0)		/* readable from device */
 #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1)	/* writable from device */
+#define VFIO_DMA_MAP_FLAG_NOEXEC (1 << 2)	/* not executable from device */
 	__u64	vaddr;				/* Process virtual address */
 	__u64	iova;				/* IO virtual address */
 	__u64	size;				/* Size of mapping (bytes) */
-- 
2.3.1

[PATCH v5 2/4] vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Baptiste Reynal]

From: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>

We introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag to the VFIO dma map call,
and expose its availability via the capability VFIO_DMA_NOEXEC_IOMMU.
This way the user can control whether the XN flag will be set on the
requested mappings. The IOMMU_NOEXEC flag needs to be available for all
the IOMMUs of the container used.

Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
Signed-off-by: Baptiste Reynal <b.reynal-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
---
 include/uapi/linux/vfio.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 5fb3d46..30801a7 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -31,6 +31,7 @@ enum vfio_iommu_cap {
 					   (ex. PCIe NoSnoop stripping) */
 	VFIO_EEH = 5,			/* Check if EEH is supported */
 	VFIO_TYPE1_NESTING_IOMMU = 6,	/* Two-stage IOMMU, implies v2  */
+	VFIO_DMA_NOEXEC_IOMMU = 7,
 };
 
 
@@ -397,12 +398,17 @@ struct vfio_iommu_type1_info {
  *
  * Map process virtual addresses to IO virtual addresses using the
  * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
+ *
+ * To use the VFIO_DMA_MAP_FLAG_NOEXEC flag, the container must support the
+ * VFIO_DMA_NOEXEC_IOMMU capability. If mappings are created using this flag,
+ * any groups subsequently added to the container must support this capability.
  */
 struct vfio_iommu_type1_dma_map {
 	__u32	argsz;
 	__u32	flags;
 #define VFIO_DMA_MAP_FLAG_READ (1 << 0)		/* readable from device */
 #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1)	/* writable from device */
+#define VFIO_DMA_MAP_FLAG_NOEXEC (1 << 2)	/* not executable from device */
 	__u64	vaddr;				/* Process virtual address */
 	__u64	iova;				/* IO virtual address */
 	__u64	size;				/* Size of mapping (bytes) */
-- 
2.3.1

[PATCH v5 3/4] vfio: type1: replace vfio_domains_have_iommu_cache with generic function

[Baptiste Reynal]

From: Antonios Motakis <a.motakis@virtualopensystems.com>

Replace the function vfio_domains_have_iommu_cache() with a more generic
function vfio_domains_have_iommu_cap() which allows to check all domains
of an vfio_iommu structure for a given cached capability.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/vfio_iommu_type1.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 57d8c37..a5847e8 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -82,6 +82,23 @@ struct vfio_group {
 	struct list_head	next;
 };
 
+static int vfio_domains_have_iommu_cap(struct vfio_iommu *iommu, int cap)
+{
+	struct vfio_domain *domain;
+	int ret = 1;
+
+	mutex_lock(&iommu->lock);
+	list_for_each_entry(domain, &iommu->domain_list, next) {
+		if (!(domain->domain->ops->capable(cap))) {
+			ret = 0;
+			break;
+		}
+	}
+	mutex_unlock(&iommu->lock);
+
+	return ret;
+}
+
 /*
  * This code handles mapping and unmapping of user data buffers
  * into DMA'ble space using the IOMMU
@@ -935,23 +952,6 @@ static void vfio_iommu_type1_release(void *iommu_data)
 	kfree(iommu);
 }
 
-static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
-{
-	struct vfio_domain *domain;
-	int ret = 1;
-
-	mutex_lock(&iommu->lock);
-	list_for_each_entry(domain, &iommu->domain_list, next) {
-		if (!(domain->prot & IOMMU_CACHE)) {
-			ret = 0;
-			break;
-		}
-	}
-	mutex_unlock(&iommu->lock);
-
-	return ret;
-}
-
 static long vfio_iommu_type1_ioctl(void *iommu_data,
 				   unsigned int cmd, unsigned long arg)
 {
@@ -967,7 +967,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 		case VFIO_DMA_CC_IOMMU:
 			if (!iommu)
 				return 0;
-			return vfio_domains_have_iommu_cache(iommu);
+			return vfio_domains_have_iommu_cap(iommu,
+						  IOMMU_CAP_CACHE_COHERENCY);
 		default:
 			return 0;
 		}
-- 
2.3.1

[PATCH v5 3/4] vfio: type1: replace vfio_domains_have_iommu_cache with generic function

[Baptiste Reynal]

From: Antonios Motakis <a.motakis@virtualopensystems.com>

Replace the function vfio_domains_have_iommu_cache() with a more generic
function vfio_domains_have_iommu_cap() which allows to check all domains
of an vfio_iommu structure for a given cached capability.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/vfio_iommu_type1.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 57d8c37..a5847e8 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -82,6 +82,23 @@ struct vfio_group {
 	struct list_head	next;
 };
 
+static int vfio_domains_have_iommu_cap(struct vfio_iommu *iommu, int cap)
+{
+	struct vfio_domain *domain;
+	int ret = 1;
+
+	mutex_lock(&iommu->lock);
+	list_for_each_entry(domain, &iommu->domain_list, next) {
+		if (!(domain->domain->ops->capable(cap))) {
+			ret = 0;
+			break;
+		}
+	}
+	mutex_unlock(&iommu->lock);
+
+	return ret;
+}
+
 /*
  * This code handles mapping and unmapping of user data buffers
  * into DMA'ble space using the IOMMU
@@ -935,23 +952,6 @@ static void vfio_iommu_type1_release(void *iommu_data)
 	kfree(iommu);
 }
 
-static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
-{
-	struct vfio_domain *domain;
-	int ret = 1;
-
-	mutex_lock(&iommu->lock);
-	list_for_each_entry(domain, &iommu->domain_list, next) {
-		if (!(domain->prot & IOMMU_CACHE)) {
-			ret = 0;
-			break;
-		}
-	}
-	mutex_unlock(&iommu->lock);
-
-	return ret;
-}
-
 static long vfio_iommu_type1_ioctl(void *iommu_data,
 				   unsigned int cmd, unsigned long arg)
 {
@@ -967,7 +967,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 		case VFIO_DMA_CC_IOMMU:
 			if (!iommu)
 				return 0;
-			return vfio_domains_have_iommu_cache(iommu);
+			return vfio_domains_have_iommu_cap(iommu,
+						  IOMMU_CAP_CACHE_COHERENCY);
 		default:
 			return 0;
 		}
-- 
2.3.1

[PATCH v5 4/4] vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Baptiste Reynal]

From: Antonios Motakis <a.motakis@virtualopensystems.com>

Some IOMMU drivers, such as the ARM SMMU driver, make available the
IOMMU_NOEXEC flag to set the page tables for a device as XN (execute never).
This affects devices such as the ARM PL330 DMA Controller, which respects
this flag and will refuse to fetch DMA instructions from memory where the
XN flag has been set.

The flag can be used only if all IOMMU domains behind the container support
the IOMMU_NOEXEC flag. Also, if any mappings are created with the flag, any
new domains with devices will have to support it as well.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/vfio_iommu_type1.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a5847e8..ec313e5 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -591,6 +591,12 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 	if (!prot || !size || (size | iova | vaddr) & mask)
 		return -EINVAL;
 
+	if (map->flags & VFIO_DMA_MAP_FLAG_NOEXEC) {
+		if (!vfio_domains_have_iommu_cap(iommu, IOMMU_CAP_NOEXEC))
+			return -EINVAL;
+		prot |= IOMMU_NOEXEC;
+	}
+
 	/* Don't allow IOVA or virtual address wrap */
 	if (iova + size - 1 < iova || vaddr + size - 1 < vaddr)
 		return -EINVAL;
@@ -672,11 +678,20 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 
 	for (; n; n = rb_next(n)) {
 		struct vfio_dma *dma;
+		const struct iommu_ops *ops = domain->domain->ops;
 		dma_addr_t iova;
 
 		dma = rb_entry(n, struct vfio_dma, node);
 		iova = dma->iova;
 
+		/*
+		 * if any of the mappings to be replayed has the NOEXEC flag
+		 * set, then the new iommu domain must support it
+		 */
+		if ((dma->prot & IOMMU_NOEXEC) &&
+				!(ops->capable(IOMMU_CAP_NOEXEC)))
+			return -EINVAL;
+
 		while (iova < dma->iova + dma->size) {
 			phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
 			size_t size;
@@ -969,6 +984,11 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 				return 0;
 			return vfio_domains_have_iommu_cap(iommu,
 						  IOMMU_CAP_CACHE_COHERENCY);
+		case VFIO_DMA_NOEXEC_IOMMU:
+			if (!iommu)
+				return 0;
+			return vfio_domains_have_iommu_cap(iommu,
+							   IOMMU_CAP_NOEXEC);
 		default:
 			return 0;
 		}
@@ -992,7 +1012,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 	} else if (cmd == VFIO_IOMMU_MAP_DMA) {
 		struct vfio_iommu_type1_dma_map map;
 		uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
-				VFIO_DMA_MAP_FLAG_WRITE;
+				VFIO_DMA_MAP_FLAG_WRITE |
+				VFIO_DMA_MAP_FLAG_NOEXEC;
 
 		minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
 
-- 
2.3.1

[PATCH v5 4/4] vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Baptiste Reynal]

From: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>

Some IOMMU drivers, such as the ARM SMMU driver, make available the
IOMMU_NOEXEC flag to set the page tables for a device as XN (execute never).
This affects devices such as the ARM PL330 DMA Controller, which respects
this flag and will refuse to fetch DMA instructions from memory where the
XN flag has been set.

The flag can be used only if all IOMMU domains behind the container support
the IOMMU_NOEXEC flag. Also, if any mappings are created with the flag, any
new domains with devices will have to support it as well.

Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
Signed-off-by: Baptiste Reynal <b.reynal-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
---
 drivers/vfio/vfio_iommu_type1.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a5847e8..ec313e5 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -591,6 +591,12 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 	if (!prot || !size || (size | iova | vaddr) & mask)
 		return -EINVAL;
 
+	if (map->flags & VFIO_DMA_MAP_FLAG_NOEXEC) {
+		if (!vfio_domains_have_iommu_cap(iommu, IOMMU_CAP_NOEXEC))
+			return -EINVAL;
+		prot |= IOMMU_NOEXEC;
+	}
+
 	/* Don't allow IOVA or virtual address wrap */
 	if (iova + size - 1 < iova || vaddr + size - 1 < vaddr)
 		return -EINVAL;
@@ -672,11 +678,20 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 
 	for (; n; n = rb_next(n)) {
 		struct vfio_dma *dma;
+		const struct iommu_ops *ops = domain->domain->ops;
 		dma_addr_t iova;
 
 		dma = rb_entry(n, struct vfio_dma, node);
 		iova = dma->iova;
 
+		/*
+		 * if any of the mappings to be replayed has the NOEXEC flag
+		 * set, then the new iommu domain must support it
+		 */
+		if ((dma->prot & IOMMU_NOEXEC) &&
+				!(ops->capable(IOMMU_CAP_NOEXEC)))
+			return -EINVAL;
+
 		while (iova < dma->iova + dma->size) {
 			phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
 			size_t size;
@@ -969,6 +984,11 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 				return 0;
 			return vfio_domains_have_iommu_cap(iommu,
 						  IOMMU_CAP_CACHE_COHERENCY);
+		case VFIO_DMA_NOEXEC_IOMMU:
+			if (!iommu)
+				return 0;
+			return vfio_domains_have_iommu_cap(iommu,
+							   IOMMU_CAP_NOEXEC);
 		default:
 			return 0;
 		}
@@ -992,7 +1012,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 	} else if (cmd == VFIO_IOMMU_MAP_DMA) {
 		struct vfio_iommu_type1_dma_map map;
 		uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
-				VFIO_DMA_MAP_FLAG_WRITE;
+				VFIO_DMA_MAP_FLAG_WRITE |
+				VFIO_DMA_MAP_FLAG_NOEXEC;
 
 		minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
 
-- 
2.3.1

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Alex Williamson]

On Wed, 2015-03-04 at 17:07 +0100, Baptiste Reynal wrote:
> This patch series makes the VFIO_IOMMU_TYPE1 driver buildable on ARM, so it
> may be used with ARM SMMUs. It also adds support for the IOMMU_NOEXEC flag
> supported by SMMUs adhering to the ARM SMMU specification so the VFIO user can
> specify whether the target memory can be executed by the device behind the
> SMMU.

Ok, so the differences between IOMMU_CACHE and IOMMU_NOEXEC are twofold.
First is that when the IOMMU is IOMMU_CAP_CACHE_COHERENCY we always use
IOMMU_CACHE for all mappings within the domain.  This is not a user
provided mapping flag.  Second, if a device is added behind an IOMMU
that is not IOMMU_CAP_CACHE_COHERENCY capable, this simply means that it
needs to be managed as a separate domain within the same container.
vfio_domains_have_iommu_cache() reports true only if all domains within
the container support, and are therefore using, IOMMU_CACHE.

On the other hand, IOMMU_NOEXEC is a feature, but not a global mapping
flag for a domain.  There are potentially some mappings with it within a
domain and some without, just like the IOMMU_READ/IOMMU_WRITE.  It's
therefore a per-DMA mapping flag as you've implemented in patch 4.
Second, as a user specified flag, support for it is mandatory.  If a
device is added behind an IOMMU that does not support NOEXEC and NOEXEC
mappings are in use, the device must be rejected from addition to the
container.

Hopefully that's correct, because I think that's what the code does.
The problem I have with the code is that I don't think it's acceptable
to call iommu_ops.capable() directly.  This is breaking an abstraction
of the IOMMU API.  The obvious problem with that is:

bool iommu_capable(struct bus_type *bus, enum iommu_cap cap)

I assume you're calling the op directly because of bus_type, but that's
no excuse to misuse the API.  Our choice is either to try to extend the
API, maybe create:

bool iommu_domain_capable(struct iommu_domain *domain, enum iommu_cap cap)

Or to use the available function by either caching the value when we do
have a bus_type available to us, or storing or discovering bus_type.  It
doesn't seem that terrible to cache it on the domain.  Be careful though
that if a device imposing lack of NOEXEC on a domain is removed, the
ability to do NOEXEC mappings should be reinstated.  That would be easy
if we use separate vfio_domains to manage NOEXEC capable vs NOEXEC
in-capable devices (like we do CACHE vs no-CACHE), but there's some
additional overhead to do that since it implies a separate iommu_domain.
Thanks,

Alex

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Baptiste Reynal]

On Wed, Mar 4, 2015 at 6:45 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Wed, 2015-03-04 at 17:07 +0100, Baptiste Reynal wrote:
>> This patch series makes the VFIO_IOMMU_TYPE1 driver buildable on ARM, so it
>> may be used with ARM SMMUs. It also adds support for the IOMMU_NOEXEC flag
>> supported by SMMUs adhering to the ARM SMMU specification so the VFIO user can
>> specify whether the target memory can be executed by the device behind the
>> SMMU.
>
> Ok, so the differences between IOMMU_CACHE and IOMMU_NOEXEC are twofold.
> First is that when the IOMMU is IOMMU_CAP_CACHE_COHERENCY we always use
> IOMMU_CACHE for all mappings within the domain.  This is not a user
> provided mapping flag.  Second, if a device is added behind an IOMMU
> that is not IOMMU_CAP_CACHE_COHERENCY capable, this simply means that it
> needs to be managed as a separate domain within the same container.
> vfio_domains_have_iommu_cache() reports true only if all domains within
> the container support, and are therefore using, IOMMU_CACHE.
>
> On the other hand, IOMMU_NOEXEC is a feature, but not a global mapping
> flag for a domain.  There are potentially some mappings with it within a
> domain and some without, just like the IOMMU_READ/IOMMU_WRITE.  It's
> therefore a per-DMA mapping flag as you've implemented in patch 4.
> Second, as a user specified flag, support for it is mandatory.  If a
> device is added behind an IOMMU that does not support NOEXEC and NOEXEC
> mappings are in use, the device must be rejected from addition to the
> container.

Thanks for taking time to explain. That is also my understanding on those flags.

>
> Hopefully that's correct, because I think that's what the code does.
> The problem I have with the code is that I don't think it's acceptable
> to call iommu_ops.capable() directly.  This is breaking an abstraction
> of the IOMMU API.  The obvious problem with that is:
>
> bool iommu_capable(struct bus_type *bus, enum iommu_cap cap)
>
> I assume you're calling the op directly because of bus_type, but that's
> no excuse to misuse the API.  Our choice is either to try to extend the
> API, maybe create:
>
> bool iommu_domain_capable(struct iommu_domain *domain, enum iommu_cap cap)
>
> Or to use the available function by either caching the value when we do
> have a bus_type available to us, or storing or discovering bus_type.  It
> doesn't seem that terrible to cache it on the domain.  Be careful though
> that if a device imposing lack of NOEXEC on a domain is removed, the
> ability to do NOEXEC mappings should be reinstated.  That would be easy
> if we use separate vfio_domains to manage NOEXEC capable vs NOEXEC
> in-capable devices (like we do CACHE vs no-CACHE), but there's some
> additional overhead to do that since it implies a separate iommu_domain.
> Thanks,
>
> Alex
>

I understand the issue on the API here. I will go for extending the
iommu API with iommu_domain_capable, except if you have any reason to
push for the second solution.

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Eric Auger]

Hi All,

Ironically, since the correction of the IOMMU_CAP_CACHE_COHERENCY bug
(https://lkml.org/lkml/2015/1/29/514) in vfio_iommu_type1.c, my Calxeda
Midway VFIO use case is not working anymore. This is also observable
when I do not apply at all the whole [PATCH v5 0/4] vfio: type1: support
for ARM SMMUS with VFIO_IOMMU_TYPE1 series.

My understanding is this series should not be requested for me since my
xgmac device does not care about the XN attribute.

My understanding is that without the bug or the series, the IOMMU_CACHE
flag is set whereas before it was not. Patching vfio_iommu_type1.c
vfio_iommu_type1_attach_group and unsetting IOMMU_CAP_CACHE_COHERENCY
makes the use case functional again.

I do not understand the exact semantic of IOMMU_CAP_CACHE_COHERENCY. I
see the arm smmu always returns true, and if my understanding is correct
the vfio_iommu_type1 sets the IOMMU_CACHE attribute which eventually
make the ARM SMMU sets the memory attribute to cacheable in the PTE. But
naively I would say the interco used in Midway may not be cache coherent
capability (ARM ACE, ACE-lite), hence the issue.

Please could you share your knowledge/understanding about this topic.
Adding Will in to ;-)

Thank you in advance

Best Regards

Eric



On 03/04/2015 06:45 PM, Alex Williamson wrote:
> On Wed, 2015-03-04 at 17:07 +0100, Baptiste Reynal wrote:
>> This patch series makes the VFIO_IOMMU_TYPE1 driver buildable on ARM, so it
>> may be used with ARM SMMUs. It also adds support for the IOMMU_NOEXEC flag
>> supported by SMMUs adhering to the ARM SMMU specification so the VFIO user can
>> specify whether the target memory can be executed by the device behind the
>> SMMU.
> 
> Ok, so the differences between IOMMU_CACHE and IOMMU_NOEXEC are twofold.
> First is that when the IOMMU is IOMMU_CAP_CACHE_COHERENCY we always use
> IOMMU_CACHE for all mappings within the domain.  This is not a user
> provided mapping flag.  Second, if a device is added behind an IOMMU
> that is not IOMMU_CAP_CACHE_COHERENCY capable, this simply means that it
> needs to be managed as a separate domain within the same container.
> vfio_domains_have_iommu_cache() reports true only if all domains within
> the container support, and are therefore using, IOMMU_CACHE.
> 
> On the other hand, IOMMU_NOEXEC is a feature, but not a global mapping
> flag for a domain.  There are potentially some mappings with it within a
> domain and some without, just like the IOMMU_READ/IOMMU_WRITE.  It's
> therefore a per-DMA mapping flag as you've implemented in patch 4.
> Second, as a user specified flag, support for it is mandatory.  If a
> device is added behind an IOMMU that does not support NOEXEC and NOEXEC
> mappings are in use, the device must be rejected from addition to the
> container.
> 
> Hopefully that's correct, because I think that's what the code does.
> The problem I have with the code is that I don't think it's acceptable
> to call iommu_ops.capable() directly.  This is breaking an abstraction
> of the IOMMU API.  The obvious problem with that is:
> 
> bool iommu_capable(struct bus_type *bus, enum iommu_cap cap)
> 
> I assume you're calling the op directly because of bus_type, but that's
> no excuse to misuse the API.  Our choice is either to try to extend the
> API, maybe create:
> 
> bool iommu_domain_capable(struct iommu_domain *domain, enum iommu_cap cap)
> 
> Or to use the available function by either caching the value when we do
> have a bus_type available to us, or storing or discovering bus_type.  It
> doesn't seem that terrible to cache it on the domain.  Be careful though
> that if a device imposing lack of NOEXEC on a domain is removed, the
> ability to do NOEXEC mappings should be reinstated.  That would be easy
> if we use separate vfio_domains to manage NOEXEC capable vs NOEXEC
> in-capable devices (like we do CACHE vs no-CACHE), but there's some
> additional overhead to do that since it implies a separate iommu_domain.
> Thanks,
> 
> Alex
> 

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Alex Williamson]

On Thu, 2015-03-05 at 18:34 +0100, Eric Auger wrote:
> Hi All,
> 
> Ironically, since the correction of the IOMMU_CAP_CACHE_COHERENCY bug
> (https://lkml.org/lkml/2015/1/29/514) in vfio_iommu_type1.c, my Calxeda
> Midway VFIO use case is not working anymore. This is also observable
> when I do not apply at all the whole [PATCH v5 0/4] vfio: type1: support
> for ARM SMMUS with VFIO_IOMMU_TYPE1 series.
> 
> My understanding is this series should not be requested for me since my
> xgmac device does not care about the XN attribute.
> 
> My understanding is that without the bug or the series, the IOMMU_CACHE
> flag is set whereas before it was not. Patching vfio_iommu_type1.c
> vfio_iommu_type1_attach_group and unsetting IOMMU_CAP_CACHE_COHERENCY
> makes the use case functional again.
> 
> I do not understand the exact semantic of IOMMU_CAP_CACHE_COHERENCY. I
> see the arm smmu always returns true, and if my understanding is correct
> the vfio_iommu_type1 sets the IOMMU_CACHE attribute which eventually
> make the ARM SMMU sets the memory attribute to cacheable in the PTE. But
> naively I would say the interco used in Midway may not be cache coherent
> capability (ARM ACE, ACE-lite), hence the issue.
> 
> Please could you share your knowledge/understanding about this topic.
> Adding Will in to ;-)

This patch series should change nothing about how the IOMMU_CACHE
mapping flag is used, it was a bug in the enum to bitmap translation
that broke/fixed your use case.  VFIO type1 IOMMU always uses
IOMMU_CACHE when available.  On x86 the effect is that DMA is coherent
with the processor cache (ie. the PCIe NoSnoop attribute is ignored).
This means that KVM can continue to ignore certain cache operations,
like writeback-invalidate (WBINVD), that would otherwise need to be
emulated.  We may need a separate flag and capability if ARM SMMU is
doing something different with the flag.  Thanks,

Alex

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Will Deacon]

On Thu, Mar 05, 2015 at 05:54:26PM +0000, Alex Williamson wrote:
> On Thu, 2015-03-05 at 18:34 +0100, Eric Auger wrote:
> > Ironically, since the correction of the IOMMU_CAP_CACHE_COHERENCY bug
> > (https://lkml.org/lkml/2015/1/29/514) in vfio_iommu_type1.c, my Calxeda
> > Midway VFIO use case is not working anymore. This is also observable
> > when I do not apply at all the whole [PATCH v5 0/4] vfio: type1: support
> > for ARM SMMUS with VFIO_IOMMU_TYPE1 series.
> > 
> > My understanding is this series should not be requested for me since my
> > xgmac device does not care about the XN attribute.
> > 
> > My understanding is that without the bug or the series, the IOMMU_CACHE
> > flag is set whereas before it was not. Patching vfio_iommu_type1.c
> > vfio_iommu_type1_attach_group and unsetting IOMMU_CAP_CACHE_COHERENCY
> > makes the use case functional again.
> > 
> > I do not understand the exact semantic of IOMMU_CAP_CACHE_COHERENCY. I
> > see the arm smmu always returns true, and if my understanding is correct
> > the vfio_iommu_type1 sets the IOMMU_CACHE attribute which eventually
> > make the ARM SMMU sets the memory attribute to cacheable in the PTE. But
> > naively I would say the interco used in Midway may not be cache coherent
> > capability (ARM ACE, ACE-lite), hence the issue.
> > 
> > Please could you share your knowledge/understanding about this topic.
> > Adding Will in to ;-)
> 
> This patch series should change nothing about how the IOMMU_CACHE
> mapping flag is used, it was a bug in the enum to bitmap translation
> that broke/fixed your use case.  VFIO type1 IOMMU always uses
> IOMMU_CACHE when available.  On x86 the effect is that DMA is coherent
> with the processor cache (ie. the PCIe NoSnoop attribute is ignored).
> This means that KVM can continue to ignore certain cache operations,
> like writeback-invalidate (WBINVD), that would otherwise need to be
> emulated.  We may need a separate flag and capability if ARM SMMU is
> doing something different with the flag.  Thanks,

On ARM, coherency isn't really a property of the SMMU. Sure, the SMMU can
emit cacheable transactions (which is what the capability basically means),
but whether or not they snoop the CPU caches depends on the system
configuration. I'd expect the "dma-coherent" property of the *master* to
indicate whether that is the case.

Will

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Alex Williamson]

On Thu, 2015-03-05 at 18:34 +0100, Eric Auger wrote:
> Hi All,
> 
> Ironically, since the correction of the IOMMU_CAP_CACHE_COHERENCY bug
> (https://lkml.org/lkml/2015/1/29/514) in vfio_iommu_type1.c, my Calxeda
> Midway VFIO use case is not working anymore. This is also observable
> when I do not apply at all the whole [PATCH v5 0/4] vfio: type1: support
> for ARM SMMUS with VFIO_IOMMU_TYPE1 series.
> 
> My understanding is this series should not be requested for me since my
> xgmac device does not care about the XN attribute.

This also raises the concern that we're continuing to put optional
features as prerequisites to base vfio-platform support and creating
self-induced stalls at getting anything upstream :-\  Thanks,

Alex

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Eric Auger]

On 03/05/2015 10:11 PM, Alex Williamson wrote:
> On Thu, 2015-03-05 at 18:34 +0100, Eric Auger wrote:
>> Hi All,
>>
>> Ironically, since the correction of the IOMMU_CAP_CACHE_COHERENCY bug
>> (https://lkml.org/lkml/2015/1/29/514) in vfio_iommu_type1.c, my Calxeda
>> Midway VFIO use case is not working anymore. This is also observable
>> when I do not apply at all the whole [PATCH v5 0/4] vfio: type1: support
>> for ARM SMMUS with VFIO_IOMMU_TYPE1 series.
>>
>> My understanding is this series should not be requested for me since my
>> xgmac device does not care about the XN attribute.
> 
> This also raises the concern that we're continuing to put optional
> features as prerequisites to base vfio-platform support and creating
> self-induced stalls at getting anything upstream :-\  Thanks,
Dear all,

Thanks Alex, Will for your inputs on this topic.

Yes to me "vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1" is
not a direct dependency of vfio-platform driver although I understand
Baptiste needs it to test with PL330 device. On my side I can test
vfio-platform driver without it, assigning the Midway Calxeda xgmac.

Also if I am not wrong the title of the series now does not really
reflect anymore what the series aims at. since "[PATCH v3 2/6] vfio:
type1: support for ARM SMMUs"  withdrawal, the series now "only" aims at
supporting the VFIO_DMA_MAP_FLAG_NOEXEC flag.

However with this issue of IOMMU_CACHE always set along with arm-smmu,
there is a need for an adaptation of either vfio_iommu_type1 or arm-smmu
since the integrated pieces are not functional.

On top of the dma-coherent property of the *master*, should not we also
query the cache-coherent property of the interconnect downstream to the
smmu?

How can we progress quickly on this topic? is it acceptable to return
false on arm_smmu_capable(IOMMU_CAP_CACHE_COHERENCY) as a quick hack? As
a longer term solution, would it make sense to add a user flag at VFIO
user API level to turn the IOMMU_CACHE on?

Best Regards

Eric


Best Regards

Eric

Best Regards

Eric
> 
> Alex
> 

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Will Deacon]

Hi Eric,

On Fri, Mar 06, 2015 at 09:04:22AM +0000, Eric Auger wrote:
> Yes to me "vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1" is
> not a direct dependency of vfio-platform driver although I understand
> Baptiste needs it to test with PL330 device. On my side I can test
> vfio-platform driver without it, assigning the Midway Calxeda xgmac.

Realistically, I don't expect anybody using device passthrough for
practical applications to be using a PL330. Your xgmac example is far
more compelling.

> Also if I am not wrong the title of the series now does not really
> reflect anymore what the series aims at. since "[PATCH v3 2/6] vfio:
> type1: support for ARM SMMUs"  withdrawal, the series now "only" aims at
> supporting the VFIO_DMA_MAP_FLAG_NOEXEC flag.
> 
> However with this issue of IOMMU_CACHE always set along with arm-smmu,
> there is a need for an adaptation of either vfio_iommu_type1 or arm-smmu
> since the integrated pieces are not functional.

Well, I'm still trying to understand exactly what is happening in your case:

  - Is the xgmac coherent or not? Does it have a "dma-coherent" property?
  - Are you installing the SMMU page tables at stage-1 or stage-2?
  - If it *is* coherent, then we should use IOMMU_CACHE mappings for the
    DMA buffers and ensure that the guest knows it is coherent (by
    preserving the "dma-coherent" flag).
  - If it is *not* coherent, then the behaviour of IOMMU_CACHE depends
    on the stage of translation:

    * Stage-1: we will make the transactions cacheable, and you'll need
      to tell the guest that the device is actually cache coherent
    * Stage-2: IOMMU_CACHE won't actually have any effect, so everything
      should work as non-coherent.

In other words, I think you're probably just telling the guest the wrong
thing.

> On top of the dma-coherent property of the *master*, should not we also
> query the cache-coherent property of the interconnect downstream to the
> smmu?

I don't think so... "dma-coherent" should only be set on a master if the
interconnect is properly configured. It's supposed to be a "If you DMA now,
it will snoop the CPU caches" flag as opposed to "If you write a random
selection of MMIO registers in the SoC, then this device will be coherent".

> How can we progress quickly on this topic? is it acceptable to return
> false on arm_smmu_capable(IOMMU_CAP_CACHE_COHERENCY) as a quick hack?

No; pretending that a device is not coherent when it actually is can lead
to corruption of DMA buffers due to unnecessary cache invalidation.

> As a longer term solution, would it make sense to add a user flag at VFIO
> user API level to turn the IOMMU_CACHE on?

I think userspace certainly needs a way to figure out if a device is
coherent or not, otherwise it can't generate the correct device-tree
properties for something like a KVM guest but the IOMMU_* setting should
remain in the kernel IMO. Similarly for things like MSI pages, which would
need to be mapped as device memory on ARM -- that should be exposed as a
higher level "please map my MSI page here" ioctl as opposed to requiring
userspace to supply the correct memory attributes.

Will

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Baptiste Reynal]

Hi everyone,

Indeed, the NOEXEC flag is needed for our tests and VFIO should work
without it for some other devices (including xgmac). I think it is
reasonable to drop this patch serie for now.

Still we have the IOMMU_CACHE issue. To answer Will, the SMMU page
tables are installed at stage-2. Currently, the dma-coherent flag is
not preserved to the guest, that may be the issue (As we don't have it
in the host in our case). "[RFC PATCH v3 0/3] vfio: platform: return
device properties for a platform device"
(http://lists.celinuxforum.org/pipermail/iommu/2014-December/011425.html)
may be a solution to figure out if the device is coherent or not.


On Fri, Mar 6, 2015 at 11:41 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Eric,
>
> On Fri, Mar 06, 2015 at 09:04:22AM +0000, Eric Auger wrote:
>> Yes to me "vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1" is
>> not a direct dependency of vfio-platform driver although I understand
>> Baptiste needs it to test with PL330 device. On my side I can test
>> vfio-platform driver without it, assigning the Midway Calxeda xgmac.
>
> Realistically, I don't expect anybody using device passthrough for
> practical applications to be using a PL330. Your xgmac example is far
> more compelling.
>
>> Also if I am not wrong the title of the series now does not really
>> reflect anymore what the series aims at. since "[PATCH v3 2/6] vfio:
>> type1: support for ARM SMMUs"  withdrawal, the series now "only" aims at
>> supporting the VFIO_DMA_MAP_FLAG_NOEXEC flag.
>>
>> However with this issue of IOMMU_CACHE always set along with arm-smmu,
>> there is a need for an adaptation of either vfio_iommu_type1 or arm-smmu
>> since the integrated pieces are not functional.
>
> Well, I'm still trying to understand exactly what is happening in your case:
>
>   - Is the xgmac coherent or not? Does it have a "dma-coherent" property?
>   - Are you installing the SMMU page tables at stage-1 or stage-2?
>   - If it *is* coherent, then we should use IOMMU_CACHE mappings for the
>     DMA buffers and ensure that the guest knows it is coherent (by
>     preserving the "dma-coherent" flag).
>   - If it is *not* coherent, then the behaviour of IOMMU_CACHE depends
>     on the stage of translation:
>
>     * Stage-1: we will make the transactions cacheable, and you'll need
>       to tell the guest that the device is actually cache coherent
>     * Stage-2: IOMMU_CACHE won't actually have any effect, so everything
>       should work as non-coherent.
>
> In other words, I think you're probably just telling the guest the wrong
> thing.
>
>> On top of the dma-coherent property of the *master*, should not we also
>> query the cache-coherent property of the interconnect downstream to the
>> smmu?
>
> I don't think so... "dma-coherent" should only be set on a master if the
> interconnect is properly configured. It's supposed to be a "If you DMA now,
> it will snoop the CPU caches" flag as opposed to "If you write a random
> selection of MMIO registers in the SoC, then this device will be coherent".
>
>> How can we progress quickly on this topic? is it acceptable to return
>> false on arm_smmu_capable(IOMMU_CAP_CACHE_COHERENCY) as a quick hack?
>
> No; pretending that a device is not coherent when it actually is can lead
> to corruption of DMA buffers due to unnecessary cache invalidation.
>
>> As a longer term solution, would it make sense to add a user flag at VFIO
>> user API level to turn the IOMMU_CACHE on?
>
> I think userspace certainly needs a way to figure out if a device is
> coherent or not, otherwise it can't generate the correct device-tree
> properties for something like a KVM guest but the IOMMU_* setting should
> remain in the kernel IMO. Similarly for things like MSI pages, which would
> need to be mapped as device memory on ARM -- that should be exposed as a
> higher level "please map my MSI page here" ioctl as opposed to requiring
> userspace to supply the correct memory attributes.
>
> Will

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Eric Auger]

Hi All,

On 03/06/2015 12:27 PM, Baptiste Reynal wrote:
> Hi everyone,
> 
> Indeed, the NOEXEC flag is needed for our tests and VFIO should work
> without it for some other devices (including xgmac). I think it is
> reasonable to drop this patch serie for now.
> 
> Still we have the IOMMU_CACHE issue. To answer Will, the SMMU page
> tables are installed at stage-2. Currently, the dma-coherent flag is
> not preserved to the guest, that may be the issue (As we don't have it
> in the host in our case). "[RFC PATCH v3 0/3] vfio: platform: return
> device properties for a platform device"
> (http://lists.celinuxforum.org/pipermail/iommu/2014-December/011425.html)
> may be a solution to figure out if the device is coherent or not.
> 
> 
> On Fri, Mar 6, 2015 at 11:41 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>> Hi Eric,
>>
>> On Fri, Mar 06, 2015 at 09:04:22AM +0000, Eric Auger wrote:
>>> Yes to me "vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1" is
>>> not a direct dependency of vfio-platform driver although I understand
>>> Baptiste needs it to test with PL330 device. On my side I can test
>>> vfio-platform driver without it, assigning the Midway Calxeda xgmac.
>>
>> Realistically, I don't expect anybody using device passthrough for
>> practical applications to be using a PL330. Your xgmac example is far
>> more compelling.
>>
>>> Also if I am not wrong the title of the series now does not really
>>> reflect anymore what the series aims at. since "[PATCH v3 2/6] vfio:
>>> type1: support for ARM SMMUs"  withdrawal, the series now "only" aims at
>>> supporting the VFIO_DMA_MAP_FLAG_NOEXEC flag.
>>>
>>> However with this issue of IOMMU_CACHE always set along with arm-smmu,
>>> there is a need for an adaptation of either vfio_iommu_type1 or arm-smmu
>>> since the integrated pieces are not functional.
>>
>> Well, I'm still trying to understand exactly what is happening in your case:
>>
>>   - Is the xgmac coherent or not? Does it have a "dma-coherent" property?
Yes it has the dma-coherent property
>>   - Are you installing the SMMU page tables at stage-1 or stage-2?
stage2
>>   - If it *is* coherent, then we should use IOMMU_CACHE mappings for the
>>     DMA buffers and ensure that the guest knows it is coherent (by
>>     preserving the "dma-coherent" flag).
indeed that's the point then.
>>   - If it is *not* coherent, then the behaviour of IOMMU_CACHE depends
>>     on the stage of translation:
>>
>>     * Stage-1: we will make the transactions cacheable, and you'll need
>>       to tell the guest that the device is actually cache coherent
>>     * Stage-2: IOMMU_CACHE won't actually have any effect, so everything
>>       should work as non-coherent.
>>
>> In other words, I think you're probably just telling the guest the wrong
>> thing.
>>
>>> On top of the dma-coherent property of the *master*, should not we also
>>> query the cache-coherent property of the interconnect downstream to the
>>> smmu?
>>
>> I don't think so... "dma-coherent" should only be set on a master if the
>> interconnect is properly configured. It's supposed to be a "If you DMA now,
>> it will snoop the CPU caches" flag as opposed to "If you write a random
>> selection of MMIO registers in the SoC, then this device will be coherent".
OK
>>
>>> How can we progress quickly on this topic? is it acceptable to return
>>> false on arm_smmu_capable(IOMMU_CAP_CACHE_COHERENCY) as a quick hack?
>>
>> No; pretending that a device is not coherent when it actually is can lead
>> to corruption of DMA buffers due to unnecessary cache invalidation.
>>
>>> As a longer term solution, would it make sense to add a user flag at VFIO
>>> user API level to turn the IOMMU_CACHE on?
>>
>> I think userspace certainly needs a way to figure out if a device is
>> coherent or not, otherwise it can't generate the correct device-tree
>> properties for something like a KVM guest but the IOMMU_* setting should
>> remain in the kernel IMO. Similarly for things like MSI pages, which would
>> need to be mapped as device memory on ARM -- that should be exposed as a
>> higher level "please map my MSI page here" ioctl as opposed to requiring
>> userspace to supply the correct memory attributes.

Many thanks for the explanations.

Best Regards

Eric
>>
>> Will

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Eric Auger]

On 03/06/2015 12:27 PM, Baptiste Reynal wrote:
> Hi everyone,
> 
> Indeed, the NOEXEC flag is needed for our tests and VFIO should work
> without it for some other devices (including xgmac). I think it is
> reasonable to drop this patch serie for now.
> 
> Still we have the IOMMU_CACHE issue. To answer Will, the SMMU page
> tables are installed at stage-2. Currently, the dma-coherent flag is
> not preserved to the guest, that may be the issue (As we don't have it
> in the host in our case). "[RFC PATCH v3 0/3] vfio: platform: return
> device properties for a platform device"
> (http://lists.celinuxforum.org/pipermail/iommu/2014-December/011425.html)
> may be a solution to figure out if the device is coherent or not.

I confirm the problem is fixed with proper guest device-tree. Sorry for
the noise and thanks for your support. indeed in the future the above
series will help in getting the info dynamically.

Best Regards

Eric
> 
> 
> On Fri, Mar 6, 2015 at 11:41 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>> Hi Eric,
>>
>> On Fri, Mar 06, 2015 at 09:04:22AM +0000, Eric Auger wrote:
>>> Yes to me "vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1" is
>>> not a direct dependency of vfio-platform driver although I understand
>>> Baptiste needs it to test with PL330 device. On my side I can test
>>> vfio-platform driver without it, assigning the Midway Calxeda xgmac.
>>
>> Realistically, I don't expect anybody using device passthrough for
>> practical applications to be using a PL330. Your xgmac example is far
>> more compelling.
>>
>>> Also if I am not wrong the title of the series now does not really
>>> reflect anymore what the series aims at. since "[PATCH v3 2/6] vfio:
>>> type1: support for ARM SMMUs"  withdrawal, the series now "only" aims at
>>> supporting the VFIO_DMA_MAP_FLAG_NOEXEC flag.
>>>
>>> However with this issue of IOMMU_CACHE always set along with arm-smmu,
>>> there is a need for an adaptation of either vfio_iommu_type1 or arm-smmu
>>> since the integrated pieces are not functional.
>>
>> Well, I'm still trying to understand exactly what is happening in your case:
>>
>>   - Is the xgmac coherent or not? Does it have a "dma-coherent" property?
>>   - Are you installing the SMMU page tables at stage-1 or stage-2?
>>   - If it *is* coherent, then we should use IOMMU_CACHE mappings for the
>>     DMA buffers and ensure that the guest knows it is coherent (by
>>     preserving the "dma-coherent" flag).
>>   - If it is *not* coherent, then the behaviour of IOMMU_CACHE depends
>>     on the stage of translation:
>>
>>     * Stage-1: we will make the transactions cacheable, and you'll need
>>       to tell the guest that the device is actually cache coherent
>>     * Stage-2: IOMMU_CACHE won't actually have any effect, so everything
>>       should work as non-coherent.
>>
>> In other words, I think you're probably just telling the guest the wrong
>> thing.
>>
>>> On top of the dma-coherent property of the *master*, should not we also
>>> query the cache-coherent property of the interconnect downstream to the
>>> smmu?
>>
>> I don't think so... "dma-coherent" should only be set on a master if the
>> interconnect is properly configured. It's supposed to be a "If you DMA now,
>> it will snoop the CPU caches" flag as opposed to "If you write a random
>> selection of MMIO registers in the SoC, then this device will be coherent".
>>
>>> How can we progress quickly on this topic? is it acceptable to return
>>> false on arm_smmu_capable(IOMMU_CAP_CACHE_COHERENCY) as a quick hack?
>>
>> No; pretending that a device is not coherent when it actually is can lead
>> to corruption of DMA buffers due to unnecessary cache invalidation.
>>
>>> As a longer term solution, would it make sense to add a user flag at VFIO
>>> user API level to turn the IOMMU_CACHE on?
>>
>> I think userspace certainly needs a way to figure out if a device is
>> coherent or not, otherwise it can't generate the correct device-tree
>> properties for something like a KVM guest but the IOMMU_* setting should
>> remain in the kernel IMO. Similarly for things like MSI pages, which would
>> need to be mapped as device memory on ARM -- that should be exposed as a
>> higher level "please map my MSI page here" ioctl as opposed to requiring
>> userspace to supply the correct memory attributes.
>>
>> Will

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Baptiste Reynal]

Good news Eric !

Here are more information about the NOEXEC flag. This flag is used to
set the XN flag on the PTE, which make the page non-executable (XN is
set if NOEXEC is set).

As it used to be an EXEC flag (XN was set by default, unset if EXEC
was set), this patch serie was mandatory for PL330 test, since it
executes some code from memory. Now that it has been reversed to
NOEXEC (XN is not set by default), every page is executable, so it
works but there may be a security issue.

I think it is better to have it if it is working, but for
simplification we can add it later (as the requirement dropped when
the flag has been reversed).

Best regards,
Baptiste


On Fri, Mar 6, 2015 at 2:17 PM, Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 03/06/2015 12:27 PM, Baptiste Reynal wrote:
>> Hi everyone,
>>
>> Indeed, the NOEXEC flag is needed for our tests and VFIO should work
>> without it for some other devices (including xgmac). I think it is
>> reasonable to drop this patch serie for now.
>>
>> Still we have the IOMMU_CACHE issue. To answer Will, the SMMU page
>> tables are installed at stage-2. Currently, the dma-coherent flag is
>> not preserved to the guest, that may be the issue (As we don't have it
>> in the host in our case). "[RFC PATCH v3 0/3] vfio: platform: return
>> device properties for a platform device"
>> (http://lists.celinuxforum.org/pipermail/iommu/2014-December/011425.html)
>> may be a solution to figure out if the device is coherent or not.
>
> I confirm the problem is fixed with proper guest device-tree. Sorry for
> the noise and thanks for your support. indeed in the future the above
> series will help in getting the info dynamically.
>
> Best Regards
>
> Eric
>>
>>
>> On Fri, Mar 6, 2015 at 11:41 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>>> Hi Eric,
>>>
>>> On Fri, Mar 06, 2015 at 09:04:22AM +0000, Eric Auger wrote:
>>>> Yes to me "vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1" is
>>>> not a direct dependency of vfio-platform driver although I understand
>>>> Baptiste needs it to test with PL330 device. On my side I can test
>>>> vfio-platform driver without it, assigning the Midway Calxeda xgmac.
>>>
>>> Realistically, I don't expect anybody using device passthrough for
>>> practical applications to be using a PL330. Your xgmac example is far
>>> more compelling.
>>>
>>>> Also if I am not wrong the title of the series now does not really
>>>> reflect anymore what the series aims at. since "[PATCH v3 2/6] vfio:
>>>> type1: support for ARM SMMUs"  withdrawal, the series now "only" aims at
>>>> supporting the VFIO_DMA_MAP_FLAG_NOEXEC flag.
>>>>
>>>> However with this issue of IOMMU_CACHE always set along with arm-smmu,
>>>> there is a need for an adaptation of either vfio_iommu_type1 or arm-smmu
>>>> since the integrated pieces are not functional.
>>>
>>> Well, I'm still trying to understand exactly what is happening in your case:
>>>
>>>   - Is the xgmac coherent or not? Does it have a "dma-coherent" property?
>>>   - Are you installing the SMMU page tables at stage-1 or stage-2?
>>>   - If it *is* coherent, then we should use IOMMU_CACHE mappings for the
>>>     DMA buffers and ensure that the guest knows it is coherent (by
>>>     preserving the "dma-coherent" flag).
>>>   - If it is *not* coherent, then the behaviour of IOMMU_CACHE depends
>>>     on the stage of translation:
>>>
>>>     * Stage-1: we will make the transactions cacheable, and you'll need
>>>       to tell the guest that the device is actually cache coherent
>>>     * Stage-2: IOMMU_CACHE won't actually have any effect, so everything
>>>       should work as non-coherent.
>>>
>>> In other words, I think you're probably just telling the guest the wrong
>>> thing.
>>>
>>>> On top of the dma-coherent property of the *master*, should not we also
>>>> query the cache-coherent property of the interconnect downstream to the
>>>> smmu?
>>>
>>> I don't think so... "dma-coherent" should only be set on a master if the
>>> interconnect is properly configured. It's supposed to be a "If you DMA now,
>>> it will snoop the CPU caches" flag as opposed to "If you write a random
>>> selection of MMIO registers in the SoC, then this device will be coherent".
>>>
>>>> How can we progress quickly on this topic? is it acceptable to return
>>>> false on arm_smmu_capable(IOMMU_CAP_CACHE_COHERENCY) as a quick hack?
>>>
>>> No; pretending that a device is not coherent when it actually is can lead
>>> to corruption of DMA buffers due to unnecessary cache invalidation.
>>>
>>>> As a longer term solution, would it make sense to add a user flag at VFIO
>>>> user API level to turn the IOMMU_CACHE on?
>>>
>>> I think userspace certainly needs a way to figure out if a device is
>>> coherent or not, otherwise it can't generate the correct device-tree
>>> properties for something like a KVM guest but the IOMMU_* setting should
>>> remain in the kernel IMO. Similarly for things like MSI pages, which would
>>> need to be mapped as device memory on ARM -- that should be exposed as a
>>> higher level "please map my MSI page here" ioctl as opposed to requiring
>>> userspace to supply the correct memory attributes.
>>>
>>> Will
>

Re: [PATCH v5 0/4] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Alex Williamson]

On Fri, 2015-03-06 at 10:41 +0000, Will Deacon wrote:
> > As a longer term solution, would it make sense to add a user flag at VFIO
> > user API level to turn the IOMMU_CACHE on?
> 
> I think userspace certainly needs a way to figure out if a device is
> coherent or not, otherwise it can't generate the correct device-tree
> properties for something like a KVM guest but the IOMMU_* setting should
> remain in the kernel IMO. Similarly for things like MSI pages, which would
> need to be mapped as device memory on ARM -- that should be exposed as a
> higher level "please map my MSI page here" ioctl as opposed to requiring
> userspace to supply the correct memory attributes.

Userspace already has a way to determine if all the IOMMUs for a
container support cache coherence, as does KVM via the kvm-vfio pseudo
device.  It's inadvisable to allow userspace to control whether
IOMMU_CACHE is used on x86 as emulating cache sync instructions hurts
the overall system performance.  We don't want users to arbitrarily
decide to enable those heavy-weight operations.  Thanks,

Alex