diff mbox

[v5,08/10] arm64/mm: Add support for XPFO to swiotlb

Message ID 20170809200755.11234-9-tycho@docker.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tycho Andersen Aug. 9, 2017, 8:07 p.m. UTC 
From: Juerg Haefliger <juerg.haefliger@hpe.com>

Pages that are unmapped by XPFO need to be mapped before and unmapped
again after (to restore the original state) the __dma_{map,unmap}_area()
operations to prevent fatal page faults.

Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
Signed-off-by: Tycho Andersen <tycho@docker.com>
---
 arch/arm64/include/asm/cacheflush.h | 11 +++++++++
 arch/arm64/mm/dma-mapping.c         | 32 +++++++++++++-------------
 arch/arm64/mm/xpfo.c                | 45 +++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 16 deletions(-)

Comments

Konrad Rzeszutek Wilk Aug. 10, 2017, 1:11 p.m. UTC | #1 
On Wed, Aug 09, 2017 at 02:07:53PM -0600, Tycho Andersen wrote:
> From: Juerg Haefliger <juerg.haefliger@hpe.com>
> 
> Pages that are unmapped by XPFO need to be mapped before and unmapped
> again after (to restore the original state) the __dma_{map,unmap}_area()
> operations to prevent fatal page faults.
> 
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> Signed-off-by: Tycho Andersen <tycho@docker.com>
> ---
>  arch/arm64/include/asm/cacheflush.h | 11 +++++++++
>  arch/arm64/mm/dma-mapping.c         | 32 +++++++++++++-------------
>  arch/arm64/mm/xpfo.c                | 45 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 72 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
> index d74a284abdc2..b6a462e3b2f9 100644
> --- a/arch/arm64/include/asm/cacheflush.h
> +++ b/arch/arm64/include/asm/cacheflush.h
> @@ -93,6 +93,17 @@ extern void __dma_map_area(const void *, size_t, int);
>  extern void __dma_unmap_area(const void *, size_t, int);
>  extern void __dma_flush_area(const void *, size_t);
>  
> +#ifdef CONFIG_XPFO
> +#include <linux/xpfo.h>
> +#define _dma_map_area(addr, size, dir) \
> +	xpfo_dma_map_unmap_area(true, addr, size, dir)
> +#define _dma_unmap_area(addr, size, dir) \
> +	xpfo_dma_map_unmap_area(false, addr, size, dir)
> +#else
> +#define _dma_map_area(addr, size, dir) __dma_map_area(addr, size, dir)
> +#define _dma_unmap_area(addr, size, dir) __dma_unmap_area(addr, size, dir)
> +#endif
> +
>  /*
>   * Copy user data from/to a page which is mapped into a different
>   * processes address space.  Really, we want to allow our "user
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index f27d4dd04384..a79f200786ab 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -204,7 +204,7 @@ static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
>  	dev_addr = swiotlb_map_page(dev, page, offset, size, dir, attrs);
>  	if (!is_device_dma_coherent(dev) &&
>  	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
> -		__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
> +		_dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
>  
>  	return dev_addr;
>  }
> @@ -216,7 +216,7 @@ static void __swiotlb_unmap_page(struct device *dev, dma_addr_t dev_addr,
>  {
>  	if (!is_device_dma_coherent(dev) &&
>  	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
> -		__dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
> +		_dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
>  	swiotlb_unmap_page(dev, dev_addr, size, dir, attrs);
>  }
>  
> @@ -231,8 +231,8 @@ static int __swiotlb_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
>  	if (!is_device_dma_coherent(dev) &&
>  	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
>  		for_each_sg(sgl, sg, ret, i)
> -			__dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
> -				       sg->length, dir);
> +			_dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
> +				      sg->length, dir);
>  
>  	return ret;
>  }
> @@ -248,8 +248,8 @@ static void __swiotlb_unmap_sg_attrs(struct device *dev,
>  	if (!is_device_dma_coherent(dev) &&
>  	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
>  		for_each_sg(sgl, sg, nelems, i)
> -			__dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
> -					 sg->length, dir);
> +			_dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
> +					sg->length, dir);
>  	swiotlb_unmap_sg_attrs(dev, sgl, nelems, dir, attrs);
>  }
>  
> @@ -258,7 +258,7 @@ static void __swiotlb_sync_single_for_cpu(struct device *dev,
>  					  enum dma_data_direction dir)
>  {
>  	if (!is_device_dma_coherent(dev))
> -		__dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
> +		_dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
>  	swiotlb_sync_single_for_cpu(dev, dev_addr, size, dir);
>  }
>  
> @@ -268,7 +268,7 @@ static void __swiotlb_sync_single_for_device(struct device *dev,
>  {
>  	swiotlb_sync_single_for_device(dev, dev_addr, size, dir);
>  	if (!is_device_dma_coherent(dev))
> -		__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
> +		_dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
>  }
>  
>  static void __swiotlb_sync_sg_for_cpu(struct device *dev,
> @@ -280,8 +280,8 @@ static void __swiotlb_sync_sg_for_cpu(struct device *dev,
>  
>  	if (!is_device_dma_coherent(dev))
>  		for_each_sg(sgl, sg, nelems, i)
> -			__dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
> -					 sg->length, dir);
> +			_dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
> +					sg->length, dir);
>  	swiotlb_sync_sg_for_cpu(dev, sgl, nelems, dir);
>  }
>  
> @@ -295,8 +295,8 @@ static void __swiotlb_sync_sg_for_device(struct device *dev,
>  	swiotlb_sync_sg_for_device(dev, sgl, nelems, dir);
>  	if (!is_device_dma_coherent(dev))
>  		for_each_sg(sgl, sg, nelems, i)
> -			__dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
> -				       sg->length, dir);
> +			_dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
> +				      sg->length, dir);
>  }
>  
>  static int __swiotlb_mmap_pfn(struct vm_area_struct *vma,
> @@ -758,7 +758,7 @@ static void __iommu_sync_single_for_cpu(struct device *dev,
>  		return;
>  
>  	phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), dev_addr);
> -	__dma_unmap_area(phys_to_virt(phys), size, dir);
> +	_dma_unmap_area(phys_to_virt(phys), size, dir);
>  }
>  
>  static void __iommu_sync_single_for_device(struct device *dev,
> @@ -771,7 +771,7 @@ static void __iommu_sync_single_for_device(struct device *dev,
>  		return;
>  
>  	phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), dev_addr);
> -	__dma_map_area(phys_to_virt(phys), size, dir);
> +	_dma_map_area(phys_to_virt(phys), size, dir);
>  }
>  
>  static dma_addr_t __iommu_map_page(struct device *dev, struct page *page,
> @@ -811,7 +811,7 @@ static void __iommu_sync_sg_for_cpu(struct device *dev,
>  		return;
>  
>  	for_each_sg(sgl, sg, nelems, i)
> -		__dma_unmap_area(sg_virt(sg), sg->length, dir);
> +		_dma_unmap_area(sg_virt(sg), sg->length, dir);
>  }
>  
>  static void __iommu_sync_sg_for_device(struct device *dev,
> @@ -825,7 +825,7 @@ static void __iommu_sync_sg_for_device(struct device *dev,
>  		return;
>  
>  	for_each_sg(sgl, sg, nelems, i)
> -		__dma_map_area(sg_virt(sg), sg->length, dir);
> +		_dma_map_area(sg_virt(sg), sg->length, dir);
>  }
>  
>  static int __iommu_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
> diff --git a/arch/arm64/mm/xpfo.c b/arch/arm64/mm/xpfo.c
> index de03a652d48a..c4deb2b720cf 100644
> --- a/arch/arm64/mm/xpfo.c
> +++ b/arch/arm64/mm/xpfo.c
> @@ -11,8 +11,10 @@
>   * the Free Software Foundation.
>   */
>  
> +#include <linux/highmem.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
> +#include <linux/xpfo.h>
>  
>  #include <asm/tlbflush.h>
>  
> @@ -62,3 +64,46 @@ inline void xpfo_flush_kernel_page(struct page *page, int order)
>  
>  	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);
>  }
> +
> +inline void xpfo_dma_map_unmap_area(bool map, const void *addr, size_t size,

And inline? You sure about that? It is quite a lot of code to duplicate
in all of those call-sites.

> +				    int dir)

Not enum dma_data_direction ?
> +{
> +	unsigned long flags;
> +	struct page *page = virt_to_page(addr);
> +
> +	/*
> +	 * +2 here because we really want
> +	 * ceil(size / PAGE_SIZE), not floor(), and one extra in case things are
> +	 * not page aligned
> +	 */
> +	int i, possible_pages = size / PAGE_SIZE + 2;

Could you use the PAGE_SHIFT macro instead? Or PFN_UP ?

And there is also the PAGE_ALIGN macro...

> +	void *buf[possible_pages];

What if you just did 'void *buf[possible_pages] = { };'

Wouldn't that eliminate the need for the memset?

> +
> +	memset(buf, 0, sizeof(void *) * possible_pages);
> +
> +	local_irq_save(flags);

?? Why?

> +
> +	/* Map the first page */
> +	if (xpfo_page_is_unmapped(page))
> +		buf[0] = kmap_atomic(page);

Especially in context of the lookup and kmap_atomic which can take
a bit of time.
> +
> +	/* Map the remaining pages */
> +	for (i = 1; i < possible_pages; i++) {
> +		if (page_to_virt(page + i) >= addr + size)
> +			break;
> +
> +		if (xpfo_page_is_unmapped(page + i))
> +			buf[i] = kmap_atomic(page + i);
> +	}
> +
> +	if (map)
> +		__dma_map_area(addr, size, dir);
> +	else
> +		__dma_unmap_area(addr, size, dir);
> +
> +	for (i = 0; i < possible_pages; i++)
> +		if (buf[i])
> +			kunmap_atomic(buf[i]);
> +
> +	local_irq_restore(flags);
> +}
> -- 
> 2.11.0
>
Tycho Andersen Aug. 10, 2017, 4:35 p.m. UTC | #2 
Hi Konrad,

Thanks for taking a look!

On Thu, Aug 10, 2017 at 09:11:12AM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 09, 2017 at 02:07:53PM -0600, Tycho Andersen wrote:
> > +
> > +inline void xpfo_dma_map_unmap_area(bool map, const void *addr, size_t size,
> 
> And inline? You sure about that? It is quite a lot of code to duplicate
> in all of those call-sites.
>
> > +				    int dir)
> 
> Not enum dma_data_direction ?

I'll fix both of these, thanks.

> > +{
> > +	unsigned long flags;
> > +	struct page *page = virt_to_page(addr);
> > +
> > +	/*
> > +	 * +2 here because we really want
> > +	 * ceil(size / PAGE_SIZE), not floor(), and one extra in case things are
> > +	 * not page aligned
> > +	 */
> > +	int i, possible_pages = size / PAGE_SIZE + 2;
> 
> Could you use the PAGE_SHIFT macro instead? Or PFN_UP ?
> 
> And there is also the PAGE_ALIGN macro...
> 
> > +	void *buf[possible_pages];
> 
> What if you just did 'void *buf[possible_pages] = { };'
> 
> Wouldn't that eliminate the need for the memset?

gcc doesn't seem to like that:

arch/arm64//mm/xpfo.c: In function ‘xpfo_dma_map_unmap_area’:
arch/arm64//mm/xpfo.c:80:2: error: variable-sized object may not be initialized
  void *buf[possible_pages] = {};
  ^~~~

I thought about putting this on the heap, but there's no real way to return
errors here if e.g. the kmalloc fails. I'm open to suggestions though, because
this is ugly.

> > +
> > +	memset(buf, 0, sizeof(void *) * possible_pages);
> > +
> > +	local_irq_save(flags);
> 
> ?? Why?

I'm afraid I don't really know. I'll drop it for the next version, thanks!

Tycho
diff mbox

Patch

diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
index d74a284abdc2..b6a462e3b2f9 100644
--- a/arch/arm64/include/asm/cacheflush.h
+++ b/arch/arm64/include/asm/cacheflush.h
@@ -93,6 +93,17 @@  extern void __dma_map_area(const void *, size_t, int);
 extern void __dma_unmap_area(const void *, size_t, int);
 extern void __dma_flush_area(const void *, size_t);
 
+#ifdef CONFIG_XPFO
+#include <linux/xpfo.h>
+#define _dma_map_area(addr, size, dir) \
+	xpfo_dma_map_unmap_area(true, addr, size, dir)
+#define _dma_unmap_area(addr, size, dir) \
+	xpfo_dma_map_unmap_area(false, addr, size, dir)
+#else
+#define _dma_map_area(addr, size, dir) __dma_map_area(addr, size, dir)
+#define _dma_unmap_area(addr, size, dir) __dma_unmap_area(addr, size, dir)
+#endif
+
 /*
  * Copy user data from/to a page which is mapped into a different
  * processes address space.  Really, we want to allow our "user
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index f27d4dd04384..a79f200786ab 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -204,7 +204,7 @@  static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
 	dev_addr = swiotlb_map_page(dev, page, offset, size, dir, attrs);
 	if (!is_device_dma_coherent(dev) &&
 	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
-		__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
+		_dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
 
 	return dev_addr;
 }
@@ -216,7 +216,7 @@  static void __swiotlb_unmap_page(struct device *dev, dma_addr_t dev_addr,
 {
 	if (!is_device_dma_coherent(dev) &&
 	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
-		__dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
+		_dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
 	swiotlb_unmap_page(dev, dev_addr, size, dir, attrs);
 }
 
@@ -231,8 +231,8 @@  static int __swiotlb_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
 	if (!is_device_dma_coherent(dev) &&
 	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
 		for_each_sg(sgl, sg, ret, i)
-			__dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
-				       sg->length, dir);
+			_dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
+				      sg->length, dir);
 
 	return ret;
 }
@@ -248,8 +248,8 @@  static void __swiotlb_unmap_sg_attrs(struct device *dev,
 	if (!is_device_dma_coherent(dev) &&
 	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
 		for_each_sg(sgl, sg, nelems, i)
-			__dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
-					 sg->length, dir);
+			_dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
+					sg->length, dir);
 	swiotlb_unmap_sg_attrs(dev, sgl, nelems, dir, attrs);
 }
 
@@ -258,7 +258,7 @@  static void __swiotlb_sync_single_for_cpu(struct device *dev,
 					  enum dma_data_direction dir)
 {
 	if (!is_device_dma_coherent(dev))
-		__dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
+		_dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
 	swiotlb_sync_single_for_cpu(dev, dev_addr, size, dir);
 }
 
@@ -268,7 +268,7 @@  static void __swiotlb_sync_single_for_device(struct device *dev,
 {
 	swiotlb_sync_single_for_device(dev, dev_addr, size, dir);
 	if (!is_device_dma_coherent(dev))
-		__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
+		_dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
 }
 
 static void __swiotlb_sync_sg_for_cpu(struct device *dev,
@@ -280,8 +280,8 @@  static void __swiotlb_sync_sg_for_cpu(struct device *dev,
 
 	if (!is_device_dma_coherent(dev))
 		for_each_sg(sgl, sg, nelems, i)
-			__dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
-					 sg->length, dir);
+			_dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
+					sg->length, dir);
 	swiotlb_sync_sg_for_cpu(dev, sgl, nelems, dir);
 }
 
@@ -295,8 +295,8 @@  static void __swiotlb_sync_sg_for_device(struct device *dev,
 	swiotlb_sync_sg_for_device(dev, sgl, nelems, dir);
 	if (!is_device_dma_coherent(dev))
 		for_each_sg(sgl, sg, nelems, i)
-			__dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
-				       sg->length, dir);
+			_dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
+				      sg->length, dir);
 }
 
 static int __swiotlb_mmap_pfn(struct vm_area_struct *vma,
@@ -758,7 +758,7 @@  static void __iommu_sync_single_for_cpu(struct device *dev,
 		return;
 
 	phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), dev_addr);
-	__dma_unmap_area(phys_to_virt(phys), size, dir);
+	_dma_unmap_area(phys_to_virt(phys), size, dir);
 }
 
 static void __iommu_sync_single_for_device(struct device *dev,
@@ -771,7 +771,7 @@  static void __iommu_sync_single_for_device(struct device *dev,
 		return;
 
 	phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), dev_addr);
-	__dma_map_area(phys_to_virt(phys), size, dir);
+	_dma_map_area(phys_to_virt(phys), size, dir);
 }
 
 static dma_addr_t __iommu_map_page(struct device *dev, struct page *page,
@@ -811,7 +811,7 @@  static void __iommu_sync_sg_for_cpu(struct device *dev,
 		return;
 
 	for_each_sg(sgl, sg, nelems, i)
-		__dma_unmap_area(sg_virt(sg), sg->length, dir);
+		_dma_unmap_area(sg_virt(sg), sg->length, dir);
 }
 
 static void __iommu_sync_sg_for_device(struct device *dev,
@@ -825,7 +825,7 @@  static void __iommu_sync_sg_for_device(struct device *dev,
 		return;
 
 	for_each_sg(sgl, sg, nelems, i)
-		__dma_map_area(sg_virt(sg), sg->length, dir);
+		_dma_map_area(sg_virt(sg), sg->length, dir);
 }
 
 static int __iommu_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
diff --git a/arch/arm64/mm/xpfo.c b/arch/arm64/mm/xpfo.c
index de03a652d48a..c4deb2b720cf 100644
--- a/arch/arm64/mm/xpfo.c
+++ b/arch/arm64/mm/xpfo.c
@@ -11,8 +11,10 @@ 
  * the Free Software Foundation.
  */
 
+#include <linux/highmem.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/xpfo.h>
 
 #include <asm/tlbflush.h>
 
@@ -62,3 +64,46 @@  inline void xpfo_flush_kernel_page(struct page *page, int order)
 
 	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);
 }
+
+inline void xpfo_dma_map_unmap_area(bool map, const void *addr, size_t size,
+				    int dir)
+{
+	unsigned long flags;
+	struct page *page = virt_to_page(addr);
+
+	/*
+	 * +2 here because we really want
+	 * ceil(size / PAGE_SIZE), not floor(), and one extra in case things are
+	 * not page aligned
+	 */
+	int i, possible_pages = size / PAGE_SIZE + 2;
+	void *buf[possible_pages];
+
+	memset(buf, 0, sizeof(void *) * possible_pages);
+
+	local_irq_save(flags);
+
+	/* Map the first page */
+	if (xpfo_page_is_unmapped(page))
+		buf[0] = kmap_atomic(page);
+
+	/* Map the remaining pages */
+	for (i = 1; i < possible_pages; i++) {
+		if (page_to_virt(page + i) >= addr + size)
+			break;
+
+		if (xpfo_page_is_unmapped(page + i))
+			buf[i] = kmap_atomic(page + i);
+	}
+
+	if (map)
+		__dma_map_area(addr, size, dir);
+	else
+		__dma_unmap_area(addr, size, dir);
+
+	for (i = 0; i < possible_pages; i++)
+		if (buf[i])
+			kunmap_atomic(buf[i]);
+
+	local_irq_restore(flags);
+}