Hi Volodymyr,
Only few NITs in this patch. See below:
On 07/03/2019 21:04, Volodymyr Babchuk wrote:
+/*
- Default maximal number concurrent threads that OP-TEE supports.
- This limits number of standard calls that guest can have.
- */
+#define DEF_MAX_OPTEE_THREADS 16
- #define OPTEE_KNOWN_NSEC_CAPS OPTEE_SMC_NSEC_CAP_UNIPROCESSOR #define OPTEE_KNOWN_SEC_CAPS (OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM | \ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
+static unsigned int max_optee_threads = DEF_MAX_OPTEE_THREADS;
NIT: This could be __read_mostly.
+/*
- Call context. OP-TEE can issue multiple RPC returns during one call.
- We need to preserve context during them.
- */
+struct optee_std_call {
- struct list_head list;
- /* Page where shadowed copy of call arguments is stored */
- struct page_info *xen_arg_pg;
- /* Above page mapped into XEN */
- struct optee_msg_arg *xen_arg;
- /* Address of original call arguments */
- paddr_t guest_arg_ipa;
- int optee_thread_id;
- int rpc_op;
- bool in_flight;
+};
+/* Domain context */ +struct optee_domain {
- struct list_head call_list;
- atomic_t call_count;
- spinlock_t lock;
+};
- static bool optee_probe(void) { struct dt_device_node *node;
@@ -48,12 +82,25 @@ static bool optee_probe(void) (uint32_t)resp.a3 != OPTEE_MSG_UID_3 ) return false;
- /* Read number of threads */
- arm_smccc_smc(OPTEE_SMC_GET_CONFIG, OPTEE_SMC_CONFIG_NUM_THREADS, &resp);
- if ( resp.a0 == OPTEE_SMC_RETURN_OK )
Out of interest, when was this call added?
- {
max_optee_threads = resp.a1;
printk(XENLOG_DEBUG "OP-TEE supports %u threads.\n", max_optee_threads);
extra NIT: I would use XENLOG_INFO rather than XENLOG_DEBUG. This is a useful information to have in bug report.
Regarding the message, what matters is the number of threads for guest. So I would rework it to make it more meaning full for a user that does not know the internal.
You might also want to move the message out of the if. So you have the message even when OP-TEE does not support the SMC.
[...]
+static struct optee_std_call *get_std_call(struct optee_domain *ctx,
int thread_id)
+{
- struct optee_std_call *call;
- spin_lock(&ctx->lock);
- list_for_each_entry( call, &ctx->call_list, list )
- {
if ( call->optee_thread_id == thread_id )
{
if ( call->in_flight )
{
gdprintk(XENLOG_WARNING, "Guest tries to execute call which is already in flight\n");
NIT: Missing full stop.
Also, the line is over 80 characters. While we don't want to split in the middle of the message (so ack/grep can be used easily), you will want to split the line after the comma.
goto out;
}
call->in_flight = true;
map_xen_arg(call);
NIT: Does this need to be done with the lock taken?
spin_unlock(&ctx->lock);
return call;
}
- }
+out:
- spin_unlock(&ctx->lock);
- return NULL;
+}
+static void put_std_call(struct optee_domain *ctx, struct optee_std_call *call) +{
- spin_lock(&ctx->lock);
- ASSERT(call->in_flight);
- unmap_xen_arg(call);
Same question for the unmap.
- call->in_flight = false;
- spin_unlock(&ctx->lock);
+}
Cheers,