[RFC] android: ion: How to properly clean caches for uncached allocations

[RFC] android: ion: How to properly clean caches for uncached allocations

[Liam Mark]

The issue:

Currently in ION if you allocate uncached memory it is possible that there
are still dirty lines in the cache.  And often these dirty lines in the
cache are the zeros which were meant to clear out any sensitive kernel
data.

What this means is that if you allocate uncached memory from ION, and then
subsequently write to that buffer (using the uncached mapping you are
provided by ION) then the data you have written could be corrupted at some
point in the future if a dirty line is evicted from the cache.

Also this means there is a potential security issue.  If an un-privileged
userspace user allocated uncached memory (for example from the system heap)
and then if they were to read from that buffer (through the un-cached
mapping they are provided by ION), and if some of the zeros which were
written to that memory are still in the cache then this un-privileged
userspace user could read potentially sensitive kernel data.


An unacceptable fix:

I have included some sample code which should resolve this issue for the
system heap and the cma heap on some architectures, however this code
would not be acceptable for upstreaming since it uses hacks to clean
the cache.
Similar changes would need to be made to carveout heap and chunk heap.

I would appreciate some feedback on the proper way for ION to clean the
caches for memory it has allocated that is intended for uncached access.

I realize that it may be tempting, as a solution to this problem, to
simply strip uncached support from ION. I hope that we can try to find a
solution which preserves uncached memory support as ION uncached memory
is often used (though perhaps not in upstreamed code) in cases such as
multimedia use cases where there is no CPU access required, in secure
heap allocations, and in some cases where there is minimal CPU access
and therefore uncached memory performs better.

Signed-off-by: Liam Mark <lmark@codeaurora.org>
---
 drivers/staging/android/ion/ion.c             | 16 ++++++++++++++++
 drivers/staging/android/ion/ion.h             |  5 ++++-
 drivers/staging/android/ion/ion_cma_heap.c    |  3 +++
 drivers/staging/android/ion/ion_page_pool.c   |  8 ++++++--
 drivers/staging/android/ion/ion_system_heap.c |  7 ++++++-
 5 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 57e0d8035b2e..10e967b0a0f4 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -38,6 +38,22 @@ bool ion_buffer_cached(struct ion_buffer *buffer)
 	return !!(buffer->flags & ION_FLAG_CACHED);
 }
 
+void ion_pages_sync_for_device(struct page *page, size_t size,
+			       enum dma_data_direction dir)
+{
+	struct scatterlist sg;
+	struct device dev = {0};
+
+	/* hack, use dummy device */
+	arch_setup_dma_ops(&dev, 0, 0, NULL, false);
+
+	sg_init_table(&sg, 1);
+	sg_set_page(&sg, page, size, 0);
+	/* hack, use phys address for dma address */
+	sg_dma_address(&sg) = page_to_phys(page);
+	dma_sync_sg_for_device(&dev, &sg, 1, dir);
+}
+
 /* this function should only be called while dev->lock is held */
 static void ion_buffer_add(struct ion_device *dev,
 			   struct ion_buffer *buffer)
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index a238f23c9116..227b9928d185 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -192,6 +192,9 @@ struct ion_heap {
  */
 bool ion_buffer_cached(struct ion_buffer *buffer);
 
+void ion_pages_sync_for_device(struct page *page, size_t size,
+			       enum dma_data_direction dir);
+
 /**
  * ion_buffer_fault_user_mappings - fault in user mappings of this buffer
  * @buffer:		buffer
@@ -333,7 +336,7 @@ struct ion_page_pool {
 struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order,
 					   bool cached);
 void ion_page_pool_destroy(struct ion_page_pool *pool);
-struct page *ion_page_pool_alloc(struct ion_page_pool *pool);
+struct page *ion_page_pool_alloc(struct ion_page_pool *pool, bool *from_pool);
 void ion_page_pool_free(struct ion_page_pool *pool, struct page *page);
 
 /** ion_page_pool_shrink - shrinks the size of the memory cached in the pool
diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
index 49718c96bf9e..82e80621d114 100644
--- a/drivers/staging/android/ion/ion_cma_heap.c
+++ b/drivers/staging/android/ion/ion_cma_heap.c
@@ -59,6 +59,9 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
 		memset(page_address(pages), 0, size);
 	}
 
+	if (!ion_buffer_cached(buffer))
+		ion_pages_sync_for_device(pages, size, DMA_BIDIRECTIONAL);
+
 	table = kmalloc(sizeof(*table), GFP_KERNEL);
 	if (!table)
 		goto err;
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index b3017f12835f..169a321778ed 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -63,7 +63,7 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high)
 	return page;
 }
 
-struct page *ion_page_pool_alloc(struct ion_page_pool *pool)
+struct page *ion_page_pool_alloc(struct ion_page_pool *pool, bool *from_pool)
 {
 	struct page *page = NULL;
 
@@ -76,8 +76,12 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool)
 		page = ion_page_pool_remove(pool, false);
 	mutex_unlock(&pool->mutex);
 
-	if (!page)
+	if (!page) {
 		page = ion_page_pool_alloc_pages(pool);
+		*from_pool = false;
+	} else {
+		*from_pool = true;
+	}
 
 	return page;
 }
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index bc19cdd30637..3bb4604e032b 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -57,13 +57,18 @@ static struct page *alloc_buffer_page(struct ion_system_heap *heap,
 	bool cached = ion_buffer_cached(buffer);
 	struct ion_page_pool *pool;
 	struct page *page;
+	bool from_pool;
 
 	if (!cached)
 		pool = heap->uncached_pools[order_to_index(order)];
 	else
 		pool = heap->cached_pools[order_to_index(order)];
 
-	page = ion_page_pool_alloc(pool);
+	page = ion_page_pool_alloc(pool, &from_pool);
+
+	if (!from_pool && !ion_buffer_cached(buffer))
+		ion_pages_sync_for_device(page, PAGE_SIZE << order,
+					  DMA_BIDIRECTIONAL);
 
 	return page;
 }
-- 
1.8.5.2


Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC] android: ion: How to properly clean caches for uncached allocations

[Laura Abbott]

On 02/28/2018 09:18 PM, Liam Mark wrote:
> The issue:
> 
> Currently in ION if you allocate uncached memory it is possible that there
> are still dirty lines in the cache.  And often these dirty lines in the
> cache are the zeros which were meant to clear out any sensitive kernel
> data.
> 
> What this means is that if you allocate uncached memory from ION, and then
> subsequently write to that buffer (using the uncached mapping you are
> provided by ION) then the data you have written could be corrupted at some
> point in the future if a dirty line is evicted from the cache.
> 
> Also this means there is a potential security issue.  If an un-privileged
> userspace user allocated uncached memory (for example from the system heap)
> and then if they were to read from that buffer (through the un-cached
> mapping they are provided by ION), and if some of the zeros which were
> written to that memory are still in the cache then this un-privileged
> userspace user could read potentially sensitive kernel data.

For the use case you are describing we don't actually need the
memory to be non-cached until it comes time to do the dma mapping.
Here's a proposal to shoot holes in:

- Before any dma_buf attach happens, all mmap mappings are cached
- At the time attach happens, we shoot down any existing userspace
mappings, do the dma_map with appropriate flags to clean the pages
and then allow remapping to userspace as uncached. Really this
looks like a variation on the old Ion faulting code which I removed
except it's for uncached buffers instead of cached buffers.

Potential problems:
- I'm not 100% about the behavior here if the attaching device
is already dma_coherent. I also consider uncached mappings
enough of a device specific optimization that you shouldn't
do them unless you know it's needed.
- The locking/sequencing with userspace could be tricky
since userspace may not like us ripping mappings out from
underneath if it's trying to access.

Thoughts?

Thanks,
Laura
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC] android: ion: How to properly clean caches for uncached allocations

[Liam Mark]

On Wed, 7 Mar 2018, Laura Abbott wrote:

> On 02/28/2018 09:18 PM, Liam Mark wrote:
> > The issue:
> > 
> > Currently in ION if you allocate uncached memory it is possible that there
> > are still dirty lines in the cache.  And often these dirty lines in the
> > cache are the zeros which were meant to clear out any sensitive kernel
> > data.
> > 
> > What this means is that if you allocate uncached memory from ION, and then
> > subsequently write to that buffer (using the uncached mapping you are
> > provided by ION) then the data you have written could be corrupted at some
> > point in the future if a dirty line is evicted from the cache.
> > 
> > Also this means there is a potential security issue.  If an un-privileged
> > userspace user allocated uncached memory (for example from the system heap)
> > and then if they were to read from that buffer (through the un-cached
> > mapping they are provided by ION), and if some of the zeros which were
> > written to that memory are still in the cache then this un-privileged
> > userspace user could read potentially sensitive kernel data.
> 
> For the use case you are describing we don't actually need the
> memory to be non-cached until it comes time to do the dma mapping.
> Here's a proposal to shoot holes in:
> 
> - Before any dma_buf attach happens, all mmap mappings are cached
> - At the time attach happens, we shoot down any existing userspace
> mappings, do the dma_map with appropriate flags to clean the pages
> and then allow remapping to userspace as uncached. Really this
> looks like a variation on the old Ion faulting code which I removed
> except it's for uncached buffers instead of cached buffers.
> 

Thanks Laura, I will take a look to see if I can think of any concerns.

Initial thoughts.
- What about any kernel mappings (kmap/vmap) the client has made?

- I guess it would be tempting to only do this behavior for memory that 
came from buddy (as opposed to the pool since it should be clean), but we 
would need to be careful that no pages sneak into the pool without being 
cleaned (example: client allocs then frees without ever call 
dma_buf_attach).

> Potential problems:
> - I'm not 100% about the behavior here if the attaching device
> is already dma_coherent. I also consider uncached mappings
> enough of a device specific optimization that you shouldn't
> do them unless you know it's needed.

I don't believe we want to allow uncached memory to be dma mapped by an 
io-coherent device and this is something I would like to eventually block.

Since there is always a kernel cached mapping for ION uncached memory then 
speculative access could still be putting lines in the cache, so when an 
io-coherent device tries to read this uncached memory its snoop into the 
cache could find one of these 'stale' clean cache lines and end up using 
stale data. 
Agree?

> - The locking/sequencing with userspace could be tricky
> since userspace may not like us ripping mappings out from
> underneath if it's trying to access.

Perhaps delay this work to the dma_map_attachment call since when the data 
is dma mapped the CPU shouldn't be accessing it? 

Or worst case perhaps fail all map attempts to uncached memory until the 
memory has been dma mapped (and cleaned) at least once?

Thanks,
Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC] android: ion: How to properly clean caches for uncached allocations

[Laura Abbott]

On 03/08/2018 04:45 PM, Liam Mark wrote:
> On Wed, 7 Mar 2018, Laura Abbott wrote:
> 
>> On 02/28/2018 09:18 PM, Liam Mark wrote:
>>> The issue:
>>>
>>> Currently in ION if you allocate uncached memory it is possible that there
>>> are still dirty lines in the cache.  And often these dirty lines in the
>>> cache are the zeros which were meant to clear out any sensitive kernel
>>> data.
>>>
>>> What this means is that if you allocate uncached memory from ION, and then
>>> subsequently write to that buffer (using the uncached mapping you are
>>> provided by ION) then the data you have written could be corrupted at some
>>> point in the future if a dirty line is evicted from the cache.
>>>
>>> Also this means there is a potential security issue.  If an un-privileged
>>> userspace user allocated uncached memory (for example from the system heap)
>>> and then if they were to read from that buffer (through the un-cached
>>> mapping they are provided by ION), and if some of the zeros which were
>>> written to that memory are still in the cache then this un-privileged
>>> userspace user could read potentially sensitive kernel data.
>>
>> For the use case you are describing we don't actually need the
>> memory to be non-cached until it comes time to do the dma mapping.
>> Here's a proposal to shoot holes in:
>>
>> - Before any dma_buf attach happens, all mmap mappings are cached
>> - At the time attach happens, we shoot down any existing userspace
>> mappings, do the dma_map with appropriate flags to clean the pages
>> and then allow remapping to userspace as uncached. Really this
>> looks like a variation on the old Ion faulting code which I removed
>> except it's for uncached buffers instead of cached buffers.
>>
> 
> Thanks Laura, I will take a look to see if I can think of any concerns.
> 
> Initial thoughts.
> - What about any kernel mappings (kmap/vmap) the client has made?
> 

We could either synchronize with dma_buf_{begin,end}_cpu_access
or just disallow the mapping to happen if there's an outstanding
kmap or vmap. Is this an actual problem or only theoretical?

> - I guess it would be tempting to only do this behavior for memory that
> came from buddy (as opposed to the pool since it should be clean), but we
> would need to be careful that no pages sneak into the pool without being
> cleaned (example: client allocs then frees without ever call
> dma_buf_attach).
> 

You're welcome to try that optimization but I think we should
focus on the basics first. Honestly it might make sense just to
have a single pool at this point since the cost of syncing
is not happening on the allocation path.

>> Potential problems:
>> - I'm not 100% about the behavior here if the attaching device
>> is already dma_coherent. I also consider uncached mappings
>> enough of a device specific optimization that you shouldn't
>> do them unless you know it's needed.
> 
> I don't believe we want to allow uncached memory to be dma mapped by an
> io-coherent device and this is something I would like to eventually block.
> 
> Since there is always a kernel cached mapping for ION uncached memory then
> speculative access could still be putting lines in the cache, so when an
> io-coherent device tries to read this uncached memory its snoop into the
> cache could find one of these 'stale' clean cache lines and end up using
> stale data.
> Agree?
> 

Sounds right.

>> - The locking/sequencing with userspace could be tricky
>> since userspace may not like us ripping mappings out from
>> underneath if it's trying to access.
> 
> Perhaps delay this work to the dma_map_attachment call since when the data
> is dma mapped the CPU shouldn't be accessing it?
> 
> Or worst case perhaps fail all map attempts to uncached memory until the
> memory has been dma mapped (and cleaned) at least once?
> 

My concern was mostly concurrent userspace access on a buffer
that's being dma_mapped but that sounds racy to begin with.

I suggested disallowing mmap until dma_mapping before and I thought
that was not possible?

Thanks,
Laura

> Thanks,
> Liam
> 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Liam Mark]

Based on the suggestions from Laura I created a first draft for a change 
which will attempt to ensure that uncached mappings are only applied to 
ION memory who's cache lines have been cleaned.
It does this by providing cached mappings (for uncached ION allocations) 
until the ION buffer is dma mapped and successfully cleaned, then it drops 
the userspace mappings and when pages are accessed they are faulted back 
in and uncached mappings are created.

This change has the following potential disadvantages:
- It assumes that userpace clients won't attempt to access the buffer 
while it is being mapped as we are removing the userpspace mappings at 
this point (though it is okay for them to have it mapped)
- It assumes that kernel clients won't hold a kernel mapping to the buffer 
(ie dma_buf_kmap) while it is being dma-mapped. What should we do if there 
is a kernel mapping at the time of dma mapping, fail the mapping, warn?
- There may be a performance penalty as a result of having to fault in the 
pages after removing the userspace mappings.

It passes basic testing involving reading writing and reading from 
uncached system heap allocations before and after dma mapping.

Please let me know if this is heading in the right direction and if there 
are any concerns.

Signed-off-by: Liam Mark <lmark@codeaurora.org>
---
 drivers/staging/android/ion/ion.c | 146 +++++++++++++++++++++++++++++++++++++-
 drivers/staging/android/ion/ion.h |   9 +++
 2 files changed, 152 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 99073325b0c0..3dc0f5a265bf 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -96,6 +96,7 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
 	}
 
 	INIT_LIST_HEAD(&buffer->attachments);
+	INIT_LIST_HEAD(&buffer->vmas);
 	mutex_init(&buffer->lock);
 	mutex_lock(&dev->buffer_lock);
 	ion_buffer_add(dev, buffer);
@@ -117,6 +118,7 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
 		buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
 	}
 	buffer->heap->ops->free(buffer);
+	vfree(buffer->pages);
 	kfree(buffer);
 }
 
@@ -245,11 +247,29 @@ static void ion_dma_buf_detatch(struct dma_buf *dmabuf,
 	kfree(a);
 }
 
+static bool ion_buffer_uncached_clean(struct ion_buffer *buffer)
+{
+	return buffer->uncached_clean;
+}
+
+/* expect buffer->lock to be already taken */
+static void ion_buffer_zap_mappings(struct ion_buffer *buffer)
+{
+	struct ion_vma_list *vma_list;
+
+	list_for_each_entry(vma_list, &buffer->vmas, list) {
+		struct vm_area_struct *vma = vma_list->vma;
+
+		zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start);
+	}
+}
+
 static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
 					enum dma_data_direction direction)
 {
 	struct ion_dma_buf_attachment *a = attachment->priv;
 	struct sg_table *table;
+	struct ion_buffer *buffer = attachment->dmabuf->priv;
 
 	table = a->table;
 
@@ -257,6 +277,19 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
 			direction))
 		return ERR_PTR(-ENOMEM);
 
+	if (!ion_buffer_cached(buffer)) {
+		mutex_lock(&buffer->lock);
+		if (!ion_buffer_uncached_clean(buffer)) {
+			ion_buffer_zap_mappings(buffer);
+			if (buffer->kmap_cnt > 0) {
+				pr_warn_once("%s: buffer still mapped in the kernel\n",
+					     __func__);
+			}
+			buffer->uncached_clean = true;
+		}
+		mutex_unlock(&buffer->lock);
+	}
+
 	return table;
 }
 
@@ -267,6 +300,94 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
 	dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
 }
 
+static void __ion_vm_open(struct vm_area_struct *vma, bool lock)
+{
+	struct ion_buffer *buffer = vma->vm_private_data;
+	struct ion_vma_list *vma_list;
+
+	vma_list = kmalloc(sizeof(*vma_list), GFP_KERNEL);
+	if (!vma_list)
+		return;
+	vma_list->vma = vma;
+
+	if (lock)
+		mutex_lock(&buffer->lock);
+	list_add(&vma_list->list, &buffer->vmas);
+	if (lock)
+		mutex_unlock(&buffer->lock);
+}
+
+static void ion_vm_open(struct vm_area_struct *vma)
+{
+	__ion_vm_open(vma, true);
+}
+
+static void ion_vm_close(struct vm_area_struct *vma)
+{
+	struct ion_buffer *buffer = vma->vm_private_data;
+	struct ion_vma_list *vma_list, *tmp;
+
+	mutex_lock(&buffer->lock);
+	list_for_each_entry_safe(vma_list, tmp, &buffer->vmas, list) {
+		if (vma_list->vma != vma)
+			continue;
+		list_del(&vma_list->list);
+		kfree(vma_list);
+		break;
+	}
+	mutex_unlock(&buffer->lock);
+}
+
+static int ion_vm_fault(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct ion_buffer *buffer = vma->vm_private_data;
+	unsigned long pfn;
+	int ret;
+
+	mutex_lock(&buffer->lock);
+	if (!buffer->pages || !buffer->pages[vmf->pgoff]) {
+		mutex_unlock(&buffer->lock);
+		return VM_FAULT_ERROR;
+	}
+
+	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+	pfn = page_to_pfn(buffer->pages[vmf->pgoff]);
+	ret = vm_insert_pfn(vma, vmf->address, pfn);
+	mutex_unlock(&buffer->lock);
+	if (ret)
+		return VM_FAULT_ERROR;
+
+	return VM_FAULT_NOPAGE;
+}
+
+static const struct vm_operations_struct ion_vma_ops = {
+	.open = ion_vm_open,
+	.close = ion_vm_close,
+	.fault = ion_vm_fault,
+};
+
+static int ion_init_fault_pages(struct ion_buffer *buffer)
+{
+	int num_pages = PAGE_ALIGN(buffer->size) / PAGE_SIZE;
+	struct scatterlist *sg;
+	int i, j, k = 0;
+	struct sg_table *table = buffer->sg_table;
+
+	buffer->pages = vmalloc(sizeof(struct page *) * num_pages);
+	if (!buffer->pages)
+		return -ENOMEM;
+
+	for_each_sg(table->sgl, sg, table->nents, i) {
+		struct page *page = sg_page(sg);
+
+		for (j = 0; j < sg->length / PAGE_SIZE; j++)
+			buffer->pages[k++] = page++;
+	}
+
+	return 0;
+}
+
 static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
 {
 	struct ion_buffer *buffer = dmabuf->priv;
@@ -278,12 +399,31 @@ static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
 		return -EINVAL;
 	}
 
-	if (!(buffer->flags & ION_FLAG_CACHED))
-		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
-
 	mutex_lock(&buffer->lock);
+
+	if (!ion_buffer_cached(buffer)) {
+		if (!ion_buffer_uncached_clean(buffer)) {
+			if (!buffer->pages)
+				ret = ion_init_fault_pages(buffer);
+
+			if (ret)
+				goto end;
+
+			vma->vm_private_data = buffer;
+			vma->vm_ops = &ion_vma_ops;
+			vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND |
+					 VM_DONTDUMP;
+			__ion_vm_open(vma, false);
+		} else {
+			vma->vm_page_prot =
+				pgprot_writecombine(vma->vm_page_prot);
+		}
+	}
+
 	/* now map it to userspace */
 	ret = buffer->heap->ops->map_user(buffer->heap, buffer, vma);
+
+end:
 	mutex_unlock(&buffer->lock);
 
 	if (ret)
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index c006fc1e5a16..438c9f4fa125 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -44,6 +44,11 @@ struct ion_platform_heap {
 	void *priv;
 };
 
+struct ion_vma_list {
+	struct list_head list;
+	struct vm_area_struct *vma;
+};
+
 /**
  * struct ion_buffer - metadata for a particular buffer
  * @ref:		reference count
@@ -59,6 +64,7 @@ struct ion_platform_heap {
  * @kmap_cnt:		number of times the buffer is mapped to the kernel
  * @vaddr:		the kernel mapping if kmap_cnt is not zero
  * @sg_table:		the sg table for the buffer if dmap_cnt is not zero
+ * @vmas:		list of vma's mapping for uncached buffer
  */
 struct ion_buffer {
 	union {
@@ -76,6 +82,9 @@ struct ion_buffer {
 	void *vaddr;
 	struct sg_table *sg_table;
 	struct list_head attachments;
+	struct list_head vmas;
+	struct page **pages;
+	bool uncached_clean;
 };
 
 void ion_buffer_destroy(struct ion_buffer *buffer);
-- 
1.9.1


Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[John Stultz]

On Thu, Nov 1, 2018 at 3:15 PM, Liam Mark <lmark@codeaurora.org> wrote:
> Based on the suggestions from Laura I created a first draft for a change
> which will attempt to ensure that uncached mappings are only applied to
> ION memory who's cache lines have been cleaned.
> It does this by providing cached mappings (for uncached ION allocations)
> until the ION buffer is dma mapped and successfully cleaned, then it drops
> the userspace mappings and when pages are accessed they are faulted back
> in and uncached mappings are created.
>
> This change has the following potential disadvantages:
> - It assumes that userpace clients won't attempt to access the buffer
> while it is being mapped as we are removing the userpspace mappings at
> this point (though it is okay for them to have it mapped)
> - It assumes that kernel clients won't hold a kernel mapping to the buffer
> (ie dma_buf_kmap) while it is being dma-mapped. What should we do if there
> is a kernel mapping at the time of dma mapping, fail the mapping, warn?
> - There may be a performance penalty as a result of having to fault in the
> pages after removing the userspace mappings.
>
> It passes basic testing involving reading writing and reading from
> uncached system heap allocations before and after dma mapping.
>
> Please let me know if this is heading in the right direction and if there
> are any concerns.
>
> Signed-off-by: Liam Mark <lmark@codeaurora.org>


Thanks for sending this out! I gave this a whirl on my HiKey960. Seems
to work ok, but I'm not sure if the board's usage benefits much from
your changes.

First, ignore how crazy overall these frame values are right off, we
have some cpuidle/cpufreq issues w/ 4.14 that we're still sorting out.

Without your patch:
default-jankview_list_view,jankbench,1,mean,0,iter_10,List View
Fling,48.1333678017,
default-jankview_list_view,jankbench,2,mean,0,iter_10,List View
Fling,55.8407417387,
default-jankview_list_view,jankbench,3,mean,0,iter_10,List View
Fling,43.88160374,
default-jankview_list_view,jankbench,4,mean,0,iter_10,List View
Fling,42.2606222784,
default-jankview_list_view,jankbench,5,mean,0,iter_10,List View
Fling,44.1791721797,
default-jankview_list_view,jankbench,6,mean,0,iter_10,List View
Fling,39.7692731775,
default-jankview_list_view,jankbench,7,mean,0,iter_10,List View
Fling,48.5462154074,
default-jankview_list_view,jankbench,8,mean,0,iter_10,List View
Fling,40.1321166548,
default-jankview_list_view,jankbench,9,mean,0,iter_10,List View
Fling,48.0163174397,
default-jankview_list_view,jankbench,10,mean,0,iter_10,List View
Fling,51.1971686844,


With your patch:
default-jankview_list_view,jankbench,1,mean,0,iter_10,List View
Fling,43.3983274772,
default-jankview_list_view,jankbench,2,mean,0,iter_10,List View
Fling,45.8456678409,
default-jankview_list_view,jankbench,3,mean,0,iter_10,List View
Fling,42.9609507211,
default-jankview_list_view,jankbench,4,mean,0,iter_10,List View
Fling,48.602186248,
default-jankview_list_view,jankbench,5,mean,0,iter_10,List View
Fling,47.9257658765,
default-jankview_list_view,jankbench,6,mean,0,iter_10,List View
Fling,47.7405384035,
default-jankview_list_view,jankbench,7,mean,0,iter_10,List View
Fling,52.0017667611,
default-jankview_list_view,jankbench,8,mean,0,iter_10,List View
Fling,43.7480812349,
default-jankview_list_view,jankbench,9,mean,0,iter_10,List View
Fling,44.8138758796,
default-jankview_list_view,jankbench,10,mean,0,iter_10,List View
Fling,46.4941804068,


Just for reference, compared to my earlier patch:
default-jankview_list_view,jankbench,1,mean,0,iter_10,List View
Fling,33.8638094852,
default-jankview_list_view,jankbench,2,mean,0,iter_10,List View
Fling,34.0859500474,
default-jankview_list_view,jankbench,3,mean,0,iter_10,List View
Fling,35.6278973379,
default-jankview_list_view,jankbench,4,mean,0,iter_10,List View
Fling,31.4999822195,
default-jankview_list_view,jankbench,5,mean,0,iter_10,List View
Fling,40.0634874771,
default-jankview_list_view,jankbench,6,mean,0,iter_10,List View
Fling,28.0633472181,
default-jankview_list_view,jankbench,7,mean,0,iter_10,List View
Fling,36.0400585616,
default-jankview_list_view,jankbench,8,mean,0,iter_10,List View
Fling,38.1871234374,
default-jankview_list_view,jankbench,9,mean,0,iter_10,List View
Fling,37.4103602014,
default-jankview_list_view,jankbench,10,mean,0,iter_10,List View
Fling,40.7147881231,


Though I'll spend some more time looking at it closer.

thanks
-john

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Liam Mark]

On Fri, 2 Nov 2018, John Stultz wrote:

> On Thu, Nov 1, 2018 at 3:15 PM, Liam Mark <lmark@codeaurora.org> wrote:
> > Based on the suggestions from Laura I created a first draft for a change
> > which will attempt to ensure that uncached mappings are only applied to
> > ION memory who's cache lines have been cleaned.
> > It does this by providing cached mappings (for uncached ION allocations)
> > until the ION buffer is dma mapped and successfully cleaned, then it drops
> > the userspace mappings and when pages are accessed they are faulted back
> > in and uncached mappings are created.
> >
> > This change has the following potential disadvantages:
> > - It assumes that userpace clients won't attempt to access the buffer
> > while it is being mapped as we are removing the userpspace mappings at
> > this point (though it is okay for them to have it mapped)
> > - It assumes that kernel clients won't hold a kernel mapping to the buffer
> > (ie dma_buf_kmap) while it is being dma-mapped. What should we do if there
> > is a kernel mapping at the time of dma mapping, fail the mapping, warn?
> > - There may be a performance penalty as a result of having to fault in the
> > pages after removing the userspace mappings.
> >
> > It passes basic testing involving reading writing and reading from
> > uncached system heap allocations before and after dma mapping.
> >
> > Please let me know if this is heading in the right direction and if there
> > are any concerns.
> >
> > Signed-off-by: Liam Mark <lmark@codeaurora.org>
> 
> 
> Thanks for sending this out! I gave this a whirl on my HiKey960. Seems
> to work ok, but I'm not sure if the board's usage benefits much from
> your changes.
> 

Thanks for testing this.
I didn't expect this patch to improve performance but I was worried it
might hurt performance.

I don't know how many uncached ION allocations Hikey960 makes, or how it
uses uncached allocations.

It is possible that Hikey960 doesn't make much usage of uncached buffers,
or if it does it may not attempt to mmap them before dma mapping them,
so it is possible this change isn't getting exercised very much in the
test you ran.

I will need to look into how best to exercise this patch on Hikey960.

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Brian Starkey]

Hi Liam,

I'm missing a bit of context here, but I did read the v1 thread.
Please accept my apologies if I'm re-treading trodden ground.

I do know we're chasing nebulous ion "problems" on our end, which
certainly seem to be related to what you're trying to fix here.

On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
>Based on the suggestions from Laura I created a first draft for a change
>which will attempt to ensure that uncached mappings are only applied to
>ION memory who's cache lines have been cleaned.
>It does this by providing cached mappings (for uncached ION allocations)
>until the ION buffer is dma mapped and successfully cleaned, then it drops
>the userspace mappings and when pages are accessed they are faulted back
>in and uncached mappings are created.

If I understand right, there's no way to portably clean the cache of
the kernel mapping before we map the pages into userspace. Is that
right?

Alternatively, can we just make ion refuse to give userspace a
non-cached mapping for pages which are mapped in the kernel as cached?
Would userspace using the dma-buf sync ioctl around its accesses do
the "right thing" in that case?

Given that as you pointed out, the kernel does still have a cached
mapping to these pages, trying to give the CPU a non-cached mapping of
those same pages while preserving consistency seems fraught. Wouldn't
it be better to make sure all CPU mappings are cached, and have CPU
clients use the dma_buf_{begin,end}_cpu_access() hooks to get
consistency where needed?

>
>This change has the following potential disadvantages:
>- It assumes that userpace clients won't attempt to access the buffer
>while it is being mapped as we are removing the userpspace mappings at
>this point (though it is okay for them to have it mapped)
>- It assumes that kernel clients won't hold a kernel mapping to the buffer
>(ie dma_buf_kmap) while it is being dma-mapped. What should we do if there
>is a kernel mapping at the time of dma mapping, fail the mapping, warn?
>- There may be a performance penalty as a result of having to fault in the
>pages after removing the userspace mappings.

I wonder if the dma-buf sync ioctl might provide a way for userspace
to opt-in to when the zap/fault happens. Zap on (DMA_BUF_SYNC_WRITE |
DMA_BUF_SYNC_WRITE_END) and fault on (DMA_BUF_SYNC_READ |
DMA_BUF_SYNC_START)

>
>It passes basic testing involving reading writing and reading from
>uncached system heap allocations before and after dma mapping.
>
>Please let me know if this is heading in the right direction and if there
>are any concerns.
>
>Signed-off-by: Liam Mark <lmark at codeaurora.org>
>---
> drivers/staging/android/ion/ion.c | 146 +++++++++++++++++++++++++++++++++++++-
> drivers/staging/android/ion/ion.h |   9 +++
> 2 files changed, 152 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>index 99073325b0c0..3dc0f5a265bf 100644
>--- a/drivers/staging/android/ion/ion.c
>+++ b/drivers/staging/android/ion/ion.c
>@@ -96,6 +96,7 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
> 	}
>
> 	INIT_LIST_HEAD(&buffer->attachments);
>+	INIT_LIST_HEAD(&buffer->vmas);
> 	mutex_init(&buffer->lock);
> 	mutex_lock(&dev->buffer_lock);
> 	ion_buffer_add(dev, buffer);
>@@ -117,6 +118,7 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
> 		buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
> 	}
> 	buffer->heap->ops->free(buffer);
>+	vfree(buffer->pages);
> 	kfree(buffer);
> }
>
>@@ -245,11 +247,29 @@ static void ion_dma_buf_detatch(struct dma_buf *dmabuf,
> 	kfree(a);
> }
>
>+static bool ion_buffer_uncached_clean(struct ion_buffer *buffer)
>+{
>+	return buffer->uncached_clean;
>+}

nit: The function name sounds like a verb to me - as in "calling this
will clean the buffer". I feel ion_buffer_is_uncached_clean() would
read better.

Thanks,
-Brian

>+
>+/* expect buffer->lock to be already taken */
>+static void ion_buffer_zap_mappings(struct ion_buffer *buffer)
>+{
>+	struct ion_vma_list *vma_list;
>+
>+	list_for_each_entry(vma_list, &buffer->vmas, list) {
>+		struct vm_area_struct *vma = vma_list->vma;
>+
>+		zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start);
>+	}
>+}
>+
> static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> 					enum dma_data_direction direction)
> {
> 	struct ion_dma_buf_attachment *a = attachment->priv;
> 	struct sg_table *table;
>+	struct ion_buffer *buffer = attachment->dmabuf->priv;
>
> 	table = a->table;
>
>@@ -257,6 +277,19 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> 			direction))
> 		return ERR_PTR(-ENOMEM);
>
>+	if (!ion_buffer_cached(buffer)) {
>+		mutex_lock(&buffer->lock);
>+		if (!ion_buffer_uncached_clean(buffer)) {
>+			ion_buffer_zap_mappings(buffer);
>+			if (buffer->kmap_cnt > 0) {
>+				pr_warn_once("%s: buffer still mapped in the kernel\n",
>+					     __func__);
>+			}
>+			buffer->uncached_clean = true;
>+		}
>+		mutex_unlock(&buffer->lock);
>+	}
>+
> 	return table;
> }
>
>@@ -267,6 +300,94 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
> 	dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
> }
>
>+static void __ion_vm_open(struct vm_area_struct *vma, bool lock)
>+{
>+	struct ion_buffer *buffer = vma->vm_private_data;
>+	struct ion_vma_list *vma_list;
>+
>+	vma_list = kmalloc(sizeof(*vma_list), GFP_KERNEL);
>+	if (!vma_list)
>+		return;
>+	vma_list->vma = vma;
>+
>+	if (lock)
>+		mutex_lock(&buffer->lock);
>+	list_add(&vma_list->list, &buffer->vmas);
>+	if (lock)
>+		mutex_unlock(&buffer->lock);
>+}
>+
>+static void ion_vm_open(struct vm_area_struct *vma)
>+{
>+	__ion_vm_open(vma, true);
>+}
>+
>+static void ion_vm_close(struct vm_area_struct *vma)
>+{
>+	struct ion_buffer *buffer = vma->vm_private_data;
>+	struct ion_vma_list *vma_list, *tmp;
>+
>+	mutex_lock(&buffer->lock);
>+	list_for_each_entry_safe(vma_list, tmp, &buffer->vmas, list) {
>+		if (vma_list->vma != vma)
>+			continue;
>+		list_del(&vma_list->list);
>+		kfree(vma_list);
>+		break;
>+	}
>+	mutex_unlock(&buffer->lock);
>+}
>+
>+static int ion_vm_fault(struct vm_fault *vmf)
>+{
>+	struct vm_area_struct *vma = vmf->vma;
>+	struct ion_buffer *buffer = vma->vm_private_data;
>+	unsigned long pfn;
>+	int ret;
>+
>+	mutex_lock(&buffer->lock);
>+	if (!buffer->pages || !buffer->pages[vmf->pgoff]) {
>+		mutex_unlock(&buffer->lock);
>+		return VM_FAULT_ERROR;
>+	}
>+
>+	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>+	pfn = page_to_pfn(buffer->pages[vmf->pgoff]);
>+	ret = vm_insert_pfn(vma, vmf->address, pfn);
>+	mutex_unlock(&buffer->lock);
>+	if (ret)
>+		return VM_FAULT_ERROR;
>+
>+	return VM_FAULT_NOPAGE;
>+}
>+
>+static const struct vm_operations_struct ion_vma_ops = {
>+	.open = ion_vm_open,
>+	.close = ion_vm_close,
>+	.fault = ion_vm_fault,
>+};
>+
>+static int ion_init_fault_pages(struct ion_buffer *buffer)
>+{
>+	int num_pages = PAGE_ALIGN(buffer->size) / PAGE_SIZE;
>+	struct scatterlist *sg;
>+	int i, j, k = 0;
>+	struct sg_table *table = buffer->sg_table;
>+
>+	buffer->pages = vmalloc(sizeof(struct page *) * num_pages);
>+	if (!buffer->pages)
>+		return -ENOMEM;
>+
>+	for_each_sg(table->sgl, sg, table->nents, i) {
>+		struct page *page = sg_page(sg);
>+
>+		for (j = 0; j < sg->length / PAGE_SIZE; j++)
>+			buffer->pages[k++] = page++;
>+	}
>+
>+	return 0;
>+}
>+
> static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> {
> 	struct ion_buffer *buffer = dmabuf->priv;
>@@ -278,12 +399,31 @@ static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> 		return -EINVAL;
> 	}
>
>-	if (!(buffer->flags & ION_FLAG_CACHED))
>-		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>-
> 	mutex_lock(&buffer->lock);
>+
>+	if (!ion_buffer_cached(buffer)) {
>+		if (!ion_buffer_uncached_clean(buffer)) {
>+			if (!buffer->pages)
>+				ret = ion_init_fault_pages(buffer);
>+
>+			if (ret)
>+				goto end;
>+
>+			vma->vm_private_data = buffer;
>+			vma->vm_ops = &ion_vma_ops;
>+			vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND |
>+					 VM_DONTDUMP;
>+			__ion_vm_open(vma, false);
>+		} else {
>+			vma->vm_page_prot =
>+				pgprot_writecombine(vma->vm_page_prot);
>+		}
>+	}
>+
> 	/* now map it to userspace */
> 	ret = buffer->heap->ops->map_user(buffer->heap, buffer, vma);
>+
>+end:
> 	mutex_unlock(&buffer->lock);
>
> 	if (ret)
>diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
>index c006fc1e5a16..438c9f4fa125 100644
>--- a/drivers/staging/android/ion/ion.h
>+++ b/drivers/staging/android/ion/ion.h
>@@ -44,6 +44,11 @@ struct ion_platform_heap {
> 	void *priv;
> };
>
>+struct ion_vma_list {
>+	struct list_head list;
>+	struct vm_area_struct *vma;
>+};
>+
> /**
>  * struct ion_buffer - metadata for a particular buffer
>  * @ref:		reference count
>@@ -59,6 +64,7 @@ struct ion_platform_heap {
>  * @kmap_cnt:		number of times the buffer is mapped to the kernel
>  * @vaddr:		the kernel mapping if kmap_cnt is not zero
>  * @sg_table:		the sg table for the buffer if dmap_cnt is not zero
>+ * @vmas:		list of vma's mapping for uncached buffer
>  */
> struct ion_buffer {
> 	union {
>@@ -76,6 +82,9 @@ struct ion_buffer {
> 	void *vaddr;
> 	struct sg_table *sg_table;
> 	struct list_head attachments;
>+	struct list_head vmas;
>+	struct page **pages;
>+	bool uncached_clean;
> };
>
> void ion_buffer_destroy(struct ion_buffer *buffer);
>-- 
>1.9.1
>
>
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>a Linux Foundation Collaborative Project
>

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Liam Mark]

On Tue, 20 Nov 2018, Brian Starkey wrote:

> Hi Liam,
> 
> I'm missing a bit of context here, but I did read the v1 thread.
> Please accept my apologies if I'm re-treading trodden ground.
> 
> I do know we're chasing nebulous ion "problems" on our end, which
> certainly seem to be related to what you're trying to fix here.
> 
> On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
> >Based on the suggestions from Laura I created a first draft for a change
> >which will attempt to ensure that uncached mappings are only applied to
> >ION memory who's cache lines have been cleaned.
> >It does this by providing cached mappings (for uncached ION allocations)
> >until the ION buffer is dma mapped and successfully cleaned, then it
> drops
> >the userspace mappings and when pages are accessed they are faulted back
> >in and uncached mappings are created.
> 
> If I understand right, there's no way to portably clean the cache of
> the kernel mapping before we map the pages into userspace. Is that
> right?
> 

Yes, it isn't always possible to clean the caches for an uncached mapping 
because a device is required by the DMA APIs to do cache maintenance and 
there isn't necessarily a device available (dma_buf_attach may not yet 
have been called).

> Alternatively, can we just make ion refuse to give userspace a
> non-cached mapping for pages which are mapped in the kernel as cached?

These pages will all be mapped as cached in the kernel for 64 bit (kernel 
logical addresses) so you would always be refusing to create a non-cached mapping.

> Would userspace using the dma-buf sync ioctl around its accesses do
> the "right thing" in that case?
> 

I don't think so, the dma-buf sync ioctl require a device to peform cache 
maintenance, but as mentioned above a device may not be available.

> Given that as you pointed out, the kernel does still have a cached
> mapping to these pages, trying to give the CPU a non-cached mapping of
> those same pages while preserving consistency seems fraught. Wouldn't
> it be better to make sure all CPU mappings are cached, and have CPU
> clients use the dma_buf_{begin,end}_cpu_access() hooks to get
> consistency where needed?
> 

It is fraught, but unfortunately you can't rely on 
dma_buf_{begin,end}_cpu_access() to do cache maintenance as these calls 
require a device, and a device is not always available.

> >
> >This change has the following potential disadvantages:
> >- It assumes that userpace clients won't attempt to access the buffer
> >while it is being mapped as we are removing the userpspace mappings at
> >this point (though it is okay for them to have it mapped)
> >- It assumes that kernel clients won't hold a kernel mapping to the
> buffer
> >(ie dma_buf_kmap) while it is being dma-mapped. What should we do if
> there
> >is a kernel mapping at the time of dma mapping, fail the mapping, warn?
> >- There may be a performance penalty as a result of having to fault in
> the
> >pages after removing the userspace mappings.
> 
> I wonder if the dma-buf sync ioctl might provide a way for userspace
> to opt-in to when the zap/fault happens. Zap on (DMA_BUF_SYNC_WRITE |
> DMA_BUF_SYNC_WRITE_END) and fault on (DMA_BUF_SYNC_READ |
> DMA_BUF_SYNC_START)
> 

Not sure I understand, can you elaborate. 
Are you also adding a requirment that ION pages can't be mmaped during a
call to dma_buf_map_attachment?

> >
> >It passes basic testing involving reading writing and reading from
> >uncached system heap allocations before and after dma mapping.
> >
> >Please let me know if this is heading in the right direction and if there
> >are any concerns.
> >
> >Signed-off-by: Liam Mark <lmark at codeaurora.org>
> >---
> > drivers/staging/android/ion/ion.c | 146
> +++++++++++++++++++++++++++++++++++++-
> > drivers/staging/android/ion/ion.h |   9 +++
> > 2 files changed, 152 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c
> >index 99073325b0c0..3dc0f5a265bf 100644
> >--- a/drivers/staging/android/ion/ion.c
> >+++ b/drivers/staging/android/ion/ion.c
> >@@ -96,6 +96,7 @@ static struct ion_buffer *ion_buffer_create(struct
> ion_heap *heap,
> > 	}
> >
> > 	INIT_LIST_HEAD(&buffer->attachments);
> >+	INIT_LIST_HEAD(&buffer->vmas);
> > 	mutex_init(&buffer->lock);
> > 	mutex_lock(&dev->buffer_lock);
> > 	ion_buffer_add(dev, buffer);
> >@@ -117,6 +118,7 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
> > 		buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
> > 	}
> > 	buffer->heap->ops->free(buffer);
> >+	vfree(buffer->pages);
> > 	kfree(buffer);
> > }
> >
> >@@ -245,11 +247,29 @@ static void ion_dma_buf_detatch(struct dma_buf
> *dmabuf,
> > 	kfree(a);
> > }
> >
> >+static bool ion_buffer_uncached_clean(struct ion_buffer *buffer)
> >+{
> >+	return buffer->uncached_clean;
> >+}
> 
> nit: The function name sounds like a verb to me - as in "calling this
> will clean the buffer". I feel ion_buffer_is_uncached_clean() would
> read better.
> 

Yes, that would be cleaner.

Liam


Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Brian Starkey]

Hi Liam,

On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote:
> On Tue, 20 Nov 2018, Brian Starkey wrote:
> 
> > Hi Liam,
> > 
> > I'm missing a bit of context here, but I did read the v1 thread.
> > Please accept my apologies if I'm re-treading trodden ground.
> > 
> > I do know we're chasing nebulous ion "problems" on our end, which
> > certainly seem to be related to what you're trying to fix here.
> > 
> > On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
> > >Based on the suggestions from Laura I created a first draft for a change
> > >which will attempt to ensure that uncached mappings are only applied to
> > >ION memory who's cache lines have been cleaned.
> > >It does this by providing cached mappings (for uncached ION allocations)
> > >until the ION buffer is dma mapped and successfully cleaned, then it
> > drops
> > >the userspace mappings and when pages are accessed they are faulted back
> > >in and uncached mappings are created.
> > 
> > If I understand right, there's no way to portably clean the cache of
> > the kernel mapping before we map the pages into userspace. Is that
> > right?
> > 
> 
> Yes, it isn't always possible to clean the caches for an uncached mapping 
> because a device is required by the DMA APIs to do cache maintenance and 
> there isn't necessarily a device available (dma_buf_attach may not yet 
> have been called).
> 
> > Alternatively, can we just make ion refuse to give userspace a
> > non-cached mapping for pages which are mapped in the kernel as cached?
> 
> These pages will all be mapped as cached in the kernel for 64 bit (kernel 
> logical addresses) so you would always be refusing to create a non-cached mapping.

And that might be the sane thing to do, no?

AFAIK there are still pages which aren't ever mapped as cached (e.g.
dma_declare_coherent_memory(), anything under /reserved-memory marked
as no-map). If those are exposed as an ion heap, then non-cached
mappings would be fine, and permitted.

> 
> > Would userspace using the dma-buf sync ioctl around its accesses do
> > the "right thing" in that case?
> > 
> 
> I don't think so, the dma-buf sync ioctl require a device to peform cache 
> maintenance, but as mentioned above a device may not be available.
> 

If a device didn't attach yet, then no cache maintenance is
necessary. The only thing accessing the memory is the CPU, via a
cached mapping, which should work just fine. So far so good.

If there are already attachments, then ion_dma_buf_begin_cpu_access()
will sync for CPU access against all of the attached devices, and
again the CPU should see the right thing.

In the other direction, ion_dma_buf_end_cpu_access() will sync for
device access for all currently attached devices. If there's no
attached devices yet, then there's nothing to do until there is (only
thing accessing is CPU via a CPU-cached mapping).

When the first (or another) device attaches, then when it maps the
buffer, the map_dma_buf callback should do whatever sync-ing is needed
for that device.

I might be way off with my understanding of the various DMA APIs, but
this is how I think they're meant to work.

> > Given that as you pointed out, the kernel does still have a cached
> > mapping to these pages, trying to give the CPU a non-cached mapping of
> > those same pages while preserving consistency seems fraught. Wouldn't
> > it be better to make sure all CPU mappings are cached, and have CPU
> > clients use the dma_buf_{begin,end}_cpu_access() hooks to get
> > consistency where needed?
> > 
> 
> It is fraught, but unfortunately you can't rely on 
> dma_buf_{begin,end}_cpu_access() to do cache maintenance as these calls 
> require a device, and a device is not always available.

As above, if there's really no device, then no syncing is needed
because only the CPU is accessing the buffer, and only ever via cached
mappings.

> 
> > >
> > >This change has the following potential disadvantages:
> > >- It assumes that userpace clients won't attempt to access the buffer
> > >while it is being mapped as we are removing the userpspace mappings at
> > >this point (though it is okay for them to have it mapped)
> > >- It assumes that kernel clients won't hold a kernel mapping to the
> > buffer
> > >(ie dma_buf_kmap) while it is being dma-mapped. What should we do if
> > there
> > >is a kernel mapping at the time of dma mapping, fail the mapping, warn?
> > >- There may be a performance penalty as a result of having to fault in
> > the
> > >pages after removing the userspace mappings.
> > 
> > I wonder if the dma-buf sync ioctl might provide a way for userspace
> > to opt-in to when the zap/fault happens. Zap on (DMA_BUF_SYNC_WRITE |
> > DMA_BUF_SYNC_WRITE_END) and fault on (DMA_BUF_SYNC_READ |
> > DMA_BUF_SYNC_START)
> > 
> 
> Not sure I understand, can you elaborate. 
> Are you also adding a requirment that ION pages can't be mmaped during a
> call to dma_buf_map_attachment?

I was only suggesting that zapping the mappings "at random" (from
userspace's perspective), and then faulting them back in (also "at
random"), might cause unexpected and not-controllable stalls in the
app. We could use the ioctl hooks as an explicit indication from the
app that now is a good time to zap the mapping and/or fault back in
the whole buffer. begin_cpu_access is allowed to be a "slow"
operation, so apps should already be expecting to get stalled on the
sync ioctl.

Cheers,
-Brian

> 
> > >
> > >It passes basic testing involving reading writing and reading from
> > >uncached system heap allocations before and after dma mapping.
> > >
> > >Please let me know if this is heading in the right direction and if there
> > >are any concerns.
> > >
> > >Signed-off-by: Liam Mark <lmark at codeaurora.org>
> > >---
> > > drivers/staging/android/ion/ion.c | 146
> > +++++++++++++++++++++++++++++++++++++-
> > > drivers/staging/android/ion/ion.h |   9 +++
> > > 2 files changed, 152 insertions(+), 3 deletions(-)
> > >
> > >diff --git a/drivers/staging/android/ion/ion.c
> > b/drivers/staging/android/ion/ion.c
> > >index 99073325b0c0..3dc0f5a265bf 100644
> > >--- a/drivers/staging/android/ion/ion.c
> > >+++ b/drivers/staging/android/ion/ion.c
> > >@@ -96,6 +96,7 @@ static struct ion_buffer *ion_buffer_create(struct
> > ion_heap *heap,
> > > 	}
> > >
> > > 	INIT_LIST_HEAD(&buffer->attachments);
> > >+	INIT_LIST_HEAD(&buffer->vmas);
> > > 	mutex_init(&buffer->lock);
> > > 	mutex_lock(&dev->buffer_lock);
> > > 	ion_buffer_add(dev, buffer);
> > >@@ -117,6 +118,7 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
> > > 		buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
> > > 	}
> > > 	buffer->heap->ops->free(buffer);
> > >+	vfree(buffer->pages);
> > > 	kfree(buffer);
> > > }
> > >
> > >@@ -245,11 +247,29 @@ static void ion_dma_buf_detatch(struct dma_buf
> > *dmabuf,
> > > 	kfree(a);
> > > }
> > >
> > >+static bool ion_buffer_uncached_clean(struct ion_buffer *buffer)
> > >+{
> > >+	return buffer->uncached_clean;
> > >+}
> > 
> > nit: The function name sounds like a verb to me - as in "calling this
> > will clean the buffer". I feel ion_buffer_is_uncached_clean() would
> > read better.
> > 
> 
> Yes, that would be cleaner.
> 
> Liam
> 
> 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Liam Mark]

On Tue, 27 Nov 2018, Brian Starkey wrote:

> Hi Liam,
> 
> On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote:
> > On Tue, 20 Nov 2018, Brian Starkey wrote:
> > 
> > > Hi Liam,
> > > 
> > > I'm missing a bit of context here, but I did read the v1 thread.
> > > Please accept my apologies if I'm re-treading trodden ground.
> > > 
> > > I do know we're chasing nebulous ion "problems" on our end, which
> > > certainly seem to be related to what you're trying to fix here.
> > > 
> > > On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
> > > >Based on the suggestions from Laura I created a first draft for a change
> > > >which will attempt to ensure that uncached mappings are only applied to
> > > >ION memory who's cache lines have been cleaned.
> > > >It does this by providing cached mappings (for uncached ION allocations)
> > > >until the ION buffer is dma mapped and successfully cleaned, then it
> > > drops
> > > >the userspace mappings and when pages are accessed they are faulted back
> > > >in and uncached mappings are created.
> > > 
> > > If I understand right, there's no way to portably clean the cache of
> > > the kernel mapping before we map the pages into userspace. Is that
> > > right?
> > > 
> > 
> > Yes, it isn't always possible to clean the caches for an uncached mapping 
> > because a device is required by the DMA APIs to do cache maintenance and 
> > there isn't necessarily a device available (dma_buf_attach may not yet 
> > have been called).
> > 
> > > Alternatively, can we just make ion refuse to give userspace a
> > > non-cached mapping for pages which are mapped in the kernel as cached?
> > 
> > These pages will all be mapped as cached in the kernel for 64 bit (kernel 
> > logical addresses) so you would always be refusing to create a non-cached mapping.
> 
> And that might be the sane thing to do, no?
> 
> AFAIK there are still pages which aren't ever mapped as cached (e.g.
> dma_declare_coherent_memory(), anything under /reserved-memory marked
> as no-map). If those are exposed as an ion heap, then non-cached
> mappings would be fine, and permitted.
> 

Sounds like you are suggesting using carveouts to support uncached?

We have many multimedia use cases which use very large amounts of uncached
memory, uncached memory is used as a performance optimization because CPU
access won't happen so it allows us to skip cache maintenance for all the
dma map and dma unmap calls. To create carveouts large enough to support
to support the worst case scenarios could result in very large carveouts.

Large carveouts like this would likely result in poor memory utilizations
(since they are tuned for worst case) which would likely have significant
performance impacts (more limited memory causes more frequent memory
reclaim ect...).

Also estimating for worst case could be difficult since the amount of
uncached memory could be app dependent.
Unfortunately I don't think this would make for a very scalable solution.

> > 
> > > Would userspace using the dma-buf sync ioctl around its accesses do
> > > the "right thing" in that case?
> > > 
> > 
> > I don't think so, the dma-buf sync ioctl require a device to peform cache 
> > maintenance, but as mentioned above a device may not be available.
> > 
> 
> If a device didn't attach yet, then no cache maintenance is
> necessary. The only thing accessing the memory is the CPU, via a
> cached mapping, which should work just fine. So far so good.
> 

Unfortunately not.
Scenario:
- Client allocates uncached memory.
- Client calls the DMA_BUF_IOCTL_SYNC IOCT IOCTL with flags
DMA_BUF_SYNC_START (but this doesn't do any cache maintenance since there
isn't any device)
- Client mmap the memory (ION creates uncached mapping)
- Client reads from that uncached mapping

Because memory has not been cleaned (we haven't had a device yet) the
zeros that were written to this memory could  still be in the cache (since
they were written with a cached mapping), this means that the unprivilived
userpace client is now potentially reading sensitive kernel data....

> If there are already attachments, then ion_dma_buf_begin_cpu_access()
> will sync for CPU access against all of the attached devices, and
> again the CPU should see the right thing.
> 
> In the other direction, ion_dma_buf_end_cpu_access() will sync for
> device access for all currently attached devices. If there's no
> attached devices yet, then there's nothing to do until there is (only
> thing accessing is CPU via a CPU-cached mapping).
> 
> When the first (or another) device attaches, then when it maps the
> buffer, the map_dma_buf callback should do whatever sync-ing is needed
> for that device.
> 
> I might be way off with my understanding of the various DMA APIs, but
> this is how I think they're meant to work.
> 
> > > Given that as you pointed out, the kernel does still have a cached
> > > mapping to these pages, trying to give the CPU a non-cached mapping of
> > > those same pages while preserving consistency seems fraught. Wouldn't
> > > it be better to make sure all CPU mappings are cached, and have CPU
> > > clients use the dma_buf_{begin,end}_cpu_access() hooks to get
> > > consistency where needed?
> > > 
> > 
> > It is fraught, but unfortunately you can't rely on 
> > dma_buf_{begin,end}_cpu_access() to do cache maintenance as these calls 
> > require a device, and a device is not always available.
> 
> As above, if there's really no device, then no syncing is needed
> because only the CPU is accessing the buffer, and only ever via cached
> mappings.
> 

Sure you can use cached mappings, but with cached memory to ensure cache 
coherency you would always need to do cache maintenance at dma map and dma 
unmap (since you can't rely on their being a device when 
dma_buf_{begin,end}_cpu_access() hooks are called).
But with this cached memory you get poor performance because you are 
frequently doing cache mainteance uncessarily because there *could* be CPU access.

The reason we want to use uncached allocations, with uncached mappings, is 
to avoid all this uncessary cache maintenance.

> > 
> > > >
> > > >This change has the following potential disadvantages:
> > > >- It assumes that userpace clients won't attempt to access the buffer
> > > >while it is being mapped as we are removing the userpspace mappings at
> > > >this point (though it is okay for them to have it mapped)
> > > >- It assumes that kernel clients won't hold a kernel mapping to the
> > > buffer
> > > >(ie dma_buf_kmap) while it is being dma-mapped. What should we do if
> > > there
> > > >is a kernel mapping at the time of dma mapping, fail the mapping, warn?
> > > >- There may be a performance penalty as a result of having to fault in
> > > the
> > > >pages after removing the userspace mappings.
> > > 
> > > I wonder if the dma-buf sync ioctl might provide a way for userspace
> > > to opt-in to when the zap/fault happens. Zap on (DMA_BUF_SYNC_WRITE |
> > > DMA_BUF_SYNC_WRITE_END) and fault on (DMA_BUF_SYNC_READ |
> > > DMA_BUF_SYNC_START)
> > > 
> > 
> > Not sure I understand, can you elaborate. 
> > Are you also adding a requirment that ION pages can't be mmaped during a
> > call to dma_buf_map_attachment?
> 
> I was only suggesting that zapping the mappings "at random" (from
> userspace's perspective), and then faulting them back in (also "at
> random"), might cause unexpected and not-controllable stalls in the
> app. We could use the ioctl hooks as an explicit indication from the
> app that now is a good time to zap the mapping and/or fault back in
> the whole buffer. begin_cpu_access is allowed to be a "slow"
> operation, so apps should already be expecting to get stalled on the
> sync ioctl.
> 

I think we have to do the zapping when have a device with which we can
then immediately clean the caches for the memory.

The dma_buf_map_attachement seems like a logical time to do this, we have
a device and the user should not be doing CPU access at this time.
There is no guarantee you will ever have a device attached when the ioctl
hooks are called so this could mean you never get a chance to switch to
actual uncached mappings if you only try to do this from the ioctl hooks.

The one-of hit of having to fault the pages back in is unfortunate but I
can't seem to find a better time to do it.

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Brian Starkey]

Hi Liam,

On Tue, Nov 27, 2018 at 10:46:07PM -0800, Liam Mark wrote:
> On Tue, 27 Nov 2018, Brian Starkey wrote:
> 
> > Hi Liam,
> > 
> > On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote:
> > > On Tue, 20 Nov 2018, Brian Starkey wrote:
> > > 
> > > > Hi Liam,
> > > > 
> > > > I'm missing a bit of context here, but I did read the v1 thread.
> > > > Please accept my apologies if I'm re-treading trodden ground.
> > > > 
> > > > I do know we're chasing nebulous ion "problems" on our end, which
> > > > certainly seem to be related to what you're trying to fix here.
> > > > 
> > > > On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
> > > > >Based on the suggestions from Laura I created a first draft for a change
> > > > >which will attempt to ensure that uncached mappings are only applied to
> > > > >ION memory who's cache lines have been cleaned.
> > > > >It does this by providing cached mappings (for uncached ION allocations)
> > > > >until the ION buffer is dma mapped and successfully cleaned, then it
> > > > drops
> > > > >the userspace mappings and when pages are accessed they are faulted back
> > > > >in and uncached mappings are created.
> > > > 
> > > > If I understand right, there's no way to portably clean the cache of
> > > > the kernel mapping before we map the pages into userspace. Is that
> > > > right?
> > > > 
> > > 
> > > Yes, it isn't always possible to clean the caches for an uncached mapping 
> > > because a device is required by the DMA APIs to do cache maintenance and 
> > > there isn't necessarily a device available (dma_buf_attach may not yet 
> > > have been called).
> > > 
> > > > Alternatively, can we just make ion refuse to give userspace a
> > > > non-cached mapping for pages which are mapped in the kernel as cached?
> > > 
> > > These pages will all be mapped as cached in the kernel for 64 bit (kernel 
> > > logical addresses) so you would always be refusing to create a non-cached mapping.
> > 
> > And that might be the sane thing to do, no?
> > 
> > AFAIK there are still pages which aren't ever mapped as cached (e.g.
> > dma_declare_coherent_memory(), anything under /reserved-memory marked
> > as no-map). If those are exposed as an ion heap, then non-cached
> > mappings would be fine, and permitted.
> > 
> 
> Sounds like you are suggesting using carveouts to support uncached?
> 

No, I'm just saying that ion can't give out uncached _CPU_ mappings
for pages which are already mapped on the CPU as cached.

> We have many multimedia use cases which use very large amounts of uncached
> memory, uncached memory is used as a performance optimization because CPU
> access won't happen so it allows us to skip cache maintenance for all the
> dma map and dma unmap calls. To create carveouts large enough to support
> to support the worst case scenarios could result in very large carveouts.
> 
> Large carveouts like this would likely result in poor memory utilizations
> (since they are tuned for worst case) which would likely have significant
> performance impacts (more limited memory causes more frequent memory
> reclaim ect...).
> 
> Also estimating for worst case could be difficult since the amount of
> uncached memory could be app dependent.
> Unfortunately I don't think this would make for a very scalable solution.
> 

Sure, I understand the desire not to use carveouts. I'm not suggesting
carveouts are a viable alternative.

> > > 
> > > > Would userspace using the dma-buf sync ioctl around its accesses do
> > > > the "right thing" in that case?
> > > > 
> > > 
> > > I don't think so, the dma-buf sync ioctl require a device to peform cache 
> > > maintenance, but as mentioned above a device may not be available.
> > > 
> > 
> > If a device didn't attach yet, then no cache maintenance is
> > necessary. The only thing accessing the memory is the CPU, via a
> > cached mapping, which should work just fine. So far so good.
> > 
> 
> Unfortunately not.
> Scenario:
> - Client allocates uncached memory.
> - Client calls the DMA_BUF_IOCTL_SYNC IOCT IOCTL with flags
> DMA_BUF_SYNC_START (but this doesn't do any cache maintenance since there
> isn't any device)
> - Client mmap the memory (ION creates uncached mapping)
> - Client reads from that uncached mapping

I think I maybe wasn't clear with my proposal. The sequence should be
like this:

 - Client allocates memory
   - If this is from a region which the CPU has mapped as cached, then
     that's not "uncached" memory - it's cached memory - and you have
     to treat it as such.
 - Client calls the DMA_BUF_IOCTL_SYNC IOCTL with flags
   DMA_BUF_SYNC_START (but this doesn't do any cache maintenance since
   there isn't any device)
 - Client mmaps the memory
   - ion creates a _cached_ mapping into the userspace process. ion
     *must not* create an uncached mapping.
 - Client reads from that cached mapping
   - It sees zeroes, as expected.

This proposal ensures that everyone will *always* see correct data if
they use the DMA APIs properly (device accesses via
dma_buf_{map,unmap}, CPU access via {begin,end}_cpu_access).

> 
> Because memory has not been cleaned (we haven't had a device yet) the
> zeros that were written to this memory could  still be in the cache (since
> they were written with a cached mapping), this means that the unprivilived
> userpace client is now potentially reading sensitive kernel data....
> 

This is precisely why you can't just "pretend" that those pages
are uncached. You can't have the same memory mapped with different
attributes and get consistent behaviour.

> > If there are already attachments, then ion_dma_buf_begin_cpu_access()
> > will sync for CPU access against all of the attached devices, and
> > again the CPU should see the right thing.
> > 
> > In the other direction, ion_dma_buf_end_cpu_access() will sync for
> > device access for all currently attached devices. If there's no
> > attached devices yet, then there's nothing to do until there is (only
> > thing accessing is CPU via a CPU-cached mapping).
> > 
> > When the first (or another) device attaches, then when it maps the
> > buffer, the map_dma_buf callback should do whatever sync-ing is needed
> > for that device.
> > 
> > I might be way off with my understanding of the various DMA APIs, but
> > this is how I think they're meant to work.
> > 
> > > > Given that as you pointed out, the kernel does still have a cached
> > > > mapping to these pages, trying to give the CPU a non-cached mapping of
> > > > those same pages while preserving consistency seems fraught. Wouldn't
> > > > it be better to make sure all CPU mappings are cached, and have CPU
> > > > clients use the dma_buf_{begin,end}_cpu_access() hooks to get
> > > > consistency where needed?
> > > > 
> > > 
> > > It is fraught, but unfortunately you can't rely on 
> > > dma_buf_{begin,end}_cpu_access() to do cache maintenance as these calls 
> > > require a device, and a device is not always available.
> > 
> > As above, if there's really no device, then no syncing is needed
> > because only the CPU is accessing the buffer, and only ever via cached
> > mappings.
> > 
> 
> Sure you can use cached mappings, but with cached memory to ensure cache 
> coherency you would always need to do cache maintenance at dma map and dma 
> unmap (since you can't rely on their being a device when 
> dma_buf_{begin,end}_cpu_access() hooks are called).

As you've said below, you can't skip cache maintenance in the general
case - the first time a device maps the buffer, you need to clean the
cache to make sure the memset(0) is seen by the device.

> But with this cached memory you get poor performance because you are 
> frequently doing cache mainteance uncessarily because there *could* be CPU access.
> 
> The reason we want to use uncached allocations, with uncached mappings, is 
> to avoid all this uncessary cache maintenance.
> 

OK I think this is the key - you don't actually care whether the
mappings are non-cached, you just don't want to pay a sync penalty if
the CPU never touched the buffer.

In that case, then to me the right thing to do is make ion use
dma_map_sg_attrs(..., DMA_ATTR_SKIP_CPU_SYNC) in ion_map_dma_buf(), if
it knows that the CPU hasn't touched the buffer (which it does - from
{begin,end}_cpu_access).

That seems to be exactly what it's there for:

 /*
  * DMA_ATTR_SKIP_CPU_SYNC: Allows platform code to skip synchronization of
  * the CPU cache for the given buffer assuming that it has been already
  * transferred to 'device' domain.
  */

The very first time you map the buffer on a device, you have to sync
(transfer to 'device' domain). After that, if you never touch the
buffer on the CPU, then you'll never pay the CPU cache maintenance
penalty.

> > > 
> > > > >
> > > > >This change has the following potential disadvantages:
> > > > >- It assumes that userpace clients won't attempt to access the buffer
> > > > >while it is being mapped as we are removing the userpspace mappings at
> > > > >this point (though it is okay for them to have it mapped)
> > > > >- It assumes that kernel clients won't hold a kernel mapping to the
> > > > buffer
> > > > >(ie dma_buf_kmap) while it is being dma-mapped. What should we do if
> > > > there
> > > > >is a kernel mapping at the time of dma mapping, fail the mapping, warn?
> > > > >- There may be a performance penalty as a result of having to fault in
> > > > the
> > > > >pages after removing the userspace mappings.
> > > > 
> > > > I wonder if the dma-buf sync ioctl might provide a way for userspace
> > > > to opt-in to when the zap/fault happens. Zap on (DMA_BUF_SYNC_WRITE |
> > > > DMA_BUF_SYNC_WRITE_END) and fault on (DMA_BUF_SYNC_READ |
> > > > DMA_BUF_SYNC_START)
> > > > 
> > > 
> > > Not sure I understand, can you elaborate. 
> > > Are you also adding a requirment that ION pages can't be mmaped during a
> > > call to dma_buf_map_attachment?
> > 
> > I was only suggesting that zapping the mappings "at random" (from
> > userspace's perspective), and then faulting them back in (also "at
> > random"), might cause unexpected and not-controllable stalls in the
> > app. We could use the ioctl hooks as an explicit indication from the
> > app that now is a good time to zap the mapping and/or fault back in
> > the whole buffer. begin_cpu_access is allowed to be a "slow"
> > operation, so apps should already be expecting to get stalled on the
> > sync ioctl.
> > 
> 
> I think we have to do the zapping when have a device with which we can
> then immediately clean the caches for the memory.
> 
> The dma_buf_map_attachement seems like a logical time to do this, we have
> a device and the user should not be doing CPU access at this time.
> There is no guarantee you will ever have a device attached when the ioctl
> hooks are called so this could mean you never get a chance to switch to
> actual uncached mappings if you only try to do this from the ioctl hooks.
> 

You can always zap in the ioctl. You just might end up having to
create a cached mapping for userspace again if a device doesn't attach
before the next time it calls the SYNC_START ioctl.

So yeah, with your approach of trying to switch userspace over to
non-cached mappings, I think map_attachment is the best place to do
the whole shebang, to avoid unnecessary work.

> The one-of hit of having to fault the pages back in is unfortunate but I
> can't seem to find a better time to do it.

That part you really could do in the SYNC_START ioctl, it's just not
symmetric.

Thanks,
-Brian

> 
> Liam
> 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Liam Mark]

On Wed, 28 Nov 2018, Brian Starkey wrote:

> Hi Liam,
> 
> On Tue, Nov 27, 2018 at 10:46:07PM -0800, Liam Mark wrote:
> > On Tue, 27 Nov 2018, Brian Starkey wrote:
> > 
> > > Hi Liam,
> > > 
> > > On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote:
> > > > On Tue, 20 Nov 2018, Brian Starkey wrote:
> > > > 
> > > > > Hi Liam,
> > > > > 
> > > > > I'm missing a bit of context here, but I did read the v1 thread.
> > > > > Please accept my apologies if I'm re-treading trodden ground.
> > > > > 
> > > > > I do know we're chasing nebulous ion "problems" on our end, which
> > > > > certainly seem to be related to what you're trying to fix here.
> > > > > 
> > > > > On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
> > > > > >Based on the suggestions from Laura I created a first draft for a change
> > > > > >which will attempt to ensure that uncached mappings are only applied to
> > > > > >ION memory who's cache lines have been cleaned.
> > > > > >It does this by providing cached mappings (for uncached ION allocations)
> > > > > >until the ION buffer is dma mapped and successfully cleaned, then it
> > > > > drops
> > > > > >the userspace mappings and when pages are accessed they are faulted back
> > > > > >in and uncached mappings are created.
> > > > > 
> > > > > If I understand right, there's no way to portably clean the cache of
> > > > > the kernel mapping before we map the pages into userspace. Is that
> > > > > right?
> > > > > 
> > > > 
> > > > Yes, it isn't always possible to clean the caches for an uncached mapping 
> > > > because a device is required by the DMA APIs to do cache maintenance and 
> > > > there isn't necessarily a device available (dma_buf_attach may not yet 
> > > > have been called).
> > > > 
> > > > > Alternatively, can we just make ion refuse to give userspace a
> > > > > non-cached mapping for pages which are mapped in the kernel as cached?
> > > > 
> > > > These pages will all be mapped as cached in the kernel for 64 bit (kernel 
> > > > logical addresses) so you would always be refusing to create a non-cached mapping.
> > > 
> > > And that might be the sane thing to do, no?
> > > 
> > > AFAIK there are still pages which aren't ever mapped as cached (e.g.
> > > dma_declare_coherent_memory(), anything under /reserved-memory marked
> > > as no-map). If those are exposed as an ion heap, then non-cached
> > > mappings would be fine, and permitted.
> > > 
> > 
> > Sounds like you are suggesting using carveouts to support uncached?
> > 
> 
> No, I'm just saying that ion can't give out uncached _CPU_ mappings
> for pages which are already mapped on the CPU as cached.
> 

Okay then I guess I am not clear on where you would get this memory 
which doesn't have a cached kernel mapping.
It sounded like you wanted to define sections of memory in the DT as not 
mapped in the kernel and then hand this memory to 
dma_declare_coherent_memory (so that it can be managed) and then use an 
ION heap as the allocator.  If the memory was defined this way it sounded 
a lot like a carveout. But I guess you have some thoughts on how this 
memory which doesn't have a kernel mapping can be made available for general
use (for example available in buddy)?

Perhaps you were thinking of dynamically removing the kernel mappings 
before handing it out as uncached, but this would have a general system 
performance impact as this memory could come from anywhere so we would 
quickly lose our 1GB block mappings (and likely many of our 2MB block 
mappings as well).


> > We have many multimedia use cases which use very large amounts of uncached
> > memory, uncached memory is used as a performance optimization because CPU
> > access won't happen so it allows us to skip cache maintenance for all the
> > dma map and dma unmap calls. To create carveouts large enough to support
> > to support the worst case scenarios could result in very large carveouts.
> > 
> > Large carveouts like this would likely result in poor memory utilizations
> > (since they are tuned for worst case) which would likely have significant
> > performance impacts (more limited memory causes more frequent memory
> > reclaim ect...).
> > 
> > Also estimating for worst case could be difficult since the amount of
> > uncached memory could be app dependent.
> > Unfortunately I don't think this would make for a very scalable solution.
> > 
> 
> Sure, I understand the desire not to use carveouts. I'm not suggesting
> carveouts are a viable alternative.
> 
> > > > 
> > > > > Would userspace using the dma-buf sync ioctl around its accesses do
> > > > > the "right thing" in that case?
> > > > > 
> > > > 
> > > > I don't think so, the dma-buf sync ioctl require a device to peform cache 
> > > > maintenance, but as mentioned above a device may not be available.
> > > > 
> > > 
> > > If a device didn't attach yet, then no cache maintenance is
> > > necessary. The only thing accessing the memory is the CPU, via a
> > > cached mapping, which should work just fine. So far so good.
> > > 
> > 
> > Unfortunately not.
> > Scenario:
> > - Client allocates uncached memory.
> > - Client calls the DMA_BUF_IOCTL_SYNC IOCT IOCTL with flags
> > DMA_BUF_SYNC_START (but this doesn't do any cache maintenance since there
> > isn't any device)
> > - Client mmap the memory (ION creates uncached mapping)
> > - Client reads from that uncached mapping
> 
> I think I maybe wasn't clear with my proposal. The sequence should be
> like this:
> 
>  - Client allocates memory
>    - If this is from a region which the CPU has mapped as cached, then
>      that's not "uncached" memory - it's cached memory - and you have
>      to treat it as such.
>  - Client calls the DMA_BUF_IOCTL_SYNC IOCTL with flags
>    DMA_BUF_SYNC_START (but this doesn't do any cache maintenance since
>    there isn't any device)
>  - Client mmaps the memory
>    - ion creates a _cached_ mapping into the userspace process. ion
>      *must not* create an uncached mapping.
>  - Client reads from that cached mapping
>    - It sees zeroes, as expected.
> 
> This proposal ensures that everyone will *always* see correct data if
> they use the DMA APIs properly (device accesses via
> dma_buf_{map,unmap}, CPU access via {begin,end}_cpu_access).
> 

I am not sure I am properly understanding as this is what my V2 patch 
does, then when it gets an opportunity it allows the memory to be 
re-mapped as uncached.

Or are you perhaps suggesting that if the memory is allocated from a 
cached region then it always remains as cached, so only provide uncached 
if it was allocated from an uncached region? If so I view all memory 
available to the ION system heap for uncached allocations as having a 
cached mapping (since it is all part of the kernel logical mappigns), so I 
can't see how this would ever be able to support uncached allocations.

I guess once I understand how you will be providing memory to ION which 
isn't mapped as cached in the kernel, and therefore can be used to satisfy 
uncached ION allocations, this will make more sense to me.


> > 
> > Because memory has not been cleaned (we haven't had a device yet) the
> > zeros that were written to this memory could  still be in the cache (since
> > they were written with a cached mapping), this means that the unprivilived
> > userpace client is now potentially reading sensitive kernel data....
> > 
> 
> This is precisely why you can't just "pretend" that those pages
> are uncached. You can't have the same memory mapped with different
> attributes and get consistent behaviour.
> 
> > > If there are already attachments, then ion_dma_buf_begin_cpu_access()
> > > will sync for CPU access against all of the attached devices, and
> > > again the CPU should see the right thing.
> > > 
> > > In the other direction, ion_dma_buf_end_cpu_access() will sync for
> > > device access for all currently attached devices. If there's no
> > > attached devices yet, then there's nothing to do until there is (only
> > > thing accessing is CPU via a CPU-cached mapping).
> > > 
> > > When the first (or another) device attaches, then when it maps the
> > > buffer, the map_dma_buf callback should do whatever sync-ing is needed
> > > for that device.
> > > 
> > > I might be way off with my understanding of the various DMA APIs, but
> > > this is how I think they're meant to work.
> > > 
> > > > > Given that as you pointed out, the kernel does still have a cached
> > > > > mapping to these pages, trying to give the CPU a non-cached mapping of
> > > > > those same pages while preserving consistency seems fraught. Wouldn't
> > > > > it be better to make sure all CPU mappings are cached, and have CPU
> > > > > clients use the dma_buf_{begin,end}_cpu_access() hooks to get
> > > > > consistency where needed?
> > > > > 
> > > > 
> > > > It is fraught, but unfortunately you can't rely on 
> > > > dma_buf_{begin,end}_cpu_access() to do cache maintenance as these calls 
> > > > require a device, and a device is not always available.
> > > 
> > > As above, if there's really no device, then no syncing is needed
> > > because only the CPU is accessing the buffer, and only ever via cached
> > > mappings.
> > > 
> > 
> > Sure you can use cached mappings, but with cached memory to ensure cache 
> > coherency you would always need to do cache maintenance at dma map and dma 
> > unmap (since you can't rely on their being a device when 
> > dma_buf_{begin,end}_cpu_access() hooks are called).
> 
> As you've said below, you can't skip cache maintenance in the general
> case - the first time a device maps the buffer, you need to clean the
> cache to make sure the memset(0) is seen by the device.
> 

Unfortunately if are only using cached mappings it isn't only the first 
time you dma map the buffer you need to do cache maintenance, you need to 
almost always do it because you don't know what CPU access happened (or 
will happen) without a device.
Explained more below.

> > But with this cached memory you get poor performance because you are 
> > frequently doing cache mainteance uncessarily because there *could* be CPU access.
> > 
> > The reason we want to use uncached allocations, with uncached mappings, is 
> > to avoid all this uncessary cache maintenance.
> > 
> 
> OK I think this is the key - you don't actually care whether the
> mappings are non-cached, you just don't want to pay a sync penalty if
> the CPU never touched the buffer.
> 
> In that case, then to me the right thing to do is make ion use
> dma_map_sg_attrs(..., DMA_ATTR_SKIP_CPU_SYNC) in ion_map_dma_buf(), if
> it knows that the CPU hasn't touched the buffer (which it does - from
> {begin,end}_cpu_access).
> 

Unfortunately that isn't the case we are trying to optimize for,  we 
aren't trying to optimize for the case where CPU *never* touches the 
buffer we are trying to optimize for the case where the CPU may *rarely* 
touch the buffer.

If a client allocates cached memory the driver calling dma map and dma 
unmap has no way of knowing if at some pointe further down the pipeline 
there will be some userspace module which will attempt to do some kind
of CPU access (example image library post processing).  This userspace 
moduel will call the required DMA_BUF_IOCTL_SYNC  IOCTLs, however there 
may no longer be a device attached, therefore these calls won't 
necessarily do the appropriate cache maintenance.

So what this means is that if a cached buffers is used you have to at 
least  always to a cache invalidating when dma unmapping (from a device 
which isn't io-coherrent that did a write)  otherwise there could be a CPU 
attempted to read that data using a cached mapping which could end up 
reading a stale cache line (for example acquired through speculative 
access).

This frequent uncessary cache maintenance adds a significant performance 
impact and that is why we use uncached memory because it allows us to skip 
all this cache maintenance.
Basically your driver can't predict the future so it has to play it safe 
when cached ION buffers are involved.

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations

[Brian Starkey]

On Wed, Nov 28, 2018 at 11:03:37PM -0800, Liam Mark wrote:
> On Wed, 28 Nov 2018, Brian Starkey wrote:
> > On Tue, Nov 27, 2018 at 10:46:07PM -0800, Liam Mark wrote:
> > > On Tue, 27 Nov 2018, Brian Starkey wrote:
> > > > On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote:
> > > > > On Tue, 20 Nov 2018, Brian Starkey wrote:

[snip]

> > > > 
> > > 
> > > Sounds like you are suggesting using carveouts to support uncached?
> > > 
> > 
> > No, I'm just saying that ion can't give out uncached _CPU_ mappings
> > for pages which are already mapped on the CPU as cached.

Probably this should have been: s/can't/shouldn't/

> > 
> 
> Okay then I guess I am not clear on where you would get this memory 
> which doesn't have a cached kernel mapping.
> It sounded like you wanted to define sections of memory in the DT as not 
> mapped in the kernel and then hand this memory to 
> dma_declare_coherent_memory (so that it can be managed) and then use an 
> ION heap as the allocator.  If the memory was defined this way it sounded 
> a lot like a carveout. But I guess you have some thoughts on how this 
> memory which doesn't have a kernel mapping can be made available for general
> use (for example available in buddy)?
> 
> Perhaps you were thinking of dynamically removing the kernel mappings 
> before handing it out as uncached, but this would have a general system 
> performance impact as this memory could come from anywhere so we would 
> quickly lose our 1GB block mappings (and likely many of our 2MB block 
> mappings as well).
> 

All I'm saying, with respect to non-cached memory and mappings, is
this:

I don't think ion should create non-cached CPU mappings of memory
which is mapped in the kernel as cached.

By extension, that means that in my opinion, the only way userspace
should be able to get a non-cached mapping, is by allocating from a
carveout.

However, I don't think this should be what we do in our complicated
media-heavy systems - carveouts are clearly impractical, as is
removing memory from the kernel map. What I think we should do, is
always do CPU access via cached mappings, for memory which is mapped
in the kernel as cached.

[snip]

> > 
> 
> I am not sure I am properly understanding as this is what my V2 patch 
> does, then when it gets an opportunity it allows the memory to be 
> re-mapped as uncached.

It's the remapping as uncached part which I'm not so keen on. It just
seems rather fragile to have mappings for the same memory with
different attributes around.

> 
> Or are you perhaps suggesting that if the memory is allocated from a 
> cached region then it always remains as cached, so only provide uncached 
> if it was allocated from an uncached region? If so I view all memory 
> available to the ION system heap for uncached allocations as having a 
> cached mapping (since it is all part of the kernel logical mappigns), so I 
> can't see how this would ever be able to support uncached allocations.

Yeah, that's exactly what I'm saying. The system heap should not
allow uncached allocations, and, memory allocated from the system heap
should always be mapped as cached for CPU accesses.

Non-cached allocations would only be allowed from carveouts (but as
discussed, I don't think carveouts are a practical solution for the
general case).

The summary of my proposal is that instead of focussing on getting
non-cached allocations, we should make cached allocations work better,
so that non-cached aliases of cached memory aren't required.

[snip]

> 
> Unfortunately if are only using cached mappings it isn't only the first 
> time you dma map the buffer you need to do cache maintenance, you need to 
> almost always do it because you don't know what CPU access happened (or 
> will happen) without a device.

I think you can always know if CPU _has_ accessed the buffer - in
begin_cpu_access, ion can set a flag, which it checks in map_dma_buf.
If that flag says it's been touched, then a cache clean is needed.

Of course you can't predict the future - there's no way to know if the
CPU _will_ access the buffer - which I think is what you're getting at
below.

> Explained more below.
> 
> > > But with this cached memory you get poor performance because you are 
> > > frequently doing cache mainteance uncessarily because there *could* be CPU access.
> > > 
> > > The reason we want to use uncached allocations, with uncached mappings, is 
> > > to avoid all this uncessary cache maintenance.
> > > 
> > 
> > OK I think this is the key - you don't actually care whether the
> > mappings are non-cached, you just don't want to pay a sync penalty if
> > the CPU never touched the buffer.
> > 
> > In that case, then to me the right thing to do is make ion use
> > dma_map_sg_attrs(..., DMA_ATTR_SKIP_CPU_SYNC) in ion_map_dma_buf(), if
> > it knows that the CPU hasn't touched the buffer (which it does - from
> > {begin,end}_cpu_access).
> > 
> 
> Unfortunately that isn't the case we are trying to optimize for,  we 
> aren't trying to optimize for the case where CPU *never* touches the 
> buffer we are trying to optimize for the case where the CPU may *rarely* 
> touch the buffer.
> 
> If a client allocates cached memory the driver calling dma map and dma 
> unmap has no way of knowing if at some pointe further down the pipeline 
> there will be some userspace module which will attempt to do some kind
> of CPU access (example image library post processing).  This userspace 
> moduel will call the required DMA_BUF_IOCTL_SYNC  IOCTLs, however there 
> may no longer be a device attached, therefore these calls won't 
> necessarily do the appropriate cache maintenance.

(as a slight aside: Is cache maintenance really slower than the CPU
running image processing algorithms on a non-cached mapping?
Intuitively it seems that doing processing on a cached mapping with
cache maintenance should far outperform direct non-cached access. I
understand that this isn't the real issue, and what you really care
about is being able to do device<->device operation without paying a
cache maintenance penalty. I'm just surprised that CPU processing on
non-cached mappings isn't *so bad* that it makes non-cached CPU access
totally untenable)

> 
> So what this means is that if a cached buffers is used you have to at 
> least  always to a cache invalidating when dma unmapping (from a device 
> which isn't io-coherrent that did a write)  otherwise there could be a CPU 
> attempted to read that data using a cached mapping which could end up 
> reading a stale cache line (for example acquired through speculative 
> access).

OK now I'm with you. As you say, before CPU access you would need to
invalidate, and the only way that's currently possible (at least on
arm64) is in unmap_dma_buf - so you're paying an invalidate penalty on
every unmap. That is the only penalty though; there's no need to do a
clean on map_dma_buf unless the CPU really did touch it.

With your patch I think you are still doing that invalidation right?
Is the performance of this patch OK, or you'll follow up with skipping
the invalidation?

It does seem a bit strange to me that begin_cpu_access() with no
device won't even invalidate the CPU cache. I had a bit of a poke
around, and that seems to be relatively specific to the arm64 dummy
ops. I'd have thought defaulting to dma_noncoherent_ops would work
better, but I don't know the specifics of those decisions.

If it were possible to skip the invalidation in unmap_dma_buf, would
cached mappings work for you? I think it should even be faster (in all
cases) than any non-cached approach.

Cheers,
-Brian

> 
> This frequent uncessary cache maintenance adds a significant performance 
> impact and that is why we use uncached memory because it allows us to skip 
> all this cache maintenance.
> Basically your driver can't predict the future so it has to play it safe 
> when cached ION buffers are involved.
> 
> Liam
> 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project