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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

Hi,

On 12.09.18 13:59, Julien Grall wrote:
> Hi Volodymyr,
>
> On 09/11/2018 08:30 PM, Volodymyr Babchuk wrote:
> > On 11.09.18 14:53, Julien Grall wrote:
> > > On 10/09/18 18:44, Volodymyr Babchuk wrote:
> > > > On 10.09.18 16:01, Julien Grall wrote:
> > > > > 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.
> I understand this... But the map/unmap should be negligible over the 
> rest of the context.
I thought that map/unmap is quite costly operation, but I can be wrong 
there.
> > 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.
> Let me write it differently, I will always chose the safe side until 
> this is strictly necessary or performance has been proven. I might be 
> convinced for just 16 pages, although it feels like a premature 
> optimization...
Okay, then I'll stick with memory copy helpers for now.

> > > > > 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
> Thank you for the explanation. As I mentioned in another thread, it 
> would be good to have some kind of highly level explanation in the tree 
> and all those interaction. If it is already existing, then pointer in 
> the code.
High level is covered at [1], and  low level is covered in already 
mentioned header files.
But I don't know about any explanation at detail level I gave you above.

> > > 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.
> I am not sure how OP-TEE internal check would help here. The user may 
> know that thread-id 1 exist and will call it from 2 vCPUs concurrently.
> So handle_rpc will find a context associated to it and use it for 
> execute_std_call. If OP-TEE return an error (or is done with it), you 
> will end up to free twice the same context.
> Did I miss anything?
Ah, I see what do you mean. Yes, I need to track call state. Some
flag to indicate that this particular call is already being processed.



> > > 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.
> I am not speaking about the cookie id but the thread_id... However this 
> statement is wrong in the context we are discussing. Both thread_id and 
> cookie are read from the guest registers.
thread_id is indeed read from guest registers. cookie id should be read 
from OP-TEE registers,  I'll re-check this.
[1] 
https://github.com/OP-TEE/optee_os/blob/master/documentation/optee_design.md
--
Volodymyr Babchuk

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