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. >> + 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel