Hi Volodymyr,
On 11/10/17 20:01, Volodymyr Babchuk wrote:
Add basic OP-TEE mediator as an example how TEE mediator framework
works.
Currently it support only calls from Dom0. Calls from other guests
will be declined. It maps OP-TEE static shared memory region into
Dom0 address space, so Dom0 is the only domain which can work with
older versions of OP-TEE.
Also it alters SMC requests by\ adding domain id to request. OP-TEE
can use this information to track requesters.
Albeit being in early development stages, this mediator already can
be used on systems where only Dom0 interacts with OP-TEE.
A link to the spec would be useful here to be able to fully review this
patch.
It was tested on RCAR Salvator-M3 board.
Is it with the stock op-tee? Or an updated version?
Signed-off-by: Volodymyr Babchuk <volodymyr_babc...@epam.com>
---
xen/arch/arm/tee/Kconfig | 4 ++
xen/arch/arm/tee/Makefile | 1 +
xen/arch/arm/tee/optee.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 183 insertions(+)
create mode 100644 xen/arch/arm/tee/optee.c
diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
index e69de29..7c6b5c6 100644
--- a/xen/arch/arm/tee/Kconfig
+++ b/xen/arch/arm/tee/Kconfig
@@ -0,0 +1,4 @@
+config ARM_OPTEE
+ bool "Enable OP-TEE mediator"
+ default n
+ depends on ARM_TEE
diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
index c54d479..9d93b42 100644
--- a/xen/arch/arm/tee/Makefile
+++ b/xen/arch/arm/tee/Makefile
@@ -1 +1,2 @@
obj-y += tee.o
+obj-$(CONFIG_ARM_OPTEE) += optee.o
diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
new file mode 100644
index 0000000..0220691
--- /dev/null
+++ b/xen/arch/arm/tee/optee.c
@@ -0,0 +1,178 @@
+/*
+ * xen/arch/arm/tee/optee.c
+ *
+ * OP-TEE mediator
+ *
+ * Volodymyr Babchuk <volodymyr_babc...@epam.com>
+ * 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 <xen/sched.h>
+
+#include <asm/p2m.h>
+#include <asm/tee.h>
+
+#include "optee_msg.h"
+#include "optee_smc.h"
+
+/*
+ * OP-TEE violates SMCCC when it defines own UID. So we need
+ * to place bytes in correct order.
Can you please point the paragraph in the spec where it says that?
+ */
+#define OPTEE_UID (xen_uuid_t){{
\
+ (uint8_t)(OPTEE_MSG_UID_0 >> 0), (uint8_t)(OPTEE_MSG_UID_0 >> 8),
\
+ (uint8_t)(OPTEE_MSG_UID_0 >> 16), (uint8_t)(OPTEE_MSG_UID_0 >> 24),
\
+ (uint8_t)(OPTEE_MSG_UID_1 >> 0), (uint8_t)(OPTEE_MSG_UID_1 >> 8),
\
+ (uint8_t)(OPTEE_MSG_UID_1 >> 16), (uint8_t)(OPTEE_MSG_UID_1 >> 24),
\
+ (uint8_t)(OPTEE_MSG_UID_2 >> 0), (uint8_t)(OPTEE_MSG_UID_2 >> 8),
\
+ (uint8_t)(OPTEE_MSG_UID_2 >> 16), (uint8_t)(OPTEE_MSG_UID_2 >> 24),
\
+ (uint8_t)(OPTEE_MSG_UID_3 >> 0), (uint8_t)(OPTEE_MSG_UID_3 >> 8),
\
+ (uint8_t)(OPTEE_MSG_UID_3 >> 16), (uint8_t)(OPTEE_MSG_UID_3 >> 24),
\
+ }}
+
+static int optee_init(void)
+{
+ printk("OP-TEE mediator init done\n");
+ return 0;
+}
+
+static void optee_domain_create(struct domain *d)
+{
+ /*
+ * Do nothing at this time.
+ * In the future this function will notify that new VM is started.
You already have a new client with the hardware domain. So don't you
already need to notifity OP-TEE?
+ */
+}
+
+static void optee_domain_destroy(struct domain *d)
+{
+ /*
+ * Do nothing at this time.
+ * In the future this function will notify that VM is being destroyed.
+ */
Same for the destruction?
+}
+
+static bool forward_call(struct cpu_user_regs *regs)
+{
+ register_t resp[4];
+
+ call_smccc_smc(get_user_reg(regs, 0),
+ get_user_reg(regs, 1),
+ get_user_reg(regs, 2),
+ get_user_reg(regs, 3),
+ get_user_reg(regs, 4),
+ get_user_reg(regs, 5),
+ get_user_reg(regs, 6),
+ /* VM id 0 is reserved for hypervisor itself */
s/VM/client/. Also, on your design document you mentioned that you did
modify OP-TEE to support multiple client ID. So how do you know whether
the TEE supports client ID?
Similarly, do you expect OP-TEE to support 16-bit of client identifier?
+ current->domain->domain_id +1,
+ resp);
+
+ set_user_reg(regs, 0, resp[0]);
+ set_user_reg(regs, 1, resp[1]);
+ set_user_reg(regs, 2, resp[2]);
+ set_user_reg(regs, 3, resp[3]);
Who will do the sanity check of the return values? Each callers? If so,
I would prefer that the results are stored in a temporary array and a
separate helpers will write them into the domain once the sanity is done.
This would avoid to mistakenly expose unwanted data to a domain.
+
+ return true;
+}
+
+static bool handle_get_shm_config(struct cpu_user_regs *regs)
+{
+ paddr_t shm_start;
+ size_t shm_size;
+ int rc;
+
+ printk("handle_get_shm_config\n");
No plain printk in code accessible by the guest. You should use gprintk
or ratelimit it.
+ /* Give all static SHM region to the Dom0 */
s/Dom0/Hardware Domain/
But I am not sure what's the point of this check given OP-TEE is only
supported for the Hardware Domain and you already have a check for that.
+ if ( current->domain->domain_id != 0 )
Please use is_hardware_domain(current->domain) and not open-code the check.
+ return false;
+
+ forward_call(regs);
+
+ /* Return error back to the guest */
+ if ( get_user_reg(regs, 0) != OPTEE_SMC_RETURN_OK)
+ return true;
This is quite confusing to read, I think it would make sense that
forward_call return the error.
+
+ shm_start = get_user_reg(regs, 1);
+ shm_size = get_user_reg(regs, 2);
+
+ /* Dom0 is mapped 1:1 */
Please don't make this assumption or at least add
ASSERT(is_domain_direct_mapped(d));
+ rc = map_regions_p2mt(current->domain, gaddr_to_gfn(shm_start),
Rather than using current->domain everywhere, I would prefer if you
introduce a temporary variable for the domain.
+ shm_size / PAGE_SIZE,
Please PFN_DOWN(...).
+ maddr_to_mfn(shm_start),
+ p2m_ram_rw);
What is this shared memory for? I know this is the hardware domain, so
using p2m_ram_rw would be ok. But I don't think this would be safe
unless TEE do proper safety check.
+ if ( rc < 0 )
+ {
+ gprintk(XENLOG_INFO, "OP-TEE: Can't map static shm for Dom0: %d", rc);
gprintk already dump the domid. So no need to say Dom0.
+ set_user_reg(regs, 0, OPTEE_SMC_RETURN_ENOTAVAIL);
+ }
+
+ return true;
+}
+
+static bool handle_exchange_capabilities(struct cpu_user_regs *regs)
+{
+ forward_call(regs);
+
+ printk("handle_exchange_capabilities\n");
Same here, no plain prink.
+ /* Return error back to the guest */
+ if ( get_user_reg(regs, 0) != OPTEE_SMC_RETURN_OK)
+ return true;
+
+ /* Don't allow guests to work without dynamic SHM */
Hmmm? But you don't support guests today. So why are you checking that?
I would prefer if you either support guest or not. But not half of it as
it is hard to know how this will end up.
+ if (current->domain->domain_id != 0 &&
+ !(get_user_reg(regs, 1) & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) )
+ set_user_reg(regs, 0, OPTEE_SMC_RETURN_ENOTAVAIL);
Missing newline.
+ return true;
+}
+
+static bool optee_handle_smc(struct cpu_user_regs *regs)
+{
+ /* At this time we support only calls from Dom0. */
+ if ( current->domain->domain_id != 0 )
is_hardware_domain(d)
+ return false;
+
+ switch ( get_user_reg(regs, 0) )
+ {
+ case OPTEE_SMC_GET_SHM_CONFIG:
+ return handle_get_shm_config(regs);
+ case OPTEE_SMC_EXCHANGE_CAPABILITIES:
+ return handle_exchange_capabilities(regs);
+ default:
+ return forward_call(regs);
+ }
+ return true;
The return here is not necessary.
+}
+
+static void optee_remove(void)
+{
+}
+
+static const struct tee_mediator_ops optee_ops =
+{
+ .init = optee_init,
+ .domain_create = optee_domain_create,
+ .domain_destroy = optee_domain_destroy,
+ .handle_smc = optee_handle_smc,
+ .remove = optee_remove,
+};
+
+REGISTER_TEE_MEDIATOR(optee, "OP-TEE", OPTEE_UID, &optee_ops);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel