On Fri, Mar 20, 2015 at 01:44:36PM +0100, Jens Wiklander wrote:
On Fri, Mar 20, 2015 at 12:00:21PM +0100, Pascal Brand wrote:
Hello Jens,
On 19 March 2015 at 14:48, Jérôme Forissier jerome.forissier@linaro.org wrote:
Hi Jens,
On 03/19/2015 11:23 AM, Jens Wiklander wrote:
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org
Changes for V3:
- Defines the flags for struct tee_shm_alloc_data flags field
- Changed line in ioctl-number to "Generic TEE subsystem" instead
Documentation/ioctl/ioctl-number.txt | 1 + include/linux/sec-hw/tee.h | 172
+++++++++++++++++++++++++++++++++++
2 files changed, 173 insertions(+) create mode 100644 include/linux/sec-hw/tee.h
diff --git a/Documentation/ioctl/ioctl-number.txt
b/Documentation/ioctl/ioctl-number.txt
index 8136e1f..6e9bd04 100644 --- a/Documentation/ioctl/ioctl-number.txt +++ b/Documentation/ioctl/ioctl-number.txt @@ -301,6 +301,7 @@ Code Seq#(hex) Include File Comments 0xA3 80-8F Port ACL in development: mailto:tlewis@mindspring.com 0xA3 90-9F linux/dtlk.h +0xA4 00-1F linux/sec-hw/tee.h Generic TEE subsystem 0xAB 00-1F linux/nbd.h 0xAC 00-1F linux/raw.h 0xAD 00 Netfilter device in development: diff --git a/include/linux/sec-hw/tee.h b/include/linux/sec-hw/tee.h new file mode 100644 index 0000000..cbaa346 --- /dev/null +++ b/include/linux/sec-hw/tee.h @@ -0,0 +1,172 @@ +/*
- Copyright (c) 2015, Linaro Limited
- This software is licensed under the terms of the GNU General Public
- License version 2, as published by the Free Software Foundation, and
- may be copied, distributed, and modified under those terms.
- 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.
- */
+#ifndef __TEE_H +#define __TEE_H
+#include <linux/ioctl.h> +#include <linux/types.h>
+/*
- This file describes the API provided by the generic TEE driver to
user
- space
- */
+#define TEE_VERSION 1
About TEE_* names: is it OK to use them, or should we use a different prefix to avoid confusions or even clashes with GP APIs? I'd say: keep them, since we're not tied to GP, and I don't think it can be a major problem.
I would say it can create confusion. In a specific backend driver which implements GP, we may have a mixed of defines coming from the generic driver and from the GP-TEE Internal API. All will look the same.
Agree, perhaps TEE_SUBSSYS_VERSION, all the ioctl related defines TEE_IOCTL_... and finally all structs 'struct tee_ioctl...'?
+1, that makes it much clearer.
+#define TEE_SHM_MAPPED 0x1 +#define TEE_SHM_GLOBAL_DMA_BUF 0x2
What do these flags mean/where can they be used? (add comments). I'm not sure 'GLOBAL' is useful -> TEE_SHM_DMA_BUF?
+1 for TEE_SHM_DMA_BUF
+1
Also, some comments on the flag would not hurt.
OK
+/**
- struct tee_version - TEE versions
- @gen_version: Generic TEE driver version
- @spec_version: Specific TEE driver version
- @uuid: Specific TEE driver uuid, zero if not used
To make the API easier to understand, I suggest adding the direction for each parameter, like this: /**
- struct tee_version - TEE versions
- @gen_version: [out] Generic TEE driver version
- @spec_version: [out] Specific TEE driver version
- @uuid: [out] Specific TEE driver uuid, zero if not used
*/
Agree.
- Identifies the generic TEE driver, and the specific TEE driver.
- */
+struct tee_version {
uint32_t gen_version;
What about uint32_t vs __u32 an so on in things shared with user space? See Documentation/CodingStyle (and an interesting discussion here: http://yarchive.net/comp/linux/kernel_headers.html)
uint32_t spec_version;uint8_t uuid[16];+};
+/**
- struct tee_cmd_data - Opaque command argument
- @buf_ptr: A __user pointer to a command buffer
- @buf_len: Length of the buffer above
- Opaque command data which is passed on to the specific driver. The
command
- buffer doesn't have to reside in shared memory.
- */
+struct tee_cmd_data {
uint64_t buf_ptr;uint64_t buf_len;+};
+/**
- struct tee_shm_alloc_data - Shared memory allocate argument
- @size: Size of shared memory to allocate
- @flags: Flags to/from allocation, only bits flags defined by
TEE_SHM_*
above are valid, the rest should be zeroIs that TEE_SHM_MAPPED and TEE_SHM_GLOBAL_DMA_BUF?
Yes.
- @fd: dma_buf file descriptor of the shared memory
- */
+struct tee_shm_alloc_data {
uint64_t size;uint32_t flags;int32_t fd;+};
Should we have more explicit name to indicate tee_shm_alloc_data will be returned to the user normal world, whereas tee_shm is for internal use? It may help the readability of the generic driver source
There's no tee_shm at all in this file. There is one in tee_drv.h, but that file is only supposed to be used by the specific driver.
+/**
- struct tee_mem_buf - share user space memory with Secure OS
- @ptr: A __user pointer to memory to share
- @size: Size of the memory to share
- */
+struct tee_mem_buf {
uint64_t ptr;uint64_t size;+};
+/**
- struct tee_mem_dma_buf - share foreign dma_buf memory
- @fd: dma_buf file descriptor
- @pad: padding, set to zero by caller
- */
+struct tee_mem_dma_buf {
int32_t fd;uint32_t pad;+};
+/*
- Bits in struct tee_mem_share_data.flags
- */
+#define TEE_MEM_SHARE_FLAG_FOREIGN_BUFFER 0x1 /* use dma_buf
field */
Here I would use DMA_BUF in the flag name, let's avoid having too many names for the same thing. TEE_MEM_SHARE_DMA_BUF maybe?
+1
OK, let's use the same flag defines as for alloc_data above.
+/**
- struct tee_mem_share_data - share memory with Secure OS
- @buf: share user space memory
- @dma_buf: share foreign dma_buf memory
- @flags: Flags to/from sharing, unused bits set to zero by caller
- @pad: Padding, set to zero by caller
- If TEE_MEM_SHARE_FLAG_FOREIGN_BUFFER is set use the dma_buf field,
else
- the buf field in the union.
- */
+struct tee_mem_share_data {
union {struct tee_mem_buf buf;struct tee_mem_dma_buf dma_buf;};uint32_t flags;uint32_t pad;+};
Yet another Shared Memory structure ;) How could we make more explicit the name "tee_mem_share_data" (YASM will not be accepted ;) )
Perhaps we should tie the different structs close to the IOCTL defines in this file. These structs are only used to pass parameter to the ioctl in questsion. The subsystem will use different types internally.
I think that is a good idea. Another thing, I've touched it before, do we actually need to have the word "mem" everywhere? tee_shm (which clashes with name in the other h-file, so that exact name wouldn't work here), tee_buf and tee_dma_buf would be sufficient according to me.
+#define TEE_IOC_MAGIC 0xa4 +#define TEE_IOC_BASE 0
+#define _TEE_IOR(nr, size) _IOR(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr),
size)
+#define _TEE_IOWR(nr, size) _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + (nr),
size)
+/**
- TEE_IOC_VERSION - query version of drivers
- Takes a tee_version struct and returns with the version numbers
filled in.
- */
+#define TEE_IOC_VERSION _TEE_IOR(0, struct tee_version)
+/**
- TEE_IOC_CMD - pass a command to the specific TEE driver
- Takes tee_cmd_data struct which is passed to the specific TEE driver.
- */
+#define TEE_IOC_CMD _TEE_IOR(1, struct tee_cmd_data)
+/**
- TEE_IOC_SHM_ALLOC - allocate shared memory
- Allocates shared memory between the user space process and secure OS.
- The returned file descriptor is used to map the shared memory into
user
- space. The shared memory is freed when the descriptor is closed and
the
- memory is unmapped.
- */
+#define TEE_IOC_SHM_ALLOC _TEE_IOWR(2, struct tee_shm_alloc_data)
Worth to indicate which members of udata are in, out, inout. This is not obvious size and flag are inout.
Agree
+/**
- TEE_IOC_MEM_SHARE - share a portion of user space memory with secure
OS
- Shares a portion of user space memory with secure OS.
- */
+#define TEE_IOC_MEM_SHARE _TEE_IOWR(3, struct tee_mem_share_data)
+/**
- TEE_IOC_MEM_UNSHARE - unshares a portion shared user space memory
- Unshares a portion of previously shared user space memory.
- */
+#define TEE_IOC_MEM_UNSHARE _TEE_IOWR(4, struct tee_mem_share_data)
+/*
- Five syscalls are used when communicating with the generic TEE
driver.
- open(): opens the device associated with the driver
- ioctl(): as described above operating on the file descripto from
open()
- close(): two cases
- closes the device file descriptor
- closes a file descriptor connected to allocated shared memory
- mmap(): maps shared memory into user space
- munmap(): unmaps previously shared memory
- */
+#endif /*__TEE_H*/
Tee-dev mailing list Tee-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/tee-dev
Tee-dev mailing list Tee-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/tee-dev