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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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.

>>
>> 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,
>>                                               P2M_ALLOC);
>>       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
inconvenient...

It just appeared to me that I can map those pages when entering
mediator and unmap when leaving.

>> +    if ( !shm_rpc->xen_arg )
>> +        goto err;
>>         shm_rpc->cookie = cookie;
>>   @@ -313,6 +320,7 @@ static struct shm_rpc
>> *allocate_and_pin_shm_rpc(struct optee_domain *ctx,
>>   err:
>>       atomic_dec(&ctx->shm_rpc_count);
>>       put_page(shm_rpc->guest_page);
>> +    free_xenheap_page(shm_rpc->xen_arg);
>>       xfree(shm_rpc);
>>         return NULL;
>> @@ -339,12 +347,32 @@ static void free_shm_rpc(struct optee_domain *ctx, 
>> uint64_t cookie)
>>       if ( !found )
>>           return;
>>   +    free_xenheap_page(shm_rpc->xen_arg);
>> +
>>       ASSERT(shm_rpc->guest_page);
>>       put_page(shm_rpc->guest_page);
>>         xfree(shm_rpc);
>>   }
>>   +static struct shm_rpc *find_shm_rpc(struct optee_domain *ctx,
>> uint64_t cookie)
>> +{
>> +    struct shm_rpc *shm_rpc;
>> +
>> +    spin_lock(&ctx->lock);
>> +    list_for_each_entry( shm_rpc, &ctx->shm_rpc_list, list )
>> +    {
>> +        if ( shm_rpc->cookie == cookie )
>> +        {
>> +                spin_unlock(&ctx->lock);
>> +                return shm_rpc;
>> +        }
>> +    }
>> +    spin_unlock(&ctx->lock);
>> +
>> +    return NULL;
>> +}
>> +
>>   static struct optee_shm_buf *allocate_optee_shm_buf(struct optee_domain 
>> *ctx,
>>                                                       uint64_t cookie,
>>                                                       unsigned int pages_cnt)
>> @@ -712,6 +740,33 @@ static void copy_std_request_back(struct optee_domain 
>> *ctx,
>>       put_page(page);
>>   }
>>   +static void handle_rpc_return(struct optee_domain *ctx,
>> +                             struct cpu_user_regs *regs,
>> +                             struct optee_std_call *call)
>> +{
>> +    call->rpc_params[0] = get_user_reg(regs, 1);
>> +    call->rpc_params[1] = get_user_reg(regs, 2);
>> +    call->optee_thread_id = get_user_reg(regs, 3);
>> +    call->rpc_op = OPTEE_SMC_RETURN_GET_RPC_FUNC(get_user_reg(regs, 0));
>> +
>> +    if ( call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD )
>> +    {
>> +        /* Copy RPC request from shadowed buffer to guest */
>> +        uint64_t cookie = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 
>> 2);
>
> I am afraid this is not going to work correctly. get_user_reg return a
> 64-bit value, so if the top bit are non-zero then the cookie value
> will actually be wrong. It is not clear to me whether the bits [63:32]
> should be 0, so the best is to ignore them.
> Given you use similar construction in a few places in the code, I
> would introduce a new helper that take 2 register indexes and return
> the 64-bit value.
Good idea, thanks.

[...]

-- 
Best regards,Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel