[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



Hi Julien,

On 11.09.18 14:53, Julien Grall wrote:


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.
Now, this copy will happen for every IRQ when CPU is in S-EL1/S-EL0 mode. Chances are quite high, I must say. Look: OP-TEE or (TA) is doing something, like encrypting some buffer, for example. IRQ fires, OP-TEE immediately executes RPC return (right from interrupt handler), so NW can handle interrupt. Then NW returns control back to OP-TEE, if it wants to.

This is how long job in OP-TEE can be preempted by linux kernel, for example. Timer IRQ ensures that control will be returned to linux, scheduler schedules some other task and OP-TEE patiently waits until its caller is scheduled back, so it can resume the work.


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).
With current configuration, this is maximum 16 pages per guest.
As for performance comparison... This is doable, but will take  some time.

[...]
+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.
Ah, I see your point. Yes, I'll add check to ensure that cookie is not reused.
Thank you for  pointing to this.


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?
Happily resume interrupted work. There is how RPC works:

1. NW client issues STD call (or yielding call in terms of SMCCC)
2. OP-TEE starts its work, but it is needed to be interrupted for some
   reason: IRQ arrived, it wants to block on a mutex, it asks NW to do
   some work (like allocating memory or loading TA). This is called "RPC
   return".
3. OP-TEE suspends thread and does return from SMC call with code
   OPTEE_SMC_RPC_VAL(SOME_CMD) in a0, and some optional parameters in
   other registers
4. NW sees that this is a RPC, and not completed STD call, so it does
   SOME_CMD and  issues another SMC with code
   OPTEE_SMC_CALL_RETURN_FROM_RPC in a0
5. OP-TEE wakes up suspended thread and continues execution
6. pts 2-5 are repeated until OP-TEE finishes the work
7. It returns from last SMC call with code OPTEE_SMC_RETURN_SUCCESS/
   OPTEE_SMC_RETURN_some_error in a0.
8. optee driver sees that call from pt.1 is finished at least and
   returns control back to client


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.
OP-TEE has internal check against this.

So it would be possible for two vCPU to call concurrently the same command and free it.
Maybe you noticed that mediator uses shadow buffer to read cookie id. So it will free the buffer mentioned by OP-TEE.
Basically what happened:

1. OP-TEE asks "free buffer with cookie X" in RPC return
2. guests says "I freed that buffer" in SMC call
3. mediator frees buffer with cookie X on its side

In this particular order.

--
Volodymyr Babchuk

_______________________________________________
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®.