Re: [PATCH v2 01/15] iommu/arm-smmu: Allow IOMMU enabled devices to skip DMA domains

[Jordan Crouse <jcrouse@codeaurora.org>]

On Tue, May 21, 2019 at 06:43:34PM +0100, Robin Murphy wrote:
> On 21/05/2019 17:13, Jordan Crouse wrote:
> >Allow IOMMU enabled devices specified on an opt-in list to create a
> >default identity domain for a new IOMMU group and bypass the DMA
> >domain created by the IOMMU core. This allows the group to be properly
> >set up but otherwise skips touching the hardware until the client
> >device attaches a unmanaged domain of its own.
> 
> All the cool kids are using iommu_request_dm_for_dev() to force an identity
> domain for particular devices, won't that suffice for this case too? There
> is definite scope for improvement in this area, so I'd really like to keep
> things as consistent as possible to make that easier in future.

I initially rejected iommu_request_dm_for_dev() since it still allowed the DMA
domain to consume the context bank but now that I look at it again as long as
the domain free returns the context bank to the pool it might work. Let me give
it a shot and see if it does what we need.

Jordan

> >Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> >---
> >
> >  drivers/iommu/arm-smmu.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/iommu/iommu.c    | 29 +++++++++++++++++++++++------
> >  include/linux/iommu.h    |  3 +++
> >  3 files changed, 68 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >index 5e54cc0..a795ada 100644
> >--- a/drivers/iommu/arm-smmu.c
> >+++ b/drivers/iommu/arm-smmu.c
> >@@ -1235,6 +1235,35 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
> >  	return 0;
> >  }
> >+struct arm_smmu_client_match_data {
> >+	bool use_identity_domain;
> >+};
> >+
> >+static const struct arm_smmu_client_match_data qcom_adreno = {
> >+	.use_identity_domain = true,
> >+};
> >+
> >+static const struct arm_smmu_client_match_data qcom_mdss = {
> >+	.use_identity_domain = true,
> >+};
> >+
> >+static const struct of_device_id arm_smmu_client_of_match[] = {
> >+	{ .compatible = "qcom,adreno", .data = &qcom_adreno },
> >+	{ .compatible = "qcom,mdp4", .data = &qcom_mdss },
> >+	{ .compatible = "qcom,mdss", .data = &qcom_mdss },
> >+	{ .compatible = "qcom,sdm845-mdss", .data = &qcom_mdss },
> >+	{},
> >+};
> >+
> >+static const struct arm_smmu_client_match_data *
> >+arm_smmu_client_data(struct device *dev)
> >+{
> >+	const struct of_device_id *match =
> >+		of_match_device(arm_smmu_client_of_match, dev);
> >+
> >+	return match ? match->data : NULL;
> >+}
> >+
> >  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  {
> >  	int ret;
> >@@ -1552,6 +1581,7 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
> >  {
> >  	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >  	struct arm_smmu_device *smmu = fwspec_smmu(fwspec);
> >+	const struct arm_smmu_client_match_data *client;
> >  	struct iommu_group *group = NULL;
> >  	int i, idx;
> >@@ -1573,6 +1603,18 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
> >  	else
> >  		group = generic_device_group(dev);
> >+	client = arm_smmu_client_data(dev);
> >+
> >+	/*
> >+	 * If the client chooses to bypass the dma domain, create a identity
> >+	 * domain as a default placeholder. This will give the device a
> >+	 * default domain but skip DMA operations and not consume a context
> >+	 * bank
> >+	 */
> >+	if (client && client->no_dma_domain)
> >+		iommu_group_set_default_domain(group, dev,
> >+			IOMMU_DOMAIN_IDENTITY);
> >+
> >  	return group;
> >  }
> >diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >index 67ee662..af3e1ed 100644
> >--- a/drivers/iommu/iommu.c
> >+++ b/drivers/iommu/iommu.c
> >@@ -1062,6 +1062,24 @@ struct iommu_group *fsl_mc_device_group(struct device *dev)
> >  	return group;
> >  }
> >+struct iommu_domain *iommu_group_set_default_domain(struct iommu_group *group,
> >+		struct device *dev, unsigned int type)
> >+{
> >+	struct iommu_domain *dom;
> >+
> >+	dom = __iommu_domain_alloc(dev->bus, type);
> >+	if (!dom)
> >+		return NULL;
> >+
> >+	/* FIXME: Error if the default domain is already set? */
> >+	group->default_domain = dom;
> >+	if (!group->domain)
> >+		group->domain = dom;
> >+
> >+	return dom;
> >+}
> >+EXPORT_SYMBOL_GPL(iommu_group_set_default_domain);
> >+
> >  /**
> >   * iommu_group_get_for_dev - Find or create the IOMMU group for a device
> >   * @dev: target device
> >@@ -1099,9 +1117,12 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
> >  	if (!group->default_domain) {
> >  		struct iommu_domain *dom;
> >-		dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> >+		dom = iommu_group_set_default_domain(group, dev,
> >+			iommu_def_domain_type);
> >+
> >  		if (!dom && iommu_def_domain_type != IOMMU_DOMAIN_DMA) {
> >-			dom = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_DMA);
> >+			dom = iommu_group_set_default_domain(group, dev,
> >+				IOMMU_DOMAIN_DMA);
> >  			if (dom) {
> >  				dev_warn(dev,
> >  					 "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",
> >@@ -1109,10 +1130,6 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
> >  			}
> >  		}
> >-		group->default_domain = dom;
> >-		if (!group->domain)
> >-			group->domain = dom;
> >-
> >  		if (dom && !iommu_dma_strict) {
> >  			int attr = 1;
> >  			iommu_domain_set_attr(dom,
> >diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> >index a815cf6..4ef8bd5 100644
> >--- a/include/linux/iommu.h
> >+++ b/include/linux/iommu.h
> >@@ -394,6 +394,9 @@ extern int iommu_group_id(struct iommu_group *group);
> >  extern struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> >  extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
> >+struct iommu_domain *iommu_group_set_default_domain(struct iommu_group *group,
> >+		struct device *dev, unsigned int type);
> >+
> >  extern int iommu_domain_get_attr(struct iommu_domain *domain, enum iommu_attr,
> >  				 void *data);
> >  extern int iommu_domain_set_attr(struct iommu_domain *domain, enum iommu_attr,
> >

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project