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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

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@xxxxxxxx>
> > ---
> >   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.

> 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.

--
Volodymyr Babchuk

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