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

[Andrew F. Davis]

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