lkml.org 
[lkml]   [2017]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v1 06/14] tee: optee: add page list manipulation functions
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.

\
 
 \ /
  Last update: 2017-09-29 15:08    [W:0.323 / U:4.532 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site