diff mbox

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

Message ID 1506621851-6929-7-git-send-email-volodymyr_babchuk@epam.com (mailing list archive)
State New, archived
Headers show

Commit Message

Volodymyr Babchuk Sept. 28, 2017, 6:04 p.m. UTC 
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(+)

Comments

Yury Norov Sept. 29, 2017, 12:23 a.m. UTC | #1 
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]);

> + * @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? :)
This is the first occurrence of the term in kernel sources, please
explain it.

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.

> +	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.

> +		*dst = page_to_phys(pages[i]);
> +	}
> +}
> +
> +static size_t get_pages_array_size(size_t num_entries)
> +{
> +	/* Number of user pages + number of pages to hold list of user pages */
> +	return sizeof(u64) *
> +		(num_entries + (sizeof(u64) * num_entries) /
> +		 OPTEE_MSG_NONCONTIG_PAGE_SIZE);
> +}
> +
> +u64 *optee_allocate_pages_array(size_t num_entries)
> +{
> +	return alloc_pages_exact(get_pages_array_size(num_entries), GFP_KERNEL);
> +}
> +
> +void optee_free_pages_array(void *array, size_t num_entries)
> +{
> +	free_pages_exact(array, get_pages_array_size(num_entries));
> +}
> +
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index c374cd5..caa3c04 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -165,6 +165,10 @@ int optee_from_msg_param(struct tee_param *params, size_t num_params,
>  int optee_to_msg_param(struct optee_msg_param *msg_params, size_t num_params,
>  		       const struct tee_param *params);
>  
> +u64 *optee_allocate_pages_array(size_t num_entries);
> +void optee_free_pages_array(void *array, size_t num_entries);
> +void optee_fill_pages_list(u64 *dst, struct page **pages, size_t num_pages);
> +
>  /*
>   * Small helpers
>   */
> -- 
> 2.7.4
Volodymyr Babchuk Sept. 29, 2017, 10:34 a.m. UTC | #2 
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?
Mark Rutland Sept. 29, 2017, 1 p.m. UTC | #3 
On Thu, Sep 28, 2017 at 09:04:03PM +0300, Volodymyr Babchuk wrote:
> +/**
> + * optee_fill_pages_list() - write list of user pages to given shared
> + * buffer.
> + *
> + * @dst: page-aligned buffer where list of pages will be stored
> + * @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;

Why size_t? It's unusual for an array index.

> +	/* TODO: add support for RichOS page sizes that != 4096 */
> +	BUILD_BUG_ON(PAGE_SIZE != OPTEE_MSG_NONCONTIG_PAGE_SIZE);

This must be fixed before this can be considered for merging.

A large number of people build arm64 kernels with 64K pages, and this
will need to see some testing.

> +	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++;
> +		}
> +		*dst = page_to_phys(pages[i]);

... so this pagelist management will need to be reworked.

> +	}
> +}
> +
> +static size_t get_pages_array_size(size_t num_entries)
> +{
> +	/* Number of user pages + number of pages to hold list of user pages */
> +	return sizeof(u64) *
> +		(num_entries + (sizeof(u64) * num_entries) /
> +		 OPTEE_MSG_NONCONTIG_PAGE_SIZE);
> +}

I don't think this is correct.

For P 4096-byte pages, we can have 511 * P (8-byte) page entries, and P
(8-byte) next entries.

So if we need to list 1023 page entries, we need 3 (4096-byte) pages.
The first page holds 511 entries, the second holds 511 entries, and the
third holds 1 entry.

However, the above calculates that we need 2 (4096-byte) pages, as it
calculates that in bytes we need:

  8 * (1023 + (8 * 1023) / 4096)
  8 * (1023 + (8184) / 4096)
  8 * (1023 + 1)
  8 * 1024
  8192

... or 2 (4096-byte) pages.


I think it would be clearer to write this over a number of steps, e.g.

/*
 * The final entry in each pagelist page is a pointer to the next
 * pagelist page.
 */
#define PAGELIST_ENTRIES_PER_PAGE \
	((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(u64)) - 1)

static size_t get_pages_array_size(size_t num_entries)
{
	int pages = DIV_ROUND_UP(num_entries, PAGELIST_ENTRIES_PER_PAGE);

	return pages * OPTEE_MSG_NONCONTIG_PAGE_SIZE;
}

> +
> +u64 *optee_allocate_pages_array(size_t num_entries)
> +{
> +	return alloc_pages_exact(get_pages_array_size(num_entries), GFP_KERNEL);
> +}
> +
> +void optee_free_pages_array(void *array, size_t num_entries)
> +{
> +	free_pages_exact(array, get_pages_array_size(num_entries));
> +}
> +
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index c374cd5..caa3c04 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -165,6 +165,10 @@ int optee_from_msg_param(struct tee_param *params, size_t num_params,
>  int optee_to_msg_param(struct optee_msg_param *msg_params, size_t num_params,
>  		       const struct tee_param *params);
>  
> +u64 *optee_allocate_pages_array(size_t num_entries);
> +void optee_free_pages_array(void *array, size_t num_entries);
> +void optee_fill_pages_list(u64 *dst, struct page **pages, size_t num_pages);

Any reason for the array/list naming disparity? IIUC, these are the same
structure.

Thanks,
Mark.
Yury Norov Sept. 29, 2017, 4:23 p.m. UTC | #4 
On Fri, Sep 29, 2017 at 01:34:13PM +0300, Volodymyr Babchuk wrote:
> 
> 
> 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).

Ah, OK. 

> 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?

It's OK, if there's the requirement to allocate the whole page for
shmem pagerefs array. If not, the proposed approach means that you'll
waste the whole page to store shared memory descriptors, even if
shared memory is as small as one page, and so a single u64 is needed
to describe it. I think it makes sense for compile-time declared
shmems.

Yury
diff mbox

Patch

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
+ * @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);
+	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++;
+		}
+		*dst = page_to_phys(pages[i]);
+	}
+}
+
+static size_t get_pages_array_size(size_t num_entries)
+{
+	/* Number of user pages + number of pages to hold list of user pages */
+	return sizeof(u64) *
+		(num_entries + (sizeof(u64) * num_entries) /
+		 OPTEE_MSG_NONCONTIG_PAGE_SIZE);
+}
+
+u64 *optee_allocate_pages_array(size_t num_entries)
+{
+	return alloc_pages_exact(get_pages_array_size(num_entries), GFP_KERNEL);
+}
+
+void optee_free_pages_array(void *array, size_t num_entries)
+{
+	free_pages_exact(array, get_pages_array_size(num_entries));
+}
+
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index c374cd5..caa3c04 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -165,6 +165,10 @@  int optee_from_msg_param(struct tee_param *params, size_t num_params,
 int optee_to_msg_param(struct optee_msg_param *msg_params, size_t num_params,
 		       const struct tee_param *params);
 
+u64 *optee_allocate_pages_array(size_t num_entries);
+void optee_free_pages_array(void *array, size_t num_entries);
+void optee_fill_pages_list(u64 *dst, struct page **pages, size_t num_pages);
+
 /*
  * Small helpers
  */