Hello Jens,
Here are some comments on patch V4.
*smccc_call32*
Do we only target aarch32 optee_os?
*Buffer management*
It looks that temporary buffers are now managed in the client API, not in
the driver anymore. The positive point is that it makes the driver
lighter. But the drawbacks is the number of extra ioctl used when using a
temporary buffer: 1 to allocate, 1 to map, and 1 to unmap. So a
TEEC_InvokeCommand could lead to 13 ioctls.
What about the performances? Could not it be optimized in some way? Could
we manage temporary buffers, dma buffers and "other" buffers in different
ways?
*Kernel API*
The GP API looks managed in the Client part now, without a 1-to-1 mapping
in kernel side. That means that we have lost the Kernel API, which is
useful when a given driver wants to use a TA.
It looks we are really missing this Kernel API. This is the most critical
point of our review.
*Non-zero copy*
When using RegisterSharedMemory(), it looks we do not benefit from the fact
that the original buffer was allocated using dma_buf. Hence we never have
zero-copy in the current version when using RegisterSharedMemory as we will
furher use the so-called shadowbuffer.
Can you confirm this limitation can be removed in further versions of the
patch?
By the way, we think xtest 5006, which calls TEEC_RegisterSharedBuffer()
twice may lead to memory leaks (it could be the same with the current
optee_linuxdriver).
*List of SHM*
The list of the SHM allocated is now contained in the device, whereas it
used to be in the context in optee_linuxdriver. This is not clear to us
whether this is an issue or not.
*tee_shm_pool_alloc_res_mem()*
If we'd like to use the generic function for a LX backend, we would need to
add the following arguments to the function:
- the alignment of the private region, and the one of the dma region
- a way to give the limits between both region (the size of the 1st
region, or anything else).
Emmanuel and I would like to check few other points in the proposal. We'll
come back to you if we have any other remarks.
Best regards,
Jean Michel / Emmanuel / Etienne / Pascal
Hi,
Here's a preview of the patchset I'm intending to send as PATCH v4 to
the kernel mailing lists later.
Please take a look to see that we're moving in the right direction.
The "arm/arm64: add smccc ARCH32" patch doesn't contain complicated code,
but the usage of Kconfig deserves some extra attention.
OP-TEE patches to work with this in:
https://github.com/jenswi-linaro/optee_client/tree/drv4_wiphttps://github.com/jenswi-linaro/optee_os/tree/drv4_wip
The kernel patches are available at
https://github.com/jenswi-linaro/linux/tree/gen_optee_driver4_wip
v4 preview:
* Documents TEE subsystem and OP-TEE driver
* Replaced TEE_IOC_CMD with TEE_IOC_OPEN_SESSION, TEE_IOC_INVOKE,
TEE_IOC_CANCEL and TEE_IOC_CLOSE_SESSION
* DT bindings in a separate patch
* Assembly parts moved to arch/arm and arch/arm64 respectively, in a
separate patch
* Redesigned the synchronization around entry exit of normal SMC
* Redefined/clarified the meaning of OPTEE_SMC_SHM_CACHED
* Removed CMA usage to limit the scope of the patch set
v3:
* Rebased on 4.1-rc3 (dma_buf_export() API change)
* A couple of small sparse fixes
* Documents bindings for OP-TEE driver
* Updated MAINTAINERS
v2:
* Replaced the stubbed OP-TEE driver with a real OP-TEE driver
* Removed most APIs not needed by OP-TEE in current state
* Update Documentation/ioctl/ioctl-number.txt with correct path to tee.h
* Rename tee_shm_pool_alloc_cma() to tee_shm_pool_alloc()
* Moved tee.h into include/uapi/linux/
* Redefined tee.h IOCTL macros to be directly based on _IOR and friends
* Removed version info on the API to user space, a data blob which
can contain an UUID is left for user space to be able to tell which
protocol to use in TEE_IOC_CMD
* Changed user space exposed structures to only have types with __ prefix
* Dropped THIS_MODULE from tee_fops
* Reworked how the driver is registered and ref counted:
- moved from using an embedded struct miscdevice to an embedded struct
device.
- uses an struct rw_semaphore as synchronization for driver detachment
- uses alloc/register pattern from TPM
Thanks,
Jens
Jens Wiklander (5):
arm/arm64: add smccc ARCH32
dt/bindings: add bindings for optee
tee: generic TEE subsystem
tee: add OP-TEE driver
Documentation: tee subsystem and op-tee driver
Documentation/00-INDEX | 2 +
Documentation/devicetree/bindings/optee/optee.txt | 17 +
.../devicetree/bindings/vendor-prefixes.txt | 1 +
Documentation/ioctl/ioctl-number.txt | 1 +
Documentation/tee.txt | 117 +++
MAINTAINERS | 14 +
arch/arm/Kconfig | 4 +
arch/arm/kernel/Makefile | 2 +
arch/arm/kernel/smccc-call.S | 26 +
arch/arm/kernel/smccc.c | 17 +
arch/arm64/Kconfig | 4 +
arch/arm64/kernel/Makefile | 1 +
arch/arm64/kernel/smccc-call.S | 34 +
arch/arm64/kernel/smccc.c | 17 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/tee/Kconfig | 18 +
drivers/tee/Makefile | 4 +
drivers/tee/optee/Kconfig | 8 +
drivers/tee/optee/Makefile | 5 +
drivers/tee/optee/call.c | 338 +++++++++
drivers/tee/optee/core.c | 430 +++++++++++
drivers/tee/optee/optee_msg.h | 355 +++++++++
drivers/tee/optee/optee_private.h | 131 ++++
drivers/tee/optee/optee_smc.h | 408 +++++++++++
drivers/tee/optee/rpc.c | 248 +++++++
drivers/tee/optee/supp.c | 210 ++++++
drivers/tee/tee.c | 802 +++++++++++++++++++++
drivers/tee/tee_private.h | 73 ++
drivers/tee/tee_shm.c | 328 +++++++++
drivers/tee/tee_shm_pool.c | 139 ++++
include/linux/arm-smccc.h | 80 ++
include/linux/tee_drv.h | 292 ++++++++
include/uapi/linux/tee.h | 360 +++++++++
34 files changed, 4489 insertions(+)
create mode 100644 Documentation/devicetree/bindings/optee/optee.txt
create mode 100644 Documentation/tee.txt
create mode 100644 arch/arm/kernel/smccc-call.S
create mode 100644 arch/arm/kernel/smccc.c
create mode 100644 arch/arm64/kernel/smccc-call.S
create mode 100644 arch/arm64/kernel/smccc.c
create mode 100644 drivers/tee/Kconfig
create mode 100644 drivers/tee/Makefile
create mode 100644 drivers/tee/optee/Kconfig
create mode 100644 drivers/tee/optee/Makefile
create mode 100644 drivers/tee/optee/call.c
create mode 100644 drivers/tee/optee/core.c
create mode 100644 drivers/tee/optee/optee_msg.h
create mode 100644 drivers/tee/optee/optee_private.h
create mode 100644 drivers/tee/optee/optee_smc.h
create mode 100644 drivers/tee/optee/rpc.c
create mode 100644 drivers/tee/optee/supp.c
create mode 100644 drivers/tee/tee.c
create mode 100644 drivers/tee/tee_private.h
create mode 100644 drivers/tee/tee_shm.c
create mode 100644 drivers/tee/tee_shm_pool.c
create mode 100644 include/linux/arm-smccc.h
create mode 100644 include/linux/tee_drv.h
create mode 100644 include/uapi/linux/tee.h
--
1.9.1
Hi,
There's been a massive resistance to the opaque TEE_IOC_CMD interface on
the kernel mailing lists. The last is from Mark Rutland:
https://lkml.org/lkml/2015/6/5/244
Jason Gunthorpe made some quite hard remarks in
https://lkml.org/lkml/2015/4/20/513
There hasn't been that many different people (3 or something)
criticizing the interface, but everyone has more or less asked us to
reconsider.
I think it's time to take one step back and reevaluate the consequences
of having a well defined user space API.
Consider dropping TEE_IOC_CMD and replace it with:
struct tee_open_session_arg {
__u8 uuid[UUID_LEN];
__u8 clnt_uuid[UUID_LEN];
__u32 clnt_login;
__u32 cookie; /* some unique value set by user space */
__u32 session;
__u32 ret;
__u32 ret_origin;
__u32 num_params;
};
TEE_IOC_OPEN_SESSION
struct tee_invoke_command_arg {
__u32 func;
__u32 cookie; /* some unique value set by user space */
__u32 session;
__u32 ret;
__u32 ret_origin;
__u32 num_params;
};
TEE_IOC_INVOKE_COMMAND
struct tee_cancel_command_arg {
__u32 cookie; /* matches the command or open session to cancel */
__u32 session;
__u32 ret;
__u32 ret_origin;
};
TEE_IOC_CANCEL_COMMAND
struct tee_close_session_arg {
__u32 session;
__u32 ret;
__u32 ret_origin;
};
TEE_IOC_CLOSE_SESSION
The tee_open_session_arg and tee_invoke_command_arg structs would be
followed by a struct msg_param as defined in optee_msg.h in patch v3
https://lkml.org/lkml/2015/5/15/48 .
TEE_IOC_VERSION would be changed to:
#define TEE_IMPL_ID_OPTEE 1
#define TEE_OPTEE_CAP_TZ (1 << 0)
struct tee_ioctl_version_data {
__u32 impl_id;
__u32 capabilities;
};
or equivalent.
Advantages:
1. User space gets a well defined interface
2. It will be easier to create a uniform API for the coming kernel internal
API
3. The subsystem could offload more from the driver.
Disadvantages:
1. Risk that we'll need to extend the API if a field is missing for some
TEE.
I'd like some feedback on providing a well defined user space API. Note
that the user space API shouldn't enforce a certain protocol between
normal and secure world.
--
Thanks,
Jens
