[PATCH 8/9] iommu: add support for ARM Ltd. System MMU architecture
Joerg Roedel
joro at 8bytes.org
Fri Jun 21 07:26:46 EST 2013
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 <robherring2 at gmail.com>
> Cc: Andreas Herrmann <andreas.herrmann at calxeda.com>
> Cc: Olav Haugan <ohaugan at codeaurora.org>
> Cc: Joerg Roedel <joro at 8bytes.org>
> Signed-off-by: Will Deacon <will.deacon at arm.com>
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;
> +}
More information about the devicetree-discuss
mailing list