Re: [PATCH v4 05/10] xen/arm: optee: add std call handling

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Julien,

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.

>> +    {
>> +        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".

> [...]
>
>> +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.

>> +    call->in_flight = false;
>> +    spin_unlock(&ctx->lock);
>> +}
>
> Cheers,


--
Best regards,Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel