Hi,

On Fri, May 15, 2015 at 07:34:27AM +0100, Jens Wiklander wrote:

Adds a OP-TEE driver which also can be compiled as a loadable module.

* Targets ARM and ARM64
* Supports using reserved memory from OP-TEE as shared memory
* CMA as shared memory is optional and only tried if OP-TEE doesn't
  supply a reserved shared memory region

How does OP-TEE provide that reserved memory? How is that described in
DT/UEFI to the OS (e.g. is there a memreserve, or is the memory not
described at all)?

* Probes OP-TEE version using SMCs
* Accepts requests on privileged and unprivileged device
* Uses OPTEE message protocol version 2

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 Documentation/devicetree/bindings/optee/optee.txt  |  17 +

I'm concerned that there's no documentation regarding the interface
exposed to userspace, for neither rationale nor usage.

I'm also very concerned that the interface exposed to userspace is
hideously low-level. Surely we'd expect kernel-side drivers to be doing
the bulk of direct communication to the OP-TEE instance? In the lack of
a provided rationale I don't see why the current messaging interface
would make sense.

s/propterty/property/

+indicating the OP-TEE protocol is used when communicating with the secure
+world.
+
+* OP-TEE based on ARM TrustZone required properties:
+
+- compatible="optee,optee-tz"
+
+Example:
+       optee {
+               compatible="optee,optee-tz";
+       };

What does the OP-TEE protocol give in the way of discoverability? Is it
expected that the specific implementation and/or features will be
detected dynamically?

Where can I find documentation on the protocol?

Please split the DT binding parts into a separate patch, at the start of
the series.

The assembly objects should probably live under the relevant arch/
folders, and can probably be shared with clients for other services
compliant with the SMC Calling Conventions.

+static void optee_call_lock(struct optee_call_sync *callsync)
+{
+       mutex_lock(&callsync->mutex);
+}
+
+static void optee_call_lock_wait_completion(struct optee_call_sync *callsync)
+{
+       /*
+        * Release the lock until "something happens" and then reacquire it
+        * again.

When you say you're waiting until "something happens", you really are
waiting until something happens. The quotes aren't helpful, please drop
them.

+        *
+        * This is needed when TEE returns "busy" and we need to try again
+        * later.
+        */
+       callsync->c_waiters++;
+       mutex_unlock(&callsync->mutex);
+       /*
+        * Wait at most one second. Secure world is normally never busy
+        * more than that so we should normally never timeout.
+        */
+       wait_for_completion_timeout(&callsync->c, HZ);
+       mutex_lock(&callsync->mutex);
+       callsync->c_waiters--;
+}
+
+static void optee_call_unlock(struct optee_call_sync *callsync)
+{
+       /*
+        * If at least one thread is waiting for "something to happen" let
+        * one thread know that "something has happened".
+        */
+       if (callsync->c_waiters)
+               complete(&callsync->c);
+       mutex_unlock(&callsync->mutex);
+}
+

You can get rid of the c_waiters variable entirely, as complete() is
safe to call when the completion has an empty waiters list (and the
manipulation of c_waiters is racy anyway...).

Also, I think you need complete_all(&callsync->c) if more than two
concurrent calls are possible. Otherwise one call might block another
indefinitely.

+static int optee_arg_from_user(struct opteem_arg *arg, size_t size,
+                       struct tee_shm **put_shm)
+{
+       struct opteem_param *param;
+       size_t n;
+
+       if (!arg->num_params || !put_shm)
+               return -EINVAL;
+
+       param = OPTEEM_GET_PARAMS(arg);

OPTEEM is a little opaque. OPTEE_MSG would be easier to grasp.

[...]

Surely this ifdeffery isn't necessary?

[...]

+static struct tee_shm_pool *optee_config_shm_ioremap(struct device *dev,
+                       void __iomem **ioremaped_shm)
+{
+       struct optee_smc_param param = { .a0 = OPTEE_SMC_GET_SHM_CONFIG };
+       struct tee_shm_pool *pool;
+       u_long vaddr;

Why not plain unsigned long?

[...]

+/*
+ * This file is exported by OP-TEE and is in kept in sync between secure
+ * world and normal world kernel driver. We're following ARM SMC Calling
+ * Convention as specified in
+ * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html

The values defined in the SMC Calling Conventions (which have nothing to
do with OP-TEE as such), we should probably prefix with SMCCC_, and have
in a shared file.

+ *
+ * This file depends on optee_msg.h being included to expand the SMC id
+ * macros below.
+ */
+
+#define OPTEE_SMC_32                   0
+#define OPTEE_SMC_64                   0x40000000
+#define OPTEE_SMC_FAST_CALL            0x80000000
+#define OPTEE_SMC_STD_CALL             0

The zero cases look a bit odd here. They might be better-documented if
you defined them with shifts, e.g.

#define SMCCC_SMC_32                    (0 << 30)
#define SMCCC_SMC_64                    (1 << 30)
#define SMCCC_FAST_CALL                 (1 << 31)
#define SMCCC_STD_CALL                  (0 << 31)

[...]

+/*
+ * Cache settings for shared memory
+ */
+#define OPTEE_SMC_SHM_NONCACHED                0ULL
+#define OPTEE_SMC_SHM_CACHED           1ULL

What precise set of memory attributes do these imply?

[...]

+/*
+ * Same values as TEE_PARAM_* from TEE Internal API
+ */
+#define OPTEEM_ATTR_TYPE_NONE          0
+#define OPTEEM_ATTR_TYPE_VALUE_INPUT   1
+#define OPTEEM_ATTR_TYPE_VALUE_OUTPUT  2
+#define OPTEEM_ATTR_TYPE_VALUE_INOUT   3
+#define OPTEEM_ATTR_TYPE_MEMREF_INPUT  5
+#define OPTEEM_ATTR_TYPE_MEMREF_OUTPUT 6
+#define OPTEEM_ATTR_TYPE_MEMREF_INOUT  7

Are these well-defined ABI values?

As mentioned previously, OPTEEM_* is opaque, and OPTEE_MSG_* would be
far clearer.

+/**
+ * struct opteem_param_memref - memory reference
+ * @buf_ptr:   Address of the buffer
+ * @size:      Size of the buffer
+ * @shm_ref:   Shared memory reference only used by normal world
+ *
+ * Secure and normal world communicates pointers as physical address
+ * instead of the virtual address. This is because secure and normal world
+ * have completely independent memory mapping. Normal world can even have a
+ * hypervisor which need to translate the guest physical address (AKA IPA
+ * in ARM documentation) to a real physical address before passing the
+ * structure to secure world.
+ */
+struct opteem_param_memref {
+       __u64 buf_ptr;
+       __u64 size;
+       __u64 shm_ref;
+};

Why does this mention physical addresses at all? What does that have to
do with userspace?

What is the shm_ref, and who allocates it?

There should really be some documentation for this.

+/**
+ * struct opteem_param_value - values
+ * @a: first value
+ * @b: second value
+ * @c: third value
+ */
+struct opteem_param_value {
+       __u64 a;
+       __u64 b;
+       __u64 c;
+};

Are OP-TEE messages defined to only ever take three parameters?

[...]

+/**
+ * struct optee_cmd_prefix - initial header for all user space buffers
+ * @func_id:   Function Id OPTEEM_FUNCID_* below
+ * @pad:       padding to make the struct size a multiple of 8 bytes
+ *
+ * This struct is 8 byte aligned since it's always followed by a struct
+ * opteem_arg which requires 8 byte alignment.
+ */
+struct opteem_cmd_prefix {
+       __u32 func_id;
+       __u32 pad __aligned(8);
+};

Shouldn't the alignment be applied to the struct as a whole rather than
the final field?

+/*
+ * Sleep mutex, helper for secure world to implement a sleeping mutex.
+ * struct opteem_arg::func     one of OPTEEM_RPC_SLEEP_MUTEX_* below
+ *
+ * OPTEEM_RPC_SLEEP_MUTEX_WAIT
+ * [in] param[0].value .a sleep mutex key
+ *                     .b wait tick
+ * [not used] param[1]
+ *
+ * OPTEEM_RPC_SLEEP_MUTEX_WAKEUP
+ * [in] param[0].value .a sleep mutex key
+ *                     .b wait after
+ * [not used] param[1]
+ *
+ * OPTEEM_RPC_SLEEP_MUTEX_DELETE
+ * [in] param[0].value .a sleep mutex key
+ * [not used] param[1]
+ */
+#define OPTEEM_RPC_SLEEP_MUTEX_WAIT    0
+#define OPTEEM_RPC_SLEEP_MUTEX_WAKEUP  1
+#define OPTEEM_RPC_SLEEP_MUTEX_DELETE  2
+#define OPTEEM_RPC_CMD_SLEEP_MUTEX     4

I'm lost. Why are mutexes exposed to userspace or the secure world in
such a manner?

Thanks,
Mark.