[PATCH v4 0/5] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[PATCH v4 0/5] 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 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 (5):
  vfio: implement iommu driver capabilities with an enum
  vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
  vfio: type1: replace domain wide protection flags with supported
    capabilities
  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 | 91 +++++++++++++++++++++++++++++------------
 include/uapi/linux/vfio.h       | 30 ++++++++------
 2 files changed, 81 insertions(+), 40 deletions(-)

-- 
2.3.1

[PATCH v4 1/5] 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 v4 2/5] 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 v4 3/5] vfio: type1: replace domain wide protection flags with supported capabilities

[Baptiste Reynal]

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

VFIO_IOMMU_TYPE1 keeps track for each domain it knows a list of protection
flags it always applies to all mappings in the domain. This is used for
domains that support IOMMU_CAP_CACHE_COHERENCY.

Refactor this slightly, by keeping track instead that a given domain
supports the capability, and applying the IOMMU_CACHE protection flag when
doing the actual DMA mappings.

This will allow us to reuse the behavior for IOMMU_CAP_NOEXEC, which we
also want to keep track of, but without applying it to all domains that
support it unless the user explicitly requests it.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
[Baptiste Reynal: Use bit shifting for domain->caps]
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/vfio_iommu_type1.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 57d8c37..998619b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -65,7 +65,7 @@ struct vfio_domain {
 	struct iommu_domain	*domain;
 	struct list_head	next;
 	struct list_head	group_list;
-	int			prot;		/* IOMMU_CACHE */
+	int			caps;
 	bool			fgsp;		/* Fine-grained super pages */
 };
 
@@ -507,7 +507,7 @@ static int map_try_harder(struct vfio_domain *domain, dma_addr_t iova,
 	for (i = 0; i < npage; i++, pfn++, iova += PAGE_SIZE) {
 		ret = iommu_map(domain->domain, iova,
 				(phys_addr_t)pfn << PAGE_SHIFT,
-				PAGE_SIZE, prot | domain->prot);
+				PAGE_SIZE, prot);
 		if (ret)
 			break;
 	}
@@ -525,11 +525,16 @@ static int vfio_iommu_map(struct vfio_iommu *iommu, dma_addr_t iova,
 	int ret;
 
 	list_for_each_entry(d, &iommu->domain_list, next) {
+		int dprot = prot;
+
+		if (d->caps & (1 << IOMMU_CAP_CACHE_COHERENCY))
+			dprot |= IOMMU_CACHE;
+
 		ret = iommu_map(d->domain, iova, (phys_addr_t)pfn << PAGE_SHIFT,
-				npage << PAGE_SHIFT, prot | d->prot);
+				npage << PAGE_SHIFT, dprot);
 		if (ret) {
 			if (ret != -EBUSY ||
-			    map_try_harder(d, iova, pfn, npage, prot))
+			    map_try_harder(d, iova, pfn, npage, dprot))
 				goto unwind;
 		}
 
@@ -644,6 +649,10 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 	struct vfio_domain *d;
 	struct rb_node *n;
 	int ret;
+	int dprot = 0;
+
+	if (domain->caps & (1 << IOMMU_CAP_CACHE_COHERENCY))
+		dprot |= IOMMU_CACHE;
 
 	/* Arbitrarily pick the first domain in the list for lookups */
 	d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
@@ -677,7 +686,7 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 				size += PAGE_SIZE;
 
 			ret = iommu_map(domain->domain, iova, phys,
-					size, dma->prot | domain->prot);
+					size, dma->prot | dprot);
 			if (ret)
 				return ret;
 
@@ -702,13 +711,17 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain)
 {
 	struct page *pages;
 	int ret, order = get_order(PAGE_SIZE * 2);
+	int dprot = 0;
+
+	if (domain->caps & (1 << IOMMU_CAP_CACHE_COHERENCY))
+		dprot |= IOMMU_CACHE;
 
 	pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
 	if (!pages)
 		return;
 
 	ret = iommu_map(domain->domain, 0, page_to_phys(pages), PAGE_SIZE * 2,
-			IOMMU_READ | IOMMU_WRITE | domain->prot);
+			IOMMU_READ | IOMMU_WRITE | dprot);
 	if (!ret) {
 		size_t unmapped = iommu_unmap(domain->domain, 0, PAGE_SIZE);
 
@@ -787,7 +800,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	}
 
 	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
-		domain->prot |= IOMMU_CACHE;
+		domain->caps |= (1 << IOMMU_CAP_CACHE_COHERENCY);
 
 	/*
 	 * Try to match an existing compatible domain.  We don't want to
@@ -798,7 +811,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	 */
 	list_for_each_entry(d, &iommu->domain_list, next) {
 		if (d->domain->ops == domain->domain->ops &&
-		    d->prot == domain->prot) {
+		    d->caps == domain->caps) {
 			iommu_detach_group(domain->domain, iommu_group);
 			if (!iommu_attach_group(d->domain, iommu_group)) {
 				list_add(&group->next, &d->group_list);
@@ -942,7 +955,7 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
 
 	mutex_lock(&iommu->lock);
 	list_for_each_entry(domain, &iommu->domain_list, next) {
-		if (!(domain->prot & IOMMU_CACHE)) {
+		if (!(domain->caps & IOMMU_CAP_CACHE_COHERENCY)) {
 			ret = 0;
 			break;
 		}
-- 
2.3.1

[PATCH v4 4/5] 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 998619b..0ea371b 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->caps & 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
@@ -948,23 +965,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->caps & IOMMU_CAP_CACHE_COHERENCY)) {
-			ret = 0;
-			break;
-		}
-	}
-	mutex_unlock(&iommu->lock);
-
-	return ret;
-}
-
 static long vfio_iommu_type1_ioctl(void *iommu_data,
 				   unsigned int cmd, unsigned long arg)
 {
@@ -980,7 +980,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 v4 5/5] 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 | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0ea371b..2bbd311 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -596,6 +596,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;
@@ -686,6 +692,14 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 		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) &&
+				!(domain->caps & 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;
@@ -819,6 +833,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
 		domain->caps |= (1 << IOMMU_CAP_CACHE_COHERENCY);
 
+	if (iommu_capable(bus, IOMMU_CAP_NOEXEC))
+		domain->caps |= IOMMU_CAP_NOEXEC;
+
 	/*
 	 * Try to match an existing compatible domain.  We don't want to
 	 * preclude an IOMMU driver supporting multiple bus_types and being
@@ -982,6 +999,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;
 		}
@@ -1005,7 +1027,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 v4 0/5] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Eric Auger]

Hi Baptiste,

In "vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag" you still
kept domain->caps |= IOMMU_CAP_NOEXEC so potentially overwriting 1<<
IOMMU_CAP_CACHE_COHERENCY I guess.

Sorry I do not have this 4th patch file in my mailbox.

Best Regards

Eric

	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
		domain->caps |= (1 << IOMMU_CAP_CACHE_COHERENCY);

	if (iommu_capable(bus, IOMMU_CAP_NOEXEC))
		domain->caps |= IOMMU_CAP_NOEXEC;

On 03/02/2015 05:58 PM, 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.
> 
> 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 (5):
>   vfio: implement iommu driver capabilities with an enum
>   vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
>   vfio: type1: replace domain wide protection flags with supported
>     capabilities
>   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 | 91 +++++++++++++++++++++++++++++------------
>  include/uapi/linux/vfio.h       | 30 ++++++++------
>  2 files changed, 81 insertions(+), 40 deletions(-)
> 

Re: [PATCH v4 0/5] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Eric Auger]

On 03/03/2015 06:46 PM, Eric Auger wrote:
> Hi Baptiste,
> 
> In "vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag" you still
> kept domain->caps |= IOMMU_CAP_NOEXEC so potentially overwriting 1<<
> IOMMU_CAP_CACHE_COHERENCY I guess.

Well sorry no risk to overwrite but not homogeneous.
Eric
> 
> Sorry I do not have this 4th patch file in my mailbox.
> 
> Best Regards
> 
> Eric
> 
> 	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
> 		domain->caps |= (1 << IOMMU_CAP_CACHE_COHERENCY);
> 
> 	if (iommu_capable(bus, IOMMU_CAP_NOEXEC))
> 		domain->caps |= IOMMU_CAP_NOEXEC;
> 
> On 03/02/2015 05:58 PM, 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.
>>
>> 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 (5):
>>   vfio: implement iommu driver capabilities with an enum
>>   vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
>>   vfio: type1: replace domain wide protection flags with supported
>>     capabilities
>>   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 | 91 +++++++++++++++++++++++++++++------------
>>  include/uapi/linux/vfio.h       | 30 ++++++++------
>>  2 files changed, 81 insertions(+), 40 deletions(-)
>>
> 

Re: [PATCH v4 0/5] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Alex Williamson]

On Tue, 2015-03-03 at 18:46 +0100, Eric Auger wrote:
> Hi Baptiste,
> 
> In "vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag" you still
> kept domain->caps |= IOMMU_CAP_NOEXEC so potentially overwriting 1<<
> IOMMU_CAP_CACHE_COHERENCY I guess.
> 
> Sorry I do not have this 4th patch file in my mailbox.
> 
> Best Regards
> 
> Eric
> 
> 	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
> 		domain->caps |= (1 << IOMMU_CAP_CACHE_COHERENCY);
> 
> 	if (iommu_capable(bus, IOMMU_CAP_NOEXEC))
> 		domain->caps |= IOMMU_CAP_NOEXEC;


Patch 4/5 has problems too, vfio_domains_have_iommu_cap() is called with
IOMMU_CAP_CACHE_COHERENCY, but nobody is shifting that into a bitmap
before doing the comparison.

TBH, I don't see the point of creating this artificial bitmap out of the
capabilities.  Why can't we keep everything in the domain of actual
flags passed to iommu_ops functions?  It's just silly to test for cached
capability and re-invent the mapping flags on every mapping call and
it's just as easy to generalize a test using the actual flags as to use
the capabilities, perhaps easier.  Thanks,

Alex



> On 03/02/2015 05:58 PM, 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.
> > 
> > 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 (5):
> >   vfio: implement iommu driver capabilities with an enum
> >   vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
> >   vfio: type1: replace domain wide protection flags with supported
> >     capabilities
> >   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 | 91 +++++++++++++++++++++++++++++------------
> >  include/uapi/linux/vfio.h       | 30 ++++++++------
> >  2 files changed, 81 insertions(+), 40 deletions(-)
> > 
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 0/5] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Baptiste Reynal]

[-- Attachment #1.1: Type: text/plain, Size: 3088 bytes --]

Thanks for your comments. A v5 is ongoing, with the removal of
domain->caps, instead domain->domain->ops->capable(cap) is tested.

Regards,
Baptiste

On Tue, Mar 3, 2015 at 7:01 PM, Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
wrote:

> On Tue, 2015-03-03 at 18:46 +0100, Eric Auger wrote:
> > Hi Baptiste,
> >
> > In "vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag" you still
> > kept domain->caps |= IOMMU_CAP_NOEXEC so potentially overwriting 1<<
> > IOMMU_CAP_CACHE_COHERENCY I guess.
> >
> > Sorry I do not have this 4th patch file in my mailbox.
> >
> > Best Regards
> >
> > Eric
> >
> >       if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
> >               domain->caps |= (1 << IOMMU_CAP_CACHE_COHERENCY);
> >
> >       if (iommu_capable(bus, IOMMU_CAP_NOEXEC))
> >               domain->caps |= IOMMU_CAP_NOEXEC;
>
>
> Patch 4/5 has problems too, vfio_domains_have_iommu_cap() is called with
> IOMMU_CAP_CACHE_COHERENCY, but nobody is shifting that into a bitmap
> before doing the comparison.
>
> TBH, I don't see the point of creating this artificial bitmap out of the
> capabilities.  Why can't we keep everything in the domain of actual
> flags passed to iommu_ops functions?  It's just silly to test for cached
> capability and re-invent the mapping flags on every mapping call and
> it's just as easy to generalize a test using the actual flags as to use
> the capabilities, perhaps easier.  Thanks,
>
> Alex
>
>
>
> > On 03/02/2015 05:58 PM, 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.
> > >
> > > 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 (5):
> > >   vfio: implement iommu driver capabilities with an enum
> > >   vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
> > >   vfio: type1: replace domain wide protection flags with supported
> > >     capabilities
> > >   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 | 91
> +++++++++++++++++++++++++++++------------
> > >  include/uapi/linux/vfio.h       | 30 ++++++++------
> > >  2 files changed, 81 insertions(+), 40 deletions(-)
> > >
> >
> > _______________________________________________
> > iommu mailing list
> > iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 4276 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v4 0/5] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Alex Williamson]

On Wed, 2015-03-04 at 16:21 +0100, Baptiste Reynal wrote:
> Thanks for your comments. A v5 is ongoing, with the removal of
> domain->caps, instead domain->domain->ops->capable(cap) is tested.

Doesn't that defeat the whole purpose of caching a subset of the mapping
flags on the domain?  I also hope we're not ignoring the abstraction of
the iommu api by following iommu_ops pointers directly.  Thanks,

Alex

> On Tue, Mar 3, 2015 at 7:01 PM, Alex Williamson <alex.williamson@redhat.com>
> wrote:
> 
> > On Tue, 2015-03-03 at 18:46 +0100, Eric Auger wrote:
> > > Hi Baptiste,
> > >
> > > In "vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag" you still
> > > kept domain->caps |= IOMMU_CAP_NOEXEC so potentially overwriting 1<<
> > > IOMMU_CAP_CACHE_COHERENCY I guess.
> > >
> > > Sorry I do not have this 4th patch file in my mailbox.
> > >
> > > Best Regards
> > >
> > > Eric
> > >
> > >       if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
> > >               domain->caps |= (1 << IOMMU_CAP_CACHE_COHERENCY);
> > >
> > >       if (iommu_capable(bus, IOMMU_CAP_NOEXEC))
> > >               domain->caps |= IOMMU_CAP_NOEXEC;
> >
> >
> > Patch 4/5 has problems too, vfio_domains_have_iommu_cap() is called with
> > IOMMU_CAP_CACHE_COHERENCY, but nobody is shifting that into a bitmap
> > before doing the comparison.
> >
> > TBH, I don't see the point of creating this artificial bitmap out of the
> > capabilities.  Why can't we keep everything in the domain of actual
> > flags passed to iommu_ops functions?  It's just silly to test for cached
> > capability and re-invent the mapping flags on every mapping call and
> > it's just as easy to generalize a test using the actual flags as to use
> > the capabilities, perhaps easier.  Thanks,
> >
> > Alex
> >
> >
> >
> > > On 03/02/2015 05:58 PM, 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.
> > > >
> > > > 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 (5):
> > > >   vfio: implement iommu driver capabilities with an enum
> > > >   vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
> > > >   vfio: type1: replace domain wide protection flags with supported
> > > >     capabilities
> > > >   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 | 91
> > +++++++++++++++++++++++++++++------------
> > > >  include/uapi/linux/vfio.h       | 30 ++++++++------
> > > >  2 files changed, 81 insertions(+), 40 deletions(-)
> > > >
> > >
> > > _______________________________________________
> > > iommu mailing list
> > > iommu@lists.linux-foundation.org
> > > https://lists.linuxfoundation.org/mailman/listinfo/iommu
> >
> >
> >
> >

Re: [PATCH v4 0/5] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Baptiste Reynal]

Actually the mapping flags are still cached on the domain, except for
IOMMU_NOEXEC (As it is used at user request).
I don't think the abstraction is ignored, since every abstraction
should implement capable.

The problem here is that IOMMU_NOEXEC can not be cached, but
IOMMU_CAP_NOEXEC has to be. Another solution could be a boolean in
vfio_domain ?

Thanks,
Baptiste

On Wed, Mar 4, 2015 at 5:10 PM, Alex Williamson
<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 2015-03-04 at 16:21 +0100, Baptiste Reynal wrote:
>> Thanks for your comments. A v5 is ongoing, with the removal of
>> domain->caps, instead domain->domain->ops->capable(cap) is tested.
>
> Doesn't that defeat the whole purpose of caching a subset of the mapping
> flags on the domain?  I also hope we're not ignoring the abstraction of
> the iommu api by following iommu_ops pointers directly.  Thanks,
>
> Alex
>
>> On Tue, Mar 3, 2015 at 7:01 PM, Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> wrote:
>>
>> > On Tue, 2015-03-03 at 18:46 +0100, Eric Auger wrote:
>> > > Hi Baptiste,
>> > >
>> > > In "vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag" you still
>> > > kept domain->caps |= IOMMU_CAP_NOEXEC so potentially overwriting 1<<
>> > > IOMMU_CAP_CACHE_COHERENCY I guess.
>> > >
>> > > Sorry I do not have this 4th patch file in my mailbox.
>> > >
>> > > Best Regards
>> > >
>> > > Eric
>> > >
>> > >       if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
>> > >               domain->caps |= (1 << IOMMU_CAP_CACHE_COHERENCY);
>> > >
>> > >       if (iommu_capable(bus, IOMMU_CAP_NOEXEC))
>> > >               domain->caps |= IOMMU_CAP_NOEXEC;
>> >
>> >
>> > Patch 4/5 has problems too, vfio_domains_have_iommu_cap() is called with
>> > IOMMU_CAP_CACHE_COHERENCY, but nobody is shifting that into a bitmap
>> > before doing the comparison.
>> >
>> > TBH, I don't see the point of creating this artificial bitmap out of the
>> > capabilities.  Why can't we keep everything in the domain of actual
>> > flags passed to iommu_ops functions?  It's just silly to test for cached
>> > capability and re-invent the mapping flags on every mapping call and
>> > it's just as easy to generalize a test using the actual flags as to use
>> > the capabilities, perhaps easier.  Thanks,
>> >
>> > Alex
>> >
>> >
>> >
>> > > On 03/02/2015 05:58 PM, 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.
>> > > >
>> > > > 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 (5):
>> > > >   vfio: implement iommu driver capabilities with an enum
>> > > >   vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
>> > > >   vfio: type1: replace domain wide protection flags with supported
>> > > >     capabilities
>> > > >   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 | 91
>> > +++++++++++++++++++++++++++++------------
>> > > >  include/uapi/linux/vfio.h       | 30 ++++++++------
>> > > >  2 files changed, 81 insertions(+), 40 deletions(-)
>> > > >
>> > >
>> > > _______________________________________________
>> > > iommu mailing list
>> > > iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>> > > https://lists.linuxfoundation.org/mailman/listinfo/iommu
>> >
>> >
>> >
>> >
>
>
>

Re: [PATCH v4 0/5] vfio: type1: support for ARM SMMUS with VFIO_IOMMU_TYPE1

[Baptiste Reynal]

[-- Attachment #1.1: Type: text/plain, Size: 1362 bytes --]

Added Eric Auger for comments.

On Mon, Mar 2, 2015 at 5:58 PM, Baptiste Reynal <
b.reynal@virtualopensystems.com> 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.
>
> 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 (5):
>   vfio: implement iommu driver capabilities with an enum
>   vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
>   vfio: type1: replace domain wide protection flags with supported
>     capabilities
>   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 | 91
> +++++++++++++++++++++++++++++------------
>  include/uapi/linux/vfio.h       | 30 ++++++++------
>  2 files changed, 81 insertions(+), 40 deletions(-)
>
> --
> 2.3.1
>
>

[-- Attachment #1.2: Type: text/html, Size: 1835 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm