[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2 10/13] optee: add support for RPC commands
Hi,
On 10.09.18 18:34, Julien Grall wrote:
Hi Volodymyr,
On 03/09/18 17:54, Volodymyr Babchuk wrote:
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, becuase mediator need to do address translation in the same
s/becuase/because/
s/need/needs/
the mediator
way as it was done for shared buffers registered by NW.
As mediator now accesses shared command buffer, we need to shadow
"As the"
it in the same way, as we shadow request buffers for STD calls.
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
---
xen/arch/arm/tee/optee.c | 137
+++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 126 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 8bfcfdc..b2d795e 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -45,6 +45,7 @@ struct std_call_ctx {
struct shm_rpc {
struct list_head list;
struct optee_msg_arg *guest_arg;
+ struct optee_msg_arg *xen_arg;
struct page *guest_page;
mfn_t guest_mfn;
uint64_t cookie;
@@ -303,6 +304,10 @@ static struct shm_rpc
*allocate_and_map_shm_rpc(struct domain_ctx *ctx, paddr_t
if ( !shm_rpc )
goto err;
+ shm_rpc->xen_arg = alloc_xenheap_page();
+ if ( !shm_rpc->xen_arg )
+ goto err;
+
shm_rpc->guest_mfn = lookup_and_pin_guest_ram_addr(gaddr, NULL);
if ( mfn_eq(shm_rpc->guest_mfn, INVALID_MFN) )
@@ -324,6 +329,10 @@ static struct shm_rpc
*allocate_and_map_shm_rpc(struct domain_ctx *ctx, paddr_t
err:
atomic_dec(&ctx->shm_rpc_count);
+
+ if ( shm_rpc->xen_arg )
+ free_xenheap_page(shm_rpc->xen_arg);
+
xfree(shm_rpc);
return NULL;
}
@@ -346,9 +355,10 @@ static void free_shm_rpc(struct domain_ctx *ctx,
uint64_t cookie)
}
spin_unlock(&ctx->lock);
- if ( !found ) {
+ if ( !found )
return;
- }
That change should be folded into the patch adding the {}.
+
+ free_xenheap_page(shm_rpc->xen_arg);
if ( shm_rpc->guest_arg ) {
unpin_guest_ram_addr(shm_rpc->guest_mfn);
@@ -358,6 +368,24 @@ static void free_shm_rpc(struct domain_ctx *ctx,
uint64_t cookie)
xfree(shm_rpc);
}
+static struct shm_rpc *find_shm_rpc(struct domain_ctx *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 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 :(
+ 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.
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:
1. OP-TEE issues RPC "allocate buffer"
2. NW returns list of pages
3. Mediator translates and stores address in non_contig[x]
4. OP-TEE begins to consume this list
5. IRQ arrives and OP-TEE forced to break the work
6. 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.
I would also prefer if you introduce a define for that RCP-allocated
buffer.
+ translate_noncontig(ctx, call, shm_rpc->xen_arg->params + 0,
What does the "+ 0" stand for?
+ MAX_NONCONTIG_ENTRIES - 1);
+}
+
+static void handle_rpc_cmd(struct domain_ctx *ctx, struct
cpu_user_regs *regs,
+ struct std_call_ctx *call)
+{
+ struct shm_rpc *shm_rpc;
+ uint64_t cookie;
+ int num_params;
+
+ cookie = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 2);
+
+ shm_rpc = find_shm_rpc(ctx, cookie);
+
+ if ( !shm_rpc )
+ {
+ gprintk(XENLOG_ERR, "Can't find SHM-RPC with cookie %lx\n",
cookie);
+ return;
+ }
+
+ num_params = shm_rpc->guest_arg->num_params;
+
+ barrier(); /* Ensure that num_params is read once */
I would use:
num_params = ACCESS_ONCE(shm_rpc->guest_arg->num_params);
I looked for this macro, but somehow missed it. Thanks for pointing out.
+ if ( OPTEE_MSG_GET_ARG_SIZE(num_params) >
OPTEE_MSG_NONCONTIG_PAGE_SIZE )
+ return;
+
+ memcpy(shm_rpc->xen_arg, shm_rpc->guest_arg,
+ OPTEE_MSG_GET_ARG_SIZE(num_params));
+
+ switch (shm_rpc->xen_arg->cmd) {
+ case OPTEE_MSG_RPC_CMD_GET_TIME:
+ break;
+ case OPTEE_MSG_RPC_CMD_WAIT_QUEUE:
+ break;
+ case OPTEE_MSG_RPC_CMD_SUSPEND:
+ break;
+ case OPTEE_MSG_RPC_CMD_SHM_ALLOC:
+ handle_rpc_cmd_alloc(ctx, regs, call, shm_rpc);
+ break;
+ case OPTEE_MSG_RPC_CMD_SHM_FREE:
+ free_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
+ break;
+ default:
+ break;
+ }
+}
+
static void handle_rpc_func_alloc(struct domain_ctx *ctx,
struct cpu_user_regs *regs)
{
@@ -796,13 +913,11 @@ static void handle_rpc_func_alloc(struct
domain_ctx *ctx,
struct shm_rpc *shm_rpc;
shm_rpc = allocate_and_map_shm_rpc(ctx, ptr, cookie);
- if ( !shm_rpc )
- {
+ if ( !shm_rpc ) {
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.
+ } else
+ ptr = virt_to_maddr(shm_rpc->xen_arg);
set_user_reg(regs, 1, ptr >> 32);
set_user_reg(regs, 2, ptr & 0xFFFFFFFF);
@@ -833,7 +948,7 @@ static bool handle_rpc(struct domain_ctx *ctx,
struct cpu_user_regs *regs)
case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR:
break;
case OPTEE_SMC_RPC_FUNC_CMD:
- /* TODO: Add handling */
+ handle_rpc_cmd(ctx, regs, call);
break;
}
Cheers,
--
Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
