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

[Mark Rutland <mark.rutland@arm.com>]

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.