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

Re: [Xen-devel] [PATCH v3 09/11] optee: add support for RPC commands


On 19/02/2019 16:14, Volodymyr Babchuk wrote:

Hi Julien,

Julien Grall writes:

Hi Volodymyr,

On 18/12/2018 21:11, Volodymyr Babchuk wrote:
From: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>

OP-TEE can issue multiple RPC requests. We are interested mostly in
request that asks NW to allocate/free shared memory for OP-TEE
needs, because mediator need to do address translation in the same

NIT: the mediator needs

way as it was done for shared buffers registered by NW.

As mediator now accesses shared command buffer, we need to shadow
it in the same way, as we shadow request buffers for STD calls.

This is a bit confusing, does it means patch #8 is not doing the right thing?
No, it was patch #6 :)

And I can't say that it did something wrong. Remember that prior to the
last patch in series DomU can't use the mediator. And for Dom0 it is okay to
map RPC command buffer directly. Description of patch #4 mentions that
we need all patches in the series for a complete mediator.

Not all the memory in Dom0 is 1:1 mapped. So you may end up to use the wrong address here.

But, it is not very intuitive to have to read the commit message of patch #4 to understand that patch #8 is fixing a flaw in patch #6.

Technically, earlier patch should not have allowed to use shared command buffer until now. While I appreciate it is hard to split big series, we at least need to write clear commit message. In that case "now accesses" clear lead to think something wrong has been done before. So a reminder in the commit message would help the reviewer here.

Signed-off-by: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>

   Changes from v2:
   - Use access_guest_memory_by_ipa() instead of direct mapping

   xen/arch/arm/tee/optee.c | 136 +++++++++++++++++++++++++++++++++++++--
   1 file changed, 130 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index cfc3b34df7..bf3535946d 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -67,6 +67,8 @@ struct optee_std_call {
   struct shm_rpc {
       struct list_head list;
       struct page_info *guest_page;
+    struct optee_msg_arg *xen_arg;
+    paddr_t guest_ipa;
       uint64_t cookie;
   @@ -290,6 +292,11 @@ static struct shm_rpc
*allocate_and_pin_shm_rpc(struct optee_domain *ctx,
       if ( !shm_rpc->guest_page )
           goto err;
+    shm_rpc->guest_ipa = gaddr;
+    shm_rpc->xen_arg = alloc_xenheap_page();

Based on the discussion in patch #6, I think you want to use to
allocate the memory from domheap.
Yes, will do.
I already did it for #6 and now there is a lots of places
where I am mapping/unmapping that page. So, it is somewhat

Well, security is always inconvenient until we found a flaw ;).


Julien Grall

Xen-devel mailing list



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