On 09/12/2018 02:51 PM, Volodymyr Babchuk wrote:
Hi,
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.
At the moment, map/unmap is nearly a nop on Arm64 because all the RAM is mapped (I would avoid to assume that thought :)). The only cost if going through the p2m to translate the IPA to PA.
For Arm32, each CPUs has its own page-tables and the map/unmap (and TLB flush) will be done locally. I would still expect the impact to be minimal.
Note that today map_domain_page on Arm32 is quite simplistic. It would be possible to optimize it for lowering the impact of map/unmap.
[...]
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:
- NW client issues STD call (or yielding call in terms of SMCCC)
- 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.
Could you add those pointers at the top of the OP-TEE file?
But I don't know about any explanation at detail level I gave you above.
That's fine. Can you add that in the commit message?
Cheers,