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

[Julien Grall <julien.grall@xxxxxxx>]

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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel