Re: [PATCH 14/14] staging: android: ion: Add UNMAPPED heap type and helper

From: Laura Abbott
Date: Tue Jan 15 2019 - 13:43:59 EST

On 1/15/19 7:58 AM, Andrew F. Davis wrote:
On 1/14/19 8:32 PM, Laura Abbott wrote:
On 1/11/19 10:05 AM, Andrew F. Davis wrote:
The "unmapped" heap is very similar to the carveout heap except
the backing memory is presumed to be unmappable by the host, in
my specific case due to firewalls. This memory can still be
allocated from and used by devices that do have access to the
backing memory.

Based originally on the secure/unmapped heap from Linaro for
the OP-TEE SDP implementation, this was re-written to match
the carveout heap helper code.

Suggested-by: Etienne Carriere <etienne.carriere@xxxxxxxxxx>
Signed-off-by: Andrew F. Davis <afd@xxxxxx>
---
 drivers/staging/android/ion/Kconfig | 10 ++
 drivers/staging/android/ion/Makefile | 1 +
 drivers/staging/android/ion/ion.h | 16 +++
 .../staging/android/ion/ion_unmapped_heap.c | 123 ++++++++++++++++++
 drivers/staging/android/uapi/ion.h | 3 +
 5 files changed, 153 insertions(+)
 create mode 100644 drivers/staging/android/ion/ion_unmapped_heap.c

diff --git a/drivers/staging/android/ion/Kconfig
b/drivers/staging/android/ion/Kconfig
index 0fdda6f62953..a117b8b91b14 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -42,3 +42,13 @@ config ION_CMA_HEAP
ÂÂÂÂÂÂÂ Choose this option to enable CMA heaps with Ion. This heap is
backed
ÂÂÂÂÂÂÂ by the Contiguous Memory Allocator (CMA). If your system has
these
ÂÂÂÂÂÂÂ regions, you should say Y here.
+
+config ION_UNMAPPED_HEAP
+ÂÂÂ bool "ION unmapped heap support"
+ÂÂÂ depends on ION
+ÂÂÂ help
+ÂÂÂÂÂ Choose this option to enable UNMAPPED heaps with Ion. This heap is
+ÂÂÂÂÂ backed in specific memory pools, carveout from the Linux memory.
+ÂÂÂÂÂ Unlike carveout heaps these are assumed to be not mappable by
+ÂÂÂÂÂ kernel or user-space.
+ÂÂÂÂÂ Unless you know your system has these regions, you should say N
here.
diff --git a/drivers/staging/android/ion/Makefile
b/drivers/staging/android/ion/Makefile
index 17f3a7569e3d..c71a1f3de581 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o
ion_page_pool.o
 obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
 obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
 obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
+obj-$(CONFIG_ION_UNMAPPED_HEAP) += ion_unmapped_heap.o
diff --git a/drivers/staging/android/ion/ion.h
b/drivers/staging/android/ion/ion.h
index 97b2876b165a..ce74332018ba 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -341,4 +341,20 @@ static inline struct ion_heap
*ion_chunk_heap_create(phys_addr_t base, size_t si
 }
 #endif
 +#ifdef CONFIG_ION_UNMAPPED_HEAP
+/**
+ * ion_unmapped_heap_create
+ * @base:ÂÂÂÂÂÂÂ base address of carveout memory
+ * @size:ÂÂÂÂÂÂÂ size of carveout memory region
+ *
+ * Creates an unmapped ion_heap using the passed in data
+ */
+struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t
size);
+#else
+struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t size)
+{
+ÂÂÂ return ERR_PTR(-ENODEV);
+}
+#endif
+
 #endif /* _ION_H */
diff --git a/drivers/staging/android/ion/ion_unmapped_heap.c
b/drivers/staging/android/ion/ion_unmapped_heap.c
new file mode 100644
index 000000000000..7602b659c2ec
--- /dev/null
+++ b/drivers/staging/android/ion/ion_unmapped_heap.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ION Memory Allocator unmapped heap helper
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated -
http://www.ti.com/
+ *ÂÂÂ Andrew F. Davis <afd@xxxxxx>
+ *
+ * ION "unmapped" heaps are physical memory heaps not by default
mapped into
+ * a virtual address space. The buffer owner can explicitly request
kernel
+ * space mappings but the underlying memory may still not be
accessible for
+ * various reasons, such as firewalls.
+ */
+
+#include <linux/err.h>
+#include <linux/genalloc.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
+
+#include "ion.h"
+
+#define ION_UNMAPPED_ALLOCATE_FAIL -1
+
+struct ion_unmapped_heap {
+ÂÂÂ struct ion_heap heap;
+ÂÂÂ struct gen_pool *pool;
+};
+
+static phys_addr_t ion_unmapped_allocate(struct ion_heap *heap,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long size)
+{
+ÂÂÂ struct ion_unmapped_heap *unmapped_heap =
+ÂÂÂÂÂÂÂ container_of(heap, struct ion_unmapped_heap, heap);
+ÂÂÂ unsigned long offset;
+
+ÂÂÂ offset = gen_pool_alloc(unmapped_heap->pool, size);
+ÂÂÂ if (!offset)
+ÂÂÂÂÂÂÂ return ION_UNMAPPED_ALLOCATE_FAIL;
+
+ÂÂÂ return offset;
+}
+
+static void ion_unmapped_free(struct ion_heap *heap, phys_addr_t addr,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long size)
+{
+ÂÂÂ struct ion_unmapped_heap *unmapped_heap =
+ÂÂÂÂÂÂÂ container_of(heap, struct ion_unmapped_heap, heap);
+
+ÂÂÂ gen_pool_free(unmapped_heap->pool, addr, size);
+}
+
+static int ion_unmapped_heap_allocate(struct ion_heap *heap,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct ion_buffer *buffer,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long flags)
+{
+ÂÂÂ struct sg_table *table;
+ÂÂÂ phys_addr_t paddr;
+ÂÂÂ int ret;
+
+ÂÂÂ table = kmalloc(sizeof(*table), GFP_KERNEL);
+ÂÂÂ if (!table)
+ÂÂÂÂÂÂÂ return -ENOMEM;
+ÂÂÂ ret = sg_alloc_table(table, 1, GFP_KERNEL);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ goto err_free;
+
+ÂÂÂ paddr = ion_unmapped_allocate(heap, size);
+ÂÂÂ if (paddr == ION_UNMAPPED_ALLOCATE_FAIL) {
+ÂÂÂÂÂÂÂ ret = -ENOMEM;
+ÂÂÂÂÂÂÂ goto err_free_table;
+ÂÂÂ }
+
+ÂÂÂ sg_set_page(table->sgl, pfn_to_page(PFN_DOWN(paddr)), size, 0);
+ÂÂÂ buffer->sg_table = table;
+


If this memory is actually unmapped this is not going to work because
the struct page will not be valid.


If it will never get mapped then it doesn't need a valid struct page as
far as I can tell. We only use it as a marker to where the start of
backing memory is, and that is calculated based on the struct page
pointer address, not its contents.


You can't rely on pfn_to_page returning a valid page pointer if the
pfn doesn't point to reserved memory. Even if you aren't relying
on the contents, that's no guarantee you can use that to to get
the valid pfn back. You can get away with that sometimes but it's
not correct to rely on it all the time.

+ÂÂÂ return 0;
+
+err_free_table:
+ÂÂÂ sg_free_table(table);
+err_free:
+ÂÂÂ kfree(table);
+ÂÂÂ return ret;
+}
+
+static void ion_unmapped_heap_free(struct ion_buffer *buffer)
+{
+ÂÂÂ struct ion_heap *heap = buffer->heap;
+ÂÂÂ struct sg_table *table = buffer->sg_table;
+ÂÂÂ struct page *page = sg_page(table->sgl);
+ÂÂÂ phys_addr_t paddr = PFN_PHYS(page_to_pfn(page));
+
+ÂÂÂ ion_unmapped_free(heap, paddr, buffer->size);
+ÂÂÂ sg_free_table(buffer->sg_table);
+ÂÂÂ kfree(buffer->sg_table);
+}
+
+static struct ion_heap_ops unmapped_heap_ops = {
+ÂÂÂ .allocate = ion_unmapped_heap_allocate,
+ÂÂÂ .free = ion_unmapped_heap_free,
+ÂÂÂ /* no .map_user, user mapping of unmapped heaps not allowed */
+ÂÂÂ .map_kernel = ion_heap_map_kernel,
+ÂÂÂ .unmap_kernel = ion_heap_unmap_kernel,
+};
+
+struct ion_heap *ion_unmapped_heap_create(phys_addr_t base, size_t size)
+{
+ÂÂÂ struct ion_unmapped_heap *unmapped_heap;
+
+ÂÂÂ unmapped_heap = kzalloc(sizeof(*unmapped_heap), GFP_KERNEL);
+ÂÂÂ if (!unmapped_heap)
+ÂÂÂÂÂÂÂ return ERR_PTR(-ENOMEM);
+
+ÂÂÂ unmapped_heap->pool = gen_pool_create(PAGE_SHIFT, -1);
+ÂÂÂ if (!unmapped_heap->pool) {
+ÂÂÂÂÂÂÂ kfree(unmapped_heap);
+ÂÂÂÂÂÂÂ return ERR_PTR(-ENOMEM);
+ÂÂÂ }
+ÂÂÂ gen_pool_add(unmapped_heap->pool, base, size, -1);
+ÂÂÂ unmapped_heap->heap.ops = &unmapped_heap_ops;
+ÂÂÂ unmapped_heap->heap.type = ION_HEAP_TYPE_UNMAPPED;
+
+ÂÂÂ return &unmapped_heap->heap;
+}
diff --git a/drivers/staging/android/uapi/ion.h
b/drivers/staging/android/uapi/ion.h
index 5d7009884c13..d5f98bc5f340 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -19,6 +19,8 @@
ÂÂ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ carveout heap, allocations are physically
ÂÂ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ contiguous
ÂÂ * @ION_HEAP_TYPE_DMA:ÂÂÂÂÂÂÂÂ memory allocated via DMA API
+ * @ION_HEAP_TYPE_UNMAPPED:ÂÂÂÂ memory not intended to be mapped into
the
+ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ linux address space unless for debug cases
ÂÂ * @ION_NUM_HEAPS:ÂÂÂÂÂÂÂÂ helper for iterating over heaps, a bit mask
ÂÂ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ is used to identify the heaps, so only 32
ÂÂ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ total heap types are supported
@@ -29,6 +31,7 @@ enum ion_heap_type {
ÂÂÂÂÂ ION_HEAP_TYPE_CARVEOUT,
ÂÂÂÂÂ ION_HEAP_TYPE_CHUNK,
ÂÂÂÂÂ ION_HEAP_TYPE_DMA,
+ÂÂÂ ION_HEAP_TYPE_UNMAPPED,
ÂÂÂÂÂ ION_HEAP_TYPE_CUSTOM, /*
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * must be last so device specific heaps always
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * are at the end of this enum


Overall this seems way too similar to the carveout heap
to justify adding another heap type. It also still missing
the part of where exactly you call ion_unmapped_heap_create.
Figuring that out is one of the blocking items for moving
Ion out of staging.


I agree with this being almost a 1:1 copy of the carveout heap, I'm just
not sure of a good way to do this without a new heap type. Adding flags
to the existing carveout type seem a bit messy. Plus then those flags
should be valid for other heap types, gets complicated quickly..

I'll reply to the second part in your other top level response.

Andrew

Thanks,
Laura