Re: [PATCH v2 1/5] dma-mapping: add dma_{map,unmap}_page_attrs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Robin,

On Wednesday 03 February 2016 13:31:16 Robin Murphy wrote:
> On 21/01/16 14:01, Niklas Söderlund wrote:
> > Add a version of dmap_{map,unmap}_page that can pass on attributes to
> > the underlaying map_page. This already exists for dma_{map,unmap}_single
> > and dmap_{map,unmap}_sg versions.
> 
> This looks reasonable in isolation, but for the task at hand I'm pretty
> sure it's the wrong thing to do. The problem is that the DMA mapping and
> IOMMU APIs are all geared around dealing with RAM, so what you're going
> to end up with if you use this on an ARM system is the slave device's
> MMIO registers mapped in the IOMMU as Normal memory. The net result of
> that is that the interconnects between the IOMMU's downstream port and
> the slave device are going to have free reign to reorder or merge the
> DMA engine's transactions, and I'm sure you can imagine how utterly
> disastrous that would be for e.g. reading/writing a FIFO. It's even
> worse if the DMA engine is cache-coherent (either naturally, or by
> virtue of the IOMMU), in which case the prospect of reads and writes
> coming out of the IOMMU marked as Normal-Cacheable and allocating into
> system caches is even more terrifyingly unpredictable.
> 
> I spent a little time recently looking into how we might handle platform
> MSIs with IOMMU-backed DMA mapping, and the notion of slave DMA being a
> similar problem did cross my mind, so I'm glad it's already being looked
> at :) My initial thought was that a robust general solution probably
> involves extending the DMA API with something like dma_map_resource(),
> which would be a no-op with no IOMMU (or with IOMMUs like VT-d where
> magic hardware solves the problem),

I had mentioned a dma_map_phys() in a previous reply to one of the patches in 
this thread, so I think we came up to the same conclusion. I'll let you decide 
whether we're both right or wrong, and hope you'll pick the first option :-) 
dma_map_resource() could be a more specific API than dma_map_phys() (depending 
on the parameters it takes), and thus less prone to abuse, or at least more 
explicit.

> interacting with something like the below extension of the IOMMU API
> (plucked from my local MSI proof-of-concept hacks).
> 
> Robin.
> 
> --->8---
> From: Robin Murphy <robin.murphy@xxxxxxx>
> Date: Wed, 23 Sep 2015 15:49:05 +0100
> Subject: [PATCH] iommu: Add MMIO mapping type
> 
> On some platforms, MMIO regions might need slightly different treatment
> compared to mapping regular memory; add the notion of MMIO mappings to
> the IOMMU API's memory type flags, so that callers can let the IOMMU
> drivers know to do the right thing.
> 
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>

This looks good to me.

Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

> ---
>   drivers/iommu/io-pgtable-arm.c | 4 +++-
>   include/linux/iommu.h          | 1 +
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 8bbcbfe..5b5c299 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -363,7 +363,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct
> arm_lpae_io_pgtable *data,
>   			pte |= ARM_LPAE_PTE_HAP_READ;
>   		if (prot & IOMMU_WRITE)
>   			pte |= ARM_LPAE_PTE_HAP_WRITE;
> -		if (prot & IOMMU_CACHE)
> +		if (prot & IOMMU_MMIO)
> +			pte |= ARM_LPAE_PTE_MEMATTR_DEV;
> +		else if (prot & IOMMU_CACHE)
>   			pte |= ARM_LPAE_PTE_MEMATTR_OIWB;
>   		else
>   			pte |= ARM_LPAE_PTE_MEMATTR_NC;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f28dff3..addd25d 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -30,6 +30,7 @@
>   #define IOMMU_WRITE	(1 << 1)
>   #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
>   #define IOMMU_NOEXEC	(1 << 3)
> +#define IOMMU_MMIO	(1 << 4) /* e.g. things like MSI doorbells */
> 
>   struct iommu_ops;
>   struct iommu_group;
> --->8---
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > 
> >   include/asm-generic/dma-mapping-common.h | 20 +++++++++++++-------
> >   1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/asm-generic/dma-mapping-common.h
> > b/include/asm-generic/dma-mapping-common.h index b1bc954..bb08302 100644
> > --- a/include/asm-generic/dma-mapping-common.h
> > +++ b/include/asm-generic/dma-mapping-common.h
> > @@ -74,29 +74,33 @@ static inline void dma_unmap_sg_attrs(struct device
> > *dev, struct scatterlist *sg> 
> >   		ops->unmap_sg(dev, sg, nents, dir, attrs);
> >   
> >   }
> > 
> > -static inline dma_addr_t dma_map_page(struct device *dev, struct page
> > *page, -				      size_t offset, size_t size,
> > -				      enum dma_data_direction dir)
> > +static inline dma_addr_t dma_map_page_attrs(struct device *dev,
> > +					    struct page *page,
> > +					    size_t offset, size_t size,
> > +					    enum dma_data_direction dir,
> > +					    struct dma_attrs *attrs)
> > 
> >   {
> >   
> >   	struct dma_map_ops *ops = get_dma_ops(dev);
> >   	dma_addr_t addr;
> >   	
> >   	kmemcheck_mark_initialized(page_address(page) + offset, size);
> >   	BUG_ON(!valid_dma_direction(dir));
> > 
> > -	addr = ops->map_page(dev, page, offset, size, dir, NULL);
> > +	addr = ops->map_page(dev, page, offset, size, dir, attrs);
> > 
> >   	debug_dma_map_page(dev, page, offset, size, dir, addr, false);
> >   	
> >   	return addr;
> >   
> >   }
> > 
> > -static inline void dma_unmap_page(struct device *dev, dma_addr_t addr,
> > -				  size_t size, enum dma_data_direction dir)
> > +static inline void dma_unmap_page_attrs(struct device *dev, dma_addr_t
> > addr, +					size_t size,
> > +					enum dma_data_direction dir,
> > +					struct dma_attrs *attrs)
> > 
> >   {
> >   
> >   	struct dma_map_ops *ops = get_dma_ops(dev);
> >   	
> >   	BUG_ON(!valid_dma_direction(dir));
> >   	if (ops->unmap_page)
> > 
> > -		ops->unmap_page(dev, addr, size, dir, NULL);
> > +		ops->unmap_page(dev, addr, size, dir, attrs);
> > 
> >   	debug_dma_unmap_page(dev, addr, size, dir, false);
> >   
> >   }
> > 
> > @@ -181,6 +185,8 @@ dma_sync_sg_for_device(struct device *dev, struct
> > scatterlist *sg,> 
> >   #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r,
> >   NULL) #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, NULL)
> >   #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, NULL)
> > 
> > +#define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s, r,
> > NULL) +#define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s,
> > r, NULL)> 
> >   extern int dma_common_mmap(struct device *dev, struct vm_area_struct
> >   *vma,
> >   
> >   			   void *cpu_addr, dma_addr_t dma_addr, size_t size);

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux