On 10/09/18 19:14, Volodymyr Babchuk wrote:
On 10.09.18 18:34, Julien Grall wrote:
On 03/09/18 17:54, Volodymyr Babchuk wrote:
static struct shm_buf *allocate_shm_buf(struct domain_ctx *ctx, uint64_t cookie, int pages_cnt) @@ -704,6 +732,28 @@ static bool copy_std_request_back(struct domain_ctx *ctx, return true; } +static void handle_rpc_return(struct domain_ctx *ctx, + struct cpu_user_regs *regs, + struct std_call_ctx *call) +{ + 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); + struct shm_rpc *shm_rpc = find_shm_rpc(ctx, cookie);
Newline between declaration and code.
Sorry, another habit from kernel coding style :(
I think you need to modify your habit because I am pretty sure Linux folks would not allow:
struct shm_rpc *rpc = find_shm(...); if ( rpc ) ...
The correct way is:
struct shm_rpc *rpc = find_shm(...);
if ( rpc )
Notice the newline between struct and if.
+ if ( !shm_rpc ) + { + gprintk(XENLOG_ERR, "Can't find SHM-RPC with cookie %lx\n", cookie); + return; + } + memcpy(shm_rpc->guest_arg, shm_rpc->xen_arg, + OPTEE_MSG_GET_ARG_SIZE(shm_rpc->xen_arg->num_params)); + } +}
static bool execute_std_call(struct domain_ctx *ctx, struct cpu_user_regs *regs, struct std_call_ctx *call) @@ -715,8 +765,7 @@ static bool execute_std_call(struct domain_ctx *ctx, 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); + handle_rpc_return(ctx, regs, call);
It would make sense to introduce handle_rpc_return where you actually add those 2 lines.
return true; } @@ -783,6 +832,74 @@ out: return ret; }
+static void handle_rpc_cmd_alloc(struct domain_ctx *ctx, + struct cpu_user_regs *regs, + struct std_call_ctx *call, + struct shm_rpc *shm_rpc) +{ + if ( shm_rpc->xen_arg->params[0].attr != (OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT | + OPTEE_MSG_ATTR_NONCONTIG) ) + { + gprintk(XENLOG_WARNING, "Invalid attrs for shared mem buffer\n"); + return; + }
+ /* Last entry in non_contig array is used to hold RPC-allocated buffer */ + if ( call->non_contig[MAX_NONCONTIG_ENTRIES - 1] ) + { + free_xenheap_pages(call->non_contig[MAX_NONCONTIG_ENTRIES - 1],
- call->non_contig_order[MAX_NONCONTIG_ENTRIES - 1]);
+ call->non_contig[MAX_NONCONTIG_ENTRIES - 1] = NULL; + }
This is quite odd. Why don't you just deny allocating information in the non_config array? This would avoid to silently dropped any page that may have been linked together and potentially used still in use.
No, this, actually is part of the protocol. OP-TEE can ask to allocate more shared buffers, one per RPC return.
Please a give link to the spec and the paragraph.
call->non_contig[x] is needed to hold list of pages until OP_TEE consumes them. The it can be freed and reused to allocate next buffer. Consider this:
What is x?
- OP-TEE issues RPC "allocate buffer"
- NW returns list of pages
- Mediator translates and stores address in non_contig[x]
- OP-TEE begins to consume this list
- IRQ arrives and OP-TEE forced to break the work
- Mediator receives control back, but it should not free non_contig[x],
because it is not sure of OP-TEE finished reading from it 7. Xen/guest handles the IRQ and returns control back to OP-TEE 8. OP-TEE finishes processing this buffers and asks for another one 9. NW returns list of pages for the next buffer 10. At this point mediator is sure that OP-TEE finished processing old non_contig[x], so it can free it and allocated another.
Thank you for the description of the protocol. However, it is still does not explain why you decided to free MAX_NONCONTIG_ENTRIES - 1. Why not 0 or 1 or n?
Overall, it feels like to me you want to write more documentation about how the mediator is supposed to work.
[...]
Please try to avoid changing the coding style in different patch. But this one is wrong.
Yep :( this is the artifact from splitting the big patch.
gprintk(XENLOG_WARNING, "Failed to allocate shm_rpc object\n"); - ptr = 0; - } - else - ptr = mfn_to_maddr(shm_rpc->guest_mfn); + ptr = ~0;
Can you explain why you change from 0 to ~0?
I had to introduce this in the original patch, actually.
What do you mean?
Cheers,