[PATCH v2 0/5] Implement SMMU passthrough using the default domain

[PATCH v2 0/5] Implement SMMU passthrough using the default domain

[Will Deacon]

Hi all,

This is version two of the patches previously posted here:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/481275.html

I've addressed the review feedback I receive there, but the main change
is that the SMMUv3 driver now handles passthrough domains independently
of the disable_bypass parameter.

All feedback welcome,

Will

--->8

Will Deacon (5):
  iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains
  iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains
  iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void
  iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY
    domains
  iommu: Allow default domain type to be set on the kernel command line

 Documentation/admin-guide/kernel-parameters.txt |  6 ++
 drivers/iommu/arm-smmu-v3.c                     | 76 +++++++++++++++----------
 drivers/iommu/arm-smmu.c                        | 26 ++++++++-
 drivers/iommu/iommu.c                           | 17 +++++-
 4 files changed, 90 insertions(+), 35 deletions(-)

-- 
2.1.4

[PATCH v2 1/5] iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains

[Will Deacon]

The ARM SMMU drivers provide a DOMAIN_ATTR_NESTING domain attribute,
which allows callers of the IOMMU API to request that the page table
for a domain is installed at stage-2, if supported by the hardware.

Since setting this attribute only makes sense for UNMANAGED domains,
this patch returns -ENODEV if the domain_{get,set}_attr operations are
called on other domain types.

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 6 ++++++
 drivers/iommu/arm-smmu.c    | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 5806a6acc94e..3d38e682071a 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1837,6 +1837,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -EINVAL;
+
 	switch (attr) {
 	case DOMAIN_ATTR_NESTING:
 		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
@@ -1852,6 +1855,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	int ret = 0;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -EINVAL;
+
 	mutex_lock(&smmu_domain->init_mutex);
 
 	switch (attr) {
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index abf6496843a6..6426819348be 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1549,6 +1549,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -EINVAL;
+
 	switch (attr) {
 	case DOMAIN_ATTR_NESTING:
 		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
@@ -1564,6 +1567,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	int ret = 0;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -EINVAL;
+
 	mutex_lock(&smmu_domain->init_mutex);
 
 	switch (attr) {
-- 
2.1.4

[PATCH v2 2/5] iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains

[Will Deacon]

In preparation for allowing the default domain type to be overridden,
this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
ARM SMMU driver.

An identity domain is created by placing the corresponding S2CR
registers into "bypass" mode, which allows transactions to flow through
the SMMU without any translation.

Reviewed-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 6426819348be..04530e968132 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -416,6 +416,7 @@ enum arm_smmu_domain_stage {
 	ARM_SMMU_DOMAIN_S1 = 0,
 	ARM_SMMU_DOMAIN_S2,
 	ARM_SMMU_DOMAIN_NESTED,
+	ARM_SMMU_DOMAIN_BYPASS,
 };
 
 struct arm_smmu_domain {
@@ -838,6 +839,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	if (smmu_domain->smmu)
 		goto out_unlock;
 
+	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
+		smmu_domain->smmu = smmu;
+		goto out_unlock;
+	}
+
 	/*
 	 * Mapping the requested stage onto what we support is surprisingly
 	 * complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -998,7 +1005,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	void __iomem *cb_base;
 	int irq;
 
-	if (!smmu)
+	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
 		return;
 
 	/*
@@ -1021,7 +1028,9 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+	if (type != IOMMU_DOMAIN_UNMANAGED &&
+	    type != IOMMU_DOMAIN_DMA &&
+	    type != IOMMU_DOMAIN_IDENTITY)
 		return NULL;
 	/*
 	 * Allocate the domain and initialise some of its data structures.
@@ -1250,10 +1259,15 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 {
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
-	enum arm_smmu_s2cr_type type = S2CR_TYPE_TRANS;
 	u8 cbndx = smmu_domain->cfg.cbndx;
+	enum arm_smmu_s2cr_type type;
 	int i, idx;
 
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS)
+		type = S2CR_TYPE_BYPASS;
+	else
+		type = S2CR_TYPE_TRANS;
+
 	for_each_cfg_sme(fwspec, i, idx) {
 		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
 			continue;
-- 
2.1.4

[PATCH v2 3/5] iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void

[Will Deacon]

arm_smmu_install_ste_for_dev cannot fail and always returns 0, however
the fact that it returns int means that callers end up implementing
redundant error handling code which complicates STE tracking and is
never executed.

This patch changes the return type of arm_smmu_install_ste_for_dev
to avoid, to make it explicit that it cannot fail.

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

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 3d38e682071a..e18dbcd26f66 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1579,7 +1579,7 @@ static __le64 *arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid)
 	return step;
 }
 
-static int arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec)
+static void arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec)
 {
 	int i;
 	struct arm_smmu_master_data *master = fwspec->iommu_priv;
@@ -1591,8 +1591,6 @@ static int arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec)
 
 		arm_smmu_write_strtab_ent(smmu, sid, step, &master->ste);
 	}
-
-	return 0;
 }
 
 static void arm_smmu_detach_dev(struct device *dev)
@@ -1600,8 +1598,7 @@ static void arm_smmu_detach_dev(struct device *dev)
 	struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
 
 	master->ste.bypass = true;
-	if (arm_smmu_install_ste_for_dev(dev->iommu_fwspec) < 0)
-		dev_warn(dev, "failed to install bypass STE\n");
+	arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
 }
 
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -1653,10 +1650,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		ste->s2_cfg = &smmu_domain->s2_cfg;
 	}
 
-	ret = arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
-	if (ret < 0)
-		ste->valid = false;
-
+	arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
 out_unlock:
 	mutex_unlock(&smmu_domain->init_mutex);
 	return ret;
-- 
2.1.4

[PATCH v2 4/5] iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains

[Will Deacon]

In preparation for allowing the default domain type to be overridden,
this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
ARM SMMUv3 driver.

An identity domain is created by placing the corresponding stream table
entries into "bypass" mode, which allows transactions to flow through
the SMMU without any translation.

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

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index e18dbcd26f66..75fa4809f49e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -554,9 +554,14 @@ struct arm_smmu_s2_cfg {
 };
 
 struct arm_smmu_strtab_ent {
-	bool				valid;
-
-	bool				bypass;	/* Overrides s1/s2 config */
+	/*
+	 * An STE is "assigned" if the master emitting the corresponding SID
+	 * is attached to a domain. The behaviour of an unassigned STE is
+	 * determined by the disable_bypass parameter, whereas an assigned
+	 * STE behaves according to s1_cfg/s2_cfg, which themselves are
+	 * configured according to the domain type.
+	 */
+	bool				assigned;
 	struct arm_smmu_s1_cfg		*s1_cfg;
 	struct arm_smmu_s2_cfg		*s2_cfg;
 };
@@ -632,6 +637,7 @@ enum arm_smmu_domain_stage {
 	ARM_SMMU_DOMAIN_S1 = 0,
 	ARM_SMMU_DOMAIN_S2,
 	ARM_SMMU_DOMAIN_NESTED,
+	ARM_SMMU_DOMAIN_BYPASS,
 };
 
 struct arm_smmu_domain {
@@ -1005,9 +1011,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 	 * This is hideously complicated, but we only really care about
 	 * three cases at the moment:
 	 *
-	 * 1. Invalid (all zero) -> bypass  (init)
-	 * 2. Bypass -> translation (attach)
-	 * 3. Translation -> bypass (detach)
+	 * 1. Invalid (all zero) -> bypass/fault (init)
+	 * 2. Bypass/fault -> translation/bypass (attach)
+	 * 3. Translation/bypass -> bypass/fault (detach)
 	 *
 	 * Given that we can't update the STE atomically and the SMMU
 	 * doesn't read the thing in a defined order, that leaves us
@@ -1046,11 +1052,15 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 	}
 
 	/* Nuke the existing STE_0 value, as we're going to rewrite it */
-	val = ste->valid ? STRTAB_STE_0_V : 0;
+	val = STRTAB_STE_0_V;
+
+	/* Bypass/fault */
+	if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) {
+		if (!ste->assigned && disable_bypass)
+			val |= STRTAB_STE_0_CFG_ABORT;
+		else
+			val |= STRTAB_STE_0_CFG_BYPASS;
 
-	if (ste->bypass) {
-		val |= disable_bypass ? STRTAB_STE_0_CFG_ABORT
-				      : STRTAB_STE_0_CFG_BYPASS;
 		dst[0] = cpu_to_le64(val);
 		dst[1] = cpu_to_le64(STRTAB_STE_1_SHCFG_INCOMING
 			 << STRTAB_STE_1_SHCFG_SHIFT);
@@ -1111,10 +1121,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 static void arm_smmu_init_bypass_stes(u64 *strtab, unsigned int nent)
 {
 	unsigned int i;
-	struct arm_smmu_strtab_ent ste = {
-		.valid	= true,
-		.bypass	= true,
-	};
+	struct arm_smmu_strtab_ent ste = { .assigned = false };
 
 	for (i = 0; i < nent; ++i) {
 		arm_smmu_write_strtab_ent(NULL, -1, strtab, &ste);
@@ -1378,7 +1385,9 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+	if (type != IOMMU_DOMAIN_UNMANAGED &&
+	    type != IOMMU_DOMAIN_DMA &&
+	    type != IOMMU_DOMAIN_IDENTITY)
 		return NULL;
 
 	/*
@@ -1509,6 +1518,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 
+	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
+		return 0;
+	}
+
 	/* Restrict the stage to what we can actually support */
 	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
 		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
@@ -1597,7 +1611,7 @@ static void arm_smmu_detach_dev(struct device *dev)
 {
 	struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
 
-	master->ste.bypass = true;
+	master->ste.assigned = false;
 	arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
 }
 
@@ -1617,7 +1631,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	ste = &master->ste;
 
 	/* Already attached to a different domain? */
-	if (!ste->bypass)
+	if (ste->assigned)
 		arm_smmu_detach_dev(dev);
 
 	mutex_lock(&smmu_domain->init_mutex);
@@ -1638,10 +1652,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		goto out_unlock;
 	}
 
-	ste->bypass = false;
-	ste->valid = true;
+	ste->assigned = true;
 
-	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS) {
+		ste->s1_cfg = NULL;
+		ste->s2_cfg = NULL;
+	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		ste->s1_cfg = &smmu_domain->s1_cfg;
 		ste->s2_cfg = NULL;
 		arm_smmu_write_ctx_desc(smmu, ste->s1_cfg);
@@ -1801,7 +1817,7 @@ static void arm_smmu_remove_device(struct device *dev)
 
 	master = fwspec->iommu_priv;
 	smmu = master->smmu;
-	if (master && master->ste.valid)
+	if (master && master->ste.assigned)
 		arm_smmu_detach_dev(dev);
 	iommu_group_remove_device(dev);
 	iommu_device_unlink(&smmu->iommu, dev);
-- 
2.1.4

[PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line

[Will Deacon]

The IOMMU core currently initialises the default domain for each group
to IOMMU_DOMAIN_DMA, under the assumption that devices will use
IOMMU-backed DMA ops by default. However, in some cases it is desirable
for the DMA ops to bypass the IOMMU for performance reasons, reserving
use of translation for subsystems such as VFIO that require it for
enforcing device isolation.

Rather than modify each IOMMU driver to provide different semantics for
DMA domains, instead we introduce a command line parameter that can be
used to change the type of the default domain. Passthrough can then be
specified using "iommu.passthrough=1" on the kernel command line.

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/admin-guide/kernel-parameters.txt |  6 ++++++
 drivers/iommu/iommu.c                           | 17 +++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 986e44387dad..243895f6872e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1635,6 +1635,12 @@
 		nobypass	[PPC/POWERNV]
 			Disable IOMMU bypass, using IOMMU for PCI devices.
 
+	iommu.passthrough=
+			[ARM64] Configure DMA to bypass the IOMMU by default.
+			Format: { "0" | "1" }
+			0 - Use IOMMU translation for DMA.
+			1 - Bypass the IOMMU for DMA.
+			unset - Use IOMMU translation for DMA.
 
 	io7=		[HW] IO7 for Marvel based alpha systems
 			See comment before marvel_specify_io7 in
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8ea14f41a979..42a842e3f95f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -36,6 +36,7 @@
 
 static struct kset *iommu_group_kset;
 static DEFINE_IDA(iommu_group_ida);
+static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
 
 struct iommu_callback_data {
 	const struct iommu_ops *ops;
@@ -111,6 +112,18 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 static void __iommu_detach_group(struct iommu_domain *domain,
 				 struct iommu_group *group);
 
+static int __init iommu_set_def_domain_type(char *str)
+{
+	bool pt;
+
+	if (!str || strtobool(str, &pt))
+		return -EINVAL;
+
+	iommu_def_domain_type = pt ? IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_DMA;
+	return 0;
+}
+early_param("iommu.passthrough", iommu_set_def_domain_type);
+
 static ssize_t iommu_group_attr_show(struct kobject *kobj,
 				     struct attribute *__attr, char *buf)
 {
@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 	 * IOMMU driver.
 	 */
 	if (!group->default_domain) {
-		group->default_domain = __iommu_domain_alloc(dev->bus,
-							     IOMMU_DOMAIN_DMA);
+		group->default_domain =
+			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
 		if (!group->domain)
 			group->domain = group->default_domain;
 	}
-- 
2.1.4

Re: [PATCH v2 4/5] iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains

[Nate Watterson]

Hi Will,

On 2017-03-10 15:49, Will Deacon wrote:
> In preparation for allowing the default domain type to be overridden,
> this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
> ARM SMMUv3 driver.
> 
> An identity domain is created by placing the corresponding stream table
> entries into "bypass" mode, which allows transactions to flow through
> the SMMU without any translation.
> 

What about masters that require SMMU intervention to override their
native memory attributes to make them consistent with the CCA (acpi)
or dma-coherent (dt) values specified in FW? To make sure those cases
are handled, you could store away the master's coherency setting in
its strtab_ent at attach time and then setup STE[MemAttr/ALLOCCFG/SHCFG]
so the attributes are forced to the correct values while still
bypassing translation.

> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  drivers/iommu/arm-smmu-v3.c | 58 
> +++++++++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index e18dbcd26f66..75fa4809f49e 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -554,9 +554,14 @@ struct arm_smmu_s2_cfg {
>  };
> 
>  struct arm_smmu_strtab_ent {
> -	bool				valid;
> -
> -	bool				bypass;	/* Overrides s1/s2 config */
> +	/*
> +	 * An STE is "assigned" if the master emitting the corresponding SID
> +	 * is attached to a domain. The behaviour of an unassigned STE is
> +	 * determined by the disable_bypass parameter, whereas an assigned
> +	 * STE behaves according to s1_cfg/s2_cfg, which themselves are
> +	 * configured according to the domain type.
> +	 */
> +	bool				assigned;
>  	struct arm_smmu_s1_cfg		*s1_cfg;
>  	struct arm_smmu_s2_cfg		*s2_cfg;
>  };
> @@ -632,6 +637,7 @@ enum arm_smmu_domain_stage {
>  	ARM_SMMU_DOMAIN_S1 = 0,
>  	ARM_SMMU_DOMAIN_S2,
>  	ARM_SMMU_DOMAIN_NESTED,
> +	ARM_SMMU_DOMAIN_BYPASS,
>  };
> 
>  struct arm_smmu_domain {
> @@ -1005,9 +1011,9 @@ static void arm_smmu_write_strtab_ent(struct
> arm_smmu_device *smmu, u32 sid,
>  	 * This is hideously complicated, but we only really care about
>  	 * three cases at the moment:
>  	 *
> -	 * 1. Invalid (all zero) -> bypass  (init)
> -	 * 2. Bypass -> translation (attach)
> -	 * 3. Translation -> bypass (detach)
> +	 * 1. Invalid (all zero) -> bypass/fault (init)
> +	 * 2. Bypass/fault -> translation/bypass (attach)
> +	 * 3. Translation/bypass -> bypass/fault (detach)
>  	 *
>  	 * Given that we can't update the STE atomically and the SMMU
>  	 * doesn't read the thing in a defined order, that leaves us
> @@ -1046,11 +1052,15 @@ static void arm_smmu_write_strtab_ent(struct
> arm_smmu_device *smmu, u32 sid,
>  	}
> 
>  	/* Nuke the existing STE_0 value, as we're going to rewrite it */
> -	val = ste->valid ? STRTAB_STE_0_V : 0;
> +	val = STRTAB_STE_0_V;
> +
> +	/* Bypass/fault */
> +	if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) {
> +		if (!ste->assigned && disable_bypass)
> +			val |= STRTAB_STE_0_CFG_ABORT;
> +		else
> +			val |= STRTAB_STE_0_CFG_BYPASS;
> 
> -	if (ste->bypass) {
> -		val |= disable_bypass ? STRTAB_STE_0_CFG_ABORT
> -				      : STRTAB_STE_0_CFG_BYPASS;
>  		dst[0] = cpu_to_le64(val);
>  		dst[1] = cpu_to_le64(STRTAB_STE_1_SHCFG_INCOMING
>  			 << STRTAB_STE_1_SHCFG_SHIFT);
> @@ -1111,10 +1121,7 @@ static void arm_smmu_write_strtab_ent(struct
> arm_smmu_device *smmu, u32 sid,
>  static void arm_smmu_init_bypass_stes(u64 *strtab, unsigned int nent)
>  {
>  	unsigned int i;
> -	struct arm_smmu_strtab_ent ste = {
> -		.valid	= true,
> -		.bypass	= true,
> -	};
> +	struct arm_smmu_strtab_ent ste = { .assigned = false };
> 
>  	for (i = 0; i < nent; ++i) {
>  		arm_smmu_write_strtab_ent(NULL, -1, strtab, &ste);
> @@ -1378,7 +1385,9 @@ static struct iommu_domain
> *arm_smmu_domain_alloc(unsigned type)
>  {
>  	struct arm_smmu_domain *smmu_domain;
> 
> -	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
> +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> +	    type != IOMMU_DOMAIN_DMA &&
> +	    type != IOMMU_DOMAIN_IDENTITY)
>  		return NULL;
> 
>  	/*
> @@ -1509,6 +1518,11 @@ static int arm_smmu_domain_finalise(struct
> iommu_domain *domain)
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>  	struct arm_smmu_device *smmu = smmu_domain->smmu;
> 
> +	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> +		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> +		return 0;
> +	}
> +
>  	/* Restrict the stage to what we can actually support */
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
>  		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
> @@ -1597,7 +1611,7 @@ static void arm_smmu_detach_dev(struct device 
> *dev)
>  {
>  	struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
> 
> -	master->ste.bypass = true;
> +	master->ste.assigned = false;
>  	arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
>  }
> 
> @@ -1617,7 +1631,7 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
>  	ste = &master->ste;
> 
>  	/* Already attached to a different domain? */
> -	if (!ste->bypass)
> +	if (ste->assigned)
>  		arm_smmu_detach_dev(dev);
> 
>  	mutex_lock(&smmu_domain->init_mutex);
> @@ -1638,10 +1652,12 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
>  		goto out_unlock;
>  	}
> 
> -	ste->bypass = false;
> -	ste->valid = true;
> +	ste->assigned = true;
> 
> -	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS) {
> +		ste->s1_cfg = NULL;
> +		ste->s2_cfg = NULL;
> +	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>  		ste->s1_cfg = &smmu_domain->s1_cfg;
>  		ste->s2_cfg = NULL;
>  		arm_smmu_write_ctx_desc(smmu, ste->s1_cfg);
> @@ -1801,7 +1817,7 @@ static void arm_smmu_remove_device(struct device 
> *dev)
> 
>  	master = fwspec->iommu_priv;
>  	smmu = master->smmu;
> -	if (master && master->ste.valid)
> +	if (master && master->ste.assigned)
>  		arm_smmu_detach_dev(dev);
>  	iommu_group_remove_device(dev);
>  	iommu_device_unlink(&smmu->iommu, dev);

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux
Foundation Collaborative Project.

Re: [PATCH v2 3/5] iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void

[Nate Watterson]

Hi Will,

On 2017-03-10 15:49, Will Deacon wrote:
> arm_smmu_install_ste_for_dev cannot fail and always returns 0, however
> the fact that it returns int means that callers end up implementing
> redundant error handling code which complicates STE tracking and is
> never executed.
> 
> This patch changes the return type of arm_smmu_install_ste_for_dev
> to avoid, to make it explicit that it cannot fail.
Did you mean "a void" or just "void" instead of "avoid"?

> 
> Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu-v3.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 3d38e682071a..e18dbcd26f66 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -1579,7 +1579,7 @@ static __le64 *arm_smmu_get_step_for_sid(struct
> arm_smmu_device *smmu, u32 sid)
>  	return step;
>  }
> 
> -static int arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec)
> +static void arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec)
>  {
>  	int i;
>  	struct arm_smmu_master_data *master = fwspec->iommu_priv;
> @@ -1591,8 +1591,6 @@ static int arm_smmu_install_ste_for_dev(struct
> iommu_fwspec *fwspec)
> 
>  		arm_smmu_write_strtab_ent(smmu, sid, step, &master->ste);
>  	}
> -
> -	return 0;
>  }
> 
>  static void arm_smmu_detach_dev(struct device *dev)
> @@ -1600,8 +1598,7 @@ static void arm_smmu_detach_dev(struct device 
> *dev)
>  	struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
> 
>  	master->ste.bypass = true;
> -	if (arm_smmu_install_ste_for_dev(dev->iommu_fwspec) < 0)
> -		dev_warn(dev, "failed to install bypass STE\n");
> +	arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
>  }
> 
>  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct 
> device *dev)
> @@ -1653,10 +1650,7 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
>  		ste->s2_cfg = &smmu_domain->s2_cfg;
>  	}
> 
> -	ret = arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
> -	if (ret < 0)
> -		ste->valid = false;
> -
> +	arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
>  out_unlock:
>  	mutex_unlock(&smmu_domain->init_mutex);
>  	return ret;

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux
Foundation Collaborative Project.

Re: [PATCH v2 4/5] iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains

[Robin Murphy]

Hi Nate, Will,

On 16/03/17 16:24, Nate Watterson wrote:
> Hi Will,
> 
> On 2017-03-10 15:49, Will Deacon wrote:
>> In preparation for allowing the default domain type to be overridden,
>> this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
>> ARM SMMUv3 driver.
>>
>> An identity domain is created by placing the corresponding stream table
>> entries into "bypass" mode, which allows transactions to flow through
>> the SMMU without any translation.
>>
> 
> What about masters that require SMMU intervention to override their
> native memory attributes to make them consistent with the CCA (acpi)
> or dma-coherent (dt) values specified in FW?

Well, we've already broken them ;) My interpretation of "dma-coherent"
is as the equivalent of DACS=1,CPM=1, i.e. not dependent on SMMU
override. For the CCA=1,DACS=0 case (I'm going to pretend the DT
equivalent will never exist...) the first problem to solve is how to
inherit the appropriate configuration from the firmware, because right
now we're not even pretending to support that.

>  To make sure those cases
> are handled, you could store away the master's coherency setting in
> its strtab_ent at attach time and then setup STE[MemAttr/ALLOCCFG/SHCFG]
> so the attributes are forced to the correct values while still
> bypassing translation.

However, for this particular piece of the puzzle, that does sound about
right - the attribute overrides are pretty much orthogonal to the stage
of translation (or bypass), so the master's strtab_ent will indeed
probably be the most appropriate place to keep them once we get there
(cf. the master's s2cr in SMMUv2).

Now, while I'm here...

>> Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 58
>> +++++++++++++++++++++++++++++----------------
>>  1 file changed, 37 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index e18dbcd26f66..75fa4809f49e 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -554,9 +554,14 @@ struct arm_smmu_s2_cfg {
>>  };
>>
>>  struct arm_smmu_strtab_ent {
>> -    bool                valid;
>> -
>> -    bool                bypass;    /* Overrides s1/s2 config */
>> +    /*
>> +     * An STE is "assigned" if the master emitting the corresponding SID
>> +     * is attached to a domain. The behaviour of an unassigned STE is
>> +     * determined by the disable_bypass parameter, whereas an assigned
>> +     * STE behaves according to s1_cfg/s2_cfg, which themselves are
>> +     * configured according to the domain type.
>> +     */
>> +    bool                assigned;
>>      struct arm_smmu_s1_cfg        *s1_cfg;
>>      struct arm_smmu_s2_cfg        *s2_cfg;
>>  };
>> @@ -632,6 +637,7 @@ enum arm_smmu_domain_stage {
>>      ARM_SMMU_DOMAIN_S1 = 0,
>>      ARM_SMMU_DOMAIN_S2,
>>      ARM_SMMU_DOMAIN_NESTED,
>> +    ARM_SMMU_DOMAIN_BYPASS,
>>  };
>>
>>  struct arm_smmu_domain {
>> @@ -1005,9 +1011,9 @@ static void arm_smmu_write_strtab_ent(struct
>> arm_smmu_device *smmu, u32 sid,
>>       * This is hideously complicated, but we only really care about
>>       * three cases at the moment:
>>       *
>> -     * 1. Invalid (all zero) -> bypass  (init)
>> -     * 2. Bypass -> translation (attach)
>> -     * 3. Translation -> bypass (detach)
>> +     * 1. Invalid (all zero) -> bypass/fault (init)
>> +     * 2. Bypass/fault -> translation/bypass (attach)
>> +     * 3. Translation/bypass -> bypass/fault (detach)
>>       *
>>       * Given that we can't update the STE atomically and the SMMU
>>       * doesn't read the thing in a defined order, that leaves us
>> @@ -1046,11 +1052,15 @@ static void arm_smmu_write_strtab_ent(struct
>> arm_smmu_device *smmu, u32 sid,
>>      }
>>
>>      /* Nuke the existing STE_0 value, as we're going to rewrite it */
>> -    val = ste->valid ? STRTAB_STE_0_V : 0;
>> +    val = STRTAB_STE_0_V;
>> +
>> +    /* Bypass/fault */
>> +    if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) {
>> +        if (!ste->assigned && disable_bypass)

...yuck. After about 5 minutes of staring at that, I've convinced myself
that it would make much more sense to always clear the strtab_ent
configs on detach, such that you never need the outer !ste->assigned
check here...

>> +            val |= STRTAB_STE_0_CFG_ABORT;
>> +        else
>> +            val |= STRTAB_STE_0_CFG_BYPASS;
>>
>> -    if (ste->bypass) {
>> -        val |= disable_bypass ? STRTAB_STE_0_CFG_ABORT
>> -                      : STRTAB_STE_0_CFG_BYPASS;
>>          dst[0] = cpu_to_le64(val);
>>          dst[1] = cpu_to_le64(STRTAB_STE_1_SHCFG_INCOMING
>>               << STRTAB_STE_1_SHCFG_SHIFT);
>> @@ -1111,10 +1121,7 @@ static void arm_smmu_write_strtab_ent(struct
>> arm_smmu_device *smmu, u32 sid,
>>  static void arm_smmu_init_bypass_stes(u64 *strtab, unsigned int nent)
>>  {
>>      unsigned int i;
>> -    struct arm_smmu_strtab_ent ste = {
>> -        .valid    = true,
>> -        .bypass    = true,
>> -    };
>> +    struct arm_smmu_strtab_ent ste = { .assigned = false };
>>
>>      for (i = 0; i < nent; ++i) {
>>          arm_smmu_write_strtab_ent(NULL, -1, strtab, &ste);
>> @@ -1378,7 +1385,9 @@ static struct iommu_domain
>> *arm_smmu_domain_alloc(unsigned type)
>>  {
>>      struct arm_smmu_domain *smmu_domain;
>>
>> -    if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
>> +    if (type != IOMMU_DOMAIN_UNMANAGED &&
>> +        type != IOMMU_DOMAIN_DMA &&
>> +        type != IOMMU_DOMAIN_IDENTITY)
>>          return NULL;
>>
>>      /*
>> @@ -1509,6 +1518,11 @@ static int arm_smmu_domain_finalise(struct
>> iommu_domain *domain)
>>      struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>      struct arm_smmu_device *smmu = smmu_domain->smmu;
>>
>> +    if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>> +        smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
>> +        return 0;
>> +    }
>> +
>>      /* Restrict the stage to what we can actually support */
>>      if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
>>          smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
>> @@ -1597,7 +1611,7 @@ static void arm_smmu_detach_dev(struct device *dev)
>>  {
>>      struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
>>
>> -    master->ste.bypass = true;
>> +    master->ste.assigned = false;
>>      arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
>>  }
>>
>> @@ -1617,7 +1631,7 @@ static int arm_smmu_attach_dev(struct
>> iommu_domain *domain, struct device *dev)
>>      ste = &master->ste;
>>
>>      /* Already attached to a different domain? */
>> -    if (!ste->bypass)
>> +    if (ste->assigned)
>>          arm_smmu_detach_dev(dev);
>>
>>      mutex_lock(&smmu_domain->init_mutex);
>> @@ -1638,10 +1652,12 @@ static int arm_smmu_attach_dev(struct
>> iommu_domain *domain, struct device *dev)
>>          goto out_unlock;
>>      }
>>
>> -    ste->bypass = false;
>> -    ste->valid = true;
>> +    ste->assigned = true;
>>
>> -    if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>> +    if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS) {
>> +        ste->s1_cfg = NULL;
>> +        ste->s2_cfg = NULL;
>> +    } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>>          ste->s1_cfg = &smmu_domain->s1_cfg;
>>          ste->s2_cfg = NULL;

...or all these explicit NULL assignments (and indeed the entire
ARM_SMMU_DOMAIN_BYPASS case) here. Deliberately keeping potentially
stale context pointers hanging around in unassigned strtab_ents seems
silly (it might trick kmemleak, for another thing).

Robin.

>>          arm_smmu_write_ctx_desc(smmu, ste->s1_cfg);
>> @@ -1801,7 +1817,7 @@ static void arm_smmu_remove_device(struct device
>> *dev)
>>
>>      master = fwspec->iommu_priv;
>>      smmu = master->smmu;
>> -    if (master && master->ste.valid)
>> +    if (master && master->ste.assigned)
>>          arm_smmu_detach_dev(dev);
>>      iommu_group_remove_device(dev);
>>      iommu_device_unlink(&smmu->iommu, dev);
> 

Re: [PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line

[Joerg Roedel]

Hi Will,

On Fri, Mar 10, 2017 at 08:49:36PM +0000, Will Deacon wrote:
> @@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>  	 * IOMMU driver.
>  	 */
>  	if (!group->default_domain) {
> -		group->default_domain = __iommu_domain_alloc(dev->bus,
> -							     IOMMU_DOMAIN_DMA);
> +		group->default_domain =
> +			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);

It would be good to have a fall-back here if we are talking to an IOMMU
driver that uses default domains, but does not support identity-mapped
domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this
category. A dev_warn() also makes sense in case allocating a identity
domain fails.


	Joerg

Re: [PATCH v2 0/5] Implement SMMU passthrough using the default domain

[Joerg Roedel]

On Fri, Mar 10, 2017 at 08:49:31PM +0000, Will Deacon wrote:
> Will Deacon (5):
>   iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains
>   iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains
>   iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void
>   iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY
>     domains
>   iommu: Allow default domain type to be set on the kernel command line
> 
>  Documentation/admin-guide/kernel-parameters.txt |  6 ++
>  drivers/iommu/arm-smmu-v3.c                     | 76 +++++++++++++++----------
>  drivers/iommu/arm-smmu.c                        | 26 ++++++++-
>  drivers/iommu/iommu.c                           | 17 +++++-
>  4 files changed, 90 insertions(+), 35 deletions(-)

Besides my one comment on the last patch this series looks good to me.
Do you plan to include it (with the fall-back) into your pull-request?



	Joerg

Re: [PATCH v2 0/5] Implement SMMU passthrough using the default domain

[Will Deacon]

Hi Joerg,

On Tue, Mar 21, 2017 at 04:46:24PM +0100, Joerg Roedel wrote:
> On Fri, Mar 10, 2017 at 08:49:31PM +0000, Will Deacon wrote:
> > Will Deacon (5):
> >   iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains
> >   iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains
> >   iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void
> >   iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY
> >     domains
> >   iommu: Allow default domain type to be set on the kernel command line
> > 
> >  Documentation/admin-guide/kernel-parameters.txt |  6 ++
> >  drivers/iommu/arm-smmu-v3.c                     | 76 +++++++++++++++----------
> >  drivers/iommu/arm-smmu.c                        | 26 ++++++++-
> >  drivers/iommu/iommu.c                           | 17 +++++-
> >  4 files changed, 90 insertions(+), 35 deletions(-)
> 
> Besides my one comment on the last patch this series looks good to me.
> Do you plan to include it (with the fall-back) into your pull-request?

Yes, that would certainly be easiest for me, given the changes to the
SMMU drivers. However, if you prefer it separately then I can do that too.

Cheers,

Will

Re: [PATCH v2 0/5] Implement SMMU passthrough using the default domain

[Joerg Roedel]

On Tue, Mar 21, 2017 at 04:42:41PM +0000, Will Deacon wrote:
> Hi Joerg,
> 
> On Tue, Mar 21, 2017 at 04:46:24PM +0100, Joerg Roedel wrote:
> > On Fri, Mar 10, 2017 at 08:49:31PM +0000, Will Deacon wrote:
> > > Will Deacon (5):
> > >   iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains
> > >   iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains
> > >   iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void
> > >   iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY
> > >     domains
> > >   iommu: Allow default domain type to be set on the kernel command line
> > > 
> > >  Documentation/admin-guide/kernel-parameters.txt |  6 ++
> > >  drivers/iommu/arm-smmu-v3.c                     | 76 +++++++++++++++----------
> > >  drivers/iommu/arm-smmu.c                        | 26 ++++++++-
> > >  drivers/iommu/iommu.c                           | 17 +++++-
> > >  4 files changed, 90 insertions(+), 35 deletions(-)
> > 
> > Besides my one comment on the last patch this series looks good to me.
> > Do you plan to include it (with the fall-back) into your pull-request?
> 
> Yes, that would certainly be easiest for me, given the changes to the
> SMMU drivers. However, if you prefer it separately then I can do that too.

No, just send it with you usual bulk of other smmu patches.


Thanks,

	Joerg

Re: [PATCH v2 4/5] iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains

[Will Deacon]

Hi Robin,

On Thu, Mar 16, 2017 at 06:19:48PM +0000, Robin Murphy wrote:
> On 16/03/17 16:24, Nate Watterson wrote:
> > On 2017-03-10 15:49, Will Deacon wrote:
> >> In preparation for allowing the default domain type to be overridden,
> >> this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
> >> ARM SMMUv3 driver.
> >>
> >> An identity domain is created by placing the corresponding stream table
> >> entries into "bypass" mode, which allows transactions to flow through
> >> the SMMU without any translation.
> >>
> > 
> > What about masters that require SMMU intervention to override their
> > native memory attributes to make them consistent with the CCA (acpi)
> > or dma-coherent (dt) values specified in FW?
> 
> Well, we've already broken them ;) My interpretation of "dma-coherent"
> is as the equivalent of DACS=1,CPM=1, i.e. not dependent on SMMU
> override. For the CCA=1,DACS=0 case (I'm going to pretend the DT
> equivalent will never exist...) the first problem to solve is how to
> inherit the appropriate configuration from the firmware, because right
> now we're not even pretending to support that.

Indeed, and that would need to be added as a separate patch series when
the need arises.

> >>      /* Nuke the existing STE_0 value, as we're going to rewrite it */
> >> -    val = ste->valid ? STRTAB_STE_0_V : 0;
> >> +    val = STRTAB_STE_0_V;
> >> +
> >> +    /* Bypass/fault */
> >> +    if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) {
> >> +        if (!ste->assigned && disable_bypass)
> 
> ...yuck. After about 5 minutes of staring at that, I've convinced myself
> that it would make much more sense to always clear the strtab_ent
> configs on detach, such that you never need the outer !ste->assigned
> check here...

I was deliberately keeping the strtab_ent intact in case we ever grow
support for nested translation, where we might well want to detach a
stage 1 but keep the stage 2 installed. I don't think the code is that
bad, so I'd like to leave it like it is for now.

Will

Re: [PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line

[Will Deacon]

On Tue, Mar 21, 2017 at 04:45:27PM +0100, Joerg Roedel wrote:
> On Fri, Mar 10, 2017 at 08:49:36PM +0000, Will Deacon wrote:
> > @@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
> >  	 * IOMMU driver.
> >  	 */
> >  	if (!group->default_domain) {
> > -		group->default_domain = __iommu_domain_alloc(dev->bus,
> > -							     IOMMU_DOMAIN_DMA);
> > +		group->default_domain =
> > +			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> 
> It would be good to have a fall-back here if we are talking to an IOMMU
> driver that uses default domains, but does not support identity-mapped
> domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this
> category. A dev_warn() also makes sense in case allocating a identity
> domain fails.

Sure, something like the diff below?

Will

--->8


diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 42a842e3f95f..f787626a745d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 	 * IOMMU driver.
 	 */
 	if (!group->default_domain) {
-		group->default_domain =
-			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
+		struct iommu_domain *dom;
+
+		dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
+		if (!dom) {
+			dev_warn(dev,
+				 "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",
+				 iommu_def_domain_type);
+			dom = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_DMA);
+		}
+
+		group->default_domain = dom;
 		if (!group->domain)
-			group->domain = group->default_domain;
+			group->domain = dom;
 	}
 
 	ret = iommu_group_add_device(group, dev);

Re: [PATCH v2 4/5] iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains

[Robin Murphy]

On 21/03/17 17:08, Will Deacon wrote:
> Hi Robin,
> 
> On Thu, Mar 16, 2017 at 06:19:48PM +0000, Robin Murphy wrote:
>> On 16/03/17 16:24, Nate Watterson wrote:
>>> On 2017-03-10 15:49, Will Deacon wrote:
>>>> In preparation for allowing the default domain type to be overridden,
>>>> this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
>>>> ARM SMMUv3 driver.
>>>>
>>>> An identity domain is created by placing the corresponding stream table
>>>> entries into "bypass" mode, which allows transactions to flow through
>>>> the SMMU without any translation.
>>>>
>>>
>>> What about masters that require SMMU intervention to override their
>>> native memory attributes to make them consistent with the CCA (acpi)
>>> or dma-coherent (dt) values specified in FW?
>>
>> Well, we've already broken them ;) My interpretation of "dma-coherent"
>> is as the equivalent of DACS=1,CPM=1, i.e. not dependent on SMMU
>> override. For the CCA=1,DACS=0 case (I'm going to pretend the DT
>> equivalent will never exist...) the first problem to solve is how to
>> inherit the appropriate configuration from the firmware, because right
>> now we're not even pretending to support that.
> 
> Indeed, and that would need to be added as a separate patch series when
> the need arises.
> 
>>>>      /* Nuke the existing STE_0 value, as we're going to rewrite it */
>>>> -    val = ste->valid ? STRTAB_STE_0_V : 0;
>>>> +    val = STRTAB_STE_0_V;
>>>> +
>>>> +    /* Bypass/fault */
>>>> +    if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) {
>>>> +        if (!ste->assigned && disable_bypass)
>>
>> ...yuck. After about 5 minutes of staring at that, I've convinced myself
>> that it would make much more sense to always clear the strtab_ent
>> configs on detach, such that you never need the outer !ste->assigned
>> check here...
> 
> I was deliberately keeping the strtab_ent intact in case we ever grow
> support for nested translation, where we might well want to detach a
> stage 1 but keep the stage 2 installed. I don't think the code is that
> bad, so I'd like to leave it like it is for now.

Sure, it would certainly be more awkward to recreate this logic from
scratch in future if we need it again. I suggested the cleanup since it
looked like an oversight, but if it's a conscious decision then that's
fine by me.

Robin.

> 
> Will
> 

Re: [PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line

[Robin Murphy]

On 21/03/17 17:21, Will Deacon wrote:
> On Tue, Mar 21, 2017 at 04:45:27PM +0100, Joerg Roedel wrote:
>> On Fri, Mar 10, 2017 at 08:49:36PM +0000, Will Deacon wrote:
>>> @@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>>>  	 * IOMMU driver.
>>>  	 */
>>>  	if (!group->default_domain) {
>>> -		group->default_domain = __iommu_domain_alloc(dev->bus,
>>> -							     IOMMU_DOMAIN_DMA);
>>> +		group->default_domain =
>>> +			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
>>
>> It would be good to have a fall-back here if we are talking to an IOMMU
>> driver that uses default domains, but does not support identity-mapped
>> domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this
>> category. A dev_warn() also makes sense in case allocating a identity
>> domain fails.
> 
> Sure, something like the diff below?
> 
> Will
> 
> --->8
> 
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 42a842e3f95f..f787626a745d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>  	 * IOMMU driver.
>  	 */
>  	if (!group->default_domain) {
> -		group->default_domain =
> -			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> +		struct iommu_domain *dom;
> +
> +		dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> +		if (!dom) {
> +			dev_warn(dev,
> +				 "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",
> +				 iommu_def_domain_type);

Conversely, that's going to be noisy if iommu_def_domain_type was
IOMMU_DOMAIN_DMA to begin with. I think it makes sense to warn if the
user asked for a specific default domain type on the command line and
that didn't work, but maybe not to bother otherwise. Plus, if they asked
for passthrough, then not allocating a default domain at all is probably
closer to the desired result than installing a DMA ops domain would be.

Robin.

> +			dom = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_DMA);
> +		}
> +
> +		group->default_domain = dom;
>  		if (!group->domain)
> -			group->domain = group->default_domain;
> +			group->domain = dom;
>  	}
>  
>  	ret = iommu_group_add_device(group, dev);
> 

Re: [PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line

[Will Deacon]

On Tue, Mar 21, 2017 at 05:46:29PM +0000, Robin Murphy wrote:
> On 21/03/17 17:21, Will Deacon wrote:
> > On Tue, Mar 21, 2017 at 04:45:27PM +0100, Joerg Roedel wrote:
> >> On Fri, Mar 10, 2017 at 08:49:36PM +0000, Will Deacon wrote:
> >>> @@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
> >>>  	 * IOMMU driver.
> >>>  	 */
> >>>  	if (!group->default_domain) {
> >>> -		group->default_domain = __iommu_domain_alloc(dev->bus,
> >>> -							     IOMMU_DOMAIN_DMA);
> >>> +		group->default_domain =
> >>> +			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> >>
> >> It would be good to have a fall-back here if we are talking to an IOMMU
> >> driver that uses default domains, but does not support identity-mapped
> >> domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this
> >> category. A dev_warn() also makes sense in case allocating a identity
> >> domain fails.
> > 
> > Sure, something like the diff below?
> > 
> > Will
> > 
> > --->8
> > 
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 42a842e3f95f..f787626a745d 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
> >  	 * IOMMU driver.
> >  	 */
> >  	if (!group->default_domain) {
> > -		group->default_domain =
> > -			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> > +		struct iommu_domain *dom;
> > +
> > +		dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> > +		if (!dom) {
> > +			dev_warn(dev,
> > +				 "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",
> > +				 iommu_def_domain_type);
> 
> Conversely, that's going to be noisy if iommu_def_domain_type was
> IOMMU_DOMAIN_DMA to begin with. I think it makes sense to warn if the
> user asked for a specific default domain type on the command line and
> that didn't work, but maybe not to bother otherwise. Plus, if they asked
> for passthrough, then not allocating a default domain at all is probably
> closer to the desired result than installing a DMA ops domain would be.

You're right -- I'll hack this about to check if the default type isn't
DOMAIN_DMA before warning about the allocation failure.

Cheers,

Will

Re: [PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line

[Joerg Roedel]

On Tue, Mar 21, 2017 at 05:21:37PM +0000, Will Deacon wrote:
> On Tue, Mar 21, 2017 at 04:45:27PM +0100, Joerg Roedel wrote:
> > On Fri, Mar 10, 2017 at 08:49:36PM +0000, Will Deacon wrote:
> > > @@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
> > >  	 * IOMMU driver.
> > >  	 */
> > >  	if (!group->default_domain) {
> > > -		group->default_domain = __iommu_domain_alloc(dev->bus,
> > > -							     IOMMU_DOMAIN_DMA);
> > > +		group->default_domain =
> > > +			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> > 
> > It would be good to have a fall-back here if we are talking to an IOMMU
> > driver that uses default domains, but does not support identity-mapped
> > domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this
> > category. A dev_warn() also makes sense in case allocating a identity
> > domain fails.
> 
> Sure, something like the diff below?

Yes, this looks good.


Thanks,

	Joerg

Re: [PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line

[Sricharan R]

Hi,

On 3/21/2017 11:47 PM, Will Deacon wrote:
> On Tue, Mar 21, 2017 at 05:46:29PM +0000, Robin Murphy wrote:
>> On 21/03/17 17:21, Will Deacon wrote:
>>> On Tue, Mar 21, 2017 at 04:45:27PM +0100, Joerg Roedel wrote:
>>>> On Fri, Mar 10, 2017 at 08:49:36PM +0000, Will Deacon wrote:
>>>>> @@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>>>>>  	 * IOMMU driver.
>>>>>  	 */
>>>>>  	if (!group->default_domain) {
>>>>> -		group->default_domain = __iommu_domain_alloc(dev->bus,
>>>>> -							     IOMMU_DOMAIN_DMA);
>>>>> +		group->default_domain =
>>>>> +			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
>>>>
>>>> It would be good to have a fall-back here if we are talking to an IOMMU
>>>> driver that uses default domains, but does not support identity-mapped
>>>> domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this
>>>> category. A dev_warn() also makes sense in case allocating a identity
>>>> domain fails.
>>>
>>> Sure, something like the diff below?
>>>
>>> Will
>>>
>>> --->8
>>>
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 42a842e3f95f..f787626a745d 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>>>  	 * IOMMU driver.
>>>  	 */
>>>  	if (!group->default_domain) {
>>> -		group->default_domain =
>>> -			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
>>> +		struct iommu_domain *dom;
>>> +
>>> +		dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
>>> +		if (!dom) {
>>> +			dev_warn(dev,
>>> +				 "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",
>>> +				 iommu_def_domain_type);
>>
>> Conversely, that's going to be noisy if iommu_def_domain_type was
>> IOMMU_DOMAIN_DMA to begin with. I think it makes sense to warn if the
>> user asked for a specific default domain type on the command line and
>> that didn't work, but maybe not to bother otherwise. Plus, if they asked
>> for passthrough, then not allocating a default domain at all is probably
>> closer to the desired result than installing a DMA ops domain would be.
>
> You're right -- I'll hack this about to check if the default type isn't
> DOMAIN_DMA before warning about the allocation failure.

if some master devices want 'IDENTITY_DOMAIN' as default (because  those 
devices do not want any iommu resources to be used/dma_ops to be set) 
and some 'DMA_DOMAIN' as default, then should the default be 
'DMA_DOMAIN' and then masters needing IDENTITY_DOMAIN explicitly do an
detach_dev later. This [1] was adding the support for detach_dev
of the default DMA_DOMAINs.

[1] https://patchwork.codeaurora.org/patch/164933/

Regards,
  Sricharan

Re: [PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line

[Sricharan R]

Hi,

Replying again, as there was some issue with mailer last time.

On 3/21/2017 11:47 PM, Will Deacon wrote:
> On Tue, Mar 21, 2017 at 05:46:29PM +0000, Robin Murphy wrote:
>> On 21/03/17 17:21, Will Deacon wrote:
>>> On Tue, Mar 21, 2017 at 04:45:27PM +0100, Joerg Roedel wrote:
>>>> On Fri, Mar 10, 2017 at 08:49:36PM +0000, Will Deacon wrote:
>>>>> @@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>>>>>  	 * IOMMU driver.
>>>>>  	 */
>>>>>  	if (!group->default_domain) {
>>>>> -		group->default_domain = __iommu_domain_alloc(dev->bus,
>>>>> -							     IOMMU_DOMAIN_DMA);
>>>>> +		group->default_domain =
>>>>> +			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
>>>>
>>>> It would be good to have a fall-back here if we are talking to an IOMMU
>>>> driver that uses default domains, but does not support identity-mapped
>>>> domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this
>>>> category. A dev_warn() also makes sense in case allocating a identity
>>>> domain fails.
>>>
>>> Sure, something like the diff below?
>>>
>>> Will
>>>
>>> --->8
>>>
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 42a842e3f95f..f787626a745d 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>>>  	 * IOMMU driver.
>>>  	 */
>>>  	if (!group->default_domain) {
>>> -		group->default_domain =
>>> -			__iommu_domain_alloc(dev->bus, iommu_def_domain_type);
>>> +		struct iommu_domain *dom;
>>> +
>>> +		dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
>>> +		if (!dom) {
>>> +			dev_warn(dev,
>>> +				 "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",
>>> +				 iommu_def_domain_type);
>>
>> Conversely, that's going to be noisy if iommu_def_domain_type was
>> IOMMU_DOMAIN_DMA to begin with. I think it makes sense to warn if the
>> user asked for a specific default domain type on the command line and
>> that didn't work, but maybe not to bother otherwise. Plus, if they asked
>> for passthrough, then not allocating a default domain at all is probably
>> closer to the desired result than installing a DMA ops domain would be.
>
> You're right -- I'll hack this about to check if the default type isn't
> DOMAIN_DMA before warning about the allocation failure.

if some master devices want 'IDENTITY_DOMAIN' as default (because
those devices do not want any iommu resources to be used/dma_ops to be
set) and some 'DMA_DOMAIN' as default, then should the default be
'DMA_DOMAIN' and then masters needing IDENTITY_DOMAIN explicitly do an
detach_dev later. This [1] was adding the support for detach_dev
of the default DMA_DOMAINs.

[1] https://patchwork.codeaurora.org/patch/164933/

Regards,
  Sricharan