diff mbox series

[1/5] xen/arm: optee: impose limit on shared buffer size

Message ID 20190823184826.14525-2-volodymyr_babchuk@epam.com (mailing list archive)
State New, archived
Headers show
Series arch/arm: optee: fix TODOs and remove "experimental" status | expand

Commit Message

Volodymyr Babchuk Aug. 23, 2019, 6:48 p.m. UTC
We want to limit number of calls to lookup_and_pin_guest_ram_addr()
per one request. There are two ways to do this: either preempt
translate_noncontig() or to limit size of one shared buffer size.

It is quite hard to preempt translate_noncontig(), because it is deep
nested. So we chose second option. We will allow 512 pages per one
shared buffer. This does not interfere with GP standard, as it
requires that size limit for shared buffer should be at lest 512kB.
Also, with this limitation OP-TEE still passes own "xtest" test suite,
so this is okay for now.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/arch/arm/tee/optee.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Comments

Julien Grall Sept. 9, 2019, 10:11 p.m. UTC | #1
Hi Volodymyr,

On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
> We want to limit number of calls to lookup_and_pin_guest_ram_addr()
> per one request. There are two ways to do this: either preempt
> translate_noncontig() or to limit size of one shared buffer size.
> 
> It is quite hard to preempt translate_noncontig(), because it is deep
> nested. So we chose second option. We will allow 512 pages per one
> shared buffer. This does not interfere with GP standard, as it
> requires that size limit for shared buffer should be at lest 512kB.

Do you mean "least" instead of "lest"? If so, why 512 pages (i.e 1MB) is 
plenty enough for most of the use cases? What does "xtest" consist on?

> Also, with this limitation OP-TEE still passes own "xtest" test suite,
> so this is okay for now.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/arch/arm/tee/optee.c | 30 ++++++++++++++++++------------
>   1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index ec5402e89b..f4fa8a7758 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -72,6 +72,17 @@
>    */
>   #define MAX_TOTAL_SMH_BUF_PG    16384
>   
> +/*
> + * Arbitrary value that limits maximum shared buffer size. It is
> + * merely coincidence that it equals to both default OP-TEE SHM buffer
> + * size limit and to (1 << CONFIG_DOMU_MAX_ORDER). Please note that
> + * this define limits number of pages. But user buffer can be not
> + * aligned to a page boundary. So it is possible that user would not
> + * be able to share exactly MAX_SHM_BUFFER_PG * PAGE_SIZE bytes with
> + * OP-TEE.
> + */
> +#define MAX_SHM_BUFFER_PG       512
> +
>   #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR
>   #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \
>                                 OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
> @@ -697,15 +708,17 @@ static int translate_noncontig(struct optee_domain *ctx,
>       size = ROUNDUP(param->u.tmem.size + offset, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
>   
>       pg_count = DIV_ROUND_UP(size, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
> +    if ( pg_count > MAX_SHM_BUFFER_PG )
> +        return -ENOMEM;
> +
>       order = get_order_from_bytes(get_pages_list_size(pg_count));
>   
>       /*
> -     * In the worst case we will want to allocate 33 pages, which is
> -     * MAX_TOTAL_SMH_BUF_PG/511 rounded up. This gives order 6 or at
> -     * most 64 pages allocated. This buffer will be freed right after
> -     * the end of the call and there can be no more than
> +     * In the worst case we will want to allocate 2 pages, which is
> +     * MAX_SHM_BUFFER_PG/511 rounded up. This buffer will be freed
> +     * right after the end of the call and there can be no more than
>        * max_optee_threads calls simultaneously. So in the worst case
> -     * guest can trick us to allocate 64 * max_optee_threads pages in
> +     * guest can trick us to allocate 2 * max_optee_threads pages in
>        * total.
>        */
>       xen_pgs = alloc_domheap_pages(current->domain, order, 0);
> @@ -747,13 +760,6 @@ static int translate_noncontig(struct optee_domain *ctx,
>               xen_data = __map_domain_page(xen_pgs);
>           }
>   
> -        /*
> -         * TODO: That function can pin up to 64MB of guest memory by
> -         * calling lookup_and_pin_guest_ram_addr() 16384 times
> -         * (assuming that PAGE_SIZE equals to 4096).
> -         * This should be addressed before declaring OP-TEE security
> -         * supported.
> -         */
>           BUILD_BUG_ON(PAGE_SIZE != 4096);

Without the comment, the BUILD_BUG_ON() looks random. So either you want 
to have a different version of the comment or you want to move the 
BUILD_BUG_ON() to somewhere else.

>           page = get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx]));
>           if ( !page )
> 

Cheers,
Volodymyr Babchuk Sept. 11, 2019, 6:48 p.m. UTC | #2
Julien Grall writes:

> Hi Volodymyr,
>
> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>> We want to limit number of calls to lookup_and_pin_guest_ram_addr()
>> per one request. There are two ways to do this: either preempt
>> translate_noncontig() or to limit size of one shared buffer size.
>>
>> It is quite hard to preempt translate_noncontig(), because it is deep
>> nested. So we chose second option. We will allow 512 pages per one
>> shared buffer. This does not interfere with GP standard, as it
>> requires that size limit for shared buffer should be at lest 512kB.
>
> Do you mean "least" instead of "lest"?
Yes

> If so, why 512 pages (i.e 1MB)
> is plenty enough for most of the use cases? What does "xtest" consist
> on?
Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB
is enough for the most cases, because OP-TEE itself have a very limited
resources. But this value is chosen arbitrary.

>
>> Also, with this limitation OP-TEE still passes own "xtest" test suite,
>> so this is okay for now.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>   xen/arch/arm/tee/optee.c | 30 ++++++++++++++++++------------
>>   1 file changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>> index ec5402e89b..f4fa8a7758 100644
>> --- a/xen/arch/arm/tee/optee.c
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -72,6 +72,17 @@
>>    */
>>   #define MAX_TOTAL_SMH_BUF_PG    16384
>>   +/*
>> + * Arbitrary value that limits maximum shared buffer size. It is
>> + * merely coincidence that it equals to both default OP-TEE SHM buffer
>> + * size limit and to (1 << CONFIG_DOMU_MAX_ORDER). Please note that
>> + * this define limits number of pages. But user buffer can be not
>> + * aligned to a page boundary. So it is possible that user would not
>> + * be able to share exactly MAX_SHM_BUFFER_PG * PAGE_SIZE bytes with
>> + * OP-TEE.
>> + */
>> +#define MAX_SHM_BUFFER_PG       512
>> +
>>   #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR
>>   #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \
>>                                 OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
>> @@ -697,15 +708,17 @@ static int translate_noncontig(struct optee_domain *ctx,
>>       size = ROUNDUP(param->u.tmem.size + offset, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
>>         pg_count = DIV_ROUND_UP(size,
>> OPTEE_MSG_NONCONTIG_PAGE_SIZE);
>> +    if ( pg_count > MAX_SHM_BUFFER_PG )
>> +        return -ENOMEM;
>> +
>>       order = get_order_from_bytes(get_pages_list_size(pg_count));
>>         /*
>> -     * In the worst case we will want to allocate 33 pages, which is
>> -     * MAX_TOTAL_SMH_BUF_PG/511 rounded up. This gives order 6 or at
>> -     * most 64 pages allocated. This buffer will be freed right after
>> -     * the end of the call and there can be no more than
>> +     * In the worst case we will want to allocate 2 pages, which is
>> +     * MAX_SHM_BUFFER_PG/511 rounded up. This buffer will be freed
>> +     * right after the end of the call and there can be no more than
>>        * max_optee_threads calls simultaneously. So in the worst case
>> -     * guest can trick us to allocate 64 * max_optee_threads pages in
>> +     * guest can trick us to allocate 2 * max_optee_threads pages in
>>        * total.
>>        */
>>       xen_pgs = alloc_domheap_pages(current->domain, order, 0);
>> @@ -747,13 +760,6 @@ static int translate_noncontig(struct optee_domain *ctx,
>>               xen_data = __map_domain_page(xen_pgs);
>>           }
>>   -        /*
>> -         * TODO: That function can pin up to 64MB of guest memory by
>> -         * calling lookup_and_pin_guest_ram_addr() 16384 times
>> -         * (assuming that PAGE_SIZE equals to 4096).
>> -         * This should be addressed before declaring OP-TEE security
>> -         * supported.
>> -         */
>>           BUILD_BUG_ON(PAGE_SIZE != 4096);
>
> Without the comment, the BUILD_BUG_ON() looks random. So either you
> want to have a different version of the comment or you want to move
> the BUILD_BUG_ON() to somewhere else.

It is still before get_domain_ram_page() call. But for clarity I can add
comment like "Only 4k pages are supported right now".
>>           page = get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx]));
>>           if ( !page )
>>
>
> Cheers,
Julien Grall Sept. 12, 2019, 7:32 p.m. UTC | #3
Hi Volodymyr,

On 9/11/19 7:48 PM, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> Hi Volodymyr,
>>
>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr()
>>> per one request. There are two ways to do this: either preempt
>>> translate_noncontig() or to limit size of one shared buffer size.
>>>
>>> It is quite hard to preempt translate_noncontig(), because it is deep
>>> nested. So we chose second option. We will allow 512 pages per one
>>> shared buffer. This does not interfere with GP standard, as it
>>> requires that size limit for shared buffer should be at lest 512kB.
>>
>> Do you mean "least" instead of "lest"?
> Yes
> 
>> If so, why 512 pages (i.e 1MB)
>> is plenty enough for most of the use cases? What does "xtest" consist
>> on?
> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB
> is enough for the most cases, because OP-TEE itself have a very limited
> resources. But this value is chosen arbitrary.

Could we potentially reduce to let say 512KB (or maybe lower) if xtest 
only allocate 32KB?

> 
>>
>>> Also, with this limitation OP-TEE still passes own "xtest" test suite,
>>> so this is okay for now.
>>>
>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>> ---
>>>    xen/arch/arm/tee/optee.c | 30 ++++++++++++++++++------------
>>>    1 file changed, 18 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>> index ec5402e89b..f4fa8a7758 100644
>>> --- a/xen/arch/arm/tee/optee.c
>>> +++ b/xen/arch/arm/tee/optee.c
>>> @@ -72,6 +72,17 @@
>>>     */
>>>    #define MAX_TOTAL_SMH_BUF_PG    16384
>>>    +/*
>>> + * Arbitrary value that limits maximum shared buffer size. It is
>>> + * merely coincidence that it equals to both default OP-TEE SHM buffer
>>> + * size limit and to (1 << CONFIG_DOMU_MAX_ORDER). Please note that
>>> + * this define limits number of pages. But user buffer can be not
>>> + * aligned to a page boundary. So it is possible that user would not
>>> + * be able to share exactly MAX_SHM_BUFFER_PG * PAGE_SIZE bytes with
>>> + * OP-TEE.
>>> + */
>>> +#define MAX_SHM_BUFFER_PG       512
>>> +
>>>    #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR
>>>    #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \
>>>                                  OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
>>> @@ -697,15 +708,17 @@ static int translate_noncontig(struct optee_domain *ctx,
>>>        size = ROUNDUP(param->u.tmem.size + offset, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
>>>          pg_count = DIV_ROUND_UP(size,
>>> OPTEE_MSG_NONCONTIG_PAGE_SIZE);
>>> +    if ( pg_count > MAX_SHM_BUFFER_PG )
>>> +        return -ENOMEM;
>>> +
>>>        order = get_order_from_bytes(get_pages_list_size(pg_count));
>>>          /*
>>> -     * In the worst case we will want to allocate 33 pages, which is
>>> -     * MAX_TOTAL_SMH_BUF_PG/511 rounded up. This gives order 6 or at
>>> -     * most 64 pages allocated. This buffer will be freed right after
>>> -     * the end of the call and there can be no more than
>>> +     * In the worst case we will want to allocate 2 pages, which is
>>> +     * MAX_SHM_BUFFER_PG/511 rounded up. This buffer will be freed
>>> +     * right after the end of the call and there can be no more than
>>>         * max_optee_threads calls simultaneously. So in the worst case
>>> -     * guest can trick us to allocate 64 * max_optee_threads pages in
>>> +     * guest can trick us to allocate 2 * max_optee_threads pages in
>>>         * total.
>>>         */
>>>        xen_pgs = alloc_domheap_pages(current->domain, order, 0);
>>> @@ -747,13 +760,6 @@ static int translate_noncontig(struct optee_domain *ctx,
>>>                xen_data = __map_domain_page(xen_pgs);
>>>            }
>>>    -        /*
>>> -         * TODO: That function can pin up to 64MB of guest memory by
>>> -         * calling lookup_and_pin_guest_ram_addr() 16384 times
>>> -         * (assuming that PAGE_SIZE equals to 4096).
>>> -         * This should be addressed before declaring OP-TEE security
>>> -         * supported.
>>> -         */
>>>            BUILD_BUG_ON(PAGE_SIZE != 4096);
>>
>> Without the comment, the BUILD_BUG_ON() looks random. So either you
>> want to have a different version of the comment or you want to move
>> the BUILD_BUG_ON() to somewhere else.
> 
> It is still before get_domain_ram_page() call. But for clarity I can add
> comment like "Only 4k pages are supported right now".
>>>            page = get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx]));
>>>            if ( !page )

That would be useful.

Cheers,
Volodymyr Babchuk Sept. 12, 2019, 7:45 p.m. UTC | #4
Hi Julien,

Julien Grall writes:

> Hi Volodymyr,
>
> On 9/11/19 7:48 PM, Volodymyr Babchuk wrote:
>>
>> Julien Grall writes:
>>
>>> Hi Volodymyr,
>>>
>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr()
>>>> per one request. There are two ways to do this: either preempt
>>>> translate_noncontig() or to limit size of one shared buffer size.
>>>>
>>>> It is quite hard to preempt translate_noncontig(), because it is deep
>>>> nested. So we chose second option. We will allow 512 pages per one
>>>> shared buffer. This does not interfere with GP standard, as it
>>>> requires that size limit for shared buffer should be at lest 512kB.
>>>
>>> Do you mean "least" instead of "lest"?
>> Yes
>>
>>> If so, why 512 pages (i.e 1MB)
>>> is plenty enough for most of the use cases? What does "xtest" consist
>>> on?
>> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB
>> is enough for the most cases, because OP-TEE itself have a very limited
>> resources. But this value is chosen arbitrary.
>
> Could we potentially reduce to let say 512KB (or maybe lower) if xtest
> only allocate 32KB?
Potentially - yes. But only to 512KB if we want to be compatible with
the Global Platform specification. Why are you asking, though?
Julien Grall Sept. 12, 2019, 7:51 p.m. UTC | #5
Hi,

On 9/12/19 8:45 PM, Volodymyr Babchuk wrote:
> 
> Hi Julien,
> 
> Julien Grall writes:
> 
>> Hi Volodymyr,
>>
>> On 9/11/19 7:48 PM, Volodymyr Babchuk wrote:
>>>
>>> Julien Grall writes:
>>>
>>>> Hi Volodymyr,
>>>>
>>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr()
>>>>> per one request. There are two ways to do this: either preempt
>>>>> translate_noncontig() or to limit size of one shared buffer size.
>>>>>
>>>>> It is quite hard to preempt translate_noncontig(), because it is deep
>>>>> nested. So we chose second option. We will allow 512 pages per one
>>>>> shared buffer. This does not interfere with GP standard, as it
>>>>> requires that size limit for shared buffer should be at lest 512kB.
>>>>
>>>> Do you mean "least" instead of "lest"?
>>> Yes
>>>
>>>> If so, why 512 pages (i.e 1MB)
>>>> is plenty enough for most of the use cases? What does "xtest" consist
>>>> on?
>>> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB
>>> is enough for the most cases, because OP-TEE itself have a very limited
>>> resources. But this value is chosen arbitrary.
>>
>> Could we potentially reduce to let say 512KB (or maybe lower) if xtest
>> only allocate 32KB?
> Potentially - yes. But only to 512KB if we want to be compatible with
> the Global Platform specification. Why are you asking, though?

Does the Global Platform specification limit to 512KB? Or is it a minimum?

Because, the smaller the buffer is, the less time it will take to 
process in the worst case. Also, if we can have a reason for the size 
(you seem to suggest the spec define a size...) then it is much better 
than a random value.

Cheers,
Volodymyr Babchuk Sept. 16, 2019, 3:26 p.m. UTC | #6
Hi Julien,

Julien Grall writes:

> Hi,
>
> On 9/12/19 8:45 PM, Volodymyr Babchuk wrote:
>>
>> Hi Julien,
>>
>> Julien Grall writes:
>>
>>> Hi Volodymyr,
>>>
>>> On 9/11/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>
>>>> Julien Grall writes:
>>>>
>>>>> Hi Volodymyr,
>>>>>
>>>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr()
>>>>>> per one request. There are two ways to do this: either preempt
>>>>>> translate_noncontig() or to limit size of one shared buffer size.
>>>>>>
>>>>>> It is quite hard to preempt translate_noncontig(), because it is deep
>>>>>> nested. So we chose second option. We will allow 512 pages per one
>>>>>> shared buffer. This does not interfere with GP standard, as it
>>>>>> requires that size limit for shared buffer should be at lest 512kB.
>>>>>
>>>>> Do you mean "least" instead of "lest"?
>>>> Yes
>>>>
>>>>> If so, why 512 pages (i.e 1MB)
I have missed that earlier. But 512 pages is 2MB, actually.

>>>>> is plenty enough for most of the use cases? What does "xtest" consist
>>>>> on?
>>>> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB
>>>> is enough for the most cases, because OP-TEE itself have a very limited
>>>> resources. But this value is chosen arbitrary.
>>>
>>> Could we potentially reduce to let say 512KB (or maybe lower) if xtest
>>> only allocate 32KB?
>> Potentially - yes. But only to 512KB if we want to be compatible with
>> the Global Platform specification. Why are you asking, though?
>
> Does the Global Platform specification limit to 512KB? Or is it a minimum?
GP Spec says, that platform should allow *at lest* 512KB. Upper limit is
not set.

> Because, the smaller the buffer is, the less time it will take to
> process in the worst case. Also, if we can have a reason for the size
> (you seem to suggest the spec define a size...) then it is much better
> than a random value.
I have no strong arguments here, but I want to allow the biggest size
possible. It seems, that 512 pages is the accepted limit in hypervisor
code (at least, in p2m.c), so I chose this value.
Julien Grall Sept. 17, 2019, 10:49 a.m. UTC | #7
Hi Volodymyr,

On 9/16/19 4:26 PM, Volodymyr Babchuk wrote:
> 
> Hi Julien,
> 
> Julien Grall writes:
> 
>> Hi,
>>
>> On 9/12/19 8:45 PM, Volodymyr Babchuk wrote:
>>>
>>> Hi Julien,
>>>
>>> Julien Grall writes:
>>>
>>>> Hi Volodymyr,
>>>>
>>>> On 9/11/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>>
>>>>> Julien Grall writes:
>>>>>
>>>>>> Hi Volodymyr,
>>>>>>
>>>>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr()
>>>>>>> per one request. There are two ways to do this: either preempt
>>>>>>> translate_noncontig() or to limit size of one shared buffer size.
>>>>>>>
>>>>>>> It is quite hard to preempt translate_noncontig(), because it is deep
>>>>>>> nested. So we chose second option. We will allow 512 pages per one
>>>>>>> shared buffer. This does not interfere with GP standard, as it
>>>>>>> requires that size limit for shared buffer should be at lest 512kB.
>>>>>>
>>>>>> Do you mean "least" instead of "lest"?
>>>>> Yes
>>>>>
>>>>>> If so, why 512 pages (i.e 1MB)
> I have missed that earlier. But 512 pages is 2MB, actually.
> 
>>>>>> is plenty enough for most of the use cases? What does "xtest" consist
>>>>>> on?
>>>>> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB
>>>>> is enough for the most cases, because OP-TEE itself have a very limited
>>>>> resources. But this value is chosen arbitrary.
>>>>
>>>> Could we potentially reduce to let say 512KB (or maybe lower) if xtest
>>>> only allocate 32KB?
>>> Potentially - yes. But only to 512KB if we want to be compatible with
>>> the Global Platform specification. Why are you asking, though?
>>
>> Does the Global Platform specification limit to 512KB? Or is it a minimum?
> GP Spec says, that platform should allow *at lest* 512KB. Upper limit is
> not set.
> 
>> Because, the smaller the buffer is, the less time it will take to
>> process in the worst case. Also, if we can have a reason for the size
>> (you seem to suggest the spec define a size...) then it is much better
>> than a random value.
> I have no strong arguments here, but I want to allow the biggest size
> possible. It seems, that 512 pages is the accepted limit in hypervisor
> code (at least, in p2m.c), so I chose this value.

If GP specific request at least 512KB, then any portable code should be 
able to deal with 512KB, right? So why would you allow more? What are 
the cons/pros?

Cheers,
Volodymyr Babchuk Sept. 17, 2019, 12:28 p.m. UTC | #8
Hi Julien,


Julien Grall writes:

> Hi Volodymyr,
>
> On 9/16/19 4:26 PM, Volodymyr Babchuk wrote:
>>
>> Hi Julien,
>>
>> Julien Grall writes:
>>
>>> Hi,
>>>
>>> On 9/12/19 8:45 PM, Volodymyr Babchuk wrote:
>>>>
>>>> Hi Julien,
>>>>
>>>> Julien Grall writes:
>>>>
>>>>> Hi Volodymyr,
>>>>>
>>>>> On 9/11/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>>>
>>>>>> Julien Grall writes:
>>>>>>
>>>>>>> Hi Volodymyr,
>>>>>>>
>>>>>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>>>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr()
>>>>>>>> per one request. There are two ways to do this: either preempt
>>>>>>>> translate_noncontig() or to limit size of one shared buffer size.
>>>>>>>>
>>>>>>>> It is quite hard to preempt translate_noncontig(), because it is deep
>>>>>>>> nested. So we chose second option. We will allow 512 pages per one
>>>>>>>> shared buffer. This does not interfere with GP standard, as it
>>>>>>>> requires that size limit for shared buffer should be at lest 512kB.
>>>>>>>
>>>>>>> Do you mean "least" instead of "lest"?
>>>>>> Yes
>>>>>>
>>>>>>> If so, why 512 pages (i.e 1MB)
>> I have missed that earlier. But 512 pages is 2MB, actually.
>>
>>>>>>> is plenty enough for most of the use cases? What does "xtest" consist
>>>>>>> on?
>>>>>> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB
>>>>>> is enough for the most cases, because OP-TEE itself have a very limited
>>>>>> resources. But this value is chosen arbitrary.
>>>>>
>>>>> Could we potentially reduce to let say 512KB (or maybe lower) if xtest
>>>>> only allocate 32KB?
>>>> Potentially - yes. But only to 512KB if we want to be compatible with
>>>> the Global Platform specification. Why are you asking, though?
>>>
>>> Does the Global Platform specification limit to 512KB? Or is it a minimum?
>> GP Spec says, that platform should allow *at lest* 512KB. Upper limit is
>> not set.
>>
>>> Because, the smaller the buffer is, the less time it will take to
>>> process in the worst case. Also, if we can have a reason for the size
>>> (you seem to suggest the spec define a size...) then it is much better
>>> than a random value.
>> I have no strong arguments here, but I want to allow the biggest size
>> possible. It seems, that 512 pages is the accepted limit in hypervisor
>> code (at least, in p2m.c), so I chose this value.
>
> If GP specific request at least 512KB, then any portable code should
> be able to deal with 512KB, right? So why would you allow more? What
> are the cons/pros?
Yes, any portable code should work with 512KB. I want to allow bigger
buffers for two reasons: on OP-TEE issues tracking people often ask how
to increase various memory limits across OP-TEE. So, apparently people
sometimes wants bigger buffers. Second reasons is the non-portable
things like Secure Data Path. For SDP one wants to pass media (like
audio and video) data to and from OP-TEE. Media requires big buffers.

Anyways, I can see that 512 pages are established limit in the p2m
code. So, why do you want OP-TEE mediator to have smaller limit?

I want to be straight there: 512KB will likely work for most of the
users. But there are always users who want more. So I would like to set
largest plausible limit just in case.

--
Volodymyr Babchuk at EPAM
Julien Grall Sept. 17, 2019, 6:46 p.m. UTC | #9
Hi,

On 9/17/19 1:28 PM, Volodymyr Babchuk wrote:
> 
> Hi Julien,
> 
> 
> Julien Grall writes:
> 
>> Hi Volodymyr,
>>
>> On 9/16/19 4:26 PM, Volodymyr Babchuk wrote:
>>>
>>> Hi Julien,
>>>
>>> Julien Grall writes:
>>>
>>>> Hi,
>>>>
>>>> On 9/12/19 8:45 PM, Volodymyr Babchuk wrote:
>>>>>
>>>>> Hi Julien,
>>>>>
>>>>> Julien Grall writes:
>>>>>
>>>>>> Hi Volodymyr,
>>>>>>
>>>>>> On 9/11/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>>>>
>>>>>>> Julien Grall writes:
>>>>>>>
>>>>>>>> Hi Volodymyr,
>>>>>>>>
>>>>>>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>>>>>> We want to limit number of calls to lookup_and_pin_guest_ram_addr()
>>>>>>>>> per one request. There are two ways to do this: either preempt
>>>>>>>>> translate_noncontig() or to limit size of one shared buffer size.
>>>>>>>>>
>>>>>>>>> It is quite hard to preempt translate_noncontig(), because it is deep
>>>>>>>>> nested. So we chose second option. We will allow 512 pages per one
>>>>>>>>> shared buffer. This does not interfere with GP standard, as it
>>>>>>>>> requires that size limit for shared buffer should be at lest 512kB.
>>>>>>>>
>>>>>>>> Do you mean "least" instead of "lest"?
>>>>>>> Yes
>>>>>>>
>>>>>>>> If so, why 512 pages (i.e 1MB)
>>> I have missed that earlier. But 512 pages is 2MB, actually.
>>>
>>>>>>>> is plenty enough for most of the use cases? What does "xtest" consist
>>>>>>>> on?
>>>>>>> Bigger buffer xtest tries to allocate is mere 32KB. I believe that 1MB
>>>>>>> is enough for the most cases, because OP-TEE itself have a very limited
>>>>>>> resources. But this value is chosen arbitrary.
>>>>>>
>>>>>> Could we potentially reduce to let say 512KB (or maybe lower) if xtest
>>>>>> only allocate 32KB?
>>>>> Potentially - yes. But only to 512KB if we want to be compatible with
>>>>> the Global Platform specification. Why are you asking, though?
>>>>
>>>> Does the Global Platform specification limit to 512KB? Or is it a minimum?
>>> GP Spec says, that platform should allow *at lest* 512KB. Upper limit is
>>> not set.
>>>
>>>> Because, the smaller the buffer is, the less time it will take to
>>>> process in the worst case. Also, if we can have a reason for the size
>>>> (you seem to suggest the spec define a size...) then it is much better
>>>> than a random value.
>>> I have no strong arguments here, but I want to allow the biggest size
>>> possible. It seems, that 512 pages is the accepted limit in hypervisor
>>> code (at least, in p2m.c), so I chose this value.
>>
>> If GP specific request at least 512KB, then any portable code should
>> be able to deal with 512KB, right? So why would you allow more? What
>> are the cons/pros?
> Yes, any portable code should work with 512KB. I want to allow bigger
> buffers for two reasons: on OP-TEE issues tracking people often ask how
> to increase various memory limits across OP-TEE. So, apparently people
> sometimes wants bigger buffers. Second reasons is the non-portable
> things like Secure Data Path. For SDP one wants to pass media (like
> audio and video) data to and from OP-TEE. Media requires big buffers.

But what's the limit in that case? Would 2MB really be sufficient for them?

> 
> Anyways, I can see that 512 pages are established limit in the p2m
> code. So, why do you want OP-TEE mediator to have smaller limit?

One limit doesn't rule them all. ;)

The preemption is slightly more elaborate than checking the preemption 
every 512 pages. For instance in p2m_cache_flush_range(), we take into 
account how much is done.

In your case, you also have to factor how long the smc call is going to 
take in your calculation. This is not an exact science, but the fact 
that at the moment the limit for OP-TEE is much lower lead to me to 
think a smaller limit is better. Most likely OP-TEE will deny it anyway...

> 
> I want to be straight there: 512KB will likely work for most of the
> users. But there are always users who want more. So I would like to set
> largest plausible limit just in case.
I am pretty unconvinced that an higher value than what the spec request 
is right. But I don't have more ground than my gut feeling (I am always 
on the safe side). You are the maintainer of that code, so I am not 
going to push more for a lower value.

However, we should at least document any reasoning because this does not 
seem to be a that random value anymore.

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index ec5402e89b..f4fa8a7758 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -72,6 +72,17 @@ 
  */
 #define MAX_TOTAL_SMH_BUF_PG    16384
 
+/*
+ * Arbitrary value that limits maximum shared buffer size. It is
+ * merely coincidence that it equals to both default OP-TEE SHM buffer
+ * size limit and to (1 << CONFIG_DOMU_MAX_ORDER). Please note that
+ * this define limits number of pages. But user buffer can be not
+ * aligned to a page boundary. So it is possible that user would not
+ * be able to share exactly MAX_SHM_BUFFER_PG * PAGE_SIZE bytes with
+ * OP-TEE.
+ */
+#define MAX_SHM_BUFFER_PG       512
+
 #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR
 #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \
                               OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
@@ -697,15 +708,17 @@  static int translate_noncontig(struct optee_domain *ctx,
     size = ROUNDUP(param->u.tmem.size + offset, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
 
     pg_count = DIV_ROUND_UP(size, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
+    if ( pg_count > MAX_SHM_BUFFER_PG )
+        return -ENOMEM;
+
     order = get_order_from_bytes(get_pages_list_size(pg_count));
 
     /*
-     * In the worst case we will want to allocate 33 pages, which is
-     * MAX_TOTAL_SMH_BUF_PG/511 rounded up. This gives order 6 or at
-     * most 64 pages allocated. This buffer will be freed right after
-     * the end of the call and there can be no more than
+     * In the worst case we will want to allocate 2 pages, which is
+     * MAX_SHM_BUFFER_PG/511 rounded up. This buffer will be freed
+     * right after the end of the call and there can be no more than
      * max_optee_threads calls simultaneously. So in the worst case
-     * guest can trick us to allocate 64 * max_optee_threads pages in
+     * guest can trick us to allocate 2 * max_optee_threads pages in
      * total.
      */
     xen_pgs = alloc_domheap_pages(current->domain, order, 0);
@@ -747,13 +760,6 @@  static int translate_noncontig(struct optee_domain *ctx,
             xen_data = __map_domain_page(xen_pgs);
         }
 
-        /*
-         * TODO: That function can pin up to 64MB of guest memory by
-         * calling lookup_and_pin_guest_ram_addr() 16384 times
-         * (assuming that PAGE_SIZE equals to 4096).
-         * This should be addressed before declaring OP-TEE security
-         * supported.
-         */
         BUILD_BUG_ON(PAGE_SIZE != 4096);
         page = get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx]));
         if ( !page )