[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 04/13] optee: add OP-TEE mediator skeleton



Hi Julien,

On 03.09.18 20:38, Julien Grall wrote:
Hi Volodymyr,

On 03/09/18 17:54, Volodymyr Babchuk wrote:
Add very basic OP-TEE mediator. It can probe for OP-TEE presence,
tell it about domain creation/destuction and forward all known

s/destuction/destruction/

calls.

This is all what is needed for Dom0 to work with OP-TEE as long
as Dom0 shares 1:1 mapped pages with OP-TEE. Any attempt to call
OP-TEE from DomU will fail and can lead to spectacular results.

Shall we expect fireworks? :).
I tried couple of time, but with no success :)

Anyway, I think this is a call for forbidding DomU access until it is supported. This also has the benefits to allow merging Dom0 support for OP-TEE without the rest.
Some time ago you said that I can't be sure that Dom0 is 1:1 mapped, because of grant refs. So, actually, any access should be forbidden. I can omit

REGISTER_TEE_MEDIATOR(optee, "OP-TEE", &optee_ops);

till the very last patch in the series.

But then it would not compile, because optee_ops is the static struct...



Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
---
  xen/arch/arm/tee/Kconfig            |   4 ++
  xen/arch/arm/tee/Makefile           |   1 +
  xen/arch/arm/tee/optee.c            | 134 ++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/tee/optee_smc.h |  50 ++++++++++++++
  4 files changed, 189 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..5b829db 100644
--- a/xen/arch/arm/tee/Kconfig
+++ b/xen/arch/arm/tee/Kconfig
@@ -0,0 +1,4 @@
+config OPTEE
+    bool "Enable OP-TEE mediator"
+    default n
+    depends on TEE
diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
index c54d479..982c879 100644
--- a/xen/arch/arm/tee/Makefile
+++ b/xen/arch/arm/tee/Makefile
@@ -1 +1,2 @@
  obj-y += tee.o
+obj-$(CONFIG_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..7bb84d9
--- /dev/null
+++ b/xen/arch/arm/tee/optee.c
@@ -0,0 +1,134 @@
+/*
+ * xen/arch/arm/tee/optee.c
+ *
+ * OP-TEE mediator
+ *
+ * Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
+ * Copyright (c) 2018 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.
+ */
+
+#include <xen/device_tree.h>
+#include <xen/sched.h>
+#include <asm/smccc.h>
+#include <asm/tee/tee.h>
+
+#include <asm/tee/optee_msg.h>
+#include <asm/tee/optee_smc.h>
+
+static bool optee_probe(void)
+{
+    struct dt_device_node *node;
+    struct arm_smccc_res resp;
+
+    /* Check for entry in dtb  */
+    node = dt_find_compatible_node(NULL, NULL, "linaro,optee-tz");
+    if ( !node )
+        return false;
+
+    /* Check UID */
+    arm_smccc_smc(ARM_SMCCC_CALL_UID_FID(TRUSTED_OS_END), &resp);
+
+    if ( resp.a0 != OPTEE_MSG_UID_0 ||
+         resp.a1 != OPTEE_MSG_UID_1 ||
+         resp.a2 != OPTEE_MSG_UID_2 ||
+         resp.a3 != OPTEE_MSG_UID_3 )

I would be extra cautious with the sign-extension here. It would be better to follow the spec regarding UUID by casting resp.a0 & co to 32-bit.

+        return false;
+
+    return true;
+}
+
+static int optee_enable(struct domain *d)
+{
+    struct arm_smccc_res resp;
+
+    arm_smccc_smc(OPTEE_SMC_VM_CREATED, d->domain_id + 1, 0, 0, 0, 0, 0, 0,
+                  &resp);
+    if ( resp.a0 != OPTEE_SMC_RETURN_OK ) {
+        gprintk(XENLOG_WARNIREGISTER_TEE_MEDIATOR(optee, "OP-TEE", &optee_ops);NG, "OP-TEE don't want to support domain: %d\n",

This message is slightly odd. It would be better to say:

"Unable to create OP-TEE client: rc=%d\n".

+                (uint32_t)resp.a0);
+        return -ENODEV;
+    }
+
+    return 0;
+}
+
+static void forward_call(struct cpu_user_regs *regs)
+{
+    struct arm_smccc_res resp;
+
+    arm_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),
+                  /* client id 0 is reserved for hypervisor itself */
+                  current->domain->domain_id + 1,

I would prefer if the client ID is encoded in a macro. So this could be re-used

Something like

 #define OPTEE_CLIENT_ID(domain) (domain->domain_id + 1)

?


+                  &resp);
+
+    set_user_reg(regs, 0, resp.a0);
+    set_user_reg(regs, 1, resp.a1);
+    set_user_reg(regs, 2, resp.a2);
+    set_user_reg(regs, 3, resp.a3);
+    set_user_reg(regs, 4, 0);
+    set_user_reg(regs, 5, 0);
+    set_user_reg(regs, 6, 0);
+    set_user_reg(regs, 7, 0);
+}
+
+static void optee_domain_destroy(struct domain *d)
+{
+    struct arm_smccc_res resp;
+
+    /* At this time all domain VCPUs should be stopped */
+
+    /* Inform OP-TEE that domain is shutting down */
+    arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, d->domain_id + 1, 0, 0, 0, 0, 0, 0,
+                  &resp);

So this SMC should never fail or even get preempted? I was kind of expecting that it may time some time to destroy a domain.

It is the fast SMCCC call, so it can't be preempted. And it is really fast, at lest in OP-TEE case. Basically, OP-TEE just removes some entries from the list and frees some memory. And yes, it can't fail.

+}
+
+static bool optee_handle_call(struct cpu_user_regs *regs)
+{
+    switch ( get_user_reg(regs, 0) )
+    {
+    case OPTEE_SMC_CALLS_COUNT:
+    case OPTEE_SMC_CALLS_UID:
+    case OPTEE_SMC_CALLS_REVISION:
+    case OPTEE_SMC_CALL_GET_OS_UUID:
+    case OPTEE_SMC_FUNCID_GET_OS_REVISION:
+    case OPTEE_SMC_ENABLE_SHM_CACHE:
+    case OPTEE_SMC_DISABLE_SHM_CACHE:
+    case OPTEE_SMC_GET_SHM_CONFIG:
+    case OPTEE_SMC_EXCHANGE_CAPABILITIES:
+    case OPTEE_SMC_CALL_WITH_ARG:
+    case OPTEE_SMC_CALL_RETURN_FROM_RPC:
+        forward_call(regs);
+        return true;
+    default:
+        return false;
+    }
+}
+
+static const struct tee_mediator_ops optee_ops =
+{
+    .probe = optee_probe,
+    .enable = optee_enable,
+    .domain_destroy = optee_domain_destroy,
+    .handle_call = optee_handle_call,
+};
+
+REGISTER_TEE_MEDIATOR(optee, "OP-TEE", &optee_ops);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/tee/optee_smc.h b/xen/include/asm-arm/tee/optee_smc.h
index 26d100e..1c5a247 100644
--- a/xen/include/asm-arm/tee/optee_smc.h
+++ b/xen/include/asm-arm/tee/optee_smc.h
@@ -305,6 +305,56 @@ struct optee_smc_disable_shm_cache_result {
      OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_ENABLE_SHM_CACHE)
  /*
+ * Inform OP-TEE about a new virtual machine
+ *
+ * Hypervisor issues this call during virtual machine (guest) creation.
+ * OP-TEE records VM_ID of new virtual machine and makes self ready
+ * to receive requests from it.
+ *
+ * Call requests usage:
+ * a0    SMC Function ID, OPTEE_SMC_VM_CREATED
+ * a1    VM_ID of newly created virtual machine
+ * a2-6 Not used
+ * a7    Hypervisor Client ID register. Must be 0, because only hypervisor
+ *      can issue this call
+ *
+ * Normal return register usage:
+ * a0    OPTEE_SMC_RETURN_OK
+ * a1-7    Preserved
+ *
+ * Error return:
+ * a0    OPTEE_SMC_RETURN_ENOTAVAIL    OP-TEE has no resources for
+ *                    another VM
+ * a1-7    Preserved
+ *
+ */
+#define OPTEE_SMC_FUNCID_VM_CREATED    13
+#define OPTEE_SMC_VM_CREATED \
+    OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_VM_CREATED)
+
+/*
+ * Inform OP-TEE about shutdown of a virtual machine
+ *
+ * Hypervisor issues this call during virtual machine (guest) destruction.
+ * OP-TEE will clean up all resources associated with this VM.
+ *
+ * Call requests usage:
+ * a0    SMC Function ID, OPTEE_SMC_VM_DESTROYED
+ * a1    VM_ID of virtual machine being shutted down
+ * a2-6 Not used
+ * a7    Hypervisor Client ID register. Must be 0, because only hypervisor
+ *      can issue this call
+ *
+ * Normal return register usage:
+ * a0    OPTEE_SMC_RETURN_OK
+ * a1-7    Preserved
+ *
+ */
+#define OPTEE_SMC_FUNCID_VM_DESTROYED    14
+#define OPTEE_SMC_VM_DESTROYED \
+    OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_VM_DESTROYED)
+
+/*
   * Resume from RPC (for example after processing a foreign interrupt)
   *
   * Call register usage:


Cheers,


--
Volodymyr Babchuk

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.