[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 08/13] optee: add support for RPC SHM buffers





On 10/09/18 18:44, Volodymyr Babchuk wrote:
Hi Julien,

On 10.09.18 16:01, Julien Grall wrote:
Hi Volodymyr,

On 03/09/18 17:54, Volodymyr Babchuk wrote:
OP-TEE usually uses the same idea with command buffers (see
previous commit) to issue RPC requests. Problem is that initially
it has no buffer, where it can write request. So the first RPC
request it makes is special: it requests NW to allocate shared
buffer for other RPC requests. Usually this buffer is allocated
only once for every OP-TEE thread and it remains allocated all
the time until shutdown.

Mediator needs to pin this buffer(s) to make sure that domain can't
transfer it to someone else. Also it should be mapped into XEN
address space, because mediator needs to check responses from
guests.

Can you explain why you always need to keep the shared buffer mapped in Xen? Why not using access_guest_memory_by_ipa every time you want to get information from the guest?
Sorry, I just didn't know about this mechanism. But for performance reasons,
I'd like to keep this buffers always mapped. You see, RPC returns are
very frequent (for every IRQ, actually). So I think, it will be costly
to map/unmap this buffer every time.

This is a bit misleading... This copy will *only* happen for IRQ during an RPC. What are the chances for that? Fairly limited. If this is happening too often, then the map/unmap here will be your least concern.

However, I would like to see any performance comparison here to weight with the memory impact in Xen (Arm32 have limited amount of VA available).



Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
---
  xen/arch/arm/tee/optee.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 119 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 1008eba..6d6b51d 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -21,6 +21,7 @@
  #include <asm/tee/optee_smc.h>
  #define MAX_STD_CALLS   16
+#define MAX_RPC_SHMS    16
  /*
   * Call context. OP-TEE can issue multiple RPC returns during one call.
@@ -35,11 +36,22 @@ struct std_call_ctx {
      int rpc_op;
  };
+/* Pre-allocated SHM buffer for RPC commands */
+struct shm_rpc {
+    struct list_head list;
+    struct optee_msg_arg *guest_arg;
+    struct page *guest_page;
+    mfn_t guest_mfn;
+    uint64_t cookie;
+};
+
  struct domain_ctx {
      struct list_head list;
      struct list_head call_ctx_list;
+    struct list_head shm_rpc_list;
      struct domain *domain;
      atomic_t call_ctx_count;
+    atomic_t shm_rpc_count;
      spinlock_t lock;
  };
@@ -145,8 +157,10 @@ static int optee_enable(struct domain *d)
      ctx->domain = d;
      INIT_LIST_HEAD(&ctx->call_ctx_list);
+    INIT_LIST_HEAD(&ctx->shm_rpc_list);
      atomic_set(&ctx->call_ctx_count, 0);
+    atomic_set(&ctx->shm_rpc_count, 0);
      spin_lock_init(&ctx->lock);
      spin_lock(&domain_ctx_list_lock);
@@ -256,11 +270,81 @@ static struct std_call_ctx *find_call_ctx(struct domain_ctx *ctx, int thread_id)
      return NULL;
  }
+static struct shm_rpc *allocate_and_map_shm_rpc(struct domain_ctx *ctx, paddr_t gaddr,

I would prefer if you pass a gfn instead of the address here.

+                                        uint64_t cookie)

NIT: Indentation

+{
+    struct shm_rpc *shm_rpc;
+    int count;
+
+    count = atomic_add_unless(&ctx->shm_rpc_count, 1, MAX_RPC_SHMS);
+    if ( count == MAX_RPC_SHMS )
+        return NULL;
+
+    shm_rpc = xzalloc(struct shm_rpc);
+    if ( !shm_rpc )
+        goto err;
+
+    shm_rpc->guest_mfn = lookup_and_pin_guest_ram_addr(gaddr, NULL);
+
+    if ( mfn_eq(shm_rpc->guest_mfn, INVALID_MFN) )
+        goto err;
+
+    shm_rpc->guest_arg = map_domain_page_global(shm_rpc->guest_mfn);
+    if ( !shm_rpc->guest_arg )
+    {
+        gprintk(XENLOG_INFO, "Could not map domain page\n");

You don't unpin the guest page if Xen can't map the page.

+        goto err;
+    }
+    shm_rpc->cookie = cookie;
+
+    spin_lock(&ctx->lock);
+    list_add_tail(&shm_rpc->list, &ctx->shm_rpc_list);
+    spin_unlock(&ctx->lock);
+
+    return shm_rpc;
+
+err:
+    atomic_dec(&ctx->shm_rpc_count);
+    xfree(shm_rpc);
+    return NULL;
+}
+
+static void free_shm_rpc(struct domain_ctx *ctx, uint64_t cookie)
+{
+    struct shm_rpc *shm_rpc;
+    bool found = false;
+
+    spin_lock(&ctx->lock);
+
+    list_for_each_entry( shm_rpc, &ctx->shm_rpc_list, list )
+    {
+        if ( shm_rpc->cookie == cookie )

What does guarantee you the cookie will be uniq?
Normal World guarantees. This is the part of the protocol.

By NW, do you mean the guest? You should know by now we should not trust what the guest is doing. If you think it is still fine, then I would like some writing to explain what is the impact of a guest putting twice the same cookie ID.

[...]

It feels quite suspicious to free the memory in Xen before calling OP-TEE. I think this need to be done afterwards.

No, it is OP-TEE asked to free buffer. This function is called, when NW returns from the RPC. So at this moment NW freed the buffer.

But you forward that call to OP-TEE after. So what would OP-TEE do with that?

Looking at that code, I just noticed there potential race condition here. Nothing prevent a guest to call twice with the same optee_thread_id.

So it would be possible for two vCPU to call concurrently the same command and free it.


          break;
+    }
      case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR:
          break;
      case OPTEE_SMC_RPC_FUNC_CMD:


Cheers,



Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.