On 20/03/2019 16:14, Volodymyr Babchuk wrote:
Hi Julien,
Hi Volodymyr,
Julien Grall writes:
[...]
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?
It is on review right now. We have achieved agreement on that this call is needed. I believe this will be merged into OP-TEE before I'll send v5 of this series.
Please mention it after --- so we don't forget to check the state of the new call before merging to Xen.
- {
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.
In that other case I don't know how much treads OP-TEE really supports... I think, I will need to rephrase that message. Or, better, I'll add another message like "Suggesting that OP-TEE supports %d threads per guest".
Was there any OP-TEE released containing your Virtualization patches?
Depending on the answer, I would consider to mandate OPTEE_SMC_CONFIG_NUM_THREADS so we don't have to deal with a default value in Xen.
Also, should we expose this call to the guest as well?
[...]
+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?
unmap_xen_arg() is also called while holding the lock. Otherwise there can be race with a other CPU.
[...]
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.
Yeah, in normal circumstances guest should not try to resume call on another vCPU, because we didn't returned from the original call on current vCPU. But what if gust will try to do this? There are chances, that current CPU will unmap buffer that was mapped by other CPU an instance ago.
Wasn't it the point to have the in_flight field? As soon as you set in_flight to true, then only one CPU can have the optee_std_call structure in hand.
So, as long as you map/unmap within the section protected by "in_flight", you protected against any race. The lock is only here to protect the field in_flight and the list. Did I miss anything?
Cheers,