Re: [Xen-devel] [PATCH v3 08/11] optee: add support for arbitrary shared memory

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hello Julien,

Julien Grall writes:

> Hi Volodymyr,
>
> On 18/12/2018 21:11, Volodymyr Babchuk wrote:
>> From: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>
>>
>> Shared memory is widely used by NW to communicate with
>> TAs in OP-TEE. NW can share part of own memory with
>> TA or OP-TEE core, by registering it OP-TEE, or by providing
>> a temporal reference. Anyways, information about such memory
>> buffers are sent to OP-TEE as a list of pages. This mechanism
>> is described in optee_msg.h.
>>
>> Mediator should step in when NW tries to share memory with
>> OP-TEE for two reasons:
>>
>> 1. Do address translation from IPA to PA.
>> 2. Pin domain pages till they are mapped into OP-TEE or TA
>
> Looking at the code, I think the page are mapped while OP-TEE is using
> them. If so, it should be s/till/while/.
>
>>     address space, so domain can't transfer this pages to
>>     other domain or balloon out them. >
>> Address translation is done by translate_noncontig(...) function.
>> It allocates new buffer from xenheap and then walks on guest
>> provided list of pages, translates addresses and stores PAs into
>> newly allocated buffer. This buffer will be provided to OP-TEE
>> instead of original buffer from the guest. This buffer will
>> be free at the end of standard call.
>>
>> In the same time this function pins pages and stores them in
>> struct optee_shm_buf object. This object will live all the time,
>> when given SHM buffer is known to OP-TEE. It will be freed
>> after guest unregisters shared buffer. At this time pages
>> will be unpinned.
>>
>> We don't need to do any special reference counting because OP-TEE
>> tracks buffer on its side. So, mediator will unpin pages only
>> when OP-TEE returns successfully from OPTEE_MSG_CMD_UNREGISTER_SHM
>> call.
>>
>> Signed-off-by: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>
>> ---
>>
>>   Changes from v2:
>>   - Made sure that guest does not tries to register shared buffer with
>>     the same cookie twice
>>   - Fixed coding style
>>   - Use access_guest_memory_by_ipa() instead of direct memory mapping
>>
>>   xen/arch/arm/tee/optee.c | 274 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 274 insertions(+)
>>
>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>> index 771148e940..cfc3b34df7 100644
>> --- a/xen/arch/arm/tee/optee.c
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -37,6 +37,10 @@
>>    */
>>   #define MAX_RPC_SHMS    MAX_STD_CALLS
>>   +/* Maximum total number of pages that guest can share with OP-TEE
>> */
>> +#define MAX_TOTAL_SMH_BUF_PG    16384
>
> Please explain in the commit message and code how you came up to this value.

Basically it is taken from head. I just needed some number. You see,
number of registered shared memory buffer solely depends on free heap in
OP-TEE. But the same heap is used for other purposes, so it is hard to
tell how much pages can be shared with OP-TEE. I assumed that 64M ought
to be enough for anybody.

Probably it is worth to make this configurable via domctl interface.

>> +#define MAX_NONCONTIG_ENTRIES   5
>
> If I understand correctly the code below, MAX_NONCONTIG_ENTIRES is
> basically linked to the number of parameters. The maximum number of
> parameters is 5.
> I see in patch #6 you check have the following check
>
> OPTEE_MSG_GET_ARG_SIZE(num_params) > OPTEE_MSG_NONCONTIG_PAGE_SIZE
> It is not entirely obvious how this ensure you have no more than 5
> parameters neither how you came up to this value. Can you please at
> least provide more documentation in the code explaining the origin of
> the value?
Sadly, it is not mentioned in standard. But it is defined locally in
OP-TEE, in "tee_api_defines.h" file in the following way:

/* Not specified in the standard */
#define TEE_NUM_PARAMS  4

Fifth parameter is added to handle RPC buffer in the same way as other 
parameters.
Actually, I think, I need to rework this, because there only hard limit
to number of parameters is mentioned check

OPTEE_MSG_GET_ARG_SIZE(num_params) > OPTEE_MSG_NONCONTIG_PAGE_SIZE


> You might also want a
>
> BUILD_BUG_ON(OPTEE_MSG_GET_ARG_SIZE(MAX_NONCONTIG_ENTRIES) ==
> OPTEE_MSG_NONCONTIG_PAGE_SIZE).
I think that

 BUILD_BUG_ON(OPTEE_MSG_GET_ARG_SIZE(MAX_NONCONTIG_ENTRIES) <=
 OPTEE_MSG_NONCONTIG_PAGE_SIZE).

would be better in this case. But yes, I think I need to revisit this part.

[...]

>> +        if ( new >= MAX_TOTAL_SMH_BUF_PG )
>> +            return NULL;
> The limitation is in number of page and quite high. What would prevent
> a guest to register shared memory page by page? If nothing, then I
> think you can end up to something interesting issue in Xen because of
> the growing list and memory used.
Are you suggesting to introduce limit both on number of buffers and
the total number of pages?

[...]
>> +    struct page_info *guest_page;
>> +    struct {
>> +        uint64_t pages_list[PAGELIST_ENTRIES_PER_PAGE];
>> +        uint64_t next_page_data;
>> +    } *pages_data_guest, *pages_data_xen, *pages_data_xen_start;
>
> The structure is the internal of OPTEE_MSG_ATTR_NONCONTIG, am I
> correct? If so, the comment at the beginning of the function should be
> on top of the structure with further clarification (i.e this is the
> layout of the memory).
Yes, this is the layout of memory which is described in
optee_msg.h. I'll add reference to this file in comment.

[...]
>> +    page_offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 
>> 1);
>> +
>> +    /* Size of the user buffer in bytes */
>> +    size = ROUNDUP(param->u.tmem.size + page_offset,
>> +                   OPTEE_MSG_NONCONTIG_PAGE_SIZE);
>> +
>> +    num_pages = DIV_ROUND_UP(size, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
>> +
>> +    order = get_order_from_bytes(get_pages_list_size(num_pages));
>> +
>> +    pages_data_xen_start = alloc_xenheap_pages(order, 0);
>
> By using alloc_xenheap_pages, you may end-up allocation more memory
> than necessary when the order is getting bigger. What is the bigger
> order you expect here?
>
It depend on MAX_TOTAL_SMH_BUF_PG. One page can contain up to 511
entries, plus reference to the next one. So, basically at max I will
need about 32 pages, which gives order 5-6.
I think, I can try xzalloc or domheap there. But looks like there is no
xmem_pool for the domheap. So, I still will be forced to use
alloc_domheap_pages().

[...]

-- 
Best regards,Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel