[Xen-devel] [PATCH v2 0/6] arch/arm: optee: fix TODOs and change status to "Tech Preview"

[Xen-devel] [PATCH v2 0/6] arch/arm: optee: fix TODOs and change status to "Tech Preview"

[Volodymyr Babchuk]

Hello,

This is the second version for maturing the OP-TEE mediator.

Changes also can be pulled from [2].

Changes from v1:
 - Added patch that updates SUPPORT.md
 - Instead of removing "experimental" status I changed it to "Tech Preview"
 - Other changes are described in the corresponding patches

Cover letter for v1:

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
[2] https://github.com/lorc/xen/tree/optee3_v2


Volodymyr Babchuk (6):
  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 shared buffer translation error
  SUPPORT.md: Describe OP-TEE mediator
  xen/arm: optee: update description in Kconfig

 SUPPORT.md               |   4 +
 xen/arch/arm/tee/Kconfig |  12 +-
 xen/arch/arm/tee/optee.c | 259 ++++++++++++++++++++++++++++++---------
 3 files changed, 213 insertions(+), 62 deletions(-)

-- 
2.22.0

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

[Xen-devel] [PATCH v2 1/6] 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 limit size of one shared buffer size.

It is quite hard to preempt translate_noncontig(), because it is deep
nested. So we chose the second option. We will allow 129 pages per one
shared buffer. This corresponds to the GP standard, as it requires
that size limit for shared buffer should be at least 512kB. One extra
page (129th) is needed to cope with the fact that user's buffer is not
necessary aligned with page boundary.

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>

---

Changes from v1:
 - Added comment before BUILD_BUG_ON(PAGE_SIZE != 4096);
 - Fixed typo in the commit message
 - Decreased MAX_SHM_BUFFER_PG to 129
---
 xen/arch/arm/tee/optee.c | 44 +++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index ec5402e89b..d64e9c3b85 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -72,6 +72,19 @@
  */
 #define MAX_TOTAL_SMH_BUF_PG    16384
 
+/*
+ * Limit for shared buffer size. 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.
+ *
+ * Global Platform specification for TEE requires that any TEE
+ * implementation should allow to share buffers with size of at least
+ * 512KB, which equals to 128 4KB pages. Due to align issue mentioned
+ * above, we need to increase this value to 129.
+ */
+#define MAX_SHM_BUFFER_PG       129
+
 #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,16 +710,29 @@ 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 1 page, 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 1 * max_optee_threads pages in
      * total.
+     *
+     * It may seem strange to have such complex calculations if we
+     * always will allocate exactly one page. Those calculations exist
+     * in the first place because earlier there were bigger limit for
+     * shared buffer size, so there were cases, when we needed more
+     * that one page there. Right now this is not true, but this code
+     * remains for two reasons:
+     * - Users can change MAX_SHM_BUFFER_PG to a higher value, in which
+     *   case they will need this code.
+     * - There is a plan to implement preemption in the code below, which
+     *   will allow use to increase default MAX_SHM_BUFFER_PG value.
      */
     xen_pgs = alloc_domheap_pages(current->domain, order, 0);
     if ( !xen_pgs )
@@ -747,13 +773,7 @@ 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.
-         */
+        /* Only 4k pages are supported right now */
         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 v2 1/6] xen/arm: optee: impose limit on shared buffer size

[Julien Grall]

Hi Volodymyr,

On 18/09/2019 19:50, 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 limit size of one shared buffer size.
> 
> It is quite hard to preempt translate_noncontig(), because it is deep
> nested. So we chose the second option. We will allow 129 pages per one
> shared buffer. This corresponds to the GP standard, as it requires
> that size limit for shared buffer should be at least 512kB. One extra
> page (129th) is needed to cope with the fact that user's buffer is not
> necessary aligned with page boundary.
> 
> 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>
> 
> ---
> 
> Changes from v1:
>   - Added comment before BUILD_BUG_ON(PAGE_SIZE != 4096);
>   - Fixed typo in the commit message
>   - Decreased MAX_SHM_BUFFER_PG to 129
> ---
>   xen/arch/arm/tee/optee.c | 44 +++++++++++++++++++++++++++++-----------
>   1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index ec5402e89b..d64e9c3b85 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -72,6 +72,19 @@
>    */
>   #define MAX_TOTAL_SMH_BUF_PG    16384
>   
> +/*
> + * Limit for shared buffer size. 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.
> + *
> + * Global Platform specification for TEE requires that any TEE
> + * implementation should allow to share buffers with size of at least
> + * 512KB, which equals to 128 4KB pages. Due to align issue mentioned
> + * above, we need to increase this value to 129.
> + */
> +#define MAX_SHM_BUFFER_PG       129
> +
>   #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,16 +710,29 @@ 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 1 page, 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 1 * max_optee_threads pages in
>        * total.
> +     *
> +     * It may seem strange to have such complex calculations if we
> +     * always will allocate exactly one page. Those calculations exist
> +     * in the first place because earlier there were bigger limit for
> +     * shared buffer size, so there were cases, when we needed more
> +     * that one page there. Right now this is not true, but this code
> +     * remains for two reasons:
> +     * - Users can change MAX_SHM_BUFFER_PG to a higher value, in which
> +     *   case they will need this code.
> +     * - There is a plan to implement preemption in the code below, which
> +     *   will allow use to increase default MAX_SHM_BUFFER_PG value.
>        */
>       xen_pgs = alloc_domheap_pages(current->domain, order, 0);
>       if ( !xen_pgs )
> @@ -747,13 +773,7 @@ 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.
> -         */
> +        /* Only 4k pages are supported right now */

NIT: s/4k/4KB/ to stay consistent.

>           BUILD_BUG_ON(PAGE_SIZE != 4096);
>           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

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

[Volodymyr Babchuk]

We can check for hypercall_preempt_check() in the loop inside
optee_relinquish_resources() to increase hypervisor responsiveness in
case if preemption is required.

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

---

Changes from v1:
 - Removed extra hypercall_preempt_check()
 - Updated the commit message
---
 xen/arch/arm/tee/optee.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index d64e9c3b85..55d11b91a9 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -633,17 +633,14 @@ static int optee_relinquish_resources(struct domain *d)
     list_for_each_entry_safe( shm_rpc, shm_rpc_tmp, &ctx->shm_rpc_list, list )
         free_shm_rpc(ctx, shm_rpc->cookie);
 
-    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 v2 2/6] xen/arm: optee: check for preemption while freeing shared buffers

[Julien Grall]

Hi Volodymyr,

On 18/09/2019 19:50, Volodymyr Babchuk wrote:
> We can check for hypercall_preempt_check() in the loop inside
> optee_relinquish_resources() to increase hypervisor responsiveness in
> case if preemption is required.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> 
> ---
> 
> Changes from v1:
>   - Removed extra hypercall_preempt_check()
>   - Updated the commit message
> ---
>   xen/arch/arm/tee/optee.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index d64e9c3b85..55d11b91a9 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -633,17 +633,14 @@ static int optee_relinquish_resources(struct domain *d)
>       list_for_each_entry_safe( shm_rpc, shm_rpc_tmp, &ctx->shm_rpc_list, list )
>           free_shm_rpc(ctx, shm_rpc->cookie);
>   
> -    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;
> 

-- 
Julien Grall

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

[Xen-devel] [PATCH v2 3/6] 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 55d11b91a9..88be959819 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -85,6 +85,14 @@
  */
 #define MAX_SHM_BUFFER_PG       129
 
+/*
+ * 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 | \
@@ -146,6 +154,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;
 };
 
@@ -233,6 +242,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;
@@ -481,23 +491,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 )
@@ -533,6 +546,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);
 }
@@ -575,6 +590,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

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

[Julien Grall]

Hi Volodymyr,

On 18/09/2019 19:50, Volodymyr Babchuk wrote:
> 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>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH v2 4/6] xen/arm: optee: handle shared 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>

---

Changes from v1:
 - Renamed OPTEEM_CALL_* to OPTEE_CALL_*
 - Fixed comments
 - Added ASSERT() in  handle_xen_rpc_return()
---
 xen/arch/arm/tee/optee.c | 172 ++++++++++++++++++++++++++++++++-------
 1 file changed, 141 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 88be959819..0a3205f9e8 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -98,6 +98,11 @@
                               OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \
                               OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
 
+enum optee_call_state {
+    OPTEE_CALL_NORMAL,
+    OPTEE_CALL_XEN_RPC,
+};
+
 static unsigned int __read_mostly max_optee_threads;
 
 /*
@@ -114,6 +119,9 @@ struct optee_std_call {
     paddr_t guest_arg_ipa;
     int optee_thread_id;
     int rpc_op;
+    /* Saved buffer type for the current 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];
@@ -301,6 +309,7 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx)
 
     call->optee_thread_id = -1;
     call->in_flight = true;
+    call->state = OPTEE_CALL_NORMAL;
 
     spin_lock(&ctx->lock);
     list_add_tail(&call->list, &ctx->call_list);
@@ -1086,6 +1095,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);
     }
 
@@ -1250,18 +1263,107 @@ 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 the 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. We already have freed all resources
+         * allocated for this buffer, but guest will never receive
+         * OPTEE_RPC_CMD_SHM_FREE request, so it will not know that it
+         * can release allocated buffer.
+         */
+        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 = OPTEE_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 = OPTEE_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 storing exact command
+     * type, only type of RPC return. So, this is the only check we
+     * can perform there.
+     */
+    ASSERT(call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD);
+
+    /*
+     * We are not checking return value from a guest because we assume
+     * that OPTEE_RPC_CMD_SHM_FREE never 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) )
@@ -1269,7 +1371,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 */
@@ -1285,21 +1387,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 reporting an 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,
@@ -1349,22 +1444,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 == OPTEE_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 v2 4/6] xen/arm: optee: handle shared buffer translation error

[Julien Grall]

Hi,

On 18/09/2019 19:51, Volodymyr Babchuk wrote:
> +/* 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 = OPTEE_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 storing exact command
> +     * type, only type of RPC return. So, this is the only check we
> +     * can perform there.
> +     */
> +    ASSERT(call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD);

As I pointed out in v1, ASSERT() is probably the less desirable solution here as 
this is an error path.

Can you explain why you chose that over the 3 solutions I suggested?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 4/6] xen/arm: optee: handle shared buffer translation error

[Volodymyr Babchuk]

Hi Julien,

Julien Grall writes:

> Hi,
>
> On 18/09/2019 19:51, Volodymyr Babchuk wrote:
>> +/* 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 = OPTEE_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 storing exact command
>> +     * type, only type of RPC return. So, this is the only check we
>> +     * can perform there.
>> +     */
>> +    ASSERT(call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD);
>
> As I pointed out in v1, ASSERT() is probably the less desirable
> solution here as this is an error path.
Looks like I misunderstood you :(

> Can you explain why you chose that over the 3 solutions I suggested?
I think I need some clarification there. ASSERT() is adopted way to tell
about invariant. Clearly, this is programming error if ASSERT() fails.

But I understand, that ASSERT() is available only in debug build. So, in
release it will never fire. As this is very unlikely error path, bug
there can live forever. Okay, so ASSERT() is the least desirable way.

Warning is not enough, as we are already in incorrect state.

So, best way is to crash guest, it this correct? I'll do this in the
next version then.

-- 
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 v2 4/6] xen/arm: optee: handle shared buffer translation error

[Julien Grall]

On 24/09/2019 12:37, Volodymyr Babchuk wrote:
> 
> Hi Julien,
> 
> Julien Grall writes:
> 
>> Hi,
>>
>> On 18/09/2019 19:51, Volodymyr Babchuk wrote:
>>> +/* 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 = OPTEE_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 storing exact command
>>> +     * type, only type of RPC return. So, this is the only check we
>>> +     * can perform there.
>>> +     */
>>> +    ASSERT(call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD);
>>
>> As I pointed out in v1, ASSERT() is probably the less desirable
>> solution here as this is an error path.
> Looks like I misunderstood you :(
> 
>> Can you explain why you chose that over the 3 solutions I suggested?
> I think I need some clarification there. ASSERT() is adopted way to tell
> about invariant. Clearly, this is programming error if ASSERT() fails.

That's correct.

> 
> But I understand, that ASSERT() is available only in debug build. So, in
> release it will never fire. As this is very unlikely error path, bug
> there can live forever. Okay, so ASSERT() is the least desirable way.

This is my concern, ASSERT() are fine in path that can be exercised quite well. 
By experience, error path as not so well tested, so any verbosity in non-debug 
build is always helpful.

> 
> Warning is not enough, as we are already in incorrect state.

Incorrect state for who? The guest?

> 
> So, best way is to crash guest, it this correct? I'll do this in the
> next version then.

A rule of thumb is
   - BUG_ON can be replaced by domain_crash() as this is a fatal error you can't 
recover (the scope depends on the function call).

   - ASSERT() can be replaced by WARN_ON() as the former will be a NOP in 
non-debug build. In both case, the error is not fatal and continue will not 
result So it means the error is not fatal.

You used ASSERT() in your code, so it feels to me that WARN_ON() would be a 
suitable replacement.

However, if the state inconsistency is for Xen, then domain_crash() is the best. 
If this is for the guest, then this is not really our business and it may be 
best to let him continue as it could provide more debug (domain_crash() will 
only dump the stack and registers).

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 4/6] xen/arm: optee: handle shared buffer translation error

[Volodymyr Babchuk]

Julien Grall writes:

> On 24/09/2019 12:37, Volodymyr Babchuk wrote:
>>
>> Hi Julien,
>>
>> Julien Grall writes:
>>
>>> Hi,
>>>
>>> On 18/09/2019 19:51, Volodymyr Babchuk wrote:
>>>> +/* 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 = OPTEE_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 storing exact command
>>>> +     * type, only type of RPC return. So, this is the only check we
>>>> +     * can perform there.
>>>> +     */
>>>> +    ASSERT(call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD);
>>>
>>> As I pointed out in v1, ASSERT() is probably the less desirable
>>> solution here as this is an error path.
>> Looks like I misunderstood you :(
>>
>>> Can you explain why you chose that over the 3 solutions I suggested?
>> I think I need some clarification there. ASSERT() is adopted way to tell
>> about invariant. Clearly, this is programming error if ASSERT() fails.
>
> That's correct.
>
>>
>> But I understand, that ASSERT() is available only in debug build. So, in
>> release it will never fire. As this is very unlikely error path, bug
>> there can live forever. Okay, so ASSERT() is the least desirable way.
>
> This is my concern, ASSERT() are fine in path that can be exercised
> quite well. By experience, error path as not so well tested, so any
> verbosity in non-debug build is always helpful.
Yep, I see.

>>
>> Warning is not enough, as we are already in incorrect state.
> Incorrect state for who? The guest?
Yes, for the current call of the current guest. State of other calls and
other guests should not be affected. But it is possible that our view of
OP-TEE state for that guest is not correct also.

>>
>> So, best way is to crash guest, it this correct? I'll do this in the
>> next version then.
>
> A rule of thumb is
>   - BUG_ON can be replaced by domain_crash() as this is a fatal error
> you can't recover (the scope depends on the function call).
This seems correct.

>
>   - ASSERT() can be replaced by WARN_ON() as the former will be a NOP
> in non-debug build. In both case, the error is not fatal and continue
> will not result So it means the error is not fatal.
I can't agree with this in general. But maybe this makes sense for Xen.
As I said, generally ASSERT() is used to, well, assert that condition is
true for any correct state of a program. So it cant' be replaced with
WARN_ON(). If we detected invalid state we should either try to correct
it (if know how) or to immediately stop the program.

But I can see, why this behavior is not desired for
hypervisor. Especially in production use.

> You used ASSERT() in your code, so it feels to me that WARN_ON() would
> be a suitable replacement.
Well, honestly I believe that it is better to crash guest to prevent
any additional harm. Look, we already detected that something is wrong
with mediator state for certain guest. We can pretend that all is fine
and try to continue the call. But who knows which consequences it will have?

> However, if the state inconsistency is for Xen, then domain_crash() is
> the best. If this is for the guest, then this is not really our
> business and it may be best to let him continue as it could provide
> more debug (domain_crash() will only dump the stack and registers).
I'm looking at this from different point: we promised to provide some
service for a guest and screwed up. It is not guest's fault. Now we know
that we can't provide reliable service for a guest anymore. From
safety point of view we should shut down the service. (But this is job
for another patch) For now, we at least should crash the
guest. This is the safest way. What do you think?

--
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 v2 4/6] xen/arm: optee: handle shared buffer translation error

[Julien Grall]

On 24/09/2019 14:30, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> On 24/09/2019 12:37, Volodymyr Babchuk wrote:
>>>
>>> Hi Julien,
>>>
>>> Julien Grall writes:
>>>
>>>> Hi,
>>>>
>>>> On 18/09/2019 19:51, Volodymyr Babchuk wrote:
>>>>> +/* 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 = OPTEE_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 storing exact command
>>>>> +     * type, only type of RPC return. So, this is the only check we
>>>>> +     * can perform there.
>>>>> +     */
>>>>> +    ASSERT(call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD);
>>>>
>>>> As I pointed out in v1, ASSERT() is probably the less desirable
>>>> solution here as this is an error path.
>>> Looks like I misunderstood you :(
>>>
>>>> Can you explain why you chose that over the 3 solutions I suggested?
>>> I think I need some clarification there. ASSERT() is adopted way to tell
>>> about invariant. Clearly, this is programming error if ASSERT() fails.
>>
>> That's correct.
>>
>>>
>>> But I understand, that ASSERT() is available only in debug build. So, in
>>> release it will never fire. As this is very unlikely error path, bug
>>> there can live forever. Okay, so ASSERT() is the least desirable way.
>>
>> This is my concern, ASSERT() are fine in path that can be exercised
>> quite well. By experience, error path as not so well tested, so any
>> verbosity in non-debug build is always helpful.
> Yep, I see.
> 
>>>
>>> Warning is not enough, as we are already in incorrect state.
>> Incorrect state for who? The guest?
> Yes, for the current call of the current guest. State of other calls and
> other guests should not be affected. But it is possible that our view of
> OP-TEE state for that guest is not correct also.
> 
>>>
>>> So, best way is to crash guest, it this correct? I'll do this in the
>>> next version then.
>>
>> A rule of thumb is
>>    - BUG_ON can be replaced by domain_crash() as this is a fatal error
>> you can't recover (the scope depends on the function call).
> This seems correct.
> 
>>
>>    - ASSERT() can be replaced by WARN_ON() as the former will be a NOP
>> in non-debug build. In both case, the error is not fatal and continue
>> will not result So it means the error is not fatal.
> I can't agree with this in general. But maybe this makes sense for Xen.
> As I said, generally ASSERT() is used to, well, assert that condition is
> true for any correct state of a program. So it cant' be replaced with
> WARN_ON(). If we detected invalid state we should either try to correct
> it (if know how) or to immediately stop the program.

I agree that assert are here to catch that any condition is true. However, as I 
pointed out, ASSERTs are turned to NOP in production build. So if there were a 
problem in that path, you would have happily continued with the error.

In other words, ASSERT() is only here to help you catch any mistake during 
development by crashing the hypervisor so you get information on what happen. 
But your code should be able to cope of any mistake in production build (or you 
know this can't happen thanks to code coverage).

This is very different from domain_crash() where you know that your code is not 
able to cope with it and you don't want the guest to continue. Note that 
domain_crash() is asynchronous, so you will still execute some part of the 
hypervisor (until leave_hypervisor_head() is called).

> 
> But I can see, why this behavior is not desired for
> hypervisor. Especially in production use.
> 
>> You used ASSERT() in your code, so it feels to me that WARN_ON() would
>> be a suitable replacement.
> Well, honestly I believe that it is better to crash guest to prevent
> any additional harm. Look, we already detected that something is wrong
> with mediator state for certain guest. We can pretend that all is fine
> and try to continue the call. But who knows which consequences it will have?
> 
>> However, if the state inconsistency is for Xen, then domain_crash() is
>> the best. If this is for the guest, then this is not really our
>> business and it may be best to let him continue as it could provide
>> more debug (domain_crash() will only dump the stack and registers).
> I'm looking at this from different point: we promised to provide some
> service for a guest and screwed up. It is not guest's fault. Now we know
> that we can't provide reliable service for a guest anymore. From
> safety point of view we should shut down the service. (But this is job
> for another patch) For now, we at least should crash the
> guest. This is the safest way. What do you think?

I am happy with domain_crash().

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH v2 5/6] SUPPORT.md: Describe OP-TEE mediator

[Volodymyr Babchuk]

With the latest patches to the mediator, it can be considered
as Technological Preview feature.

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

---

Note for commiter:
Obviously this patch should be merged after all other patches in
this series.
---
 SUPPORT.md | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/SUPPORT.md b/SUPPORT.md
index 375473a456..8d50a72dcb 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -660,6 +660,10 @@ No support for QEMU backends in a 16K or 64K domain.
 
     Status: Supported
 
+### ARM: OP-TEE Mediator
+
+    Status: Tech Preview
+
 ## Virtual Hardware, QEMU
 
 This section describes supported devices available in HVM mode using a
-- 
2.22.0

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

Re: [Xen-devel] [PATCH v2 5/6] SUPPORT.md: Describe OP-TEE mediator

[Julien Grall]

Hi,

On 18/09/2019 19:51, Volodymyr Babchuk wrote:
> With the latest patches to the mediator, it can be considered
> as Technological Preview feature.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

With one change below:

Acked-by: Julien Grall <julien.grall@arm.com>

> 
> ---
> 
> Note for commiter:
> Obviously this patch should be merged after all other patches in
> this series.
> ---
>   SUPPORT.md | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/SUPPORT.md b/SUPPORT.md
> index 375473a456..8d50a72dcb 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -660,6 +660,10 @@ No support for QEMU backends in a 16K or 64K domain.
>   
>       Status: Supported
>   
> +### ARM: OP-TEE Mediator

NIT: s/ARM/Arm/

> +
> +    Status: Tech Preview
> +
>   ## Virtual Hardware, QEMU
>   
>   This section describes supported devices available in HVM mode using a
> 

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH v2 6/6] xen/arm: optee: update description in Kconfig

[Volodymyr Babchuk]

OP-TEE mediator now is "Tech Preview" state, and we want to update
it's description in Kconfig accordingly.

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

---

Note to commiter: this patch depends on first 4 patches in the series.
---
 xen/arch/arm/tee/Kconfig | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
index b4b6aa2610..a4a598191e 100644
--- a/xen/arch/arm/tee/Kconfig
+++ b/xen/arch/arm/tee/Kconfig
@@ -3,7 +3,11 @@ config OPTEE
 	default n
 	depends on TEE
 	help
-	  Enable experimental 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
+	  Enable the 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
+
+	  Right now OP-TEE mediator is "Tech Preview" state, so it is
+	  not good idea to use it in production.
-- 
2.22.0

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

Re: [Xen-devel] [PATCH v2 6/6] xen/arm: optee: update description in Kconfig

[Julien Grall]

Hi Volodymyr,

On 18/09/2019 19:51, Volodymyr Babchuk wrote:
> OP-TEE mediator now is "Tech Preview" state, and we want to update
> it's description in Kconfig accordingly.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> ---
> 
> Note to commiter: this patch depends on first 4 patches in the series.
> ---
>   xen/arch/arm/tee/Kconfig | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
> index b4b6aa2610..a4a598191e 100644
> --- a/xen/arch/arm/tee/Kconfig
> +++ b/xen/arch/arm/tee/Kconfig
> @@ -3,7 +3,11 @@ config OPTEE
>   	default n
>   	depends on TEE
>   	help
> -	  Enable experimental 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
> +	  Enable the 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
> +
> +	  Right now OP-TEE mediator is "Tech Preview" state, so it is
> +	  not good idea to use it in production.

Well, the whole TEE support is under "EXPERT" so someone selecting this option 
already know this should not be used in production. We also have SUPPORT.MD 
describing the state of the feature.

So I would drop completely the last sentence and potentially gate OPTEE with 
"EXPERT" as well. Note that the last bits is optional.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v2 0/6] arch/arm: optee: fix TODOs and change status to "Tech Preview"

[Julien Grall]

On 18/09/2019 19:50, Volodymyr Babchuk wrote:
> Hello,

Hi,

> This is the second version for maturing the OP-TEE mediator.
> 
> Changes also can be pulled from [2].
> 
> Changes from v1:
>   - Added patch that updates SUPPORT.md
>   - Instead of removing "experimental" status I changed it to "Tech Preview"
>   - Other changes are described in the corresponding patches
> 
> Cover letter for v1:
> 
> 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
> [2] https://github.com/lorc/xen/tree/optee3_v2
> 
> 
> Volodymyr Babchuk (6):
>    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

I have committed patch 1-3.

Cheers,

-- 
Julien Grall

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