Re: [PATCH v2 06/13] optee: add domain contexts

[Julien Grall <julien.grall@arm.com>]

On 09/05/2018 03:23 PM, Volodymyr Babchuk wrote:
> Hi,

Hi,

> On 05.09.18 17:10, Julien Grall wrote:
>> Hi,
>>
>> On 09/03/2018 05:54 PM, Volodymyr Babchuk wrote:
>>> OP-TEE meditor needs to store per-domain context, as will be seen
>>
>> s/meditor/mediator/
>>
>>> in the next patches. At this moment it stores only reference to domain.
>>>
>>> This allows us to filter out calls from domains that are not allowed
>>> to work with OP-TEE.
>>>
>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>> ---
>>>   xen/arch/arm/tee/optee.c | 66 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 66 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>> index 48bff5d..c895a99 100644
>>> --- a/xen/arch/arm/tee/optee.c
>>> +++ b/xen/arch/arm/tee/optee.c
>>> @@ -19,6 +19,14 @@
>>>   #include <asm/tee/optee_msg.h>
>>>   #include <asm/tee/optee_smc.h>
>>> +struct domain_ctx {
>>
>> I would prefer if the structure has optee in its name such as 
>> optee_domain.
> It is supposed to be internal for this file, so I chose this name. But I
> can rename it, no problem.

Even if it is internal, the structure will appear in debug symbols. If 
you use tools like pahole it will be quite difficult to know what the 
structure is for without a grep.

> 
>> We also tend to abbreviate context in Xen using the ctxt.
>>
>>> +    struct list_head list;
>>> +    struct domain *domain;
>>> +};
>>> +
>>> +static LIST_HEAD(domain_ctx_list);
>>> +static DEFINE_SPINLOCK(domain_ctx_list_lock);
>>> +
>>>   static bool optee_probe(void)
>>>   {
>>>       struct dt_device_node *node;
>>> @@ -41,18 +49,49 @@ static bool optee_probe(void)
>>>       return true;
>>>   }
>>> +static struct domain_ctx *find_domain_ctx(struct domain* d)
>>> +{
>>> +    struct domain_ctx *ctx;
>>> +
>>> +    spin_lock(&domain_ctx_list_lock);
>>> +
>>> +    list_for_each_entry( ctx, &domain_ctx_list, list )
>>> +    {
>>> +        if ( ctx->domain == d )
>>> +        {
>>> +                spin_unlock(&domain_ctx_list_lock);
>>> +                return ctx;
>>> +        }
>>> +    }
>>> +
>>> +    spin_unlock(&domain_ctx_list_lock);
>>> +    return NULL;
>>> +}
>>
>> Why do you need to store the context in a list? As you have a context 
>> per domain it would be better to store that directly in struct domain 
>> by introduce a field "void * tee".
> Yes, this was my first intention. But I was unsure on if it is 
> appropriate to add driver-specific fields to struct arch_domain. If you 
> sat that it is okay (under #ifdef CONFIG_TEE), then I'll do in this way.

I would definitely prefer the "driver-specific" fields. This is likely 
going to be useful for other TEE as well.

Cheers,

-- 
Julien Grall

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