[PATCH v6 01/25] arm: dma-mapping: add support for creating reserved mappings in iova space

[Robin Murphy]

Hi Marek,

On 04/05/15 09:15, Marek Szyprowski wrote:
> Some devices (like frame buffers) are enabled by bootloader and configured
> to perform DMA operations automatically (like displaying boot logo or splash
> screen). Such devices operate and perform DMA operation usually until the
> proper driver for them is loaded and probed. However before that happens,
> system usually loads IOMMU drivers and configures dma parameters for each
> device. When such initial configuration is created and enabled, it usually
> contains empty translation rules betweem IO address space and physical
> memory, because no buffers nor memory regions have been requested by the
> respective driver.
>
> This patch adds support for "iommu-reserved-mapping", which can be used
> to provide definitions for mappings that need to be created on system
> boot to let such devices (enabled by bootloader) to operate properly
> until respective driver is probed.

This appears to only work if you assume the driver is going to tear down 
the existing domain entirely; what about drivers that don't manage 
IOMMUs explicitly, or if there are multiple active devices behind the 
same IOMMU which (in future) start out in the same default domain? If 
any device is happy to remain in the default domain then it would be 
nice to clear the reservations once they are no longer needed.

Could we not address the issue in a more robust way, like fleshing out 
an implementation of the nascent IOMMU_DOMAIN_IDENTITY type, then just 
flagging such devices to stipulate that their boot-time default domain 
must be an identity-mapped one?

> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
> ---
>   Documentation/devicetree/bindings/iommu/iommu.txt |  44 +++++++++
>   arch/arm/mm/dma-mapping.c                         | 112 ++++++++++++++++++++++
>   2 files changed, 156 insertions(+)
>
[...]
> @@ -2048,6 +2092,66 @@ void arm_iommu_detach_device(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
>
> +static int arm_iommu_add_reserved(struct device *dev,
> +                       struct dma_iommu_mapping *domain, phys_addr_t phys,
> +                       dma_addr_t dma, size_t size)
> +{
> +       int ret;
> +
> +       ret = __reserve_iova(domain, dma, size);
> +       if (ret) {
> +               dev_err(dev, "failed to reserve mapping\n");
> +               return -EINVAL;
> +       }
> +
> +       ret = iommu_map(domain->domain, dma, phys, size, IOMMU_READ);
> +       if (ret != 0) {
> +               dev_err(dev, "create IOMMU mapping\n");
> +               return ret;
> +       }
> +
> +       dev_info(dev, "created reserved DMA mapping (%pa -> %pad, %zu bytes)\n",
> +                &phys, &dma, size);
> +
> +       return 0;
> +}
> +
> +static int arm_iommu_init_reserved(struct device *dev,
> +                                  struct dma_iommu_mapping *domain)
> +{
> +       const char *name = "iommu-reserved-mapping";
> +       const __be32 *prop = NULL;
> +       int len, naddr, nsize;
> +       struct device_node *node = dev->of_node;
> +       phys_addr_t phys;
> +       dma_addr_t dma;
> +       size_t size;
> +
> +       if (!node)
> +               return 0;
> +
> +       naddr = of_n_addr_cells(node);
> +       nsize = of_n_size_cells(node);
> +
> +       prop = of_get_property(node, name, &len);
> +       if (!prop)
> +               return 0;
> +
> +       len /= sizeof(u32);
> +
> +       if (len < 2 * naddr + nsize) {
> +               dev_err(dev, "invalid length (%d cells) of %s property\n",
> +                       len, name);
> +               return -EINVAL;
> +       }
> +
> +       phys = of_read_number(prop, naddr);
> +       dma = of_read_number(prop + naddr, naddr);
> +       size = of_read_number(prop + 2*naddr, nsize);
> +
> +       return arm_iommu_add_reserved(dev, domain, phys, dma, size);
 > +}

I may be missing something, but I don't see how this can handle multiple 
ranges for the same device as the binding says.

>   static struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
>   {
>          return coherent ? &iommu_coherent_ops : &iommu_ops;
> @@ -2068,6 +2172,14 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
>                  return false;
>          }
>
> +       if (arm_iommu_init_reserved(dev, mapping) != 0) {
> +               pr_warn("Failed to initialize reserved mapping for device %s\n",
> +                       dev_name(dev));
> +               __arm_iommu_detach_device(dev);
> +               arm_iommu_release_mapping(mapping);
> +               return false;
> +       }
> +
>          if (__arm_iommu_attach_device(dev, mapping)) {
>                  pr_warn("Failed to attached device %s to IOMMU_mapping\n",
>                                  dev_name(dev));

I'm hoping Joerg is still working on his default domain series, because 
the domain creation in arch_setup_dma_ops turns out to be horrible on a 
number of levels (like everything happening in the wrong order for 
platform devices). If that doesn't negate this issue entirely, it's 
going to significantly break this way of hooking up the solution 
(depending on what the drivers do) - worth some consideration, at least.

Robin.