Re: [Xen-devel] [RFC 2/4] arm: add generic TEE mediator framework

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 17/10/17 18:22, Volodymyr Babchuk wrote:
> Hi Julien,
>
> On Tue, Oct 17, 2017 at 05:39:29PM +0100, Julien Grall wrote:
>
> Excuse me, looks like you skipped my thoughts about how to detect
> TEE if we are not sure, that we are running on SMCCC-capable platform.
>
> How do you think, is it appropriate to rely on DT?
I didn't have any opinion as long as it covers most of the TEE and does 
not make any assumption on the platform other than written in the ARM ARM.
I have no idea how Linux is doing the detection, it might be worth for 
you to have a look there.
> [...]
> > > > > @@ -673,6 +674,9 @@ int arch_domain_create(struct domain *d, unsigned int 
> > > > > domcr_flags,
> > > > >       if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
> > > > >           goto fail;
> > > > > +    /* Notify TEE that new domain was created */
> > > > > +    tee_domain_create(d);
> > > > I am not a big fan to see this in arch_domain_create until we see how this
> > > > is going to fit with guest. For instance, will TEE be for every guests? What
> > > > would be the other necessary information to configure it?...
> > > I think I'll call XSM in tee_domain_create() to check if this domain allowed
> > > to work with TEE. I can't imagine what additional information will be needed.
> > > This interface can be extended in the future, though.
> > You will never need to inform TEE that a new client (aka domain) is been
> > created, nor allocated memory for the TEE at domain creation in Xen?
> Yes. You are right. But then there are another question: what to do
> if tee_domain_create() failed? Prevent domain creation? Or create
> domain anyways, but forbid it to call TEE?
I would expect TEE to not be enabled by default for guests. This would 
be user configurable. So if it requests TEE and it is not possible to 
instantiate one, then the domain creation should failed.
> > [...]
> >
> > > > > diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c
> > > > > new file mode 100644
> > > > > index 0000000..7f7a846
> > > > > --- /dev/null
> > > > > +++ b/xen/arch/arm/tee/tee.c
> > > > > @@ -0,0 +1,134 @@
> > > > > +/*
> > > > > + * xen/arch/arm/tee/tee.c
> > > > > + *
> > > > > + * Generic part of TEE mediator subsystem
> > > > > + *
> > > > > + * Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > > > > + * Copyright (c) 2017 EPAM Systems.
> > > > > + *
> > > > > + * This program is free software; you can redistribute it and/or modify
> > > > > + * it under the terms of the GNU General Public License version 2 as
> > > > > + * published by the Free Software Foundation.
> > > > > + *
> > > > > + * This program is distributed in the hope that it will be useful,
> > > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > > + * GNU General Public License for more details.
> > > > > + */
> > > > > +
> > > > > +#include <xen/types.h>
> > > > > +#include <asm/smccc.h>
> > > > > +#include <asm/tee.h>
> > > > > +
> > > > > +/*
> > > > > + * According to ARM SMCCC (ARM DEN 0028B, page 17), service owner
> > > > > + * for generic TEE queries is 63.
> > > > > + */
> > > > > +#define TRUSTED_OS_GENERIC_API_OWNER 63
> > > > > +
> > > > > +#define ARM_SMCCC_FUNC_GET_TEE_UID                                      \
> > > > > +        ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                         \
> > > > > +                           ARM_SMCCC_CONV_32,                           \
> > > > > +                           TRUSTED_OS_GENERIC_API_OWNER,                \
> > > > > +                           ARM_SMCCC_FUNC_CALL_UID)
> > > > This likely needs to be defined in smccc as AFAIU it is part of the SMCCC.
> > > It only used there. I'm not sure if I should define it globally.
> > Maybe ARM_SMCCC_FUNC_GET_TEE_UID, but definitely
> > TRUSTED_OS_GENERIC_API_OWNER should stick with the rest of the subsystem
> > definition in smccc.h.
> Yes, will do in this way.
>
> > [...]
> >
> > > > > +        printk("No TEE found\n");
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    parse_uid(resp, &tee_uid);
> > > > > +
> > > > > +    printk("TEE UID: 
> > > > > %02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n",
> > > > > +           tee_uid.a[0 ], tee_uid.a[1 ], tee_uid.a[2 ], tee_uid.a[3 ],
> > > > Please no space before ]. This is making more confusing to read.
> > > I put it for neat formatting. Probably, I can put double space after commas.
> > > Will be okay?
> > That is that really important to have them? I mean, ok it is not going to be
> > neat but the format string is already ugly and it would not be too difficult
> > to read the arguments.
> Dunno. As for me, it eases parsing. For example, it is easy to see that
> all indexes are correct. But if this is inappropriate, I can remove all
> extra spaces.
Well, if you are going to do a different probe than via SMCCC, then this 
discussion is not necessary as the printk would likely disappear.
> > > > > +           tee_uid.a[4 ], tee_uid.a[5 ], tee_uid.a[6 ], tee_uid.a[7 ],
> > > > > +           tee_uid.a[8 ], tee_uid.a[9 ], tee_uid.a[10], tee_uid.a[11],
> > > > > +           tee_uid.a[12], tee_uid.a[13], tee_uid.a[14], tee_uid.a[15]);
> > > > > +
> > > > > +    for ( desc = _steemediator; desc != _eteemediator; desc++ )
> > > > {
> > > >
> > > > > +        if ( memcmp(&desc->uid, &tee_uid, sizeof(xen_uuid_t)) == 0 )
> > > > > +        {
> > > > > +            printk("Using TEE mediator for %sp\n", desc->name);
> > > > > +            mediator_ops = desc->ops;
> > > > > +            break;
> > > > > +        }
> > > > }
> > > >
> > > > > +
> > > > > +    if ( !mediator_ops )
> > > > A warning here would be useful.
> > > Why? Platform is not obligued to have any TEE.
> > What do you mean? You can only be here because the platform has TEE but Xen
> > does not have any mediator. You actually print "no TEE found" a bit above.
> > So why not printing for when Xen is unable to use it?
> Ah, yes. This makes sense.
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel