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

Re: [Xen-devel] [PATCH v2 07/13] optee: add std call handling





On 10/09/18 18:37, Volodymyr Babchuk wrote:
Hi Julien,

Hi,

On 05.09.18 18:17, Julien Grall wrote:
Hi,

On 09/03/2018 05:54 PM, Volodymyr Babchuk wrote:
Main way to communicate with OP-TEE is to issue standard SMCCC

NIT: The main way

call. "Standard" is a SMCCC term and it means that call can be
interrupted and OP-TEE can return control to NW before completing
the call.

In contranst with fast calls, where arguments and return values

NIT: s/contranst/contrast/

are passed in registers, standard calls use shared memory. Register
pair r1,r2 holds 64-bit PA of command buffer, where all arguments

Do you mean w1, w2?
Good question. How to call the registers, so it would be valid both
for ARMv7 and ARMv8?

Which naming does OP-TEE use? Is it a*?

[...]

+static struct std_call_ctx *allocate_std_call_ctx(struct domain_ctx *ctx)
+{
+    struct std_call_ctx *call;
+    int count;
+
+    count = atomic_add_unless(&ctx->call_ctx_count, 1, MAX_STD_CALLS);

Please a comment explaining the rationale for this.
E.g. "/* Limit number of calls for the current domain */" ?

Yes please.

[...]



+    if ( !call->xen_arg ) {
+        unpin_guest_ram_addr(call->guest_arg_mfn);
+        return false;
+    }
+
+    memcpy(call->xen_arg, call->guest_arg, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
+
+    xen_addr = virt_to_maddr(call->xen_arg);
+
+    set_user_reg(regs, 1, xen_addr >> 32);
+    set_user_reg(regs, 2, xen_addr & 0xFFFFFFFF);
+
+    return true;
+}
+
+static bool copy_std_request_back(struct domain_ctx *ctx,
+                                  struct cpu_user_regs *regs,
+                                  struct std_call_ctx *call)
+{
+    unsigned int i;
+    uint32_t attr;

This function might be a bit tricky to implement using access_guest_memory_by_ipa. However, you can just map the region temporarily. This would avoid to use map_domain_page_global() within then code and avoid one less potentially error in the cleanup path.
I think, I can make all needed manipulations in the shadowed buffer
and then copy it as a whole to the guest's one.

That would quite dangerous as you may copy back data in the guest you don't want. So I would prefer the whitelist solution as you do below.


+
+    call->guest_arg->ret = call->xen_arg->ret;
+    call->guest_arg->ret_origin = call->xen_arg->ret_origin;
+    call->guest_arg->session = call->xen_arg->session;
+    for ( i = 0; i < call->xen_arg->num_params; i++ ) {

for ( ... )
{

+        attr = call->xen_arg->params[i].attr;
+
+        switch ( attr & OPTEE_MSG_ATTR_TYPE_MASK ) {

switch ( ... )
{

+        case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT:
+        case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT:
+            call->guest_arg->params[i].u.tmem.size =
+                call->xen_arg->params[i].u.tmem.size;
+            continue;
+        case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
+        case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
+            call->guest_arg->params[i].u.value.a =
+                call->xen_arg->params[i].u.value.a;
+            call->guest_arg->params[i].u.value.b =
+                call->xen_arg->params[i].u.value.b;
+            continue;
+        case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT:
+        case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT:
+            call->guest_arg->params[i].u.rmem.size =
+                call->xen_arg->params[i].u.rmem.size;
+            continue;
+        case OPTEE_MSG_ATTR_TYPE_NONE:
+        case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT:
+        case OPTEE_MSG_ATTR_TYPE_TMEM_INPUT:
+            continue;
+        }
+    }
+
+    return true;
+}
+
+static bool execute_std_call(struct domain_ctx *ctx,
+                             struct cpu_user_regs *regs,
+                             struct std_call_ctx *call)
+{
+    register_t optee_ret;
+
+    forward_call(regs);

I find a bit odd that you introduce a return for forward_call in the previous patch. But you barely use it.
You see, division of original big patch into smaller ones is a bit
arbitrary, so you can spot artifacts like this. I'll try to fix
them.

I would recommend to think about the split from the beginning of your work, not afterward. If you can't split easily, then it likely means the code review is going to be difficult to do.

But here the main problem is not really the split. The problem is you have a function return a value. That value should *always* be used or checked. If you don't care about the value, the you mostly likely want a comment on top (and possible an ASSERT) stating this will never fail.


I think I would prefer if the function does not return a value and you just rely on get_user_reg(regs, 0).

+
+    optee_ret = get_user_reg(regs, 0);
+    if ( OPTEE_SMC_RETURN_IS_RPC(optee_ret) )
+    {
+        call->optee_thread_id = get_user_reg(regs, 3);
+        call->rpc_op = OPTEE_SMC_RETURN_GET_RPC_FUNC(optee_ret);
+        return true;
+    }
+
+    copy_std_request_back(ctx, regs, call);

Here another function return a value but it is not used. If you don't plan to use the return, then please drop. If you plan to use it, then do it everywhere.
The same thing. It is used later in the series, when there will be additional logic.

See above.

Cheers,

--
Julien Grall

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

 


Rackspace

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