diff mbox series

[02/20] iommu/terga-gart: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM

Message ID 2-v1-21cc72fcfb22+a7a-iommu_all_defdom_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series iommu: Make default_domain's mandatory | expand

Commit Message

Jason Gunthorpe May 1, 2023, 6:02 p.m. UTC 
tegra-gart seems to be kind of wonky since from the start its 'detach_dev'
op doesn't actually touch hardware. It is supposed to empty the GART of
all translations loaded into it.

Call this weirdness PLATFORM which keeps the basic original
ops->detach_dev() semantic alive without needing much special core code
support. I'm guessing it really ends up in a BLOCKING configuration, but
without any forced cleanup it is unsafe.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/tegra-gart.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

Comments

Robin Murphy May 3, 2023, 9:17 a.m. UTC | #1 
On 2023-05-01 19:02, Jason Gunthorpe wrote:
> tegra-gart seems to be kind of wonky since from the start its 'detach_dev'
> op doesn't actually touch hardware. It is supposed to empty the GART of
> all translations loaded into it.

No, detach should never tear down translations - what if other devices 
are still using the domain?

> Call this weirdness PLATFORM which keeps the basic original
> ops->detach_dev() semantic alive without needing much special core code
> support. I'm guessing it really ends up in a BLOCKING configuration, but
> without any forced cleanup it is unsafe.

The GART translation aperture is in physical address space, so the truth 
is that all devices have access to it at the same time as having access 
to the rest of physical address space. Attach/detach here really are 
only bookkeeping for which domain currently owns the aperture.

FWIW I wrote up this patch a while ago, not sure if it needs rebasing 
again...

Thanks,
Robin.

----->8-----
Subject: [PATCH] iommu/tegra-gart: Add default identity domain support

The nature of a GART means that supporting identity domains is as easy
as doing nothing, so bring the Tegra driver into the modern world of
default domains with a trivial implementation. Identity domains are
allowed to exist alongside any explicit domain for the translation
aperture, since they both simply represent regions of the physical
address space with no isolation from each other. As such we'll continue
to do the "wrong" thing with groups to allow that to work, since the
notion of isolation that groups represent is counterproductive to the
GART's established usage model.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
  drivers/iommu/tegra-gart.c | 39 +++++++++++++++++++-------------------
  1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index c4136eec1f97..07aa7ea6a306 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -111,7 +111,13 @@ static int gart_iommu_attach_dev(struct 
iommu_domain *domain,

  	spin_lock(&gart->dom_lock);

-	if (gart->active_domain && gart->active_domain != domain) {
+	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+		if (dev_iommu_priv_get(dev)) {
+			dev_iommu_priv_set(dev, NULL);
+			if (--gart->active_devices == 0)
+				gart->active_domain = NULL;
+		}
+	} else if (gart->active_domain && gart->active_domain != domain) {
  		ret = -EINVAL;
  	} else if (dev_iommu_priv_get(dev) != domain) {
  		dev_iommu_priv_set(dev, domain);
@@ -124,28 +130,15 @@ static int gart_iommu_attach_dev(struct 
iommu_domain *domain,
  	return ret;
  }

-static void gart_iommu_set_platform_dma(struct device *dev)
-{
-	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
-	struct gart_device *gart = gart_handle;
-
-	spin_lock(&gart->dom_lock);
-
-	if (dev_iommu_priv_get(dev) == domain) {
-		dev_iommu_priv_set(dev, NULL);
-
-		if (--gart->active_devices == 0)
-			gart->active_domain = NULL;
-	}
-
-	spin_unlock(&gart->dom_lock);
-}
-
  static struct iommu_domain *gart_iommu_domain_alloc(struct device *dev,
  						    unsigned type)
  {
+	static struct iommu_domain identity;
  	struct iommu_domain *domain;

+	if (type == IOMMU_DOMAIN_IDENTITY)
+		return &identity;
+
  	if (type != IOMMU_DOMAIN_UNMANAGED)
  		return NULL;

@@ -162,7 +155,8 @@ static struct iommu_domain 
*gart_iommu_domain_alloc(struct device *dev,
  static void gart_iommu_domain_free(struct iommu_domain *domain)
  {
  	WARN_ON(gart_handle->active_domain == domain);
-	kfree(domain);
+	if (domain->type != IOMMU_DOMAIN_IDENTITY)
+		kfree(domain);
  }

  static inline int __gart_iommu_map(struct gart_device *gart, unsigned 
long iova,
@@ -247,6 +241,11 @@ static struct iommu_device 
*gart_iommu_probe_device(struct device *dev)
  	return &gart_handle->iommu;
  }

+static int gart_iommu_def_domain_type(struct device *dev)
+{
+	return IOMMU_DOMAIN_IDENTITY;
+}
+
  static int gart_iommu_of_xlate(struct device *dev,
  			       struct of_phandle_args *args)
  {
@@ -271,9 +270,9 @@ static const struct iommu_ops gart_iommu_ops = {
  	.domain_alloc	= gart_iommu_domain_alloc,
  	.probe_device	= gart_iommu_probe_device,
  	.device_group	= generic_device_group,
-	.set_platform_dma_ops = gart_iommu_set_platform_dma,
  	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
  	.of_xlate	= gart_iommu_of_xlate,
+	.def_domain_type = gart_iommu_def_domain_type,
  	.default_domain_ops = &(const struct iommu_domain_ops) {
  		.attach_dev	= gart_iommu_attach_dev,
  		.map		= gart_iommu_map,
diff mbox series

Patch

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index a482ff838b5331..09865889ff2480 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -124,11 +124,27 @@  static int gart_iommu_attach_dev(struct iommu_domain *domain,
 	return ret;
 }
 
-static void gart_iommu_set_platform_dma(struct device *dev)
+/*
+ * FIXME: This weird function that doesn't touch the HW, but it is supposed to
+ * zap any current translation from the HW.
+ *
+ * Preserve whatever this was doing in 2011 as basically the same in the
+ * new API. The IOMMU_DOMAIN_PLATFORM's attach_dev function is called at almost
+ * the same times as the old detach_dev.
+ *
+ * I suspect the reality is that the UNMANAGED domain is never actually detached
+ * in real systems, or it is only detached once eveything is already unmapped,
+ * so this could get by this way.
+ */
+static int gart_iommu_platform_attach(struct iommu_domain *identity_domain,
+				      struct device *dev)
 {
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 	struct gart_device *gart = gart_handle;
 
+	if (domain == identity_domain || !domain)
+		return 0;
+
 	spin_lock(&gart->dom_lock);
 
 	if (dev_iommu_priv_get(dev) == domain) {
@@ -139,8 +155,18 @@  static void gart_iommu_set_platform_dma(struct device *dev)
 	}
 
 	spin_unlock(&gart->dom_lock);
+	return 0;
 }
 
+static struct iommu_domain_ops gart_iommu_platform_ops = {
+	.attach_dev = gart_iommu_platform_attach,
+};
+
+static struct iommu_domain gart_iommu_platform_domain = {
+	.type = IOMMU_DOMAIN_PLATFORM,
+	.ops = &gart_iommu_platform_ops,
+};
+
 static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
 {
 	struct iommu_domain *domain;
@@ -267,10 +293,10 @@  static void gart_iommu_sync(struct iommu_domain *domain,
 }
 
 static const struct iommu_ops gart_iommu_ops = {
+	.default_domain = &gart_iommu_platform_domain,
 	.domain_alloc	= gart_iommu_domain_alloc,
 	.probe_device	= gart_iommu_probe_device,
 	.device_group	= generic_device_group,
-	.set_platform_dma_ops = gart_iommu_set_platform_dma,
 	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
 	.of_xlate	= gart_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {