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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

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