[Xen-devel] [PATCH 0/5] arch/arm: optee: fix TODOs and remove "experimental" status

[Xen-devel] [PATCH 0/5] arch/arm: optee: fix TODOs and remove "experimental" status

[Volodymyr Babchuk]

Hello,

This patch series fixes various unfinished items in the OP-TEE mediator.
Mostly this is about limiting resources that guest can consume. This
includes both memory and time - how many buffers guest can share with
OP-TEE (this uses Xen memory) and when mediator should preempt itself,
to make sure that guest does not stress scheduling.

Apart from this, there were one case, when mediator's actions might lead
to memory leak in a good-behaving guest. To fix this issue I had to
extend mediator logic, so now it can issue RPC requests to guest in the
same way, as OP-TEE does this. This is useful feature, because it
allows to preempt mediator during long operations. So, in the future
it will be possible to remove shared buffer size limitation, because
mediator can preempt self during buffer translation.

This patch series can be pulled from [1].

[1] https://github.com/lorc/xen/tree/optee3_v1

Volodymyr Babchuk (5):
  xen/arm: optee: impose limit on shared buffer size
  xen/arm: optee: check for preemption while freeing shared buffers
  xen/arm: optee: limit number of shared buffers
  xen/arm: optee: handle share buffer translation error
  xen/arm: optee: remove experimental status

 xen/arch/arm/Kconfig     |   2 +-
 xen/arch/arm/tee/Kconfig |   2 +-
 xen/arch/arm/tee/optee.c | 237 ++++++++++++++++++++++++++++++---------
 3 files changed, 184 insertions(+), 57 deletions(-)

-- 
2.22.0

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Volodymyr Babchuk]

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(-)

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 )
-- 
2.22.0

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Volodymyr Babchuk]

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,


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Volodymyr Babchuk]

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?


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Volodymyr Babchuk]

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.


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Volodymyr Babchuk]

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/5] xen/arm: optee: impose limit on shared buffer size

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/5] xen/arm: optee: check for preemption while freeing shared buffers

[Volodymyr Babchuk]

Now we have limit for one shared buffer size, so we can be sure that
one call to free_optee_shm_buf() will not free all
MAX_TOTAL_SMH_BUF_PG pages at once. Thus, we now can check for
hypercall_preempt_check() in the loop inside
optee_relinquish_resources() and this will ensure that we are not
missing preemption.

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

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index f4fa8a7758..a84ffa3089 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -634,14 +634,14 @@ static int optee_relinquish_resources(struct domain *d)
     if ( hypercall_preempt_check() )
         return -ERESTART;
 
-    /*
-     * TODO: Guest can pin up to MAX_TOTAL_SMH_BUF_PG pages and all of
-     * them will be put in this loop. It is worth considering to
-     * check for preemption inside the loop.
-     */
     list_for_each_entry_safe( optee_shm_buf, optee_shm_buf_tmp,
                               &ctx->optee_shm_buf_list, list )
+    {
+        if ( hypercall_preempt_check() )
+            return -ERESTART;
+
         free_optee_shm_buf(ctx, optee_shm_buf->cookie);
+    }
 
     if ( hypercall_preempt_check() )
         return -ERESTART;
-- 
2.22.0

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/5] xen/arm: optee: check for preemption while freeing shared buffers

[Julien Grall]

Hi Volodymyr,

On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
> Now we have limit for one shared buffer size, so we can be sure that
> one call to free_optee_shm_buf() will not free all
> MAX_TOTAL_SMH_BUF_PG pages at once. Thus, we now can check for
> hypercall_preempt_check() in the loop inside
> optee_relinquish_resources() and this will ensure that we are not
> missing preemption.

I am not sure to understand the correlation between the two sentences. 
Even if previously the guest could pin up to MAX_TOTAL_SHM_BUF_PG in one 
call, a well-behaved guest would result to do multiple calls and 
therefore preemption would have been useful.

So I think the commit message needs some rewording.

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/arch/arm/tee/optee.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index f4fa8a7758..a84ffa3089 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -634,14 +634,14 @@ static int optee_relinquish_resources(struct domain *d)
>       if ( hypercall_preempt_check() )
>           return -ERESTART;
>   
> -    /*
> -     * TODO: Guest can pin up to MAX_TOTAL_SMH_BUF_PG pages and all of
> -     * them will be put in this loop. It is worth considering to
> -     * check for preemption inside the loop.
> -     */
>       list_for_each_entry_safe( optee_shm_buf, optee_shm_buf_tmp,
>                                 &ctx->optee_shm_buf_list, list )
> +    {
> +        if ( hypercall_preempt_check() )

So on the first iteration, you will check twice preemption (one before 
the loop and just entering). hypercall_preempt_check(). The function is 
not entirely free on Arm because of the implementation of 
vgic_vcpu_pending_irq(). So preventing pointless call would be nice.

In this case, the hypercall_preempt_check() before the loop could be 
dropped.

> +            return -ERESTART;
> +
>           free_optee_shm_buf(ctx, optee_shm_buf->cookie);
> +    }
>   
>       if ( hypercall_preempt_check() )
>           return -ERESTART;
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/5] xen/arm: optee: check for preemption while freeing shared buffers

[Volodymyr Babchuk]

Julien Grall writes:

> Hi Volodymyr,
>
> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>> Now we have limit for one shared buffer size, so we can be sure that
>> one call to free_optee_shm_buf() will not free all
>> MAX_TOTAL_SMH_BUF_PG pages at once. Thus, we now can check for
>> hypercall_preempt_check() in the loop inside
>> optee_relinquish_resources() and this will ensure that we are not
>> missing preemption.
>
> I am not sure to understand the correlation between the two
> sentences. Even if previously the guest could pin up to
> MAX_TOTAL_SHM_BUF_PG in one call, a well-behaved guest would result to
> do multiple calls and therefore preemption would have been useful.
Looks like now I don't understand you.

I'm talking about shared buffers. We have limited shared buffer to some
reasonable size. There is bad- or well-behaving guests in this context,
because guest can't share one big buffer in multiple calls. In other
worlds, if guest *needs* to share 512MB buffer with OP-TEE, it will be
forced to do this in one call. But we are forbidding big buffers right
now.

optee_relinquish_resources() is called during domain destruction. At
this time we can have a number of still living shared buffers, each of
one is no bigger than 512 pages. Thanks to this, we can call
hypercall_preempt_check() only in optee_relinquish_resources(), but not
in free_optee_shm_buf().

If we will allow guest to register bigger buffer, than we will be forced
to check for preemption in free_optee_shm_buf() as well.

This is what I meant in the commit message.

> So I think the commit message needs some rewording.
Probably yes...

>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>   xen/arch/arm/tee/optee.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>> index f4fa8a7758..a84ffa3089 100644
>> --- a/xen/arch/arm/tee/optee.c
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -634,14 +634,14 @@ static int optee_relinquish_resources(struct domain *d)
>>       if ( hypercall_preempt_check() )
>>           return -ERESTART;
>>   -    /*
>> -     * TODO: Guest can pin up to MAX_TOTAL_SMH_BUF_PG pages and all of
>> -     * them will be put in this loop. It is worth considering to
>> -     * check for preemption inside the loop.
>> -     */
>>       list_for_each_entry_safe( optee_shm_buf, optee_shm_buf_tmp,
>>                                 &ctx->optee_shm_buf_list, list )
>> +    {
>> +        if ( hypercall_preempt_check() )
>
> So on the first iteration, you will check twice preemption (one before
> the loop and just entering). hypercall_preempt_check(). The function
> is not entirely free on Arm because of the implementation of
> vgic_vcpu_pending_irq(). So preventing pointless call would be nice.
>
> In this case, the hypercall_preempt_check() before the loop could be
> dropped.
Yes, you are right.

>
>> +            return -ERESTART;
>> +
>>           free_optee_shm_buf(ctx, optee_shm_buf->cookie);
>> +    }
>>         if ( hypercall_preempt_check() )
>>           return -ERESTART;
>>
>
> Cheers,


--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/5] xen/arm: optee: check for preemption while freeing shared buffers

[Julien Grall]

Hi Volodymyr,

On 9/11/19 7:53 PM, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> Hi Volodymyr,
>>
>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>> Now we have limit for one shared buffer size, so we can be sure that
>>> one call to free_optee_shm_buf() will not free all
>>> MAX_TOTAL_SMH_BUF_PG pages at once. Thus, we now can check for
>>> hypercall_preempt_check() in the loop inside
>>> optee_relinquish_resources() and this will ensure that we are not
>>> missing preemption.
>>
>> I am not sure to understand the correlation between the two
>> sentences. Even if previously the guest could pin up to
>> MAX_TOTAL_SHM_BUF_PG in one call, a well-behaved guest would result to
>> do multiple calls and therefore preemption would have been useful.
> Looks like now I don't understand you.
> 
> I'm talking about shared buffers. We have limited shared buffer to some
> reasonable size. There is bad- or well-behaving guests in this context,
> because guest can't share one big buffer in multiple calls. In other
> worlds, if guest *needs* to share 512MB buffer with OP-TEE, it will be
> forced to do this in one call. But we are forbidding big buffers right
> now.
> 
> optee_relinquish_resources() is called during domain destruction. At
> this time we can have a number of still living shared buffers, each of
> one is no bigger than 512 pages. Thanks to this, we can call
> hypercall_preempt_check() only in optee_relinquish_resources(), but not
> in free_optee_shm_buf().

I understand what you mean, however my point is that this patch does not 
dependent of the previous patch. Even if this patch goes alone, you will 
improve well-behaved guest. For ill-behaved guest, the problem will stay 
the same so no change.

> 
> If we will allow guest to register bigger buffer, than we will be forced
> to check for preemption in free_optee_shm_buf() as well.

Well yes, however this patch would still be useful independently of the 
size of the buffer.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/5] xen/arm: optee: check for preemption while freeing shared buffers

[Volodymyr Babchuk]

Julien Grall writes:

> Hi Volodymyr,
>
> On 9/11/19 7:53 PM, Volodymyr Babchuk wrote:
>>
>> Julien Grall writes:
>>
>>> Hi Volodymyr,
>>>
>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>>> Now we have limit for one shared buffer size, so we can be sure that
>>>> one call to free_optee_shm_buf() will not free all
>>>> MAX_TOTAL_SMH_BUF_PG pages at once. Thus, we now can check for
>>>> hypercall_preempt_check() in the loop inside
>>>> optee_relinquish_resources() and this will ensure that we are not
>>>> missing preemption.
>>>
>>> I am not sure to understand the correlation between the two
>>> sentences. Even if previously the guest could pin up to
>>> MAX_TOTAL_SHM_BUF_PG in one call, a well-behaved guest would result to
>>> do multiple calls and therefore preemption would have been useful.
>> Looks like now I don't understand you.
>>
>> I'm talking about shared buffers. We have limited shared buffer to some
>> reasonable size. There is bad- or well-behaving guests in this context,
>> because guest can't share one big buffer in multiple calls. In other
>> worlds, if guest *needs* to share 512MB buffer with OP-TEE, it will be
>> forced to do this in one call. But we are forbidding big buffers right
>> now.
>>
>> optee_relinquish_resources() is called during domain destruction. At
>> this time we can have a number of still living shared buffers, each of
>> one is no bigger than 512 pages. Thanks to this, we can call
>> hypercall_preempt_check() only in optee_relinquish_resources(), but not
>> in free_optee_shm_buf().
>
> I understand what you mean, however my point is that this patch does
> not dependent of the previous patch. Even if this patch goes alone,
> you will improve well-behaved guest. For ill-behaved guest, the
> problem will stay the same so no change.
>
Ah, I see now. Okay, I'll rework the commit description.

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/5] xen/arm: optee: limit number of shared buffers

[Volodymyr Babchuk]

We want to limit number of shared buffers that guest can register in
OP-TEE. Every such buffer consumes XEN resources and we don't want
guest to exhaust XEN. So we choose arbitrary limit for shared buffers.

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

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index a84ffa3089..3ce6e7fa55 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -83,6 +83,14 @@
  */
 #define MAX_SHM_BUFFER_PG       512
 
+/*
+ * Limits the number of shared buffers that guest can have at once.
+ * This is to prevent case, when guests tricks XEN into exhausting
+ * own memory by allocating zillions of one-byte buffers. Value is
+ * chosen arbitrary.
+ */
+#define MAX_SHM_BUFFER_COUNT   16
+
 #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 | \
@@ -144,6 +152,7 @@ struct optee_domain {
     struct list_head optee_shm_buf_list;
     atomic_t call_count;
     atomic_t optee_shm_buf_pages;
+    atomic_t optee_shm_buf_count;
     spinlock_t lock;
 };
 
@@ -231,6 +240,7 @@ static int optee_domain_init(struct domain *d)
     INIT_LIST_HEAD(&ctx->optee_shm_buf_list);
     atomic_set(&ctx->call_count, 0);
     atomic_set(&ctx->optee_shm_buf_pages, 0);
+    atomic_set(&ctx->optee_shm_buf_count, 0);
     spin_lock_init(&ctx->lock);
 
     d->arch.tee = ctx;
@@ -479,23 +489,26 @@ static struct optee_shm_buf *allocate_optee_shm_buf(struct optee_domain *ctx,
     struct optee_shm_buf *optee_shm_buf, *optee_shm_buf_tmp;
     int old, new;
     int err_code;
+    int count;
+
+    count = atomic_add_unless(&ctx->optee_shm_buf_count, 1,
+                              MAX_SHM_BUFFER_COUNT);
+    if ( count == MAX_SHM_BUFFER_COUNT )
+        return ERR_PTR(-ENOMEM);
 
     do
     {
         old = atomic_read(&ctx->optee_shm_buf_pages);
         new = old + pages_cnt;
         if ( new >= MAX_TOTAL_SMH_BUF_PG )
-            return ERR_PTR(-ENOMEM);
+        {
+            err_code = -ENOMEM;
+            goto err_dec_cnt;
+        }
     }
     while ( unlikely(old != atomic_cmpxchg(&ctx->optee_shm_buf_pages,
                                            old, new)) );
 
-    /*
-     * TODO: Guest can try to register many small buffers, thus, forcing
-     * XEN to allocate context for every buffer. Probably we need to
-     * limit not only total number of pages pinned but also number
-     * of buffer objects.
-     */
     optee_shm_buf = xzalloc_bytes(sizeof(struct optee_shm_buf) +
                                   pages_cnt * sizeof(struct page *));
     if ( !optee_shm_buf )
@@ -531,6 +544,8 @@ static struct optee_shm_buf *allocate_optee_shm_buf(struct optee_domain *ctx,
 err:
     xfree(optee_shm_buf);
     atomic_sub(pages_cnt, &ctx->optee_shm_buf_pages);
+err_dec_cnt:
+    atomic_dec(&ctx->optee_shm_buf_count);
 
     return ERR_PTR(err_code);
 }
@@ -573,6 +588,7 @@ static void free_optee_shm_buf(struct optee_domain *ctx, uint64_t cookie)
     free_pg_list(optee_shm_buf);
 
     atomic_sub(optee_shm_buf->page_cnt, &ctx->optee_shm_buf_pages);
+    atomic_dec(&ctx->optee_shm_buf_count);
 
     xfree(optee_shm_buf);
 }
-- 
2.22.0

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 4/5] xen/arm: optee: handle share buffer translation error

[Volodymyr Babchuk]

There is a case possible, when OP-TEE asks guest to allocate shared
buffer, but Xen for some reason can't translate buffer's addresses. In
this situation we should do two things:

1. Tell guest to free allocated buffer, so there will be no memory
leak for guest.

2. Tell OP-TEE that buffer allocation failed.

To ask guest to free allocated buffer we should perform the same
thing, as OP-TEE does - issue RPC request. This is done by filling
request buffer (luckily we can reuse the same buffer, that OP-TEE used
to issue original request) and then return to guest with special
return code.

Then we need to handle next call from guest in a special way: as RPC
was issued by Xen, not by OP-TEE, it should be handled by Xen.
Basically, this is the mechanism to preempt OP-TEE mediator.

The same mechanism can be used in the future to preempt mediator
during translation large (>512 pages) shared buffers.

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

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 3ce6e7fa55..4eebc60b62 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -96,6 +96,11 @@
                               OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
                               OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
 
+enum optee_call_state {
+    OPTEEM_CALL_NORMAL = 0,
+    OPTEEM_CALL_XEN_RPC,
+};
+
 static unsigned int __read_mostly max_optee_threads;
 
 /*
@@ -112,6 +117,9 @@ struct optee_std_call {
     paddr_t guest_arg_ipa;
     int optee_thread_id;
     int rpc_op;
+    /* Saved buffer type for the last buffer allocate request */
+    unsigned int rpc_buffer_type;
+    enum optee_call_state state;
     uint64_t rpc_data_cookie;
     bool in_flight;
     register_t rpc_params[2];
@@ -299,6 +307,7 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx)
 
     call->optee_thread_id = -1;
     call->in_flight = true;
+    call->state = OPTEEM_CALL_NORMAL;
 
     spin_lock(&ctx->lock);
     list_add_tail(&call->list, &ctx->call_list);
@@ -1075,6 +1084,10 @@ static int handle_rpc_return(struct optee_domain *ctx,
             ret = -ERESTART;
         }
 
+        /* Save the buffer type in case we will want to free it */
+        if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
+            call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
+
         unmap_domain_page(shm_rpc->xen_arg);
     }
 
@@ -1239,18 +1252,102 @@ err:
     return;
 }
 
+/*
+ * Prepare RPC request to free shared buffer in the same way, as
+ * OP-TEE does this.
+ *
+ * Return values:
+ *  true  - successfully prepared RPC request
+ *  false - there was an error
+ */
+static bool issue_rpc_cmd_free(struct optee_domain *ctx,
+                               struct cpu_user_regs *regs,
+                               struct optee_std_call *call,
+                               struct shm_rpc *shm_rpc,
+                               uint64_t cookie)
+{
+    register_t r1, r2;
+
+    /* In case if guest will forget to update it with meaningful value */
+    shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
+    shm_rpc->xen_arg->cmd = OPTEE_RPC_CMD_SHM_FREE;
+    shm_rpc->xen_arg->num_params = 1;
+    shm_rpc->xen_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
+    shm_rpc->xen_arg->params[0].u.value.a = call->rpc_buffer_type;
+    shm_rpc->xen_arg->params[0].u.value.b = cookie;
+
+    if ( access_guest_memory_by_ipa(current->domain,
+                                    gfn_to_gaddr(shm_rpc->gfn),
+                                    shm_rpc->xen_arg,
+                                    OPTEE_MSG_GET_ARG_SIZE(1),
+                                    true) )
+    {
+        /*
+         * Well, this is quite bad. We have error in error path.
+         * This can happen only if guest behaves badly, so all
+         * we can do is to return error to OP-TEE and leave
+         * guest's memory leaked.
+         */
+        shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
+        shm_rpc->xen_arg->num_params = 0;
+
+        return false;
+    }
+
+    uint64_to_regpair(&r1, &r2, shm_rpc->cookie);
+
+    call->state = OPTEEM_CALL_XEN_RPC;
+    call->rpc_op = OPTEE_SMC_RPC_FUNC_CMD;
+    call->rpc_params[0] = r1;
+    call->rpc_params[1] = r2;
+    call->optee_thread_id = get_user_reg(regs, 3);
+
+    set_user_reg(regs, 0, OPTEE_SMC_RETURN_RPC_CMD);
+    set_user_reg(regs, 1, r1);
+    set_user_reg(regs, 2, r2);
+
+    return true;
+}
+
+/* Handles return from Xen-issued RPC */
+static void handle_xen_rpc_return(struct optee_domain *ctx,
+                                  struct cpu_user_regs *regs,
+                                  struct optee_std_call *call,
+                                  struct shm_rpc *shm_rpc)
+{
+    call->state = OPTEEM_CALL_NORMAL;
+
+    /*
+     * Right now we have only one reason to be there - we asked guest
+     * to free shared buffer and it did it. Now we can tell OP-TEE that
+     * buffer allocation failed.
+     */
+
+    /*
+     * We are not checking return value from a guest because we assume
+     * that OPTEE_RPC_CMD_SHM_FREE newer fails.
+     */
+
+    shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
+    shm_rpc->xen_arg->num_params = 0;
+}
+
 /*
  * This function is called when guest is finished processing RPC
  * request from OP-TEE and wished to resume the interrupted standard
  * call.
+ *
+ * Return values:
+ *  false - there was an error, do not call OP-TEE
+ *  true  - success, proceed as normal
  */
-static void handle_rpc_cmd_alloc(struct optee_domain *ctx,
+static bool handle_rpc_cmd_alloc(struct optee_domain *ctx,
                                  struct cpu_user_regs *regs,
                                  struct optee_std_call *call,
                                  struct shm_rpc *shm_rpc)
 {
     if ( shm_rpc->xen_arg->ret || shm_rpc->xen_arg->num_params != 1 )
-        return;
+        return true;
 
     if ( shm_rpc->xen_arg->params[0].attr != (OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
                                               OPTEE_MSG_ATTR_NONCONTIG) )
@@ -1258,7 +1355,7 @@ static void handle_rpc_cmd_alloc(struct optee_domain *ctx,
         gdprintk(XENLOG_WARNING,
                  "Invalid attrs for shared mem buffer: %"PRIx64"\n",
                  shm_rpc->xen_arg->params[0].attr);
-        return;
+        return true;
     }
 
     /* Free pg list for buffer */
@@ -1274,21 +1371,14 @@ static void handle_rpc_cmd_alloc(struct optee_domain *ctx,
     {
         call->rpc_data_cookie = 0;
         /*
-         * Okay, so there was problem with guest's buffer and we need
-         * to tell about this to OP-TEE.
-         */
-        shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
-        shm_rpc->xen_arg->num_params = 0;
-        /*
-         * TODO: With current implementation, OP-TEE will not issue
-         * RPC to free this buffer. Guest and OP-TEE will be out of
-         * sync: guest believes that it provided buffer to OP-TEE,
-         * while OP-TEE thinks of opposite. Ideally, we need to
-         * emulate RPC with OPTEE_MSG_RPC_CMD_SHM_FREE command.
+         * We are unable to translate guest's buffer, so we need tell guest
+         * to free it, before returning error to OP-TEE.
          */
-        gprintk(XENLOG_WARNING,
-                "translate_noncontig() failed, OP-TEE/guest state is out of sync.\n");
+        return !issue_rpc_cmd_free(ctx, regs, call, shm_rpc,
+                                   shm_rpc->xen_arg->params[0].u.tmem.shm_ref);
     }
+
+    return true;
 }
 
 static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs,
@@ -1338,22 +1428,37 @@ static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs,
         goto out;
     }
 
-    switch (shm_rpc->xen_arg->cmd)
+    if ( call->state == OPTEEM_CALL_NORMAL )
     {
-    case OPTEE_RPC_CMD_GET_TIME:
-    case OPTEE_RPC_CMD_WAIT_QUEUE:
-    case OPTEE_RPC_CMD_SUSPEND:
-        break;
-    case OPTEE_RPC_CMD_SHM_ALLOC:
-        handle_rpc_cmd_alloc(ctx, regs, call, shm_rpc);
-        break;
-    case OPTEE_RPC_CMD_SHM_FREE:
-        free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
-        if ( call->rpc_data_cookie == shm_rpc->xen_arg->params[0].u.value.b )
-            call->rpc_data_cookie = 0;
-        break;
-    default:
-        break;
+        switch (shm_rpc->xen_arg->cmd)
+        {
+        case OPTEE_RPC_CMD_GET_TIME:
+        case OPTEE_RPC_CMD_WAIT_QUEUE:
+        case OPTEE_RPC_CMD_SUSPEND:
+            break;
+        case OPTEE_RPC_CMD_SHM_ALLOC:
+            if ( !handle_rpc_cmd_alloc(ctx, regs, call, shm_rpc) )
+            {
+                /* We failed to translate buffer, report back to guest */
+                unmap_domain_page(shm_rpc->xen_arg);
+                put_std_call(ctx, call);
+
+                return;
+            }
+            break;
+        case OPTEE_RPC_CMD_SHM_FREE:
+            free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
+            if ( call->rpc_data_cookie ==
+                 shm_rpc->xen_arg->params[0].u.value.b )
+                call->rpc_data_cookie = 0;
+            break;
+        default:
+            break;
+        }
+    }
+    else
+    {
+        handle_xen_rpc_return(ctx, regs, call, shm_rpc);
     }
 
 out:
-- 
2.22.0

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/5] xen/arm: optee: handle share buffer translation error

[Julien Grall]

Hi Volodymyr,

On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
> There is a case possible, when OP-TEE asks guest to allocate shared
> buffer, but Xen for some reason can't translate buffer's addresses. In
> this situation we should do two things:
> 
> 1. Tell guest to free allocated buffer, so there will be no memory
> leak for guest.
> 
> 2. Tell OP-TEE that buffer allocation failed.
> 
> To ask guest to free allocated buffer we should perform the same
> thing, as OP-TEE does - issue RPC request. This is done by filling
> request buffer (luckily we can reuse the same buffer, that OP-TEE used
> to issue original request) and then return to guest with special
> return code.
> 
> Then we need to handle next call from guest in a special way: as RPC
> was issued by Xen, not by OP-TEE, it should be handled by Xen.
> Basically, this is the mechanism to preempt OP-TEE mediator.
> 
> The same mechanism can be used in the future to preempt mediator
> during translation large (>512 pages) shared buffers.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/arch/arm/tee/optee.c | 167 +++++++++++++++++++++++++++++++--------
>   1 file changed, 136 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 3ce6e7fa55..4eebc60b62 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -96,6 +96,11 @@
>                                 OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
>                                 OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
>   
> +enum optee_call_state {
> +    OPTEEM_CALL_NORMAL = 0,

enum always start counting at 0. Also, looking at the code, it does not 
seem you need to know the value. Right?

> +    OPTEEM_CALL_XEN_RPC,

I am a bit confused, the enum is called optee_call_state but all the 
enum are prefixed with OPTEEM_CALL_. Why the discrepancy?

> +};
> +
>   static unsigned int __read_mostly max_optee_threads;
>   
>   /*
> @@ -112,6 +117,9 @@ struct optee_std_call {
>       paddr_t guest_arg_ipa;
>       int optee_thread_id;
>       int rpc_op;
> +    /* Saved buffer type for the last buffer allocate request */

Looking at the code, it feels to me you are saving the buffer type for 
the current command and not the last. Did I miss anything?

> +    unsigned int rpc_buffer_type;
> +    enum optee_call_state state;
>       uint64_t rpc_data_cookie;
>       bool in_flight;
>       register_t rpc_params[2];
> @@ -299,6 +307,7 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx)
>   
>       call->optee_thread_id = -1;
>       call->in_flight = true;
> +    call->state = OPTEEM_CALL_NORMAL;
>   
>       spin_lock(&ctx->lock);
>       list_add_tail(&call->list, &ctx->call_list);
> @@ -1075,6 +1084,10 @@ static int handle_rpc_return(struct optee_domain *ctx,
>               ret = -ERESTART;
>           }
>   
> +        /* Save the buffer type in case we will want to free it */
> +        if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
> +            call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
> +
>           unmap_domain_page(shm_rpc->xen_arg);
>       }
>   
> @@ -1239,18 +1252,102 @@ err:
>       return;
>   }
>   
> +/*
> + * Prepare RPC request to free shared buffer in the same way, as
> + * OP-TEE does this.
> + *
> + * Return values:
> + *  true  - successfully prepared RPC request
> + *  false - there was an error
> + */
> +static bool issue_rpc_cmd_free(struct optee_domain *ctx,
> +                               struct cpu_user_regs *regs,
> +                               struct optee_std_call *call,
> +                               struct shm_rpc *shm_rpc,
> +                               uint64_t cookie)
> +{
> +    register_t r1, r2;
> +
> +    /* In case if guest will forget to update it with meaningful value */
> +    shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
> +    shm_rpc->xen_arg->cmd = OPTEE_RPC_CMD_SHM_FREE;
> +    shm_rpc->xen_arg->num_params = 1;
> +    shm_rpc->xen_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
> +    shm_rpc->xen_arg->params[0].u.value.a = call->rpc_buffer_type;
> +    shm_rpc->xen_arg->params[0].u.value.b = cookie;
> +
> +    if ( access_guest_memory_by_ipa(current->domain,
> +                                    gfn_to_gaddr(shm_rpc->gfn),
> +                                    shm_rpc->xen_arg,
> +                                    OPTEE_MSG_GET_ARG_SIZE(1),
> +                                    true) )
> +    {
> +        /*
> +         * Well, this is quite bad. We have error in error path.
> +         * This can happen only if guest behaves badly, so all
> +         * we can do is to return error to OP-TEE and leave
> +         * guest's memory leaked.

Could you expand a bit more what you mean by "guest's memory leaked"? 
What the state of the page from Xen PoV? I.e. is there any reference 
taken by the OP-TEE mediator? Will the page be freed once the guest is 
destroyed?...

> +         */
> +        shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
> +        shm_rpc->xen_arg->num_params = 0;
> +
> +        return false;
> +    }
> +
> +    uint64_to_regpair(&r1, &r2, shm_rpc->cookie);
> +
> +    call->state = OPTEEM_CALL_XEN_RPC;
> +    call->rpc_op = OPTEE_SMC_RPC_FUNC_CMD;
> +    call->rpc_params[0] = r1;
> +    call->rpc_params[1] = r2;
> +    call->optee_thread_id = get_user_reg(regs, 3);
> +
> +    set_user_reg(regs, 0, OPTEE_SMC_RETURN_RPC_CMD);
> +    set_user_reg(regs, 1, r1);
> +    set_user_reg(regs, 2, r2);
> +
> +    return true;
> +}
> +
> +/* Handles return from Xen-issued RPC */
> +static void handle_xen_rpc_return(struct optee_domain *ctx,
> +                                  struct cpu_user_regs *regs,
> +                                  struct optee_std_call *call,
> +                                  struct shm_rpc *shm_rpc)
> +{
> +    call->state = OPTEEM_CALL_NORMAL;
> +
> +    /*
> +     * Right now we have only one reason to be there - we asked guest
> +     * to free shared buffer and it did it. Now we can tell OP-TEE that
> +     * buffer allocation failed.
> +     */

Should we add an ASSERT to ensure the command is the one we expect?

> +
> +    /*
> +     * We are not checking return value from a guest because we assume
> +     * that OPTEE_RPC_CMD_SHM_FREE newer fails.

s/newer/never/

> +     */
> +
> +    shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
> +    shm_rpc->xen_arg->num_params = 0;
> +}
> +
>   /*
>    * This function is called when guest is finished processing RPC
>    * request from OP-TEE and wished to resume the interrupted standard
>    * call.
> + *
> + * Return values:
> + *  false - there was an error, do not call OP-TEE
> + *  true  - success, proceed as normal
>    */
> -static void handle_rpc_cmd_alloc(struct optee_domain *ctx,
> +static bool handle_rpc_cmd_alloc(struct optee_domain *ctx,
>                                    struct cpu_user_regs *regs,
>                                    struct optee_std_call *call,
>                                    struct shm_rpc *shm_rpc)
>   {
>       if ( shm_rpc->xen_arg->ret || shm_rpc->xen_arg->num_params != 1 )
> -        return;
> +        return true;
>   
>       if ( shm_rpc->xen_arg->params[0].attr != (OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
>                                                 OPTEE_MSG_ATTR_NONCONTIG) )
> @@ -1258,7 +1355,7 @@ static void handle_rpc_cmd_alloc(struct optee_domain *ctx,
>           gdprintk(XENLOG_WARNING,
>                    "Invalid attrs for shared mem buffer: %"PRIx64"\n",
>                    shm_rpc->xen_arg->params[0].attr);
> -        return;
> +        return true;
>       }
>   
>       /* Free pg list for buffer */
> @@ -1274,21 +1371,14 @@ static void handle_rpc_cmd_alloc(struct optee_domain *ctx,
>       {
>           call->rpc_data_cookie = 0;
>           /*
> -         * Okay, so there was problem with guest's buffer and we need
> -         * to tell about this to OP-TEE.
> -         */
> -        shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
> -        shm_rpc->xen_arg->num_params = 0;
> -        /*
> -         * TODO: With current implementation, OP-TEE will not issue
> -         * RPC to free this buffer. Guest and OP-TEE will be out of
> -         * sync: guest believes that it provided buffer to OP-TEE,
> -         * while OP-TEE thinks of opposite. Ideally, we need to
> -         * emulate RPC with OPTEE_MSG_RPC_CMD_SHM_FREE command.
> +         * We are unable to translate guest's buffer, so we need tell guest
> +         * to free it, before returning error to OP-TEE.

Do you mean "reporting" instead of "returning"?
Also s/error/an error/

>            */
> -        gprintk(XENLOG_WARNING,
> -                "translate_noncontig() failed, OP-TEE/guest state is out of sync.\n");
> +        return !issue_rpc_cmd_free(ctx, regs, call, shm_rpc,
> +                                   shm_rpc->xen_arg->params[0].u.tmem.shm_ref);
>       }
> +
> +    return true;
>   }
>   
>   static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs,
> @@ -1338,22 +1428,37 @@ static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs,
>           goto out;
>       }
>   
> -    switch (shm_rpc->xen_arg->cmd)
> +    if ( call->state == OPTEEM_CALL_NORMAL )
>       {
> -    case OPTEE_RPC_CMD_GET_TIME:
> -    case OPTEE_RPC_CMD_WAIT_QUEUE:
> -    case OPTEE_RPC_CMD_SUSPEND:
> -        break;
> -    case OPTEE_RPC_CMD_SHM_ALLOC:
> -        handle_rpc_cmd_alloc(ctx, regs, call, shm_rpc);
> -        break;
> -    case OPTEE_RPC_CMD_SHM_FREE:
> -        free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
> -        if ( call->rpc_data_cookie == shm_rpc->xen_arg->params[0].u.value.b )
> -            call->rpc_data_cookie = 0;
> -        break;
> -    default:
> -        break;
> +        switch (shm_rpc->xen_arg->cmd)
> +        {
> +        case OPTEE_RPC_CMD_GET_TIME:
> +        case OPTEE_RPC_CMD_WAIT_QUEUE:
> +        case OPTEE_RPC_CMD_SUSPEND:
> +            break;
> +        case OPTEE_RPC_CMD_SHM_ALLOC:
> +            if ( !handle_rpc_cmd_alloc(ctx, regs, call, shm_rpc) )
> +            {
> +                /* We failed to translate buffer, report back to guest */
> +                unmap_domain_page(shm_rpc->xen_arg);
> +                put_std_call(ctx, call);
> +
> +                return;
> +            }
> +            break;
> +        case OPTEE_RPC_CMD_SHM_FREE:
> +            free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
> +            if ( call->rpc_data_cookie ==
> +                 shm_rpc->xen_arg->params[0].u.value.b )
> +                call->rpc_data_cookie = 0;
> +            break;
> +        default:
> +            break;
> +        }
> +    }
> +    else
> +    {
> +        handle_xen_rpc_return(ctx, regs, call, shm_rpc);
>       }
>   
>   out:
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/5] xen/arm: optee: handle share buffer translation error

[Volodymyr Babchuk]

Julien Grall writes:

> Hi Volodymyr,
>
> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>> There is a case possible, when OP-TEE asks guest to allocate shared
>> buffer, but Xen for some reason can't translate buffer's addresses. In
>> this situation we should do two things:
>>
>> 1. Tell guest to free allocated buffer, so there will be no memory
>> leak for guest.
>>
>> 2. Tell OP-TEE that buffer allocation failed.
>>
>> To ask guest to free allocated buffer we should perform the same
>> thing, as OP-TEE does - issue RPC request. This is done by filling
>> request buffer (luckily we can reuse the same buffer, that OP-TEE used
>> to issue original request) and then return to guest with special
>> return code.
>>
>> Then we need to handle next call from guest in a special way: as RPC
>> was issued by Xen, not by OP-TEE, it should be handled by Xen.
>> Basically, this is the mechanism to preempt OP-TEE mediator.
>>
>> The same mechanism can be used in the future to preempt mediator
>> during translation large (>512 pages) shared buffers.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>   xen/arch/arm/tee/optee.c | 167 +++++++++++++++++++++++++++++++--------
>>   1 file changed, 136 insertions(+), 31 deletions(-)
>>
>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>> index 3ce6e7fa55..4eebc60b62 100644
>> --- a/xen/arch/arm/tee/optee.c
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -96,6 +96,11 @@
>>                                 OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
>>                                 OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
>>   +enum optee_call_state {
>> +    OPTEEM_CALL_NORMAL = 0,
>
> enum always start counting at 0. Also, looking at the code, it does
> not seem you need to know the value. Right?
Yep. This is a bad habit. Will remove.

>
>> +    OPTEEM_CALL_XEN_RPC,
>
> I am a bit confused, the enum is called optee_call_state but all the
> enum are prefixed with OPTEEM_CALL_. Why the discrepancy?
Because I'm bad at naming things :)

OPTEEM_CALL_STATE_XEN_RPC looks too long. But you are right, so I'll
rename the enum values. Unless, you have a better idea for this.

>
>> +};
>> +
>>   static unsigned int __read_mostly max_optee_threads;
>>     /*
>> @@ -112,6 +117,9 @@ struct optee_std_call {
>>       paddr_t guest_arg_ipa;
>>       int optee_thread_id;
>>       int rpc_op;
>> +    /* Saved buffer type for the last buffer allocate request */
>
> Looking at the code, it feels to me you are saving the buffer type for
> the current command and not the last. Did I miss anything?
Yes, right. Will rename.

>> +    unsigned int rpc_buffer_type;
>> +    enum optee_call_state state;
>>       uint64_t rpc_data_cookie;
>>       bool in_flight;
>>       register_t rpc_params[2];
>> @@ -299,6 +307,7 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx)
>>         call->optee_thread_id = -1;
>>       call->in_flight = true;
>> +    call->state = OPTEEM_CALL_NORMAL;
>>         spin_lock(&ctx->lock);
>>       list_add_tail(&call->list, &ctx->call_list);
>> @@ -1075,6 +1084,10 @@ static int handle_rpc_return(struct optee_domain *ctx,
>>               ret = -ERESTART;
>>           }
>>   +        /* Save the buffer type in case we will want to free it
>> */
>> +        if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
>> +            call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
>> +
>>           unmap_domain_page(shm_rpc->xen_arg);
>>       }
>>   @@ -1239,18 +1252,102 @@ err:
>>       return;
>>   }
>>   +/*
>> + * Prepare RPC request to free shared buffer in the same way, as
>> + * OP-TEE does this.
>> + *
>> + * Return values:
>> + *  true  - successfully prepared RPC request
>> + *  false - there was an error
>> + */
>> +static bool issue_rpc_cmd_free(struct optee_domain *ctx,
>> +                               struct cpu_user_regs *regs,
>> +                               struct optee_std_call *call,
>> +                               struct shm_rpc *shm_rpc,
>> +                               uint64_t cookie)
>> +{
>> +    register_t r1, r2;
>> +
>> +    /* In case if guest will forget to update it with meaningful value */
>> +    shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
>> +    shm_rpc->xen_arg->cmd = OPTEE_RPC_CMD_SHM_FREE;
>> +    shm_rpc->xen_arg->num_params = 1;
>> +    shm_rpc->xen_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
>> +    shm_rpc->xen_arg->params[0].u.value.a = call->rpc_buffer_type;
>> +    shm_rpc->xen_arg->params[0].u.value.b = cookie;
>> +
>> +    if ( access_guest_memory_by_ipa(current->domain,
>> +                                    gfn_to_gaddr(shm_rpc->gfn),
>> +                                    shm_rpc->xen_arg,
>> +                                    OPTEE_MSG_GET_ARG_SIZE(1),
>> +                                    true) )
>> +    {
>> +        /*
>> +         * Well, this is quite bad. We have error in error path.
>> +         * This can happen only if guest behaves badly, so all
>> +         * we can do is to return error to OP-TEE and leave
>> +         * guest's memory leaked.
>
> Could you expand a bit more what you mean by "guest's memory leaked"?
There will be memory leak somewhere in the guest. Yes, looks
like it is misleading...

What I mean, is that OP-TEE requests guest to allocate some
memory. Guest does not know, when OP-TEE finishes using this memory, so
guest will free the memory only by OP-TEE's request. We can't emulate
this request in current circumstances, so guest will keep part of own
memory reserved for OP-TEE infinitely.

> What the state of the page from Xen PoV?
From Xen point of view all will be perfectly fine.

> I.e. is there any reference
> taken by the OP-TEE mediator? Will the page be freed once the guest is
> destroyed?...
As I said, it has nothing to do with the page as Xen it sees. Mediator
will call put_page() prior to entering this function. So, no Xen
resources are used.

>
>> +         */
>> +        shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
>> +        shm_rpc->xen_arg->num_params = 0;
>> +
>> +        return false;
>> +    }
>> +
>> +    uint64_to_regpair(&r1, &r2, shm_rpc->cookie);
>> +
>> +    call->state = OPTEEM_CALL_XEN_RPC;
>> +    call->rpc_op = OPTEE_SMC_RPC_FUNC_CMD;
>> +    call->rpc_params[0] = r1;
>> +    call->rpc_params[1] = r2;
>> +    call->optee_thread_id = get_user_reg(regs, 3);
>> +
>> +    set_user_reg(regs, 0, OPTEE_SMC_RETURN_RPC_CMD);
>> +    set_user_reg(regs, 1, r1);
>> +    set_user_reg(regs, 2, r2);
>> +
>> +    return true;
>> +}
>> +
>> +/* Handles return from Xen-issued RPC */
>> +static void handle_xen_rpc_return(struct optee_domain *ctx,
>> +                                  struct cpu_user_regs *regs,
>> +                                  struct optee_std_call *call,
>> +                                  struct shm_rpc *shm_rpc)
>> +{
>> +    call->state = OPTEEM_CALL_NORMAL;
>> +
>> +    /*
>> +     * Right now we have only one reason to be there - we asked guest
>> +     * to free shared buffer and it did it. Now we can tell OP-TEE that
>> +     * buffer allocation failed.
>> +     */
>
> Should we add an ASSERT to ensure the command is the one we expect?
It is strange, that it is missing, actually. Looks like I forgot to add
it. But, looking at xen-error-handling, maybe BOG_ON() would be better?

>> +
>> +    /*
>> +     * We are not checking return value from a guest because we assume
>> +     * that OPTEE_RPC_CMD_SHM_FREE newer fails.
>
> s/newer/never/
Oops. Thank you.

>> +     */
>> +
>> +    shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
>> +    shm_rpc->xen_arg->num_params = 0;
>> +}
>> +
>>   /*
>>    * This function is called when guest is finished processing RPC
>>    * request from OP-TEE and wished to resume the interrupted standard
>>    * call.
>> + *
>> + * Return values:
>> + *  false - there was an error, do not call OP-TEE
>> + *  true  - success, proceed as normal
>>    */
>> -static void handle_rpc_cmd_alloc(struct optee_domain *ctx,
>> +static bool handle_rpc_cmd_alloc(struct optee_domain *ctx,
>>                                    struct cpu_user_regs *regs,
>>                                    struct optee_std_call *call,
>>                                    struct shm_rpc *shm_rpc)
>>   {
>>       if ( shm_rpc->xen_arg->ret || shm_rpc->xen_arg->num_params != 1 )
>> -        return;
>> +        return true;
>>         if ( shm_rpc->xen_arg->params[0].attr !=
>> (OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
>>                                                 OPTEE_MSG_ATTR_NONCONTIG) )
>> @@ -1258,7 +1355,7 @@ static void handle_rpc_cmd_alloc(struct optee_domain *ctx,
>>           gdprintk(XENLOG_WARNING,
>>                    "Invalid attrs for shared mem buffer: %"PRIx64"\n",
>>                    shm_rpc->xen_arg->params[0].attr);
>> -        return;
>> +        return true;
>>       }
>>         /* Free pg list for buffer */
>> @@ -1274,21 +1371,14 @@ static void handle_rpc_cmd_alloc(struct optee_domain *ctx,
>>       {
>>           call->rpc_data_cookie = 0;
>>           /*
>> -         * Okay, so there was problem with guest's buffer and we need
>> -         * to tell about this to OP-TEE.
>> -         */
>> -        shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
>> -        shm_rpc->xen_arg->num_params = 0;
>> -        /*
>> -         * TODO: With current implementation, OP-TEE will not issue
>> -         * RPC to free this buffer. Guest and OP-TEE will be out of
>> -         * sync: guest believes that it provided buffer to OP-TEE,
>> -         * while OP-TEE thinks of opposite. Ideally, we need to
>> -         * emulate RPC with OPTEE_MSG_RPC_CMD_SHM_FREE command.
>> +         * We are unable to translate guest's buffer, so we need tell guest
>> +         * to free it, before returning error to OP-TEE.
>
> Do you mean "reporting" instead of "returning"?
Yes, I do.

> Also s/error/an error/
Sure. Thank you.

--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/5] xen/arm: optee: handle share buffer translation error

[Julien Grall]

Hi Volodymyr,

On 9/11/19 7:32 PM, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> Hi Volodymyr,
>>
>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>> There is a case possible, when OP-TEE asks guest to allocate shared
>>> buffer, but Xen for some reason can't translate buffer's addresses. In
>>> this situation we should do two things:
>>>
>>> 1. Tell guest to free allocated buffer, so there will be no memory
>>> leak for guest.
>>>
>>> 2. Tell OP-TEE that buffer allocation failed.
>>>
>>> To ask guest to free allocated buffer we should perform the same
>>> thing, as OP-TEE does - issue RPC request. This is done by filling
>>> request buffer (luckily we can reuse the same buffer, that OP-TEE used
>>> to issue original request) and then return to guest with special
>>> return code.
>>>
>>> Then we need to handle next call from guest in a special way: as RPC
>>> was issued by Xen, not by OP-TEE, it should be handled by Xen.
>>> Basically, this is the mechanism to preempt OP-TEE mediator.
>>>
>>> The same mechanism can be used in the future to preempt mediator
>>> during translation large (>512 pages) shared buffers.
>>>
>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>> ---
>>>    xen/arch/arm/tee/optee.c | 167 +++++++++++++++++++++++++++++++--------
>>>    1 file changed, 136 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>> index 3ce6e7fa55..4eebc60b62 100644
>>> --- a/xen/arch/arm/tee/optee.c
>>> +++ b/xen/arch/arm/tee/optee.c
>>> @@ -96,6 +96,11 @@
>>>                                  OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
>>>                                  OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
>>>    +enum optee_call_state {
>>> +    OPTEEM_CALL_NORMAL = 0,
>>
>> enum always start counting at 0. Also, looking at the code, it does
>> not seem you need to know the value. Right?
> Yep. This is a bad habit. Will remove.
> 
>>
>>> +    OPTEEM_CALL_XEN_RPC,
>>
>> I am a bit confused, the enum is called optee_call_state but all the
>> enum are prefixed with OPTEEM_CALL_. Why the discrepancy?
> Because I'm bad at naming things :)
> 
> OPTEEM_CALL_STATE_XEN_RPC looks too long. But you are right, so I'll
> rename the enum values. Unless, you have a better idea for this.

My point was not about adding _STATE to the enum values but the fact you 
call the enum optee but the value OPTEEM (note the extra M in the later).

So my only request here is to call the enum opteem_call_state or prefix 
all the enum value with OPTEE.

> 
>>
>>> +};
>>> +
>>>    static unsigned int __read_mostly max_optee_threads;
>>>      /*
>>> @@ -112,6 +117,9 @@ struct optee_std_call {
>>>        paddr_t guest_arg_ipa;
>>>        int optee_thread_id;
>>>        int rpc_op;
>>> +    /* Saved buffer type for the last buffer allocate request */
>>
>> Looking at the code, it feels to me you are saving the buffer type for
>> the current command and not the last. Did I miss anything?
> Yes, right. Will rename.
> 
>>> +    unsigned int rpc_buffer_type;
>>> +    enum optee_call_state state;
>>>        uint64_t rpc_data_cookie;
>>>        bool in_flight;
>>>        register_t rpc_params[2];
>>> @@ -299,6 +307,7 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx)
>>>          call->optee_thread_id = -1;
>>>        call->in_flight = true;
>>> +    call->state = OPTEEM_CALL_NORMAL;
>>>          spin_lock(&ctx->lock);
>>>        list_add_tail(&call->list, &ctx->call_list);
>>> @@ -1075,6 +1084,10 @@ static int handle_rpc_return(struct optee_domain *ctx,
>>>                ret = -ERESTART;
>>>            }
>>>    +        /* Save the buffer type in case we will want to free it
>>> */
>>> +        if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
>>> +            call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
>>> +
>>>            unmap_domain_page(shm_rpc->xen_arg);
>>>        }
>>>    @@ -1239,18 +1252,102 @@ err:
>>>        return;
>>>    }
>>>    +/*
>>> + * Prepare RPC request to free shared buffer in the same way, as
>>> + * OP-TEE does this.
>>> + *
>>> + * Return values:
>>> + *  true  - successfully prepared RPC request
>>> + *  false - there was an error
>>> + */
>>> +static bool issue_rpc_cmd_free(struct optee_domain *ctx,
>>> +                               struct cpu_user_regs *regs,
>>> +                               struct optee_std_call *call,
>>> +                               struct shm_rpc *shm_rpc,
>>> +                               uint64_t cookie)
>>> +{
>>> +    register_t r1, r2;
>>> +
>>> +    /* In case if guest will forget to update it with meaningful value */
>>> +    shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
>>> +    shm_rpc->xen_arg->cmd = OPTEE_RPC_CMD_SHM_FREE;
>>> +    shm_rpc->xen_arg->num_params = 1;
>>> +    shm_rpc->xen_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
>>> +    shm_rpc->xen_arg->params[0].u.value.a = call->rpc_buffer_type;
>>> +    shm_rpc->xen_arg->params[0].u.value.b = cookie;
>>> +
>>> +    if ( access_guest_memory_by_ipa(current->domain,
>>> +                                    gfn_to_gaddr(shm_rpc->gfn),
>>> +                                    shm_rpc->xen_arg,
>>> +                                    OPTEE_MSG_GET_ARG_SIZE(1),
>>> +                                    true) )
>>> +    {
>>> +        /*
>>> +         * Well, this is quite bad. We have error in error path.
>>> +         * This can happen only if guest behaves badly, so all
>>> +         * we can do is to return error to OP-TEE and leave
>>> +         * guest's memory leaked.
>>
>> Could you expand a bit more what you mean by "guest's memory leaked"?
> There will be memory leak somewhere in the guest. Yes, looks
> like it is misleading...
> 
> What I mean, is that OP-TEE requests guest to allocate some
> memory. Guest does not know, when OP-TEE finishes using this memory, so
> guest will free the memory only by OP-TEE's request. We can't emulate
> this request in current circumstances, so guest will keep part of own
> memory reserved for OP-TEE infinitely.
> 
>> What the state of the page from Xen PoV?
>  From Xen point of view all will be perfectly fine.
> 
>> I.e. is there any reference
>> taken by the OP-TEE mediator? Will the page be freed once the guest is
>> destroyed?...
> As I said, it has nothing to do with the page as Xen it sees. Mediator
> will call put_page() prior to entering this function. So, no Xen
> resources are used.

It makes sense, Thank you for the explanation. Please update the comment 
accordingly.

> 
>>
>>> +         */
>>> +        shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
>>> +        shm_rpc->xen_arg->num_params = 0;
>>> +
>>> +        return false;
>>> +    }
>>> +
>>> +    uint64_to_regpair(&r1, &r2, shm_rpc->cookie);
>>> +
>>> +    call->state = OPTEEM_CALL_XEN_RPC;
>>> +    call->rpc_op = OPTEE_SMC_RPC_FUNC_CMD;
>>> +    call->rpc_params[0] = r1;
>>> +    call->rpc_params[1] = r2;
>>> +    call->optee_thread_id = get_user_reg(regs, 3);
>>> +
>>> +    set_user_reg(regs, 0, OPTEE_SMC_RETURN_RPC_CMD);
>>> +    set_user_reg(regs, 1, r1);
>>> +    set_user_reg(regs, 2, r2);
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +/* Handles return from Xen-issued RPC */
>>> +static void handle_xen_rpc_return(struct optee_domain *ctx,
>>> +                                  struct cpu_user_regs *regs,
>>> +                                  struct optee_std_call *call,
>>> +                                  struct shm_rpc *shm_rpc)
>>> +{
>>> +    call->state = OPTEEM_CALL_NORMAL;
>>> +
>>> +    /*
>>> +     * Right now we have only one reason to be there - we asked guest
>>> +     * to free shared buffer and it did it. Now we can tell OP-TEE that
>>> +     * buffer allocation failed.
>>> +     */
>>
>> Should we add an ASSERT to ensure the command is the one we expect?
> It is strange, that it is missing, actually. Looks like I forgot to add
> it. But, looking at xen-error-handling, maybe BOG_ON() would be better?

The documentation in xen-error-handling needs some update. IIRC George 
had a patch for updating the documentation on the mailing list.

BUG_ON() (and BUG()) should only be used if this is an error the 
hypervisor can't recover. I am actually slowly go through the tree and 
removing those who are in the guest path as some could be triggered on 
new revision of the architecture :(.

In this case, this is in guest path and an error case. If something has 
been missed and the guest may trigger the BUG_ON(). While this is a DOS, 
this is still not desirable.

So there are three solutions:
    1) Crash the guest
    2) Add an ASSERT()
    3) Print a warning

This is an error path so 2) might be less desirable if we don't do full 
coverage of the code in debug mode.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 5/5] xen/arm: optee: remove experimental status

[Volodymyr Babchuk]

As all TODOs and potential security issues are resolved now,
remove experimental status from OP-TEE mediator.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---

Note for maintainer: obviously this patch should be committed
only if all other patches in this series are committed as well
---
 xen/arch/arm/Kconfig     | 2 +-
 xen/arch/arm/tee/Kconfig | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index c2db2a6953..9b35783f68 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -107,7 +107,7 @@ config HARDEN_BRANCH_PREDICTOR
 	  If unsure, say Y.
 
 config TEE
-	bool "Enable TEE mediators support" if EXPERT = "y"
+	bool "Enable TEE mediators support"
 	default n
 	help
 	  This option enables generic TEE mediators support. It allows guests
diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
index b4b6aa2610..0b463ba368 100644
--- a/xen/arch/arm/tee/Kconfig
+++ b/xen/arch/arm/tee/Kconfig
@@ -3,7 +3,7 @@ config OPTEE
 	default n
 	depends on TEE
 	help
-	  Enable experimental OP-TEE mediator. It allows guests to access
+	  Enable OP-TEE mediator. It allows guests to access
 	  OP-TEE running on your platform. This requires virtualization-enabled
 	  OP-TEE present. You can learn more about virtualization for OP-TEE
 	  at https://optee.readthedocs.io/architecture/virtualization.html
-- 
2.22.0

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/5] xen/arm: optee: remove experimental status

[Julien Grall]

Hi,

On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
> As all TODOs and potential security issues are resolved now,
> remove experimental status from OP-TEE mediator.

Looking at SUPPORT.MD, I think OP-TEE without this series would be 
considered as "Experimental".

With this series applied, I still think we should keep the Kconfig 
behind EXPERT but mark it as "Technical Preview" for at least a release. 
This would encourage people to test and report any potential issues with 
OP-TEE.

We can re-discuss about the state in a few months for future release.

BTW, SUPPORT.MD should be updated to reflect the state of OP-TEE in Xen.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/5] xen/arm: optee: remove experimental status

[Volodymyr Babchuk]

Hi Julien,

Julien Grall writes:

> Hi,
>
> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>> As all TODOs and potential security issues are resolved now,
>> remove experimental status from OP-TEE mediator.
>
> Looking at SUPPORT.MD, I think OP-TEE without this series would be
> considered as "Experimental".
Right.

>
> With this series applied, I still think we should keep the Kconfig
> behind EXPERT but mark it as "Technical Preview" for at least a
> release. This would encourage people to test and report any potential
> issues with OP-TEE.
>
> We can re-discuss about the state in a few months for future release.
>
> BTW, SUPPORT.MD should be updated to reflect the state of OP-TEE in Xen.
Fair enough. In the next version I'll replace this patch with patch to
SUPPORT.md. Or it is better to push separate patch for the documentation?

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/5] xen/arm: optee: remove experimental status

[Julien Grall]

On 8/23/19 8:20 PM, Volodymyr Babchuk wrote:
> 
> Hi Julien,

Hi,

Apologies for the delay.

> 
> Julien Grall writes:
> 
>> Hi,
>>
>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>> As all TODOs and potential security issues are resolved now,
>>> remove experimental status from OP-TEE mediator.
>>
>> Looking at SUPPORT.MD, I think OP-TEE without this series would be
>> considered as "Experimental".
> Right.
> 
>>
>> With this series applied, I still think we should keep the Kconfig
>> behind EXPERT but mark it as "Technical Preview" for at least a
>> release. This would encourage people to test and report any potential
>> issues with OP-TEE.
>>
>> We can re-discuss about the state in a few months for future release.
>>
>> BTW, SUPPORT.MD should be updated to reflect the state of OP-TEE in Xen.
> Fair enough. In the next version I'll replace this patch with patch to
> SUPPORT.md. Or it is better to push separate patch for the documentation?

I think the patch in SUPPORT.MD should go regardless of the state of the 
rest. It is fine to keep it in this series.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/5] xen/arm: optee: remove experimental status

[Volodymyr Babchuk]

Julien Grall writes:

> On 8/23/19 8:20 PM, Volodymyr Babchuk wrote:
>>
>> Hi Julien,
>
> Hi,
>
> Apologies for the delay.
It is okay. I myself was busy a bit.

>
>>
>> Julien Grall writes:
>>
>>> Hi,
>>>
>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>>> As all TODOs and potential security issues are resolved now,
>>>> remove experimental status from OP-TEE mediator.
>>>
>>> Looking at SUPPORT.MD, I think OP-TEE without this series would be
>>> considered as "Experimental".
>> Right.
>>
>>>
>>> With this series applied, I still think we should keep the Kconfig
>>> behind EXPERT but mark it as "Technical Preview" for at least a
>>> release. This would encourage people to test and report any potential
>>> issues with OP-TEE.
>>>
>>> We can re-discuss about the state in a few months for future release.
>>>
>>> BTW, SUPPORT.MD should be updated to reflect the state of OP-TEE in Xen.
>> Fair enough. In the next version I'll replace this patch with patch to
>> SUPPORT.md. Or it is better to push separate patch for the documentation?
>
> I think the patch in SUPPORT.MD should go regardless of the state of
> the rest. It is fine to keep it in this series.
Okay. By the way, I skimmed thru SUPPORT.MD and I'm not sure what is the
best place to describe mediator. So I could use some advice there.


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/5] xen/arm: optee: remove experimental status

[Julien Grall]

Hi Volodymyr,

On 9/11/19 7:41 PM, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> On 8/23/19 8:20 PM, Volodymyr Babchuk wrote:
>>>
>>> Hi Julien,
>>
>> Hi,
>>
>> Apologies for the delay.
> It is okay. I myself was busy a bit.
> 
>>
>>>
>>> Julien Grall writes:
>>>
>>>> Hi,
>>>>
>>>> On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
>>>>> As all TODOs and potential security issues are resolved now,
>>>>> remove experimental status from OP-TEE mediator.
>>>>
>>>> Looking at SUPPORT.MD, I think OP-TEE without this series would be
>>>> considered as "Experimental".
>>> Right.
>>>
>>>>
>>>> With this series applied, I still think we should keep the Kconfig
>>>> behind EXPERT but mark it as "Technical Preview" for at least a
>>>> release. This would encourage people to test and report any potential
>>>> issues with OP-TEE.
>>>>
>>>> We can re-discuss about the state in a few months for future release.
>>>>
>>>> BTW, SUPPORT.MD should be updated to reflect the state of OP-TEE in Xen.
>>> Fair enough. In the next version I'll replace this patch with patch to
>>> SUPPORT.md. Or it is better to push separate patch for the documentation?
>>
>> I think the patch in SUPPORT.MD should go regardless of the state of
>> the rest. It is fine to keep it in this series.
> Okay. By the way, I skimmed thru SUPPORT.MD and I'm not sure what is the
> best place to describe mediator. So I could use some advice there.

Good question. I would put it under "## Virtual Hardware, Hypervisor". 
Maybe after the subsection "### ARM: Guest ACPI support"?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel