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