[PATCH v1 06/14] tee: optee: add page list manipulation functions

[Volodymyr Babchuk]

On 29.09.17 03:23, Yury Norov wrote:
> On Thu, Sep 28, 2017 at 09:04:03PM +0300, Volodymyr Babchuk wrote:
>> From: Volodymyr Babchuk <vlad.babchuk at gmail.com>
>>
>> These functions will be used to pass information about shared
>> buffers to OP-TEE.
>>
>> Signed-off-by: Volodymyr Babchuk <vlad.babchuk at gmail.com>
>> ---
>>   drivers/tee/optee/call.c          | 48 +++++++++++++++++++++++++++++++++++++++
>>   drivers/tee/optee/optee_private.h |  4 ++++
>>   2 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
>> index f7b7b40..f8e044d 100644
>> --- a/drivers/tee/optee/call.c
>> +++ b/drivers/tee/optee/call.c
>> @@ -11,6 +11,7 @@
>>    * GNU General Public License for more details.
>>    *
>>    */
>> +#include <asm/pgtable.h>
>>   #include <linux/arm-smccc.h>
>>   #include <linux/device.h>
>>   #include <linux/err.h>
>> @@ -442,3 +443,50 @@ void optee_disable_shm_cache(struct optee *optee)
>>   	}
>>   	optee_cq_wait_final(&optee->call_queue, &w);
>>   }
>> +
>> +/**
>> + * optee_fill_pages_list() - write list of user pages to given shared
>> + * buffer.
>> + *
>> + * @dst: page-aligned buffer where list of pages will be stored
> 
> I'm not much familiar with the subsystem you work on, but I don't
> understand why the type of dst is u64*. If it's just a buffer, it
> should be void *. Also, if we assuming running it on arm were pointers
> are 32-bit, the result of page_to_phys() will be u32, and you will
> waste half of your u64 array for storing zeroes; this line:
> 		*dst = page_to_phys(pages[i]);
Yep. There is defined ABI between OP-TEE OS and OP-TEE clients. That ABI 
demands that page addresses should be stored in 64-bit fields even on 
32-bit architectures.


>> + * @pages: array of pages that represents shared buffer
>> + * @num_pages: number of entries in @pages
>> + *
>> + * @dst should be big enough to hold list of user page addresses and
>> + *	links to the next pages of buffer
>> + */
>> +void optee_fill_pages_list(u64 *dst, struct page **pages, size_t num_pages)
>> +{
>> +	size_t i;
>> +
>> +	/* TODO: add support for RichOS page sizes that != 4096 */
>> +	BUILD_BUG_ON(PAGE_SIZE != OPTEE_MSG_NONCONTIG_PAGE_SIZE);
> 
> RichOS stands for Linux? Why I am still not a rich OS developer? :)
I'm asking the same question :) Yes, in terms of TEE, Linux is RichOS
and OP-TEE is TrustedOS.

> This is the first occurrence of the term in kernel sources, please
> explain it.
I'd rather change "RichOS" to "Linux".

> Also, I think that it would be more logical to add the dependency on
> page size to Kconfig, not here, and move the comment there, so user
> will be simply unable to build the whole module.
I event didn't thought of this. Thank you for suggestion. Will do in 
this way.

>> +	for (i = 0; i < num_pages; i++, dst++) {
>> +		/* Check if we are going to roll over the page boundary */
>> +		if (IS_ALIGNED((uintptr_t)(dst + 1),
>> +			       OPTEE_MSG_NONCONTIG_PAGE_SIZE)) {
>> +			*dst = virt_to_phys(dst + 1);
>> +			dst++;
>> +		}
> 
> Is my understanding correct that @dst is not a simple array of buffer
> page addresses?  Instead, it has a complex structure: First 511 records
> store buffer page entries, and last one points to the next page of dst.
> Is it somehow documented? Also, did you consider to create a header structure
> for the buffer page, like memory allocators do? You can place there number
> of entries, pointer to the next page, maybe some flags. I think it will be
> more transparent, especially if we consider communication protocol between
> independent software products.
This is documented in the previous patch "tee: optee: Update protocol 
definitions" (5/14).
I like your idea about header structure. Just to clarify: it should be 
structure that covers whole page. Like that described in the previous patch:

+ * struct page_data {
+ *   uint64_t 
pages_array[OPTEE_MSG_NONCONTIG_PAGE_SIZE/sizeof(uint64_t) - 1];
+ *   uint64_t next_page_data;
+ * };

Right?