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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

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?

[...]
> >>>@@ -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?

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


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