[PATCH v2 1/6] vfio: implement iommu driver capabilities with an enum

[PATCH v2 1/6] vfio: implement iommu driver capabilities with an enum

[Antonios Motakis]

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>
---
 include/uapi/linux/vfio.h | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 6612974..1e39842 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -19,19 +19,18 @@
 
 /* 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 */
+};
 
 /*
  * The IOCTL interface is designed for extensibility by embedding the
-- 
2.1.1

Re: [PATCH v2 1/6] vfio: implement iommu driver capabilities with an enum

[Alex Williamson]

On Mon, 2014-10-27 at 19:05 +0100, Antonios Motakis wrote:
> 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>
> ---
>  include/uapi/linux/vfio.h | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 6612974..1e39842 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -19,19 +19,18 @@
>  
>  /* 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 */
> +};

Your code base is a little out of date, you're missing:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/vfio.h?id=f5c9ecebaf2a2c9381973798e389cc019dd983e0

I think the logic looks ok in the rest, but you'll need to use index 7
and the above commit touched the type1 c file as well so you may need to
adjustment there too.  Thanks,

Alex
>  
>  /*
>   * The IOCTL interface is designed for extensibility by embedding the

Re: [PATCH v2 1/6] vfio: implement iommu driver capabilities with an enum

[Antonios Motakis]

On Fri, Oct 31, 2014 at 9:04 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Mon, 2014-10-27 at 19:05 +0100, Antonios Motakis wrote:
> > 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>
> > ---
> >  include/uapi/linux/vfio.h | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 6612974..1e39842 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -19,19 +19,18 @@
> >
> >  /* 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 */
> > +};
>
> Your code base is a little out of date, you're missing:
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/vfio.h?id=f5c9ecebaf2a2c9381973798e389cc019dd983e0
>
> I think the logic looks ok in the rest, but you'll need to use index 7
> and the above commit touched the type1 c file as well so you may need to
> adjustment there too.  Thanks,

Hello,

I will reapply on the latest version then.

Thanks

>
> Alex
> >
> >  /*
> >   * The IOCTL interface is designed for extensibility by embedding the
>
>
>

[PATCH v2 2/6] vfio: type1: support for platform bus devices on ARM

[Antonios Motakis]

This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
devices on ARM. The driver can then be used with an Exynos SMMU, or
ARM SMMU driver.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index d8c5763..a0abe04 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -16,7 +16,7 @@ config VFIO_SPAPR_EEH
 menuconfig VFIO
 	tristate "VFIO Non-Privileged userspace driver framework"
 	depends on IOMMU_API
-	select VFIO_IOMMU_TYPE1 if X86
+	select VFIO_IOMMU_TYPE1 if X86 || ARM
 	select VFIO_IOMMU_SPAPR_TCE if (PPC_POWERNV || PPC_PSERIES)
 	select VFIO_SPAPR_EEH if (PPC_POWERNV || PPC_PSERIES)
 	select ANON_INODES
-- 
2.1.1

[PATCH v2 3/6] vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Antonios Motakis]

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>
---
 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 1e39842..06d66c9 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -30,6 +30,7 @@ enum vfio_iommu_cap {
 	VFIO_DMA_CC_IOMMU	= 4, /* IOMMU enforces DMA cache coherence
 					(ex. PCIe NoSnoop stripping) */
 	VFIO_EEH		= 5, /* Check if EEH is supported */
+	VFIO_DMA_NOEXEC_IOMMU	= 6,
 };
 
 /*
@@ -394,12 +395,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.1.1

[PATCH v2 4/6] vfio: type1: replace domain wide protection flags with supported capabilities

[Antonios Motakis]

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>
---
 drivers/vfio/vfio_iommu_type1.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 562f686..aefb3c0 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -64,7 +64,7 @@ struct vfio_domain {
 	struct iommu_domain	*domain;
 	struct list_head	next;
 	struct list_head	group_list;
-	int			prot;		/* IOMMU_CACHE */
+	int			caps;
 };
 
 struct vfio_dma {
@@ -485,7 +485,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;
 	}
@@ -503,11 +503,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 & 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;
 		}
 	}
@@ -620,6 +625,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 & 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);
@@ -653,7 +662,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;
 
@@ -721,7 +730,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 |= IOMMU_CAP_CACHE_COHERENCY;
 
 	/*
 	 * Try to match an existing compatible domain.  We don't want to
@@ -732,7 +741,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);
@@ -865,7 +874,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.1.1

[PATCH v2 5/6] vfio: type1: replace vfio_domains_have_iommu_cache with generic function

[Antonios Motakis]

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>
---
 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 aefb3c0..34472e4 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -80,6 +80,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
@@ -867,23 +884,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)
 {
@@ -898,7 +898,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.1.1

[PATCH v2 6/6] vfio: type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Antonios Motakis]

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>
---
 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 34472e4..c669b5b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -572,6 +572,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;
@@ -662,6 +668,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;
@@ -749,6 +763,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
 		domain->caps |= 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
@@ -900,6 +917,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;
 		}
@@ -923,7 +945,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.1.1