[PATCH 8/9] iommu: add support for ARM Ltd. System MMU architecture

Hi Will,

On Mon, Jun 10, 2013 at 07:34:44PM +0100, Will Deacon wrote:

This patch adds support for SMMUs implementing the ARM System MMU
architecture versions 1 or 2. Both arm and arm64 are supported, although
the v7s descriptor format is not used.

Cc: Rob Herring <redacted>
Cc: Andreas Herrmann <redacted>
Cc: Olav Haugan <redacted>
Cc: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Will Deacon <redacted>

A few general questions:

How have you tested this code? Has it been run on real hardware? What
were the results?

The code looks good and clean in general, minus a few places mentioned
below were I have questions and/or suggestions:

+static struct arm_smmu_device *find_parent_smmu(struct arm_smmu_device *smmu)
+{
+	struct arm_smmu_device *parent, *tmp;
+
+	if (!smmu->parent_of_node)
+		return NULL;
+
+	list_for_each_entry_safe(parent, tmp, &arm_smmu_devices, list)
+		if (parent->dev->of_node == smmu->parent_of_node)
+			return parent;

Why do you need the _safe variant here? You are not changing the list in
this loop so you should be fine with list_for_each_entry().

+
+	dev_warn(smmu->dev,
+		 "Failed to find SMMU parent despite parent in DT\n");
+	return NULL;
+}

+/* Wait for any pending TLB invalidations to complete */
+static void arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
+{
+	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+
+	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_sTLBGSYNC);
+	while (readl_relaxed(gr0_base + ARM_SMMU_GR0_sTLBGSTATUS)
+	       & sTLBGSTATUS_GSACTIVE)
+		cpu_relax();

Other IOMMU drivers have a timeout for this loop and report an error
when the state does not change. I think this makes sense here too so
that the kernel will not just stop spinning in that loop if something
goes wrong but prints an error instead.

+}
+static void arm_smmu_flush_pgtable(struct arm_smmu_device *smmu, void *addr,
+				   size_t size)
+{
+	unsigned long offset = (unsigned long)addr & ~PAGE_MASK;
+
+	/*
+	 * If the SMMU can't walk tables in the CPU caches, treat them
+	 * like non-coherent DMA...
+	 */
+	if (!(smmu->features & ARM_SMMU_FEAT_COHERENT_WALK))
+		dma_map_page(smmu->dev, virt_to_page(addr), offset, size,
+			     DMA_TO_DEVICE);

Why can you call into DMA-API here? A DMA-API implementation may call
back into this IOMMU driver, no? So this looks a little bit like a
layering violation.

+}

+static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
+			phys_addr_t paddr, size_t size, int flags)
+{
+	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_device *smmu = smmu_domain->leaf_smmu;
+
+	if (!smmu_domain || !smmu)
+		return -ENODEV;
+
+	/*
+	 * Check for silent address truncation up the SMMU chain.
+	 */
+	do {
+		phys_addr_t output_mask = (1ULL << smmu->s2_output_size) - 1;
+		if ((phys_addr_t)iova & ~output_mask)
+			return -ERANGE;
+	} while ((smmu = find_parent_smmu(smmu)));

This looks a bit too expensive to have in the map path. How about saving
something like an effective_output_mask (or output_size) which contains
the logical OR of every mask up the path? This would make this check a
lot cheaper.

+
+	return arm_smmu_create_mapping(smmu_domain, iova, paddr, size, flags);
+}
+
+static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
+			     size_t size)
+{
+	int ret;
+	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_cfg *root_cfg = &smmu_domain->root_cfg;
+	struct arm_smmu_device *smmu = root_cfg->smmu;
+	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+
+	ret = arm_smmu_create_mapping(smmu_domain, iova, 0, size, 0);

Since this function does also unmapping, how about renaming it to
arm_smmu_handle_mapping(). The 'create' part in there is misleading.

+	writel_relaxed(root_cfg->vmid, gr0_base + ARM_SMMU_GR0_TLBIVMID);
+	arm_smmu_tlb_sync(smmu);
+	return ret ? ret : size;
+}
+static int arm_smmu_add_device(struct device *dev)
+{
+	struct arm_smmu_device *child, *parent, *smmu;
+	struct arm_smmu_device *tmp[2];
+	struct arm_smmu_master *master = NULL;
+
+	list_for_each_entry_safe(parent, tmp[0], &arm_smmu_devices, list) {

Again, why do you use the _safe variant, you do not seem to change the
lists traversed here.

+		smmu = parent;
+
+		/* Try to find a child of the current SMMU. */
+		list_for_each_entry_safe(child, tmp[1], &arm_smmu_devices, list) {
+			if (child->parent_of_node == parent->dev->of_node) {
+				/* Does the child sit above our master? */
+				master = find_smmu_master(child, dev->of_node);
+				if (master) {
+					smmu = NULL;
+					break;
+				}
+			}
+		}
+
+		/* We found some children, so keep searching. */
+		if (!smmu) {
+			master = NULL;
+			continue;
+		}
+
+		master = find_smmu_master(smmu, dev->of_node);
+		if (master)
+			break;
+	}
+
+	if (!master)
+		return -ENODEV;
+
+	dev->archdata.iommu = smmu;
+	return 0;
+}