lkml.org 
[lkml]   [2017]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 06/14] tee: optee: add page list manipulation functions
From
Date


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@gmail.com>
>>
>> These functions will be used to pass information about shared
>> buffers to OP-TEE.
>>
>> Signed-off-by: Volodymyr Babchuk <vlad.babchuk@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?

\
 
 \ /
  Last update: 2017-09-29 12:36    [W:0.101 / U:9.536 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site