[RFC PATCHv2 0/3] Support for nesting IOMMUs in VFIO

[RFC PATCHv2 0/3] Support for nesting IOMMUs in VFIO

[Will Deacon]

Hello,

This is v2 of the RFC I originally posted here:

 RFCv1: http://permalink.gmane.org/gmane.linux.kernel.iommu/5552

It's changed significantly since then, based on helpful feedback from
Alex. In particular, I no longer butcher the IOMMU API, instead making
use of a new iommu_attr to indicate that a domain requires support for
nesting.

All feedback welcome (this is still an RFC after all),

Cheers,

Will


Will Deacon (3):
  iommu: introduce domain attribute for nesting IOMMUs
  vfio/iommu_type1: add new VFIO_TYPE1_NESTING_IOMMU IOMMU type
  iommu/arm-smmu: add support for DOMAIN_ATTR_NESTING attribute

 drivers/iommu/arm-smmu.c        | 112 +++++++++++++++++++++++++++++++++-------
 drivers/vfio/vfio_iommu_type1.c |  56 +++++++++++++++++---
 include/linux/iommu.h           |   1 +
 include/uapi/linux/vfio.h       |   2 +
 4 files changed, 144 insertions(+), 27 deletions(-)

-- 
2.0.0

[RFC PATCHv2 1/3] iommu: introduce domain attribute for nesting IOMMUs

[Will Deacon]

Some IOMMUs, such as the ARM SMMU, support two stages of translation.
The idea behind such a scheme is to allow a guest operating system to
use the IOMMU for DMA mappings in the first stage of translation, with
the hypervisor then installing mappings in the second stage to provide
isolation of the DMA to the physical range assigned to that virtual
machine.

In order to allow IOMMU domains to be used for second-stage translation,
this patch adds a new iommu_attr (IOMMU_ATTR_NESTING) for setting
second-stage domains prior to device attach. The attribute can also be
queried to see if a domain is actually making use of nesting.

Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 include/linux/iommu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b96a5b2136e4..0550286df49b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -80,6 +80,7 @@ enum iommu_attr {
 	DOMAIN_ATTR_FSL_PAMU_STASH,
 	DOMAIN_ATTR_FSL_PAMU_ENABLE,
 	DOMAIN_ATTR_FSL_PAMUV1,
+	DOMAIN_ATTR_NESTING,	/* two stages of translation */
 	DOMAIN_ATTR_MAX,
 };
 
-- 
2.0.0

[RFC PATCHv2 2/3] vfio/iommu_type1: add new VFIO_TYPE1_NESTING_IOMMU IOMMU type

[Will Deacon]

VFIO allows devices to be safely handed off to userspace by putting
them behind an IOMMU configured to ensure DMA and interrupt isolation.
This enables userspace KVM clients, such as kvmtool and qemu, to further
map the device into a virtual machine.

With IOMMUs such as the ARM SMMU, it is then possible to provide SMMU
translation services to the guest operating system, which are nested
with the existing translation installed by VFIO. However, enabling this
feature means that the IOMMU driver must be informed that the VFIO domain
is being created for the purposes of nested translation.

This patch adds a new IOMMU type (VFIO_TYPE1_NESTING_IOMMU) to the VFIO
type-1 driver. The new IOMMU type acts identically to the
VFIO_TYPE1v2_IOMMU type, but additionally sets the DOMAIN_ATTR_NESTING
attribute on its IOMMU domains. Userspace can check whether nesting is
actually available using the VFIO_CHECK_EXTENSION ioctl, in a similar
manner to checking for cache-coherent DMA.

Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 drivers/vfio/vfio_iommu_type1.c | 56 +++++++++++++++++++++++++++++++++++------
 include/uapi/linux/vfio.h       |  2 ++
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0734fbe5b651..24cfe69a3c83 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -53,11 +53,15 @@ module_param_named(disable_hugepages,
 MODULE_PARM_DESC(disable_hugepages,
 		 "Disable VFIO IOMMU support for IOMMU hugepages.");
 
+/* Feature flags for VFIO Type-1 IOMMUs */
+#define VFIO_IOMMU_FEAT_V2	(1 << 0)
+#define VFIO_IOMMU_FEAT_NESTING	(1 << 1)
+
 struct vfio_iommu {
 	struct list_head	domain_list;
 	struct mutex		lock;
 	struct rb_root		dma_list;
-	bool v2;
+	int			features;
 };
 
 struct vfio_domain {
@@ -441,7 +445,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
 	 * will only return success and a size of zero if there were no
 	 * mappings within the range.
 	 */
-	if (iommu->v2) {
+	if (iommu->features & VFIO_IOMMU_FEAT_V2) {
 		dma = vfio_find_dma(iommu, unmap->iova, 0);
 		if (dma && dma->iova != unmap->iova) {
 			ret = -EINVAL;
@@ -455,7 +459,8 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
 	}
 
 	while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
-		if (!iommu->v2 && unmap->iova > dma->iova)
+		if (!(iommu->features & VFIO_IOMMU_FEAT_V2) &&
+		    unmap->iova > dma->iova)
 			break;
 		unmapped += dma->size;
 		vfio_remove_dma(iommu, dma);
@@ -671,7 +676,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	struct vfio_group *group, *g;
 	struct vfio_domain *domain, *d;
 	struct bus_type *bus = NULL;
-	int ret;
+	int ret, attr = 1;
 
 	mutex_lock(&iommu->lock);
 
@@ -705,6 +710,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_free;
 	}
 
+	if (iommu->features & VFIO_IOMMU_FEAT_NESTING)
+		iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING, &attr);
+
 	ret = iommu_attach_group(domain->domain, iommu_group);
 	if (ret)
 		goto out_domain;
@@ -818,9 +826,19 @@ done:
 static void *vfio_iommu_type1_open(unsigned long arg)
 {
 	struct vfio_iommu *iommu;
-
-	if (arg != VFIO_TYPE1_IOMMU && arg != VFIO_TYPE1v2_IOMMU)
+	int features = 0;
+
+	switch (arg) {
+	case VFIO_TYPE1_IOMMU:
+		break;
+	case VFIO_TYPE1_NESTING_IOMMU:
+		features |= VFIO_IOMMU_FEAT_NESTING;
+	case VFIO_TYPE1v2_IOMMU:
+		features |= VFIO_IOMMU_FEAT_V2;
+		break;
+	default:
 		return ERR_PTR(-EINVAL);
+	}
 
 	iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
 	if (!iommu)
@@ -829,7 +847,7 @@ static void *vfio_iommu_type1_open(unsigned long arg)
 	INIT_LIST_HEAD(&iommu->domain_list);
 	iommu->dma_list = RB_ROOT;
 	mutex_init(&iommu->lock);
-	iommu->v2 = (arg == VFIO_TYPE1v2_IOMMU);
+	iommu->features = features;
 
 	return iommu;
 }
@@ -875,6 +893,26 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
 	return ret;
 }
 
+static int vfio_domains_have_iommu_nesting(struct vfio_iommu *iommu)
+{
+	struct vfio_domain *domain;
+	int ret = 1;
+
+	mutex_lock(&iommu->lock);
+	list_for_each_entry(domain, &iommu->domain_list, next) {
+		int nesting;
+
+		if (iommu_domain_get_attr(domain->domain, DOMAIN_ATTR_NESTING,
+					  &nesting) || !nesting) {
+			ret = 0;
+			break;
+		}
+	}
+	mutex_unlock(&iommu->lock);
+
+	return ret;
+}
+
 static long vfio_iommu_type1_ioctl(void *iommu_data,
 				   unsigned int cmd, unsigned long arg)
 {
@@ -886,6 +924,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 		case VFIO_TYPE1_IOMMU:
 		case VFIO_TYPE1v2_IOMMU:
 			return 1;
+		case VFIO_TYPE1_NESTING_IOMMU:
+			if (!iommu)
+				return 1;
+			return vfio_domains_have_iommu_nesting(iommu);
 		case VFIO_DMA_CC_IOMMU:
 			if (!iommu)
 				return 0;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index cb9023d4f063..babcb33a2756 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -24,11 +24,13 @@
 #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.
  */
 #define VFIO_DMA_CC_IOMMU		4
+#define VFIO_TYPE1_NESTING_IOMMU	5	/* Implies v2 */
 
 /*
  * The IOCTL interface is designed for extensibility by embedding the
-- 
2.0.0

[RFC PATCHv2 3/3] iommu/arm-smmu: add support for DOMAIN_ATTR_NESTING attribute

[Will Deacon]

When domains are set with the DOMAIN_ATTR_NESTING flag, we must ensure
that we allocate them to stage-2 context banks if the hardware permits
it.

This patch adds support for the attribute to the ARM SMMU driver, with
the actual stage being determined depending on the features supported
by the hardware.

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 112 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 92 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index f3f66416e252..7d4bac59129b 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -391,9 +391,16 @@ struct arm_smmu_cfg {
 #define ARM_SMMU_CB_ASID(cfg)		((cfg)->cbndx)
 #define ARM_SMMU_CB_VMID(cfg)		((cfg)->cbndx + 1)
 
+enum arm_smmu_domain_stage {
+	ARM_SMMU_DOMAIN_S1 = 0,
+	ARM_SMMU_DOMAIN_S2,
+	ARM_SMMU_DOMAIN_NESTED,
+};
+
 struct arm_smmu_domain {
 	struct arm_smmu_device		*smmu;
 	struct arm_smmu_cfg		cfg;
+	enum arm_smmu_domain_stage	stage;
 	spinlock_t			lock;
 };
 
@@ -872,19 +879,48 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	struct arm_smmu_domain *smmu_domain = domain->priv;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 
-	if (smmu->features & ARM_SMMU_FEAT_TRANS_NESTED) {
+	/*
+	 * Mapping the requested stage onto what we support is surprisingly
+	 * complicated, mainly because the spec allows S1+S2 SMMUs without
+	 * support for nested translation. That means we end up with the
+	 * following table:
+	 *
+	 * Requested        Supported        Actual
+	 *     S1               N              S1
+	 *     S1             S1+S2            S1
+	 *     S1               S2             S2
+	 *     S1               S1             S1
+	 *     N                N              N
+	 *     N              S1+S2            S2
+	 *     N                S2             S2
+	 *     N                S1             S1
+	 *
+	 * Note that you can't actually request stage-2 mappings.
+	 */
+	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
+		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
+	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
+		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_NESTED) &&
+	    smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED)
+		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
+
+	switch (smmu_domain->stage) {
+	case ARM_SMMU_DOMAIN_S1:
+		cfg->cbar = CBAR_TYPE_S1_TRANS_S2_BYPASS;
+		start = smmu->num_s2_context_banks;
+		break;
+	case ARM_SMMU_DOMAIN_NESTED:
 		/*
 		 * We will likely want to change this if/when KVM gets
 		 * involved.
 		 */
-		cfg->cbar = CBAR_TYPE_S1_TRANS_S2_BYPASS;
-		start = smmu->num_s2_context_banks;
-	} else if (smmu->features & ARM_SMMU_FEAT_TRANS_S1) {
-		cfg->cbar = CBAR_TYPE_S1_TRANS_S2_BYPASS;
-		start = smmu->num_s2_context_banks;
-	} else {
+	case ARM_SMMU_DOMAIN_S2:
 		cfg->cbar = CBAR_TYPE_S2_TRANS;
 		start = 0;
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
@@ -1597,20 +1633,56 @@ static void arm_smmu_remove_device(struct device *dev)
 	iommu_group_remove_device(dev);
 }
 
+static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
+				    enum iommu_attr attr, void *data)
+{
+	struct arm_smmu_domain *smmu_domain = domain->priv;
+
+	switch (attr) {
+	case DOMAIN_ATTR_NESTING:
+		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
+		return 0;
+	default:
+		return -ENODEV;
+	}
+}
+
+static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
+				    enum iommu_attr attr, void *data)
+{
+	struct arm_smmu_domain *smmu_domain = domain->priv;
+
+	switch (attr) {
+	case DOMAIN_ATTR_NESTING:
+		if (smmu_domain->smmu)
+			return -EPERM;
+		if (*(int *)data)
+			smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED;
+		else
+			smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+
+		return 0;
+	default:
+		return -ENODEV;
+	}
+}
+
 static struct iommu_ops arm_smmu_ops = {
-	.domain_init	= arm_smmu_domain_init,
-	.domain_destroy	= arm_smmu_domain_destroy,
-	.attach_dev	= arm_smmu_attach_dev,
-	.detach_dev	= arm_smmu_detach_dev,
-	.map		= arm_smmu_map,
-	.unmap		= arm_smmu_unmap,
-	.iova_to_phys	= arm_smmu_iova_to_phys,
-	.domain_has_cap	= arm_smmu_domain_has_cap,
-	.add_device	= arm_smmu_add_device,
-	.remove_device	= arm_smmu_remove_device,
-	.pgsize_bitmap	= (SECTION_SIZE |
-			   ARM_SMMU_PTE_CONT_SIZE |
-			   PAGE_SIZE),
+	.domain_init		= arm_smmu_domain_init,
+	.domain_destroy		= arm_smmu_domain_destroy,
+	.attach_dev		= arm_smmu_attach_dev,
+	.detach_dev		= arm_smmu_detach_dev,
+	.map			= arm_smmu_map,
+	.unmap			= arm_smmu_unmap,
+	.iova_to_phys		= arm_smmu_iova_to_phys,
+	.domain_has_cap		= arm_smmu_domain_has_cap,
+	.add_device		= arm_smmu_add_device,
+	.remove_device		= arm_smmu_remove_device,
+	.domain_get_attr	= arm_smmu_domain_get_attr,
+	.domain_set_attr	= arm_smmu_domain_set_attr,
+	.pgsize_bitmap		= (SECTION_SIZE |
+				   ARM_SMMU_PTE_CONT_SIZE |
+				   PAGE_SIZE),
 };
 
 static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
-- 
2.0.0

Re: [RFC PATCHv2 0/3] Support for nesting IOMMUs in VFIO

[Alex Williamson]

On Wed, 2014-07-09 at 19:28 +0100, Will Deacon wrote:
> Hello,
> 
> This is v2 of the RFC I originally posted here:
> 
>  RFCv1: http://permalink.gmane.org/gmane.linux.kernel.iommu/5552
> 
> It's changed significantly since then, based on helpful feedback from
> Alex. In particular, I no longer butcher the IOMMU API, instead making
> use of a new iommu_attr to indicate that a domain requires support for
> nesting.
> 
> All feedback welcome (this is still an RFC after all),

Hi Will,

It's a lot cleaner this way, but I'm concerned that allowing a "nesting"
IOMMU to be created and used just like a TYPE1v2 IOMMU, even if the
IOMMU doesn't support it, puts us in a corner for compatibility later.
The safer approach might be to fail the attach_group function if we
can't set the domain attribute, ie. enforce that the IOMMU must support
it.  I don't know how you'd handle it if only some of the domains within
a container supported it, so enforcement may simplify things down the
road too.  We wouldn't need the vfio_domains_have_iommu_nesting()
function that way either.  Thanks,

Alex

Re: [RFC PATCHv2 0/3] Support for nesting IOMMUs in VFIO

[Will Deacon]

Hey Alex,

On Wed, Jul 09, 2014 at 09:37:15PM +0100, Alex Williamson wrote:
> On Wed, 2014-07-09 at 19:28 +0100, Will Deacon wrote:
> > Hello,
> > 
> > This is v2 of the RFC I originally posted here:
> > 
> >  RFCv1: http://permalink.gmane.org/gmane.linux.kernel.iommu/5552
> > 
> > It's changed significantly since then, based on helpful feedback from
> > Alex. In particular, I no longer butcher the IOMMU API, instead making
> > use of a new iommu_attr to indicate that a domain requires support for
> > nesting.
> > 
> > All feedback welcome (this is still an RFC after all),
> 
> It's a lot cleaner this way, but I'm concerned that allowing a "nesting"
> IOMMU to be created and used just like a TYPE1v2 IOMMU, even if the
> IOMMU doesn't support it, puts us in a corner for compatibility later.
> The safer approach might be to fail the attach_group function if we
> can't set the domain attribute, ie. enforce that the IOMMU must support
> it. 

I also contemplated this (and was my source of confusion when we were
discussing using iommu_attr before), however from a user's perspective it's
not at all clear what's gone wrong. Essentially, VFIO would advertise
support for VFIO_TYPE1_NESTING_IOMMU but any attempt to VFIO_SET_IOMMU
for a container would fail. Knowing to try and fall-back on VFIO_TYPE1v2_IOMMU
isn't at all obvious.

> I don't know how you'd handle it if only some of the domains within
> a container supported it, so enforcement may simplify things down the
> road too.  We wouldn't need the vfio_domains_have_iommu_nesting()
> function that way either.

Agreed, it would certainly make things simpler in the kernel. Although, if
we allow a subset of domains in a container to use nesting, I think that's
possible by only allowing groups in those domains to be part of the virtual
SMMU interface.

Do you think that returning something like -EOPNOTSUPP from an attach is
sufficient for userspace to figure out what's gone wrong?

Will

Re: [RFC PATCHv2 0/3] Support for nesting IOMMUs in VFIO

[Alex Williamson]

On Thu, 2014-07-10 at 12:34 +0100, Will Deacon wrote:
> Hey Alex,
> 
> On Wed, Jul 09, 2014 at 09:37:15PM +0100, Alex Williamson wrote:
> > On Wed, 2014-07-09 at 19:28 +0100, Will Deacon wrote:
> > > Hello,
> > > 
> > > This is v2 of the RFC I originally posted here:
> > > 
> > >  RFCv1: http://permalink.gmane.org/gmane.linux.kernel.iommu/5552
> > > 
> > > It's changed significantly since then, based on helpful feedback from
> > > Alex. In particular, I no longer butcher the IOMMU API, instead making
> > > use of a new iommu_attr to indicate that a domain requires support for
> > > nesting.
> > > 
> > > All feedback welcome (this is still an RFC after all),
> > 
> > It's a lot cleaner this way, but I'm concerned that allowing a "nesting"
> > IOMMU to be created and used just like a TYPE1v2 IOMMU, even if the
> > IOMMU doesn't support it, puts us in a corner for compatibility later.
> > The safer approach might be to fail the attach_group function if we
> > can't set the domain attribute, ie. enforce that the IOMMU must support
> > it. 
> 
> I also contemplated this (and was my source of confusion when we were
> discussing using iommu_attr before), however from a user's perspective it's
> not at all clear what's gone wrong. Essentially, VFIO would advertise
> support for VFIO_TYPE1_NESTING_IOMMU but any attempt to VFIO_SET_IOMMU
> for a container would fail. Knowing to try and fall-back on VFIO_TYPE1v2_IOMMU
> isn't at all obvious.
> 
> > I don't know how you'd handle it if only some of the domains within
> > a container supported it, so enforcement may simplify things down the
> > road too.  We wouldn't need the vfio_domains_have_iommu_nesting()
> > function that way either.
> 
> Agreed, it would certainly make things simpler in the kernel. Although, if
> we allow a subset of domains in a container to use nesting, I think that's
> possible by only allowing groups in those domains to be part of the virtual
> SMMU interface.
> 
> Do you think that returning something like -EOPNOTSUPP from an attach is
> sufficient for userspace to figure out what's gone wrong?

It seems like this would all be a little easier if we had some sort of
bus_type iterator, then when we want to see if nesting is supported we'd
iterate the bus_types, test iommu_present(), iommu_domain_alloc(),
iommu_domain_set_attr(), iommu_domain_free().  We'd only allow a nesting
domain to be set if the IOMMU driver for at least one bus_type supports
it.  Then I think enforcing it would be a little more obvious to
userspace.  There is a bus_kset, but we'd need an iterator or at least
to enable find_bus() and have type1 walk a list of bus names known to
possibly support nesting (uglier, but functional).  Thanks,

Alex

Re: [RFC PATCHv2 0/3] Support for nesting IOMMUs in VFIO

[Will Deacon]

On Thu, Jul 10, 2014 at 04:38:18PM +0100, Alex Williamson wrote:
> On Thu, 2014-07-10 at 12:34 +0100, Will Deacon wrote:
> > On Wed, Jul 09, 2014 at 09:37:15PM +0100, Alex Williamson wrote:
> > > On Wed, 2014-07-09 at 19:28 +0100, Will Deacon wrote:
> > > > Hello,
> > > > 
> > > > This is v2 of the RFC I originally posted here:
> > > > 
> > > >  RFCv1: http://permalink.gmane.org/gmane.linux.kernel.iommu/5552
> > > > 
> > > > It's changed significantly since then, based on helpful feedback from
> > > > Alex. In particular, I no longer butcher the IOMMU API, instead making
> > > > use of a new iommu_attr to indicate that a domain requires support for
> > > > nesting.
> > > > 
> > > > All feedback welcome (this is still an RFC after all),
> > > 
> > > It's a lot cleaner this way, but I'm concerned that allowing a "nesting"
> > > IOMMU to be created and used just like a TYPE1v2 IOMMU, even if the
> > > IOMMU doesn't support it, puts us in a corner for compatibility later.
> > > The safer approach might be to fail the attach_group function if we
> > > can't set the domain attribute, ie. enforce that the IOMMU must support
> > > it. 
> > 
> > I also contemplated this (and was my source of confusion when we were
> > discussing using iommu_attr before), however from a user's perspective it's
> > not at all clear what's gone wrong. Essentially, VFIO would advertise
> > support for VFIO_TYPE1_NESTING_IOMMU but any attempt to VFIO_SET_IOMMU
> > for a container would fail. Knowing to try and fall-back on VFIO_TYPE1v2_IOMMU
> > isn't at all obvious.
> > 
> > > I don't know how you'd handle it if only some of the domains within
> > > a container supported it, so enforcement may simplify things down the
> > > road too.  We wouldn't need the vfio_domains_have_iommu_nesting()
> > > function that way either.
> > 
> > Agreed, it would certainly make things simpler in the kernel. Although, if
> > we allow a subset of domains in a container to use nesting, I think that's
> > possible by only allowing groups in those domains to be part of the virtual
> > SMMU interface.
> > 
> > Do you think that returning something like -EOPNOTSUPP from an attach is
> > sufficient for userspace to figure out what's gone wrong?
> 
> It seems like this would all be a little easier if we had some sort of
> bus_type iterator, then when we want to see if nesting is supported we'd
> iterate the bus_types, test iommu_present(), iommu_domain_alloc(),
> iommu_domain_set_attr(), iommu_domain_free().  We'd only allow a nesting
> domain to be set if the IOMMU driver for at least one bus_type supports
> it.  Then I think enforcing it would be a little more obvious to
> userspace.  There is a bus_kset, but we'd need an iterator or at least
> to enable find_bus() and have type1 walk a list of bus names known to
> possibly support nesting (uglier, but functional).  Thanks,

That would work fine if we only had one IOMMU instance per bus. The problem
is, the ARM SMMU driver (at least) can handle multiple SMMU instances on a
single bus, only some of which may be capable of nesting. Until we've done
an attach, it's impossible to know what the capabilities are (since the
attach is what binds the domain to a physical IOMMU).

So you'd actually need to iterate the bus_type, test iommu_present(),
iommu_domain_alloc(), iommu_domain_attach(), iommu_domain_free(). The
attach, of course, then requires devices which means you ultimately end up
rolling VFIO_SET_IOMMU and VFIO_SET_CONTAINER into a single operation. I
don't think that helps the group abstraction much!

Will

Re: [RFC PATCHv2 0/3] Support for nesting IOMMUs in VFIO

[Alex Williamson]

On Thu, 2014-07-10 at 16:45 +0100, Will Deacon wrote:
> On Thu, Jul 10, 2014 at 04:38:18PM +0100, Alex Williamson wrote:
> > On Thu, 2014-07-10 at 12:34 +0100, Will Deacon wrote:
> > > On Wed, Jul 09, 2014 at 09:37:15PM +0100, Alex Williamson wrote:
> > > > On Wed, 2014-07-09 at 19:28 +0100, Will Deacon wrote:
> > > > > Hello,
> > > > > 
> > > > > This is v2 of the RFC I originally posted here:
> > > > > 
> > > > >  RFCv1: http://permalink.gmane.org/gmane.linux.kernel.iommu/5552
> > > > > 
> > > > > It's changed significantly since then, based on helpful feedback from
> > > > > Alex. In particular, I no longer butcher the IOMMU API, instead making
> > > > > use of a new iommu_attr to indicate that a domain requires support for
> > > > > nesting.
> > > > > 
> > > > > All feedback welcome (this is still an RFC after all),
> > > > 
> > > > It's a lot cleaner this way, but I'm concerned that allowing a "nesting"
> > > > IOMMU to be created and used just like a TYPE1v2 IOMMU, even if the
> > > > IOMMU doesn't support it, puts us in a corner for compatibility later.
> > > > The safer approach might be to fail the attach_group function if we
> > > > can't set the domain attribute, ie. enforce that the IOMMU must support
> > > > it. 
> > > 
> > > I also contemplated this (and was my source of confusion when we were
> > > discussing using iommu_attr before), however from a user's perspective it's
> > > not at all clear what's gone wrong. Essentially, VFIO would advertise
> > > support for VFIO_TYPE1_NESTING_IOMMU but any attempt to VFIO_SET_IOMMU
> > > for a container would fail. Knowing to try and fall-back on VFIO_TYPE1v2_IOMMU
> > > isn't at all obvious.
> > > 
> > > > I don't know how you'd handle it if only some of the domains within
> > > > a container supported it, so enforcement may simplify things down the
> > > > road too.  We wouldn't need the vfio_domains_have_iommu_nesting()
> > > > function that way either.
> > > 
> > > Agreed, it would certainly make things simpler in the kernel. Although, if
> > > we allow a subset of domains in a container to use nesting, I think that's
> > > possible by only allowing groups in those domains to be part of the virtual
> > > SMMU interface.
> > > 
> > > Do you think that returning something like -EOPNOTSUPP from an attach is
> > > sufficient for userspace to figure out what's gone wrong?
> > 
> > It seems like this would all be a little easier if we had some sort of
> > bus_type iterator, then when we want to see if nesting is supported we'd
> > iterate the bus_types, test iommu_present(), iommu_domain_alloc(),
> > iommu_domain_set_attr(), iommu_domain_free().  We'd only allow a nesting
> > domain to be set if the IOMMU driver for at least one bus_type supports
> > it.  Then I think enforcing it would be a little more obvious to
> > userspace.  There is a bus_kset, but we'd need an iterator or at least
> > to enable find_bus() and have type1 walk a list of bus names known to
> > possibly support nesting (uglier, but functional).  Thanks,
> 
> That would work fine if we only had one IOMMU instance per bus. The problem
> is, the ARM SMMU driver (at least) can handle multiple SMMU instances on a
> single bus, only some of which may be capable of nesting. Until we've done
> an attach, it's impossible to know what the capabilities are (since the
> attach is what binds the domain to a physical IOMMU).
> 
> So you'd actually need to iterate the bus_type, test iommu_present(),
> iommu_domain_alloc(), iommu_domain_attach(), iommu_domain_free(). The
> attach, of course, then requires devices which means you ultimately end up
> rolling VFIO_SET_IOMMU and VFIO_SET_CONTAINER into a single operation. I
> don't think that helps the group abstraction much!

I don't really see how that diminishes the value.  It still means that
it's only possible to set the IOMMU type of a container to nesting if
it's possible to support a nesting IOMMU.  The user would get an error
trying to attach any group to the container that can't be supported as
nesting, whether that's behind the wrong bus_type or behind the wrong
IOMMU in the correct bus_type.  The iommu-sysfs extensions that recently
went into the iommu next branch would help to expose this kind of
information to the user.

It seems like an improvement versus statically advertising support for
something that actually cannot be enabled.  Thanks,

Alex

Re: [RFC PATCHv2 0/3] Support for nesting IOMMUs in VFIO

[Will Deacon]

On Thu, Jul 10, 2014 at 05:14:05PM +0100, Alex Williamson wrote:
> On Thu, 2014-07-10 at 16:45 +0100, Will Deacon wrote:
> > On Thu, Jul 10, 2014 at 04:38:18PM +0100, Alex Williamson wrote:
> > > On Thu, 2014-07-10 at 12:34 +0100, Will Deacon wrote:
> > > > Do you think that returning something like -EOPNOTSUPP from an attach is
> > > > sufficient for userspace to figure out what's gone wrong?
> > > 
> > > It seems like this would all be a little easier if we had some sort of
> > > bus_type iterator, then when we want to see if nesting is supported we'd
> > > iterate the bus_types, test iommu_present(), iommu_domain_alloc(),
> > > iommu_domain_set_attr(), iommu_domain_free().  We'd only allow a nesting
> > > domain to be set if the IOMMU driver for at least one bus_type supports
> > > it.  Then I think enforcing it would be a little more obvious to
> > > userspace.  There is a bus_kset, but we'd need an iterator or at least
> > > to enable find_bus() and have type1 walk a list of bus names known to
> > > possibly support nesting (uglier, but functional).  Thanks,
> > 
> > That would work fine if we only had one IOMMU instance per bus. The problem
> > is, the ARM SMMU driver (at least) can handle multiple SMMU instances on a
> > single bus, only some of which may be capable of nesting. Until we've done
> > an attach, it's impossible to know what the capabilities are (since the
> > attach is what binds the domain to a physical IOMMU).
> > 
> > So you'd actually need to iterate the bus_type, test iommu_present(),
> > iommu_domain_alloc(), iommu_domain_attach(), iommu_domain_free(). The
> > attach, of course, then requires devices which means you ultimately end up
> > rolling VFIO_SET_IOMMU and VFIO_SET_CONTAINER into a single operation. I
> > don't think that helps the group abstraction much!
> 
> I don't really see how that diminishes the value.  It still means that
> it's only possible to set the IOMMU type of a container to nesting if
> it's possible to support a nesting IOMMU.  The user would get an error
> trying to attach any group to the container that can't be supported as
> nesting, whether that's behind the wrong bus_type or behind the wrong
> IOMMU in the correct bus_type.  The iommu-sysfs extensions that recently
> went into the iommu next branch would help to expose this kind of
> information to the user.
> 
> It seems like an improvement versus statically advertising support for
> something that actually cannot be enabled.  Thanks,

Ok, I suppose it's an improvement. Given that we don't have a bus_type
iterator, would you object if I only do this for the pci_bus_type for now
(i.e. the only bus that's VFIO currently supports).

Will