Hi Pascal and all,
Thanks for taking the time to do a thorough review.
On Tue, Jun 23, 2015 at 09:37:13PM +0200, Pascal Brand wrote:
*smccc_call32* Do we only target aarch32 optee_os?
No, this for Aarch64 optee_os too. It's just that we don't use Aarch64 SMC Calling convention. Aarch32 SMC Calling Convention is enough to pass the information we need.
*Buffer management* It looks that temporary buffers are now managed in the client API, not in the driver anymore. The positive point is that it makes the driver lighter. But the drawbacks is the number of extra ioctl used when using a temporary buffer: 1 to allocate, 1 to map, and 1 to unmap. So a TEEC_InvokeCommand could lead to 13 ioctls. What about the performances? Could not it be optimized in some way? Could we manage temporary buffers, dma buffers and "other" buffers in different ways?
Temporary buffers should be avoided completely if the client is concerned about performance. Allocated shared memory is the most efficient way of shared memory. DMA buffers should be treated in the same way as allocated shared memory.
*Kernel API* The GP API looks managed in the Client part now, without a 1-to-1 mapping in kernel side. That means that we have lost the Kernel API, which is useful when a given driver wants to use a TA. It looks we are really missing this Kernel API. This is the most critical point of our review.
The kernel API isn't there right now because it's not used by anything. This patch-set is kept as small as possible while still being useful, to avoid making it harder than necessary to upstream. Once this patch-set is upstream we can start to add different features, for instance a kernel API.
*Non-zero copy* When using RegisterSharedMemory(), it looks we do not benefit from the fact that the original buffer was allocated using dma_buf. Hence we never have zero-copy in the current version when using RegisterSharedMemory as we will furher use the so-called shadowbuffer. Can you confirm this limitation can be removed in further versions of the patch?
How can TEEC_RegisterSharedMemory() tell if the supplied memory range is backed by a dma_buf? Perhaps we can add another TEE_IOC_XXX to given an address get a file descriptor etc needed to use that memory as shared memory. The supplied address must of course be within a range of a mmaped dma_buf for it to work.
Another option would be to extend TEE Client API with a way of initializing a TEEC_SharedMemory based on a given dma_buf.
Yes, I think it should be possible to remove this limitation in future versions.
By the way, we think xtest 5006, which calls TEEC_RegisterSharedBuffer() twice may lead to memory leaks (it could be the same with the current optee_linuxdriver).
Agree, the test is broken in that sense. The memory will be reclaimed when the process dies though.
*List of SHM* The list of the SHM allocated is now contained in the device, whereas it used to be in the context in optee_linuxdriver. This is not clear to us whether this is an issue or not.
This list is only used by the currently unused functions tee_shm_find_by_va() and tee_shm_find_by_pa(). I don't think they will ever be needed by the OP-TEE driver. I added those functions because other drivers may need them. In the OP-TEE driver we avoid tee_shm_find_by_pa() by always passing a shm_ref to secure world and secure world passes it back when needed.
*tee_shm_pool_alloc_res_mem()* If we'd like to use the generic function for a LX backend, we would need to add the following arguments to the function:
- the alignment of the private region, and the one of the dma region
- a way to give the limits between both region (the size of the 1st
region, or anything else).
No problem, this is only a kernel internal API so it can be changed whenever we need to. But we should try to get it as good as possible from the start. By alignment I guess it's alignment of the different areas, not the alignment of the buffers allocated from each area.
Would this be OK?
struct tee_shm_pool_res_mem_info { unsigned long priv_vaddr; unsigned long priv_paddr; size_t priv_size; unsigned long dma_buf_vaddr; unsigned long dma_buf_paddr; size_t dma_buf_size; };
struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev, struct tee_shm_pool_res_mem_info *info);
Emmanuel and I would like to check few other points in the proposal. We'll come back to you if we have any other remarks.
OK. I plan to have something ready to send to the kernel lists during week 28.
-- Thanks, Jens