Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on PR #16309: URL: https://github.com/apache/nuttx/pull/16309#issuecomment-2865483158 > Thanks everyone for your reviews and patience. Your contribution improve the original driver a lot, thanks too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on PR #16309: URL: https://github.com/apache/nuttx/pull/16309#issuecomment-2865334420 Thanks everyone for your reviews and patience. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 merged PR #16309: URL: https://github.com/apache/nuttx/pull/16309 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2079713761
##
drivers/misc/optee.c:
##
@@ -200,7 +522,19 @@ static int optee_open(FAR struct file *filep)
static int optee_close(FAR struct file *filep)
{
FAR struct optee_priv_data *priv = filep->f_priv;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_shm_entry *tmp;
+ while (nxrmutex_lock(&priv->lock) < 0);
Review Comment:
don't need hold lock at all since no other party can issue request to this
driver in optee_close
##
drivers/misc/optee.c:
##
@@ -594,6 +993,204 @@ static int optee_ioctl(FAR struct file *filep, int cmd,
unsigned long arg)
* Public Functions
/
+/
+ * Name: optee_va_to_pa
+ *
+ * Description:
+ * Convert the specified virtual address to a physical address. If the
+ * virtual address does not belong to the user, it is assumed to be a
+ * kernel virtual address with a 1-1 mapping and the VA is returned as-is.
+ * The VA is also returned as-is if this is a build without
+ * CONFIG_ARCH_ADDRENV.
+ *
+ * Parameters:
+ * va- The virtual address to convert.
+ *
+ * Returned Values:
+ * The physical address corresponding to the specified VA.
+ *
+ /
+
+#ifdef CONFIG_ARCH_ADDRENV
+uintptr_t optee_va_to_pa(FAR const void *va)
+{
+ FAR arch_addrenv_t *addrenv;
+ FAR struct tcb_s *tcb;
+ uintptr_t page;
+
+ tcb = nxsched_self();
+ addrenv = &tcb->addrenv_curr->addrenv;
+
+ page = up_addrenv_find_page(addrenv, (uintptr_t)va);
+ if (page == 0)
+{
+ return (uintptr_t)va;
+}
+
+ return page | ((uintptr_t)va & MM_PGMASK);
+}
+#endif
+
+/
+ * Name: optee_shm_alloc
+ *
+ * Description:
+ * Allocate, register and/or add shared memory for use with OP-TEE calls.
+ * Shared memory entry suitable for use in shared memory linked list
+ * returned in pass-by-reference `shmep` pointer. This function always
+ * allocates a shared memory entry, regardless of flags. The rest of this
+ * function's behaviour is largely determined by `flags`:
+ * - If `TEE_SHM_ALLOC` is specified, then a buffer of length `size` will
+ * be allocated. In this case `addr` will be ignored. The allocated
+ * buffer will be aligned to `priv->alignment`. `TEE_SHM_ALLOC` flag
+ * is reserved for kernel use only.
+ * - If `TEE_SHM_SEC_REGISTER` is specified, then the memory specified by
+ * `addr` or allocated through `TEE_SHM_ALLOC`, will be registered with
+ * OP-TEE as dynamic shared memory.
+ * - If `TEE_SHM_REGISTER` is specified, then the memory specified by
+ * `addr` or allocated through `TEE_SHM_ALLOC`, will be added to the
+ * driver's private data structure linked list of shared memory chunks.
+ *
+ * Use `optee_shm_free()` to undo this operation, i.e. to free,
+ * unregister, and/or remove from the list the entry returned in `shmep`
+ * and the contained buffer.
+ *
+ * Parameters:
+ * priv - The driver's private data structure
+ * addr - The address of the shared memory to register with OP-TEE and/or
+ * add to the driver's linked list of shared memory chunks.
+ * size - The size of the shared memory buffer to allocate/add/register.
+ * flags - Flags specifying the behaviour of this function. Supports
+ * combinations of `TEE_SHM_{ALLOC,REGISTER,SEC_REGISTER}`.
+ * shmep - Pass-by-reference pointer to return the shared memory entry
+ * allocated. Cannot be NULL.
+ *
+ * Returned Values:
+ * 0 on success, negative error code otherwise.
+ *
+ /
+
+int optee_shm_alloc(FAR struct optee_priv_data *priv, FAR void *addr,
+size_t size, uint32_t flags,
+FAR struct optee_shm_entry **shmep)
+{
+ FAR struct optee_shm_entry *shme;
+ FAR void *ptr;
+ int ret;
+
+ shme = kmm_zalloc(sizeof(struct optee_shm_entry));
+ if (shme == NULL)
+{
+ return -ENOMEM;
+}
+
+ if (flags & TEE_SHM_ALLOC)
+{
+ if (priv->alignment)
+{
+ ptr = kmm_memalign(priv->alignment, size);
+}
+ else
+{
+ ptr = kmm_malloc(size);
+}
+}
+ else
+{
+ ptr = addr;
+}
+
+ if (ptr == NULL)
+{
+ ret = -ENOMEM;
+ goto err;
+}
+
+ shme->shm.addr = (uintptr_t)ptr;
+ shme->shm.length = size;
+ shme->shm.flags = flags;
+
+ if (flags & TEE_SHM_SEC_REGISTER)
+{
+ ret = optee_shm_sec_register(priv, shme);
+ if (ret < 0)
+{
+ goto err;
+}
+}
+
+ if (flags & TEE_SHM_REGISTER)
+{
+ while (nxrmutex_lock(&pr
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2079738587
##
drivers/misc/optee.c:
##
@@ -594,6 +993,204 @@ static int optee_ioctl(FAR struct file *filep, int cmd,
unsigned long arg)
* Public Functions
/
+/
+ * Name: optee_va_to_pa
+ *
+ * Description:
+ * Convert the specified virtual address to a physical address. If the
+ * virtual address does not belong to the user, it is assumed to be a
+ * kernel virtual address with a 1-1 mapping and the VA is returned as-is.
+ * The VA is also returned as-is if this is a build without
+ * CONFIG_ARCH_ADDRENV.
+ *
+ * Parameters:
+ * va- The virtual address to convert.
+ *
+ * Returned Values:
+ * The physical address corresponding to the specified VA.
+ *
+ /
+
+#ifdef CONFIG_ARCH_ADDRENV
+uintptr_t optee_va_to_pa(FAR const void *va)
+{
+ FAR arch_addrenv_t *addrenv;
+ FAR struct tcb_s *tcb;
+ uintptr_t page;
+
+ tcb = nxsched_self();
+ addrenv = &tcb->addrenv_curr->addrenv;
+
+ page = up_addrenv_find_page(addrenv, (uintptr_t)va);
+ if (page == 0)
+{
+ return (uintptr_t)va;
+}
+
+ return page | ((uintptr_t)va & MM_PGMASK);
+}
+#endif
+
+/
+ * Name: optee_shm_alloc
+ *
+ * Description:
+ * Allocate, register and/or add shared memory for use with OP-TEE calls.
+ * Shared memory entry suitable for use in shared memory linked list
+ * returned in pass-by-reference `shmep` pointer. This function always
+ * allocates a shared memory entry, regardless of flags. The rest of this
+ * function's behaviour is largely determined by `flags`:
+ * - If `TEE_SHM_ALLOC` is specified, then a buffer of length `size` will
+ * be allocated. In this case `addr` will be ignored. The allocated
+ * buffer will be aligned to `priv->alignment`. `TEE_SHM_ALLOC` flag
+ * is reserved for kernel use only.
+ * - If `TEE_SHM_SEC_REGISTER` is specified, then the memory specified by
+ * `addr` or allocated through `TEE_SHM_ALLOC`, will be registered with
+ * OP-TEE as dynamic shared memory.
+ * - If `TEE_SHM_REGISTER` is specified, then the memory specified by
+ * `addr` or allocated through `TEE_SHM_ALLOC`, will be added to the
+ * driver's private data structure linked list of shared memory chunks.
+ *
+ * Use `optee_shm_free()` to undo this operation, i.e. to free,
+ * unregister, and/or remove from the list the entry returned in `shmep`
+ * and the contained buffer.
+ *
+ * Parameters:
+ * priv - The driver's private data structure
+ * addr - The address of the shared memory to register with OP-TEE and/or
+ * add to the driver's linked list of shared memory chunks.
+ * size - The size of the shared memory buffer to allocate/add/register.
+ * flags - Flags specifying the behaviour of this function. Supports
+ * combinations of `TEE_SHM_{ALLOC,REGISTER,SEC_REGISTER}`.
+ * shmep - Pass-by-reference pointer to return the shared memory entry
+ * allocated. Cannot be NULL.
+ *
+ * Returned Values:
+ * 0 on success, negative error code otherwise.
+ *
+ /
+
+int optee_shm_alloc(FAR struct optee_priv_data *priv, FAR void *addr,
+size_t size, uint32_t flags,
+FAR struct optee_shm_entry **shmep)
+{
+ FAR struct optee_shm_entry *shme;
+ FAR void *ptr;
+ int ret;
+
+ shme = kmm_zalloc(sizeof(struct optee_shm_entry));
+ if (shme == NULL)
+{
+ return -ENOMEM;
+}
+
+ if (flags & TEE_SHM_ALLOC)
+{
+ if (priv->alignment)
+{
+ ptr = kmm_memalign(priv->alignment, size);
+}
+ else
+{
+ ptr = kmm_malloc(size);
+}
+}
+ else
+{
+ ptr = addr;
+}
+
+ if (ptr == NULL)
+{
+ ret = -ENOMEM;
+ goto err;
+}
+
+ shme->shm.addr = (uintptr_t)ptr;
+ shme->shm.length = size;
+ shme->shm.flags = flags;
+
+ if (flags & TEE_SHM_SEC_REGISTER)
+{
+ ret = optee_shm_sec_register(priv, shme);
+ if (ret < 0)
+{
+ goto err;
+}
+}
+
+ if (flags & TEE_SHM_REGISTER)
+{
+ while (nxrmutex_lock(&priv->lock) < 0);
Review Comment:
Because of `optee_close()`, which iterates over the list and while doing so,
calls `optee_shm_free()`, which attempts to lock as well:
- optee_close
- lock
- loop for each
- optee_shm_free
- lock
- list_delete
- unlock
- unlock
If it weren't for `optee_smc_handle_rpc()` also calling `optee_shm_free()`,
then we would be able to get rid of the loc
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2079758107
##
drivers/misc/optee.c:
##
@@ -594,6 +993,204 @@ static int optee_ioctl(FAR struct file *filep, int cmd,
unsigned long arg)
* Public Functions
/
+/
+ * Name: optee_va_to_pa
+ *
+ * Description:
+ * Convert the specified virtual address to a physical address. If the
+ * virtual address does not belong to the user, it is assumed to be a
+ * kernel virtual address with a 1-1 mapping and the VA is returned as-is.
+ * The VA is also returned as-is if this is a build without
+ * CONFIG_ARCH_ADDRENV.
+ *
+ * Parameters:
+ * va- The virtual address to convert.
+ *
+ * Returned Values:
+ * The physical address corresponding to the specified VA.
+ *
+ /
+
+#ifdef CONFIG_ARCH_ADDRENV
+uintptr_t optee_va_to_pa(FAR const void *va)
+{
+ FAR arch_addrenv_t *addrenv;
+ FAR struct tcb_s *tcb;
+ uintptr_t page;
+
+ tcb = nxsched_self();
+ addrenv = &tcb->addrenv_curr->addrenv;
+
+ page = up_addrenv_find_page(addrenv, (uintptr_t)va);
+ if (page == 0)
+{
+ return (uintptr_t)va;
+}
+
+ return page | ((uintptr_t)va & MM_PGMASK);
+}
+#endif
+
+/
+ * Name: optee_shm_alloc
+ *
+ * Description:
+ * Allocate, register and/or add shared memory for use with OP-TEE calls.
+ * Shared memory entry suitable for use in shared memory linked list
+ * returned in pass-by-reference `shmep` pointer. This function always
+ * allocates a shared memory entry, regardless of flags. The rest of this
+ * function's behaviour is largely determined by `flags`:
+ * - If `TEE_SHM_ALLOC` is specified, then a buffer of length `size` will
+ * be allocated. In this case `addr` will be ignored. The allocated
+ * buffer will be aligned to `priv->alignment`. `TEE_SHM_ALLOC` flag
+ * is reserved for kernel use only.
+ * - If `TEE_SHM_SEC_REGISTER` is specified, then the memory specified by
+ * `addr` or allocated through `TEE_SHM_ALLOC`, will be registered with
+ * OP-TEE as dynamic shared memory.
+ * - If `TEE_SHM_REGISTER` is specified, then the memory specified by
+ * `addr` or allocated through `TEE_SHM_ALLOC`, will be added to the
+ * driver's private data structure linked list of shared memory chunks.
+ *
+ * Use `optee_shm_free()` to undo this operation, i.e. to free,
+ * unregister, and/or remove from the list the entry returned in `shmep`
+ * and the contained buffer.
+ *
+ * Parameters:
+ * priv - The driver's private data structure
+ * addr - The address of the shared memory to register with OP-TEE and/or
+ * add to the driver's linked list of shared memory chunks.
+ * size - The size of the shared memory buffer to allocate/add/register.
+ * flags - Flags specifying the behaviour of this function. Supports
+ * combinations of `TEE_SHM_{ALLOC,REGISTER,SEC_REGISTER}`.
+ * shmep - Pass-by-reference pointer to return the shared memory entry
+ * allocated. Cannot be NULL.
+ *
+ * Returned Values:
+ * 0 on success, negative error code otherwise.
+ *
+ /
+
+int optee_shm_alloc(FAR struct optee_priv_data *priv, FAR void *addr,
+size_t size, uint32_t flags,
+FAR struct optee_shm_entry **shmep)
+{
+ FAR struct optee_shm_entry *shme;
+ FAR void *ptr;
+ int ret;
+
+ shme = kmm_zalloc(sizeof(struct optee_shm_entry));
+ if (shme == NULL)
+{
+ return -ENOMEM;
+}
+
+ if (flags & TEE_SHM_ALLOC)
+{
+ if (priv->alignment)
+{
+ ptr = kmm_memalign(priv->alignment, size);
+}
+ else
+{
+ ptr = kmm_malloc(size);
+}
+}
+ else
+{
+ ptr = addr;
+}
+
+ if (ptr == NULL)
+{
+ ret = -ENOMEM;
+ goto err;
+}
+
+ shme->shm.addr = (uintptr_t)ptr;
+ shme->shm.length = size;
+ shme->shm.flags = flags;
+
+ if (flags & TEE_SHM_SEC_REGISTER)
+{
+ ret = optee_shm_sec_register(priv, shme);
+ if (ret < 0)
+{
+ goto err;
+}
+}
+
+ if (flags & TEE_SHM_REGISTER)
+{
+ while (nxrmutex_lock(&priv->lock) < 0);
Review Comment:
you don't need protect the concurrent access in optee_close, since it is
impossible other thread can enter any optee_xxx function after optee_close get
called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this s
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2079759628
##
drivers/misc/optee.h:
##
@@ -44,8 +46,17 @@
* Public Types
/
+struct optee_shm_entry
+{
+ struct list_node node;
+ struct tee_ioctl_shm_register_data shm;
+};
+
struct optee_priv_data
{
+ uintptr_t alignment;/* Transport-specified message alignment */
+ struct list_node shm_list; /* A list of all shm registered */
+ rmutex_t lock; /* Mutex used to guard list accesses */
Review Comment:
let' remove the protection from optee_close, so you can use the simple
spinlock
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2079753290
##
drivers/misc/optee.c:
##
@@ -200,7 +522,19 @@ static int optee_open(FAR struct file *filep)
static int optee_close(FAR struct file *filep)
{
FAR struct optee_priv_data *priv = filep->f_priv;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_shm_entry *tmp;
+ while (nxrmutex_lock(&priv->lock) < 0);
Review Comment:
no, the close only happen when all handle get closed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2079727596
##
drivers/misc/optee.c:
##
@@ -200,7 +522,19 @@ static int optee_open(FAR struct file *filep)
static int optee_close(FAR struct file *filep)
{
FAR struct optee_priv_data *priv = filep->f_priv;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_shm_entry *tmp;
+ while (nxrmutex_lock(&priv->lock) < 0);
Review Comment:
Can't it be issued by another thread of the same task group?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2079744696
##
drivers/misc/optee.h:
##
@@ -44,8 +46,17 @@
* Public Types
/
+struct optee_shm_entry
+{
+ struct list_node node;
+ struct tee_ioctl_shm_register_data shm;
+};
+
struct optee_priv_data
{
+ uintptr_t alignment;/* Transport-specified message alignment */
+ struct list_node shm_list; /* A list of all shm registered */
+ rmutex_t lock; /* Mutex used to guard list accesses */
Review Comment:
We would need to resolve the recursion to do that. The only (somewhat)
reasonable way I can think of is to give up thread-safety for
`optee_shm_free()`, and delegate responsibility to synchronize to the caller,
which I considered sub-optimal, and prone-to-error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2079738587
##
drivers/misc/optee.c:
##
@@ -594,6 +993,204 @@ static int optee_ioctl(FAR struct file *filep, int cmd,
unsigned long arg)
* Public Functions
/
+/
+ * Name: optee_va_to_pa
+ *
+ * Description:
+ * Convert the specified virtual address to a physical address. If the
+ * virtual address does not belong to the user, it is assumed to be a
+ * kernel virtual address with a 1-1 mapping and the VA is returned as-is.
+ * The VA is also returned as-is if this is a build without
+ * CONFIG_ARCH_ADDRENV.
+ *
+ * Parameters:
+ * va- The virtual address to convert.
+ *
+ * Returned Values:
+ * The physical address corresponding to the specified VA.
+ *
+ /
+
+#ifdef CONFIG_ARCH_ADDRENV
+uintptr_t optee_va_to_pa(FAR const void *va)
+{
+ FAR arch_addrenv_t *addrenv;
+ FAR struct tcb_s *tcb;
+ uintptr_t page;
+
+ tcb = nxsched_self();
+ addrenv = &tcb->addrenv_curr->addrenv;
+
+ page = up_addrenv_find_page(addrenv, (uintptr_t)va);
+ if (page == 0)
+{
+ return (uintptr_t)va;
+}
+
+ return page | ((uintptr_t)va & MM_PGMASK);
+}
+#endif
+
+/
+ * Name: optee_shm_alloc
+ *
+ * Description:
+ * Allocate, register and/or add shared memory for use with OP-TEE calls.
+ * Shared memory entry suitable for use in shared memory linked list
+ * returned in pass-by-reference `shmep` pointer. This function always
+ * allocates a shared memory entry, regardless of flags. The rest of this
+ * function's behaviour is largely determined by `flags`:
+ * - If `TEE_SHM_ALLOC` is specified, then a buffer of length `size` will
+ * be allocated. In this case `addr` will be ignored. The allocated
+ * buffer will be aligned to `priv->alignment`. `TEE_SHM_ALLOC` flag
+ * is reserved for kernel use only.
+ * - If `TEE_SHM_SEC_REGISTER` is specified, then the memory specified by
+ * `addr` or allocated through `TEE_SHM_ALLOC`, will be registered with
+ * OP-TEE as dynamic shared memory.
+ * - If `TEE_SHM_REGISTER` is specified, then the memory specified by
+ * `addr` or allocated through `TEE_SHM_ALLOC`, will be added to the
+ * driver's private data structure linked list of shared memory chunks.
+ *
+ * Use `optee_shm_free()` to undo this operation, i.e. to free,
+ * unregister, and/or remove from the list the entry returned in `shmep`
+ * and the contained buffer.
+ *
+ * Parameters:
+ * priv - The driver's private data structure
+ * addr - The address of the shared memory to register with OP-TEE and/or
+ * add to the driver's linked list of shared memory chunks.
+ * size - The size of the shared memory buffer to allocate/add/register.
+ * flags - Flags specifying the behaviour of this function. Supports
+ * combinations of `TEE_SHM_{ALLOC,REGISTER,SEC_REGISTER}`.
+ * shmep - Pass-by-reference pointer to return the shared memory entry
+ * allocated. Cannot be NULL.
+ *
+ * Returned Values:
+ * 0 on success, negative error code otherwise.
+ *
+ /
+
+int optee_shm_alloc(FAR struct optee_priv_data *priv, FAR void *addr,
+size_t size, uint32_t flags,
+FAR struct optee_shm_entry **shmep)
+{
+ FAR struct optee_shm_entry *shme;
+ FAR void *ptr;
+ int ret;
+
+ shme = kmm_zalloc(sizeof(struct optee_shm_entry));
+ if (shme == NULL)
+{
+ return -ENOMEM;
+}
+
+ if (flags & TEE_SHM_ALLOC)
+{
+ if (priv->alignment)
+{
+ ptr = kmm_memalign(priv->alignment, size);
+}
+ else
+{
+ ptr = kmm_malloc(size);
+}
+}
+ else
+{
+ ptr = addr;
+}
+
+ if (ptr == NULL)
+{
+ ret = -ENOMEM;
+ goto err;
+}
+
+ shme->shm.addr = (uintptr_t)ptr;
+ shme->shm.length = size;
+ shme->shm.flags = flags;
+
+ if (flags & TEE_SHM_SEC_REGISTER)
+{
+ ret = optee_shm_sec_register(priv, shme);
+ if (ret < 0)
+{
+ goto err;
+}
+}
+
+ if (flags & TEE_SHM_REGISTER)
+{
+ while (nxrmutex_lock(&priv->lock) < 0);
Review Comment:
> why need the recursive lock?
Because of `optee_close()`, which iterates over the list and while doing so,
calls `optee_shm_free()`, which attempts to lock as well:
- optee_close
- lock
- loop for each
- optee_shm_free
- lock
- list_delete
- unlock
- unlock
If it weren't for `optee_smc_handle_rpc()` also calling `optee_shm_free()`,
then
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2079727596
##
drivers/misc/optee.c:
##
@@ -200,7 +522,19 @@ static int optee_open(FAR struct file *filep)
static int optee_close(FAR struct file *filep)
{
FAR struct optee_priv_data *priv = filep->f_priv;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_shm_entry *tmp;
+ while (nxrmutex_lock(&priv->lock) < 0);
Review Comment:
> don't need hold lock at all since no other party can issue request to this
driver in optee_close
Can't it be issued by another thread of the same task group?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#issuecomment-2862951631
> > @gpoulios why mark the patch to draft?
>
> 3 reasons:
>
> * I don't see this getting @raiden00pl 's approval so I thought I'd
try re-writing some parts of the code but it turns out I can't. On the
contrary, I'm thinking of putting back `reg_pair_to_64` and co. as they are
[actually licensed under
BSD](https://github.com/tiiuae/imx-optee-os/blob/8dd180b6d149c1e1314b5869697179f665bd9ca3/lib/libutils/ext/include/util.h#L169).
>
> * I need to guard all list accesses around a mutex. Btw,
@xiaoxiang781216 would you rather see this as a new commit or an amendment to
the existing one?
>
both are fine, if the defect in the unmerged code, it's better to fix the
problem direclty.
> * I noticed the IDs in
`tee_ioctl_shm_{alloc,register,register_fd}_data` are _all_ output parameters
(whereas I was using `rdata.id` as input):
>
>
>
https://github.com/apache/nuttx/blob/468c9eacd8fd30817cd683a07a10ead8d2652a0a/include/nuttx/tee.h#L113
>
>
https://github.com/apache/nuttx/blob/468c9eacd8fd30817cd683a07a10ead8d2652a0a/include/nuttx/tee.h#L145
>
>
https://github.com/apache/nuttx/blob/468c9eacd8fd30817cd683a07a10ead8d2652a0a/include/nuttx/tee.h#L393
>
> @xiaoxiang781216 Do we really need those? I'm having a hard time
generating unique IDs for shared memory registrations. Thinking of the
following options:
>
> 1. casting the actual buffer address to int32
>
>* not guaranteed to be unique with 64-bit addresses
>
> 2. traversing the list to find a free integer ID starting from
`INT32_MIN`
>
>* needs one full list traversal at best, multiple if we've done
`UIN32_MAX` allocations-deallocations; Without deallocations we don't expect
IDs to overflow as we will be out of memory much sooner
>
> 3. using list head's ID + 1
>
>* we need to stick to adding new entries at the head of the list
and might overflow sooner, but is efficient
>
> 4. using a `shm_nextid` field in `struct optee_priv_data`
>
>* need to guard with mutex (which I'm also doing for list accesses
right now, so not such a big deal)
>
>
> Leaning towards 3, but would appreciate your thoughts on this.
>
Yes, method 3 is the simple and good enough.
> On the contrary, points for removing the IDs entirely:
>
> * we can also remove this (correct me if I'm wrong)
https://github.com/apache/nuttx/blob/1a8fba827a73bb842a60f86b610470c2f38ad4f4/drivers/misc/optee.c#L586
>
> * it's not like we can prevent duplicate list entries anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on PR #16309: URL: https://github.com/apache/nuttx/pull/16309#issuecomment-2862772230 Summary of changes in order of commit log: - **sanity**: return `-EINVAL` (instead of `-EACCESS`) for all failures of `optee_is_valid_range()` - **shm**: add `static` to `optee_shm_to_page_list()` - **shm**: guard all accesses of `shm_list` with a mutex - **smc**: put back `reg_pair_to_uintptr()/reg_pair_from_64()` as they are taken [from BSD-licensed code] - **doco**: correct usage of ID during shm registration (should have been used as output) (https://github.com/tiiuae/imx-optee-os/blob/8dd180b6d149c1e1314b5869697179f665bd9ca3/lib/libutils/ext/include/util.h#L169) CI failure appears to be a problem with the CI, not the PR, correct? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#issuecomment-2862471447
And one more thing regarding mutexes: I see half of the code base checks
return codes of `nx{r}mutex_lock()`, half doesn't, and some outliers do `while
(nxmutex_lock(&lock) < 0);`. I would prefer to do the later, but what is the
recommended approach @xiaoxiang781216?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on PR #16309: URL: https://github.com/apache/nuttx/pull/16309#issuecomment-2862158650 > @gpoulios why mark the patch to draft? 3 reasons: - I don't see this getting @raiden00pl 's approval so I thought I'd try re-writing some parts of the code but it turns out I can't. On the contrary, I'm thinking of putting back `reg_pair_to_64` and co. as they are [actually licensed under BSD](https://github.com/tiiuae/imx-optee-os/blob/8dd180b6d149c1e1314b5869697179f665bd9ca3/lib/libutils/ext/include/util.h#L169). - I need to guard all list accesses around a mutex. Btw, @xiaoxiang781216 would you rather see this as a new commit or an amendment to the existing one? - I noticed the IDs in `tee_ioctl_shm_alloc_data`, `tee_ioctl_shm_register_fd_data`, and `tee_ioctl_shm_register_data` are _all_ output parameters (whereas I was using `rdata.id` as input): https://github.com/apache/nuttx/blob/468c9eacd8fd30817cd683a07a10ead8d2652a0a/include/nuttx/tee.h#L113 https://github.com/apache/nuttx/blob/468c9eacd8fd30817cd683a07a10ead8d2652a0a/include/nuttx/tee.h#L145 https://github.com/apache/nuttx/blob/468c9eacd8fd30817cd683a07a10ead8d2652a0a/include/nuttx/tee.h#L393 @xiaoxiang781216 Do we really need those? I'm having a hard time generating unique IDs for shared memory registrations. Thinking of the following options: 1. casting the actual buffer address to int32 - not guaranteed to be unique, either because of 64-bit addresses or due to overall between user and kernel VA space 2. traversing the list to find a free integer ID starting from 0 - needs at least one list traversal on best case, more if `MAX_INT32` has been reached 3. using list head's ID + 1 - we need to stick to adding new entries at the head of the list but is efficient 4. using a `shm_nextid` field in `struct optee_priv_data` - need to guard with mutex (which I'm also doing for list accesses right now, so not such a big deal) Leaning towards 3, but would appreciate your thoughts on this. On the contrary, points for removing the IDs entirely: - we can also remove this (correct me if I'm wrong) https://github.com/apache/nuttx/blob/1a8fba827a73bb842a60f86b610470c2f38ad4f4/drivers/misc/optee.c#L586 - it's not like we can prevent duplicate list entries anyways (especially given we have both user and kernel registrations). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on PR #16309: URL: https://github.com/apache/nuttx/pull/16309#issuecomment-2862125127 @gpoulios why mark the patch to draft? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2078061212
##
drivers/misc/optee.c:
##
@@ -55,21 +57,87 @@
/* Some GlobalPlatform error codes used in this driver */
#define TEE_SUCCESS0x
+#define TEE_ERROR_ACCESS_DENIED0x0001
+#define TEE_ERROR_BAD_FORMAT 0x0005
#define TEE_ERROR_BAD_PARAMETERS 0x0006
#define TEE_ERROR_NOT_SUPPORTED0x000A
-#define TEE_ERROR_COMMUNICATION0x000E
#define TEE_ERROR_OUT_OF_MEMORY0x000C
#define TEE_ERROR_BUSY 0x000D
+#define TEE_ERROR_COMMUNICATION0x000E
+#define TEE_ERROR_SECURITY 0x000F
#define TEE_ERROR_SHORT_BUFFER 0x0010
+#define TEE_ERROR_TIMEOUT 0x3001
#define TEE_ORIGIN_COMMS 0x0002
#define OPTEE_DEV_PATH "/dev/tee0"
+/* According to optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG */
+
+#define OPTEE_PAGES_ARRAY_LEN \
+((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(uint64_t)) - 1)
+
+/* Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `numparams` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `optee_msg_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - pointer to the driver's optee_priv_data struct
+ * numparams - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified
+ * number of parameters.
+ * On failure, NULL.
+ */
+
+#define optee_msg_alloc(priv, numparams) \
+ ({ \
Review Comment:
> What about this?
>
>
https://github.com/apache/nuttx/blob/358469e5bbb5e950c8c0b563c761213ececd/include/nuttx/list.h#L160
>
> Other compiler is blocked anyways
Yes, this is few place that we can't find a method to remove the expression
statement. Since this is public API well defined by Linux/LittleKernel, we have
to keep the prototype as other's. In your case, it's an internal API, it can be
fixed easily by adjusting the function prototype.
> That’s your code in list.h @xiaoxiang781216.
>
> Why is it allowed there but not here?
Anyway, we will try our best to remove this special usage as much as
possible. If you have a good solution, let's fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077933601
##
drivers/misc/optee.c:
##
@@ -55,21 +57,87 @@
/* Some GlobalPlatform error codes used in this driver */
#define TEE_SUCCESS0x
+#define TEE_ERROR_ACCESS_DENIED0x0001
+#define TEE_ERROR_BAD_FORMAT 0x0005
#define TEE_ERROR_BAD_PARAMETERS 0x0006
#define TEE_ERROR_NOT_SUPPORTED0x000A
-#define TEE_ERROR_COMMUNICATION0x000E
#define TEE_ERROR_OUT_OF_MEMORY0x000C
#define TEE_ERROR_BUSY 0x000D
+#define TEE_ERROR_COMMUNICATION0x000E
+#define TEE_ERROR_SECURITY 0x000F
#define TEE_ERROR_SHORT_BUFFER 0x0010
+#define TEE_ERROR_TIMEOUT 0x3001
#define TEE_ORIGIN_COMMS 0x0002
#define OPTEE_DEV_PATH "/dev/tee0"
+/* According to optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG */
+
+#define OPTEE_PAGES_ARRAY_LEN \
+((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(uint64_t)) - 1)
+
+/* Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `numparams` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `optee_msg_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - pointer to the driver's optee_priv_data struct
+ * numparams - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified
+ * number of parameters.
+ * On failure, NULL.
+ */
+
+#define optee_msg_alloc(priv, numparams) \
+ ({ \
Review Comment:
That’s your code in list.h @xiaoxiang781216.
Why is it allowed there but not here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077895877
##
drivers/misc/optee.c:
##
@@ -55,21 +57,87 @@
/* Some GlobalPlatform error codes used in this driver */
#define TEE_SUCCESS0x
+#define TEE_ERROR_ACCESS_DENIED0x0001
+#define TEE_ERROR_BAD_FORMAT 0x0005
#define TEE_ERROR_BAD_PARAMETERS 0x0006
#define TEE_ERROR_NOT_SUPPORTED0x000A
-#define TEE_ERROR_COMMUNICATION0x000E
#define TEE_ERROR_OUT_OF_MEMORY0x000C
#define TEE_ERROR_BUSY 0x000D
+#define TEE_ERROR_COMMUNICATION0x000E
+#define TEE_ERROR_SECURITY 0x000F
#define TEE_ERROR_SHORT_BUFFER 0x0010
+#define TEE_ERROR_TIMEOUT 0x3001
#define TEE_ORIGIN_COMMS 0x0002
#define OPTEE_DEV_PATH "/dev/tee0"
+/* According to optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG */
+
+#define OPTEE_PAGES_ARRAY_LEN \
+((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(uint64_t)) - 1)
+
+/* Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `numparams` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `optee_msg_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - pointer to the driver's optee_priv_data struct
+ * numparams - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified
+ * number of parameters.
+ * On failure, NULL.
+ */
+
+#define optee_msg_alloc(priv, numparams) \
+ ({ \
Review Comment:
the expression statement is gcc extension, which mayn't support by other c
compiler, that's why I suggest pass msg as macro parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077908265
##
drivers/misc/optee.c:
##
@@ -154,6 +202,245 @@ static bool optee_is_valid_range(FAR const void *va,
size_t size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
Review Comment:
Right
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077905466
##
drivers/misc/optee.c:
##
@@ -55,21 +57,87 @@
/* Some GlobalPlatform error codes used in this driver */
#define TEE_SUCCESS0x
+#define TEE_ERROR_ACCESS_DENIED0x0001
+#define TEE_ERROR_BAD_FORMAT 0x0005
#define TEE_ERROR_BAD_PARAMETERS 0x0006
#define TEE_ERROR_NOT_SUPPORTED0x000A
-#define TEE_ERROR_COMMUNICATION0x000E
#define TEE_ERROR_OUT_OF_MEMORY0x000C
#define TEE_ERROR_BUSY 0x000D
+#define TEE_ERROR_COMMUNICATION0x000E
+#define TEE_ERROR_SECURITY 0x000F
#define TEE_ERROR_SHORT_BUFFER 0x0010
+#define TEE_ERROR_TIMEOUT 0x3001
#define TEE_ORIGIN_COMMS 0x0002
#define OPTEE_DEV_PATH "/dev/tee0"
+/* According to optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG */
+
+#define OPTEE_PAGES_ARRAY_LEN \
+((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(uint64_t)) - 1)
+
+/* Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `numparams` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `optee_msg_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - pointer to the driver's optee_priv_data struct
+ * numparams - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified
+ * number of parameters.
+ * On failure, NULL.
+ */
+
+#define optee_msg_alloc(priv, numparams) \
+ ({ \
Review Comment:
What about this?
https://github.com/apache/nuttx/blob/358469e5bbb5e950c8c0b563c761213ececd/include/nuttx/list.h#L160
Other compiler is blocked anyways
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077901273
##
drivers/misc/optee.c:
##
@@ -154,6 +202,245 @@ static bool optee_is_valid_range(FAR const void *va,
size_t size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
Review Comment:
alloca should always return the buffer align to the machine word at least,
so it's should be `>` to switch to malloc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077496878
##
drivers/misc/optee.c:
##
@@ -154,6 +202,245 @@ static bool optee_is_valid_range(FAR const void *va,
size_t size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
Review Comment:
sure, though I think it makes more sense to do:
```
if (priv->alignment >= sizeof(uintptr_t))
```
right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077150237
##
drivers/misc/optee.c:
##
@@ -154,6 +202,245 @@ static bool optee_is_valid_range(FAR const void *va,
size_t size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
+{
+ msg = kmm_memalign(priv->alignment,
+ OPTEE_MSG_GET_ARG_SIZE(num_params));
+}
+ else
+{
+ msg = kmm_malloc(OPTEE_MSG_GET_ARG_SIZE(num_params));
Review Comment:
something like this:
```
#define optee_msg_alloc(priv, uint32_t num_params, msg) \
do
{
if ((priv)->alignment)
{
(msg) = kmm_memalign((priv)->alignment,
OPTEE_MSG_GET_ARG_SIZE(num_params));
}
else
{
(msg) = alloca(OPTEE_MSG_GET_ARG_SIZE(num_params));
}
if (msg)
{
memset(msg, 0, OPTEE_MSG_GET_ARG_SIZE(num_params));
msg->num_params = num_params;
}
}
while (0)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077032401
##
drivers/misc/optee.c:
##
@@ -154,6 +202,245 @@ static bool optee_is_valid_range(FAR const void *va,
size_t size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
+{
+ msg = kmm_memalign(priv->alignment,
+ OPTEE_MSG_GET_ARG_SIZE(num_params));
+}
+ else
+{
+ msg = kmm_malloc(OPTEE_MSG_GET_ARG_SIZE(num_params));
Review Comment:
How are we going to "change optee_msg_alloc to macro" based on a runtime
check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077026793
##
drivers/misc/optee.c:
##
@@ -154,6 +202,245 @@ static bool optee_is_valid_range(FAR const void *va,
size_t size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
+{
+ msg = kmm_memalign(priv->alignment,
+ OPTEE_MSG_GET_ARG_SIZE(num_params));
+}
+ else
+{
+ msg = kmm_malloc(OPTEE_MSG_GET_ARG_SIZE(num_params));
Review Comment:
`priv->alignment <= sizeof(uintptr_t)` or even `priv->alignment == 0`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2077000910
##
drivers/misc/optee.c:
##
@@ -154,6 +202,245 @@ static bool optee_is_valid_range(FAR const void *va,
size_t size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
+{
+ msg = kmm_memalign(priv->alignment,
+ OPTEE_MSG_GET_ARG_SIZE(num_params));
+}
+ else
+{
+ msg = kmm_malloc(OPTEE_MSG_GET_ARG_SIZE(num_params));
Review Comment:
Define "simple case"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076634069
##
drivers/misc/optee.c:
##
@@ -70,7 +70,7 @@
#define TEE_ORIGIN_COMMS 0x0002
-#define OPTEE_DEV_PATH "/dev/tee0"
+#define OPTEE_DEV_PATH "/dev/tee0"
Review Comment:
let's move to the first patch too
##
drivers/misc/optee.h:
##
@@ -27,9 +27,11 @@
* Included Files
/
+#include
Review Comment:
remove, don't need now
##
drivers/misc/optee.c:
##
@@ -154,6 +202,245 @@ static bool optee_is_valid_range(FAR const void *va,
size_t size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
+{
+ msg = kmm_memalign(priv->alignment,
+ OPTEE_MSG_GET_ARG_SIZE(num_params));
+}
+ else
+{
+ msg = kmm_malloc(OPTEE_MSG_GET_ARG_SIZE(num_params));
Review Comment:
could we change to alloca here to avoid the dynamic allocation in the simple
case? of course, we may need change optee_msg_alloc to macro and add a simple
optee_msg_free macro to skip free in the simple case.
##
drivers/misc/optee.c:
##
@@ -154,6 +202,245 @@ static bool optee_is_valid_range(FAR const void *va,
size_t size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
Review Comment:
we can optimize to:
```
if (priv->alignment > sizeof(uintptr_t))
```
##
drivers/misc/optee_smc.c:
##
@@ -0,0 +1,338 @@
+/
+ * drivers/misc/optee_smc.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+#include
+
+#include "optee.h"
+#include "optee_s
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076606320
##
drivers/misc/optee.c:
##
@@ -125,52 +115,24 @@ static const struct file_operations g_optee_ops =
static int optee_open(FAR struct file *filep)
{
- FAR struct socket *psock;
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- struct sockaddr_un addr;
-#else
- struct sockaddr_rpmsg addr;
-#endif
+ FAR struct optee_priv_data *priv;
int ret;
- psock = (FAR struct socket *)kmm_zalloc(sizeof(struct socket));
- if (psock == NULL)
+ priv = (FAR struct optee_priv_data *)
+kmm_zalloc(sizeof(struct optee_priv_data));
+ if (priv == NULL)
{
return -ENOMEM;
}
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- ret = psock_socket(AF_UNIX, SOCK_STREAM, 0, psock);
-#else
- ret = psock_socket(AF_RPMSG, SOCK_STREAM, 0, psock);
-#endif
+ ret = optee_transport_open(priv);
if (ret < 0)
{
- kmm_free(psock);
Review Comment:
Ok, it's no problem now with the new one time allocation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076605132 ## include/nuttx/tee.h: ## @@ -378,6 +378,13 @@ struct tee_iocl_supp_send_arg #define TEE_IOC_SUPPL_SEND _IOC(TEE_IOC_MAGIC << 8, TEE_IOC_BASE + 7) +/* Shared memory specific defines */ + +#define TEE_SHM_REGISTER(1 << 0) /* In list of shared memory */ +#define TEE_SHM_SEC_REGISTER(1 << 1) /* TEE notified of this memory */ Review Comment: like this: `#define TEE_SHM_SEC_REGISTER (1 << 1) /* TEE notified of this memory */` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076604374
##
drivers/misc/optee.c:
##
@@ -125,52 +115,24 @@ static const struct file_operations g_optee_ops =
static int optee_open(FAR struct file *filep)
{
- FAR struct socket *psock;
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- struct sockaddr_un addr;
-#else
- struct sockaddr_rpmsg addr;
-#endif
+ FAR struct optee_priv_data *priv;
int ret;
- psock = (FAR struct socket *)kmm_zalloc(sizeof(struct socket));
- if (psock == NULL)
+ priv = (FAR struct optee_priv_data *)
+kmm_zalloc(sizeof(struct optee_priv_data));
+ if (priv == NULL)
{
return -ENOMEM;
}
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- ret = psock_socket(AF_UNIX, SOCK_STREAM, 0, psock);
-#else
- ret = psock_socket(AF_RPMSG, SOCK_STREAM, 0, psock);
-#endif
+ ret = optee_transport_open(priv);
if (ret < 0)
{
- kmm_free(psock);
Review Comment:
but this is fail path, optee_close/optee_transport_close doesn't have any
chance to run
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076570540
##
drivers/misc/optee.c:
##
@@ -154,6 +202,248 @@ static bool optee_is_valid_range(FAR void *va, size_t
size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
+{
+ msg = kmm_memalign(priv->alignment,
+ OPTEE_MSG_GET_ARG_SIZE(num_params));
+}
+ else
+{
+ msg = kmm_malloc(OPTEE_MSG_GET_ARG_SIZE(num_params));
+}
+
+ if (msg == NULL)
+{
+ return NULL;
+}
+
+ memset(msg, 0, OPTEE_MSG_GET_ARG_SIZE(num_params));
+ msg->num_params = num_params;
+
+ return msg;
+}
+
+/
+ * Name: optee_shm_to_page_list
+ *
+ * Description:
+ * Provide a page list of a shared memory buffer. Secure world doesn't
+ * know about the address environment mapping of NuttX, so physical
+ * pointers are needed when sharing memory. This implementation enables
+ * sharing of physically non-contiguous buffers according to
+ * optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG.
+ * Each entry in the generated page list is an array of the physical,
+ * potentially non-contiguous pages making up the actual buffer to
+ * represent. Note that this representation does not account for the page
+ * offset of the shared memory buffer. The offset is encoded in the
+ * physical address returned in `list_pa`.
+ *
+ * Parameters:
+ * shme- Shared memory entry to create a page list for.
+ * list_pa - If not NULL, will be set to the page list's physical
+ * address (which is aligned to
+ * OPTEE_MSG_NONCONTIG_PAGE_SIZE) added with shared memory
+ * page offset.
+ *
+ * Returned Values:
+ * A pointer to the kernel virtual address of the page list on success.
+ * NULL on failure. Caller responsible to free the returned list using
+ * `kmm_free()`.
+ *
+ /
+
+FAR void *optee_shm_to_page_list(FAR struct optee_shm_entry *shme,
+ FAR uintptr_t *list_pa)
+{
+ size_t pgsize = OPTEE_MSG_NONCONTIG_PAGE_SIZE;
+ uintptr_t pgoff;
+ uintptr_t page;
+ uint32_t total_pages;
+ uint32_t list_size;
+ FAR void *list;
+ FAR struct optee_page_list_entry *list_entry;
+ uint32_t i = 0;
Review Comment:
Can you explain the reasoning?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076562339 ## drivers/misc/optee.h: ## @@ -64,6 +72,16 @@ extern "C" #define EXTERN extern #endif +#ifdef CONFIG_ARCH_ADDRENV +uintptr_t optee_va_to_pa(FAR void *va); +#else +# define optee_va_to_pa(va) ((uintptr_t)va) +#endif +int optee_shm_alloc(FAR struct optee_priv_data *priv, bool align, Review Comment: The idea was to allow allocation of shm without alignment (registered shm doesn't have to be aligned). But we don't use it anywhere (would be kernel-only / user allocates through memfd or other), so I'll remove it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076288814
##
drivers/misc/optee.c:
##
@@ -125,52 +115,24 @@ static const struct file_operations g_optee_ops =
static int optee_open(FAR struct file *filep)
{
- FAR struct socket *psock;
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- struct sockaddr_un addr;
-#else
- struct sockaddr_rpmsg addr;
-#endif
+ FAR struct optee_priv_data *priv;
int ret;
- psock = (FAR struct socket *)kmm_zalloc(sizeof(struct socket));
- if (psock == NULL)
+ priv = (FAR struct optee_priv_data *)
+kmm_zalloc(sizeof(struct optee_priv_data));
+ if (priv == NULL)
{
return -ENOMEM;
}
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- ret = psock_socket(AF_UNIX, SOCK_STREAM, 0, psock);
-#else
- ret = psock_socket(AF_RPMSG, SOCK_STREAM, 0, psock);
-#endif
+ ret = optee_transport_open(priv);
if (ret < 0)
{
- kmm_free(psock);
Review Comment:
??? `kmm_free(priv)` is in `open_close()`... and will be moved into
`optee_transport_close()` as per your last suggestion to use `priv_`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076288814
##
drivers/misc/optee.c:
##
@@ -125,52 +115,24 @@ static const struct file_operations g_optee_ops =
static int optee_open(FAR struct file *filep)
{
- FAR struct socket *psock;
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- struct sockaddr_un addr;
-#else
- struct sockaddr_rpmsg addr;
-#endif
+ FAR struct optee_priv_data *priv;
int ret;
- psock = (FAR struct socket *)kmm_zalloc(sizeof(struct socket));
- if (psock == NULL)
+ priv = (FAR struct optee_priv_data *)
+kmm_zalloc(sizeof(struct optee_priv_data));
+ if (priv == NULL)
{
return -ENOMEM;
}
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- ret = psock_socket(AF_UNIX, SOCK_STREAM, 0, psock);
-#else
- ret = psock_socket(AF_RPMSG, SOCK_STREAM, 0, psock);
-#endif
+ ret = optee_transport_open(priv);
if (ret < 0)
{
- kmm_free(psock);
Review Comment:
??? `kmm_free(priv)` is in `open_close()`... and will be moved into
`open_transport_close()` as per your last suggestion to use `priv_`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076301484 ## include/nuttx/tee.h: ## @@ -378,6 +378,13 @@ struct tee_iocl_supp_send_arg #define TEE_IOC_SUPPL_SEND _IOC(TEE_IOC_MAGIC << 8, TEE_IOC_BASE + 7) +/* Shared memory specific defines */ + +#define TEE_SHM_REGISTER(1 << 0) /* In list of shared memory */ +#define TEE_SHM_SEC_REGISTER(1 << 1) /* TEE notified of this memory */ Review Comment: what extra spaces? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2076288814
##
drivers/misc/optee.c:
##
@@ -125,52 +115,24 @@ static const struct file_operations g_optee_ops =
static int optee_open(FAR struct file *filep)
{
- FAR struct socket *psock;
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- struct sockaddr_un addr;
-#else
- struct sockaddr_rpmsg addr;
-#endif
+ FAR struct optee_priv_data *priv;
int ret;
- psock = (FAR struct socket *)kmm_zalloc(sizeof(struct socket));
- if (psock == NULL)
+ priv = (FAR struct optee_priv_data *)
+kmm_zalloc(sizeof(struct optee_priv_data));
+ if (priv == NULL)
{
return -ENOMEM;
}
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- ret = psock_socket(AF_UNIX, SOCK_STREAM, 0, psock);
-#else
- ret = psock_socket(AF_RPMSG, SOCK_STREAM, 0, psock);
-#endif
+ ret = optee_transport_open(priv);
if (ret < 0)
{
- kmm_free(psock);
Review Comment:
??? `kmm_free(priv)` is in `open_close()`..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075981968
##
drivers/misc/optee_smc.c:
##
@@ -0,0 +1,322 @@
+/
+ * drivers/misc/optee_smc.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+#include
+#include
+
+#include "optee.h"
+#include "optee_smc.h"
+
+/
+ * Pre-processor Definitions
+ /
+
+#if defined(CONFIG_ARCH_ARM)
+# define smccc_smc arm_smccc_smc
+# define smccc_hvc arm_smccc_hvc
+# define smccc_res_tarm_smccc_res_t
+#elif defined(CONFIG_ARCH_ARM64)
+# define smccc_smc arm64_smccc_smc
+# define smccc_hvc arm64_smccc_hvc
+# define smccc_res_tarm64_smccc_res_t
+#else
+# error "CONFIG_DEV_OPTEE_SMC is only supported on arm and arm64"
+#endif
+
+#ifdef CONFIG_DEV_OPTEE_SMC_CONDUIT_SMC
+# define smc_conduitsmccc_smc
+#else
+# define smc_conduitsmccc_hvc
+#endif
+
+/
+ * Private Types
+ /
+
+typedef void (*optee_smc_fn)(unsigned long, unsigned long, unsigned long,
+ unsigned long, unsigned long, unsigned long,
+ unsigned long, unsigned long,
+ FAR smccc_res_t *);
+
+union optee_os_revision
+{
+ smccc_res_t smccc;
+ struct optee_smc_call_get_os_revision_result result;
+};
+
+union optee_calls_revision
+{
+ smccc_res_t smccc;
+ struct optee_smc_calls_revision_result result;
+};
+
+union optee_exchg_caps
+{
+ smccc_res_t smccc;
+ struct optee_smc_exchange_capabilities_result result;
+};
+
+/
+ * Private Data
+ /
+
+/
+ * Private Functions
+ /
+
+static bool optee_smc_is_compatible(optee_smc_fn smc_fn)
+{
+ smccc_res_t callsuid;
+ union optee_os_revision osrev;
+ union optee_calls_revision callsrev;
+ union optee_exchg_caps xchgcaps;
+
+ /* Print the OS revision and build ID (if reported) */
+
+ osrev.result.build_id = 0;
+
+ smc_fn(OPTEE_SMC_CALL_GET_OS_REVISION, 0, 0, 0, 0, 0, 0, 0, &osrev.smccc);
+
+ if (osrev.result.build_id)
+{
+ syslog(LOG_INFO, "OP-TEE: OS revision %lu.%lu (%08lx)\n",
+ osrev.result.major, osrev.result.minor,
+ osrev.result.build_id);
+}
+ else
+{
+ syslog(LOG_INFO, "OP-TEE: OS revision %lu.%lu\n",
+ osrev.result.major, osrev.result.minor);
+}
+
+ /* Check the API UID */
+
+ smc_fn(OPTEE_SMC_CALLS_UID, 0, 0, 0, 0, 0, 0, 0, &callsuid);
+
+ if (callsuid.a0 != OPTEE_MSG_UID_0 || callsuid.a1 != OPTEE_MSG_UID_1 ||
+ callsuid.a2 != OPTEE_MSG_UID_2 || callsuid.a3 != OPTEE_MSG_UID_3)
+{
+ syslog(LOG_ERR, "OP-TEE: API UID mismatch\n");
+ return false;
+}
+
+ /* Check the API revision */
+
+ smc_fn(OPTEE_SMC_CALLS_REVISION, 0, 0, 0, 0, 0, 0, 0, &callsrev.smccc);
+
+ if (callsrev.result.major != OPTEE_MSG_REVISION_MAJOR ||
+ (int)callsrev.result.minor < OPTEE_MSG_REVISION_MINOR)
+{
+ syslog(LOG_ERR, "OP-TEE: API revision incompatible\n");
+ return false;
+}
+
+ /* Check the capabilities */
+
+ smc_fn(OPTEE_SMC_EXCHANGE_CAPABILITIES, OPTEE_SMC_NSEC_CAP_UNIPROCESSOR,
+ 0
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075950987
##
drivers/misc/optee.c:
##
@@ -154,6 +202,248 @@ static bool optee_is_valid_range(FAR void *va, size_t
size)
# define optee_is_valid_range(addr, size) (true)
#endif
+/
+ * Name: optee_msg_alloc
+ *
+ * Description:
+ * Allocate OP-TEE page-aligned memory for use as arguments to OP-TEE
+ * calls, large enough to fit `num_params` parameters. Initialize the
+ * buffer to 0, and set the `.num_params` field to the specified value.
+ *
+ * Use `kmm_free()` to free the memory returned.
+ *
+ * Parameters:
+ * priv - the driver's private data structure
+ * num_params - the number of arguments to allocate shared memory for.
+ *Can be zero.
+ *
+ * Returned Values:
+ * On success, pointer to OP-TEE message arguments struct initialized
+ * to 0 except for `.num_params`, which is initialized to the specified.
+ * On failure, NULL.
+ *
+ /
+
+static FAR struct optee_msg_arg *
+optee_msg_alloc(FAR struct optee_priv_data *priv, uint32_t num_params)
+{
+ FAR struct optee_msg_arg *msg;
+
+ if (priv->alignment)
+{
+ msg = kmm_memalign(priv->alignment,
+ OPTEE_MSG_GET_ARG_SIZE(num_params));
+}
+ else
+{
+ msg = kmm_malloc(OPTEE_MSG_GET_ARG_SIZE(num_params));
+}
+
+ if (msg == NULL)
+{
+ return NULL;
+}
+
+ memset(msg, 0, OPTEE_MSG_GET_ARG_SIZE(num_params));
+ msg->num_params = num_params;
+
+ return msg;
+}
+
+/
+ * Name: optee_shm_to_page_list
+ *
+ * Description:
+ * Provide a page list of a shared memory buffer. Secure world doesn't
+ * know about the address environment mapping of NuttX, so physical
+ * pointers are needed when sharing memory. This implementation enables
+ * sharing of physically non-contiguous buffers according to
+ * optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG.
+ * Each entry in the generated page list is an array of the physical,
+ * potentially non-contiguous pages making up the actual buffer to
+ * represent. Note that this representation does not account for the page
+ * offset of the shared memory buffer. The offset is encoded in the
+ * physical address returned in `list_pa`.
+ *
+ * Parameters:
+ * shme- Shared memory entry to create a page list for.
+ * list_pa - If not NULL, will be set to the page list's physical
+ * address (which is aligned to
+ * OPTEE_MSG_NONCONTIG_PAGE_SIZE) added with shared memory
+ * page offset.
+ *
+ * Returned Values:
+ * A pointer to the kernel virtual address of the page list on success.
+ * NULL on failure. Caller responsible to free the returned list using
+ * `kmm_free()`.
+ *
+ /
+
+FAR void *optee_shm_to_page_list(FAR struct optee_shm_entry *shme,
+ FAR uintptr_t *list_pa)
+{
+ size_t pgsize = OPTEE_MSG_NONCONTIG_PAGE_SIZE;
+ uintptr_t pgoff;
+ uintptr_t page;
+ uint32_t total_pages;
+ uint32_t list_size;
+ FAR void *list;
+ FAR struct optee_page_list_entry *list_entry;
+ uint32_t i = 0;
+
+ pgoff = shme->shm.addr & (pgsize - 1);
+ total_pages = (uint32_t)div_round_up(pgoff + shme->shm.length, pgsize);
+ list_size = div_round_up(total_pages, OPTEE_PAGES_ARRAY_LEN)
+ * sizeof(struct optee_page_list_entry);
+
+ /* Page list's address should be page aligned, conveniently leaving
+ * log2() zero least-significant bits to use as the page
+ * offset of the shm buffer (added last before return below).
+ */
+
+ list = kmm_memalign(pgsize, list_size);
+ if (!list)
+{
+ return NULL;
+}
+
+ list_entry = (FAR struct optee_page_list_entry *)list;
+ page = ALIGN_DOWN(shme->shm.addr, pgsize);
+ while (total_pages)
+{
+ list_entry->pages_array[i++] = optee_va_to_pa((FAR void *)page);
+ page += pgsize;
+ total_pages--;
+
+ if (i == OPTEE_PAGES_ARRAY_LEN)
+{
+ list_entry->next_entry = optee_va_to_pa(list_entry + 1);
+ list_entry++;
+ i = 0;
+}
+}
+
+ if (list_pa)
+{
+ *list_pa = optee_va_to_pa(list) | pgoff;
+}
+
+ return list;
+}
+
+/
+ * Name: optee_shm_sec_register
+ *
+ * Description:
+ * Register specified shared memory entry with OP-TEE.
+ *
+ * Parameters:
+ * priv - The driver's private data structure
+ * shme - Pointer to shared memory entry to register. The entry, the
+ * contained shared memory object, or the referenced shared buffer
+ * cannot be NULL.
+ *
+ * Returne
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075217001
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
> Could you move it to SMC backend.
It's not exactly something I can move as it's being used by all IOCTLs. I
can introduce a field in `struct optee_priv_data` like `uintptr_t align`.
During `optee_transport_open()` set this to 0 in optee_socket.c, and 4KB in
optee_smc.c. Then during allocations, use `kmm_memalign(priv->align, ...)` or
even `kmm_malloc(...)` if `priv->align == 0`.
@xiaoxiang781216 Do you agree with that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075276174
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
OK, we can skip allocation if the buffer is already aligned
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075276174
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
OK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075217001
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
> Could you move it to SMC backend.
It's not exactly something I can move as it's being used by all IOCTLs. I
can introduce a field in `struct optee_priv_data` like `uintptr_t align`.
During `optee_transport_init()` set this to 0 in optee_socket.c, and 4KB in
optee_smc.c. Then during allocations, use `kmm_memalign(priv->align, ...)` or
even `kmm_malloc(...)` if `priv->align == 0`.
@xiaoxiang781216 Do you agree with that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075194354
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
> OP-TEE rejects it if it's not 4KB-aligned
(`OPTEE_MSG_NONCONTIG_PAGE_SIZE`) and `optee_shm_alloc()` aligns it. The same
goes for all calls.
>
Ok, this may because MMU require 4KB page size, and OP-TEE access the
parameter pointer directly in SMC case. But for socket ,OP-TEE side can't
access the buffer directly, but need call recv to copy the parameter into it's
internal buffer, so the alignment isn't required at all.
Could you move it to SMC backend.
> `OPTEE_MSG_MAX_NUM_PARAMS()` operates on this assumption, too. See
[here](https://github.com/OP-TEE/optee_os/blob/76d920d354df0940181bfbd0d7f4a71883e0a2bf/core/include/optee_msg.h#L234C9-L234C33).
>
> This must be something new compared to when the initial contribution to
NuttX was made, cause I don't see it in our optee_msg.h
Yes, we run OP-TEE in Cortex-M TrustZone which is totally different from
Cortex-A environment.
> What we could perhaps do is replace the call to `optee_shm_alloc()` with a
direct call to `kmm_memalign()`, and save one allocation for the shm entry.
>
> I'll do that.
Yes, I think so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075194354
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
> OP-TEE rejects it if it's not 4KB-aligned
(`OPTEE_MSG_NONCONTIG_PAGE_SIZE`) and `optee_shm_alloc()` aligns it. The same
goes for all calls.
>
Ok, this may because MMU require 4KB page size, and OP-TEE access the
parameter pointer directly in SMC case. But for socket ,OP-TEE side can't
access the buffer directly, but need call recv to copy the parameter into it's
internal buffer, so the alignment isn't required at all.
Could you move it to SMC backend.
> `OPTEE_MSG_MAX_NUM_PARAMS()` operates on this assumption, too. See
[here](https://github.com/OP-TEE/optee_os/blob/76d920d354df0940181bfbd0d7f4a71883e0a2bf/core/include/optee_msg.h#L234C9-L234C33).
>
> This must be something new compared to when the initial contribution to
NuttX was made, cause I don't see it in our optee_msg.h
Yes, we run OP-TEE in Cortex-M TrustZone which is totally different from
Cortex-A environment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075185420 ## include/nuttx/tee.h: ## @@ -198,6 +198,10 @@ struct tee_ioctl_shm_register_fd_data #define TEE_IOCTL_LOGIN_REE_KERNEL 0x8000 +/* Macro to help calculate the total size of n params */ + +#define TEE_IOCTL_PARAM_SIZE(x)(sizeof(struct tee_ioctl_param) * (x)) Review Comment: you don't need split to multiple line since nxstyle don't check the length of macro definition. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075183253
##
drivers/misc/optee.c:
##
@@ -145,10 +207,398 @@ static bool optee_safe_va_range(FAR void *va, size_t
size)
return false;
}
-#else
+#else /* !CONFIG_ARCH_ADDRENV */
+static uintptr_t optee_va_to_pa(FAR void *va)
+{
+ return (uintptr_t)va;
+}
+
#define optee_safe_va_range(addr, size) (true)
#endif
+/
+ * Name: optee_shm_to_page_list
+ *
+ * Description:
+ * Provide a page list of a shared memory buffer. Secure world doesn't
+ * know about the address environment mapping of NuttX, so physical
+ * pointers are needed when sharing memory. This implementation enables
+ * sharing of physically non-contiguous buffers according to
+ * optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG.
+ * Each entry in the generated page list is an array of the physical,
+ * potentially non-contiguous pages making up the actual buffer to
+ * represent. Note that this representation does not account for the page
+ * offset of the shared memory buffer. The offset is encoded in the
+ * physical address returned in `list_pa_p`.
+ *
+ * Parameters:
+ * shme- Shared memory entry to create a page list for.
+ * list_pa_p - If not NULL, will be set to the page list's physical
+ * address (which is aligned to
+ * OPTEE_MSG_NONCONTIG_PAGE_SIZE) added with shared memory
+ * page offset.
+ *
+ * Returned Values:
+ * A pointer to the kernel virtual address of the page list on success.
+ * NULL on failure. Caller responsible to free the returned list using
+ * `kmm_free()`.
+ *
+ /
+
+FAR void *optee_shm_to_page_list(FAR struct optee_shm_entry *shme,
+ FAR uintptr_t *list_pa_p)
+{
+ const size_t pgsize = OPTEE_MSG_NONCONTIG_PAGE_SIZE;
Review Comment:
nuttx code base never add const to the integer type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075016912
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
What we could perhaps do is replace the call to `optee_shm_alloc()` with a
plain call to `kmm_memalign()`, and save one allocation for the shm entry.
I'll do that.
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
What we could perhaps do is replace the call to `optee_shm_alloc()` with a
direct call to `kmm_memalign()`, and save one allocation for the shm entry.
I'll do that.
--
This is an automated message from the Apache
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2075016912
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
What we could do is perhaps replace the call to `optee_shm_alloc()` with a
plain call to `kmm_memalign()`, and save one allocation for the shm entry.
I'll do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074827254
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
OP-TEE rejects it if it's not 4KB-aligned (`OPTEE_MSG_NONCONTIG_PAGE_SIZE`)
and `optee_shm_alloc()` aligns it. The same goes for all calls.
`OPTEE_MSG_MAX_NUM_PARAMS()` operates on this assumption, too. See
[here](https://github.com/OP-TEE/optee_os/blob/76d920d354df0940181bfbd0d7f4a71883e0a2bf/core/include/optee_msg.h#L234C9-L234C33).
This must be something new compared to when the initial contribution to
NuttX was made, cause I don't see it in our optee_msg.h
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074827254
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
OP-TEE rejects it if it's not 4KB-aligned (`OPTEE_MSG_NONCONTIG_PAGE_SIZE`)
and `optee_shm_alloc_msg()` aligns it. The same goes for all calls.
`OPTEE_MSG_MAX_NUM_PARAMS()` operates on this assumption, too. See
[here](https://github.com/OP-TEE/optee_os/blob/76d920d354df0940181bfbd0d7f4a71883e0a2bf/core/include/optee_msg.h#L234C9-L234C33).
This must be something new compared to when the initial contribution to
NuttX was made, cause I don't see it in our optee_msg.h
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074983072
##
drivers/misc/optee.c:
##
@@ -145,10 +207,398 @@ static bool optee_safe_va_range(FAR void *va, size_t
size)
return false;
}
-#else
+#else /* !CONFIG_ARCH_ADDRENV */
+static uintptr_t optee_va_to_pa(FAR void *va)
+{
+ return (uintptr_t)va;
+}
+
#define optee_safe_va_range(addr, size) (true)
#endif
+/
+ * Name: optee_shm_to_page_list
+ *
+ * Description:
+ * Provide a page list of a shared memory buffer. Secure world doesn't
+ * know about the address environment mapping of NuttX, so physical
+ * pointers are needed when sharing memory. This implementation enables
+ * sharing of physically non-contiguous buffers according to
+ * optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG.
+ * Each entry in the generated page list is an array of the physical,
+ * potentially non-contiguous pages making up the actual buffer to
+ * represent. Note that this representation does not account for the page
+ * offset of the shared memory buffer. The offset is encoded in the
+ * physical address returned in `list_pa_p`.
+ *
+ * Parameters:
+ * shme- Shared memory entry to create a page list for.
+ * list_pa_p - If not NULL, will be set to the page list's physical
+ * address (which is aligned to
+ * OPTEE_MSG_NONCONTIG_PAGE_SIZE) added with shared memory
+ * page offset.
+ *
+ * Returned Values:
+ * A pointer to the kernel virtual address of the page list on success.
+ * NULL on failure. Caller responsible to free the returned list using
+ * `kmm_free()`.
+ *
+ /
+
+FAR void *optee_shm_to_page_list(FAR struct optee_shm_entry *shme,
+ FAR uintptr_t *list_pa_p)
+{
+ const size_t pgsize = OPTEE_MSG_NONCONTIG_PAGE_SIZE;
Review Comment:
Ok, but why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074827254
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
OP-TEE rejects it if it's not 4KB-aligned (`OPTEE_MSG_NONCONTIG_PAGE_SIZE`).
The same goes for all calls.
`OPTEE_MSG_MAX_NUM_PARAMS()` operates on this assumption, too. See
[here](https://github.com/OP-TEE/optee_os/blob/76d920d354df0940181bfbd0d7f4a71883e0a2bf/core/include/optee_msg.h#L234C9-L234C33).
This must be something new compared to when the initial contribution to
NuttX was made, cause I don't see it in our optee_msg.h
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074827254
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
OP-TEE rejects it if it's not 4KB-aligned. The same goes for all calls.
`OPTEE_MSG_MAX_NUM_PARAMS()` operates on this assumption, too. See
[here](https://github.com/OP-TEE/optee_os/blob/76d920d354df0940181bfbd0d7f4a71883e0a2bf/core/include/optee_msg.h#L234C9-L234C33).
This must be something new compared to when the initial contribution to
NuttX was made, cause I don't see it in our optee_msg.h
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074827254
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
OP-TEE rejects it if it's not 4KB-aligned. The same goes for all calls.
`OPTEE_MSG_MAX_NUM_PARAMS()` operates on this assumption, too. See
[here](https://github.com/OP-TEE/optee_os/blob/76d920d354df0940181bfbd0d7f4a71883e0a2bf/core/include/optee_msg.h#L234C9-L234C33).
This must be something new, compared to when the initial contribution to
NuttX was made, cause I don't see it in our optee_msg.h
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074827254
##
drivers/misc/optee.c:
##
@@ -476,62 +956,83 @@ static int optee_ioctl_invoke(FAR struct optee_priv_data
*priv,
arg->ret = TEE_ERROR_COMMUNICATION;
arg->ret_origin = TEE_ORIGIN_COMMS;
- memset(msg_buf, 0, sizeof(msg_buf));
- msg = (FAR struct optee_msg_arg *)&msg_buf[0];
+ msg = optee_shm_alloc_msg(priv, arg->num_params, &shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
msg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
msg->func = arg->func;
msg->session = arg->session;
msg->cancel_id = arg->cancel_id;
- msg->num_params = arg->num_params;
ret = optee_to_msg_param(msg->params, arg->num_params, arg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_transport_call(priv, msg);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
ret = optee_from_msg_param(arg->params, arg->num_params, msg->params);
if (ret < 0)
{
- return ret;
+ goto errout_with_msg;
}
arg->ret = msg->ret;
arg->ret_origin = msg->ret_origin;
+errout_with_msg:
+ optee_shm_free(shme);
+
return ret;
}
-static int optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
+static int
+optee_ioctl_version(FAR struct tee_ioctl_version_data *vers)
{
vers->impl_id = TEE_IMPL_ID_OPTEE;
vers->impl_caps = TEE_OPTEE_CAP_TZ;
vers->gen_caps = TEE_GEN_CAP_GP | TEE_GEN_CAP_MEMREF_NULL;
+#if 0
+ vers->gen_caps |= TEE_GEN_CAP_REG_MEM;
+#endif
return 0;
}
-static int optee_ioctl_cancel(FAR struct optee_priv_data *priv,
- FAR struct tee_ioctl_cancel_arg *arg)
+static int
+optee_ioctl_cancel(FAR struct optee_priv_data *priv,
+ FAR struct tee_ioctl_cancel_arg *arg)
{
- struct optee_msg_arg msg;
+ FAR struct optee_shm_entry *shme;
+ FAR struct optee_msg_arg *msg;
+ int ret;
if (!optee_safe_va_range(arg, sizeof(*arg)))
{
return -EACCES;
}
- memset(&msg, 0, sizeof(struct optee_msg_arg));
- msg.cmd = OPTEE_MSG_CMD_CANCEL;
- msg.session = arg->session;
- msg.cancel_id = arg->cancel_id;
- return optee_transport_call(priv, &msg);
+ msg = optee_shm_alloc_msg(priv, 0, &shme);
Review Comment:
OP-TEE rejects it if it's not 4KB-aligned. The same goes for all calls.
`OPTEE_MSG_GET_ARG_SIZE()` operates on this assumption, too. See
[here](https://github.com/OP-TEE/optee_os/blob/76d920d354df0940181bfbd0d7f4a71883e0a2bf/core/include/optee_msg.h#L226).
This must be something new, compared to when the initial contribution to
NuttX was made, cause I don't see it in our optee_msg.h
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074817879
##
drivers/misc/optee.c:
##
@@ -348,8 +419,8 @@ static int optee_ioctl_open_session(FAR struct
optee_priv_data *priv,
msg->params + 2);
if (ret < 0)
{
- close_args.session = msg->session;
- optee_ioctl_close_session(priv, &close_args);
+ close_arg.session = msg->session;
+ optee_ioctl_close_session(priv, &close_arg);
Review Comment:
Nice catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074805787 ## include/nuttx/tee.h: ## @@ -198,6 +198,10 @@ struct tee_ioctl_shm_register_fd_data #define TEE_IOCTL_LOGIN_REE_KERNEL 0x8000 +/* Macro to help calculate the total size of n params */ + +#define TEE_IOCTL_PARAM_SIZE(x)(sizeof(struct tee_ioctl_param) * (x)) Review Comment: It's actually 76 (up to column 77), and if I left-align `(sizeof(...` it goes over this vertical limit. I could start breaking it using `\` but it would still require 3 lines, unless I do it like this: ``` #define TEE_IOCTL_PARAM_SIZE(x)\ (sizeof(struct tee_ioctl_param) * (x)) ``` ...which is still not aligned. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074805787 ## include/nuttx/tee.h: ## @@ -198,6 +198,10 @@ struct tee_ioctl_shm_register_fd_data #define TEE_IOCTL_LOGIN_REE_KERNEL 0x8000 +/* Macro to help calculate the total size of n params */ + +#define TEE_IOCTL_PARAM_SIZE(x)(sizeof(struct tee_ioctl_param) * (x)) Review Comment: It's actually 76 (up to column 77), and if I left-align `(sizeof(...` it goes over this vertical limit. I could start breaking it using `\` but it would still require 3 lines, unless I do it like this: ``` #define TEE_IOCTL_PARAM_SIZE(x)\ (sizeof(struct tee_ioctl_param) * (x)) ``` ...which is still not aligned. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074805787 ## include/nuttx/tee.h: ## @@ -198,6 +198,10 @@ struct tee_ioctl_shm_register_fd_data #define TEE_IOCTL_LOGIN_REE_KERNEL 0x8000 +/* Macro to help calculate the total size of n params */ + +#define TEE_IOCTL_PARAM_SIZE(x)(sizeof(struct tee_ioctl_param) * (x)) Review Comment: It's actually 76 (up to column 77), if I left-align `(sizeof(...` it goes over the vertical limit of 78. I could start breaking it using `\` but it would still require 3 lines, unless I do it like this: ``` #define TEE_IOCTL_PARAM_SIZE(x)\ (sizeof(struct tee_ioctl_param) * (x)) ``` ...which is still not aligned. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074809921
##
drivers/misc/optee.c:
##
@@ -145,10 +207,398 @@ static bool optee_safe_va_range(FAR void *va, size_t
size)
return false;
}
-#else
+#else /* !CONFIG_ARCH_ADDRENV */
+static uintptr_t optee_va_to_pa(FAR void *va)
+{
+ return (uintptr_t)va;
+}
+
#define optee_safe_va_range(addr, size) (true)
#endif
+/
+ * Name: optee_shm_to_page_list
+ *
+ * Description:
+ * Provide a page list of a shared memory buffer. Secure world doesn't
+ * know about the address environment mapping of NuttX, so physical
+ * pointers are needed when sharing memory. This implementation enables
+ * sharing of physically non-contiguous buffers according to
+ * optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG.
+ * Each entry in the generated page list is an array of the physical,
+ * potentially non-contiguous pages making up the actual buffer to
+ * represent. Note that this representation does not account for the page
+ * offset of the shared memory buffer. The offset is encoded in the
+ * physical address returned in `list_pa_p`.
+ *
+ * Parameters:
+ * shme- Shared memory entry to create a page list for.
+ * list_pa_p - If not NULL, will be set to the page list's physical
+ * address (which is aligned to
+ * OPTEE_MSG_NONCONTIG_PAGE_SIZE) added with shared memory
+ * page offset.
+ *
+ * Returned Values:
+ * A pointer to the kernel virtual address of the page list on success.
+ * NULL on failure. Caller responsible to free the returned list using
+ * `kmm_free()`.
+ *
+ /
+
+FAR void *optee_shm_to_page_list(FAR struct optee_shm_entry *shme,
+ FAR uintptr_t *list_pa_p)
+{
+ const size_t pgsize = OPTEE_MSG_NONCONTIG_PAGE_SIZE;
+ uintptr_t shm_pgoff;
+ uintptr_t shm_page;
+ uint32_t shm_total_pages;
+ uint32_t list_size;
+ FAR void *list;
+ FAR struct page_list_entry *list_entry;
+ uint32_t i;
+
+ shm_pgoff = shme->shm.addr & (pgsize - 1);
+ shm_total_pages = (uint32_t)
+ (ALIGN_UP(shm_pgoff + shme->shm.length, pgsize) / pgsize);
+ list_size = div_round_up(shm_total_pages, PAGES_ARRAY_LEN) * pgsize;
+
+ /* Page list's address should be page aligned, conveniently leaving
+ * log2() zero least-significant bits to use as the page
+ * offset of the shm buffer (added last before return below).
+ */
+
+ list = kmm_memalign(pgsize, list_size);
+ if (!list)
+{
+ return NULL;
+}
+
+ list_entry = (FAR struct page_list_entry *)list;
+ shm_page = ALIGN_DOWN(shme->shm.addr, pgsize);
+ i = 0;
+ while (shm_total_pages)
+{
+ list_entry->pages_array[i++] = optee_va_to_pa((FAR void *)shm_page);
+ shm_page += pgsize;
+ shm_total_pages--;
+
+ if (i == PAGES_ARRAY_LEN)
+{
+ list_entry->next_page_data = optee_va_to_pa(list_entry + 1);
+ list_entry++;
+ i = 0;
+}
+}
+
+ if (list_pa_p)
+{
+ *list_pa_p = optee_va_to_pa(list) | shm_pgoff;
+}
+
+ return list;
+}
+
+/
+ * Name: optee_shm_sec_register
+ *
+ * Description:
+ * Register specified shared memory entry with OP-TEE.
+ *
+ * Parameters:
+ * priv - The driver's private data structure
+ * shme - Pointer to shared memory entry to register. The entry, the
+ * contained shared memory object, or the referenced shared buffer
+ * cannot be NULL.
+ *
+ * Returned Values:
+ * 0 on success, negative error code otherwise.
+ *
+ /
+
+static int optee_shm_sec_register(FAR struct optee_priv_data *priv,
+ FAR struct optee_shm_entry *shme)
+{
+ FAR struct optee_shm_entry *msg_shme;
+ FAR struct optee_msg_arg *msg;
+ FAR void *page_list;
+ uintptr_t page_list_pa;
+ int ret = 0;
+
+ msg = optee_shm_alloc_msg(priv, 1, &msg_shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
+
+ page_list = optee_shm_to_page_list(shme, &page_list_pa);
+ if (page_list == NULL)
+{
+ ret = -ENOMEM;
+ goto errout_with_msg;
+}
+
+ msg->cmd = OPTEE_MSG_CMD_REGISTER_SHM;
+ msg->params[0].attr = OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
+OPTEE_MSG_ATTR_NONCONTIG;
+ msg->params[0].u.tmem.buf_ptr = page_list_pa;
+ msg->params[0].u.tmem.shm_ref = shme->shm.addr;
+ msg->params[0].u.tmem.size = shme->shm.length;
+
+ if (optee_transport_call(priv, msg) || msg->ret)
+{
+ ret = -EINVAL;
+}
+
+ kmm_free(page_list);
+
+errout_with_msg:
+ optee_shm_free(msg_shme);
+
+ return ret;
+}
+
+/*
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074805787 ## include/nuttx/tee.h: ## @@ -198,6 +198,10 @@ struct tee_ioctl_shm_register_fd_data #define TEE_IOCTL_LOGIN_REE_KERNEL 0x8000 +/* Macro to help calculate the total size of n params */ + +#define TEE_IOCTL_PARAM_SIZE(x)(sizeof(struct tee_ioctl_param) * (x)) Review Comment: It's actually 76 (up to column 77), and if I left-align `(sizeof(...` it goes over the vertical limit of 78. I could start breaking it using `\` but it would still require 3 lines, unless I do it like this: ``` #define TEE_IOCTL_PARAM_SIZE(x)\ (sizeof(struct tee_ioctl_param) * (x)) ``` ...which is still not aligned. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074805206
##
drivers/misc/optee.h:
##
@@ -0,0 +1,77 @@
+/
+ * drivers/misc/optee.h
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+#ifndef __DRIVERS_MISC_OPTEE_H
+#define __DRIVERS_MISC_OPTEE_H
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+
+#include "optee_msg.h"
+
+/
+ * Pre-processor Definitions
+ /
+
+#define OPTEE_SERVER_PATH "optee"
+#define OPTEE_MAX_PARAM_NUM6
+
+/
+ * Public Types
+ /
+
+struct optee_priv_data;
+
+struct optee_priv_data
+{
+ FAR void *transport;
+};
+
+/
+ * Public Functions Definitions
+ /
+
+#undef EXTERN
+#if defined(__cplusplus)
+#define EXTERN extern "C"
+extern "C"
+{
+#else
+#define EXTERN extern
+#endif
+
+int optee_transport_init(void);
+int optee_transport_open(FAR struct optee_priv_data *priv);
+int optee_transport_close(FAR struct optee_priv_data *priv);
+int optee_transport_call(FAR struct optee_priv_data *priv,
Review Comment:
please ignore
##
drivers/misc/optee.c:
##
@@ -125,52 +115,24 @@ static const struct file_operations g_optee_ops =
static int optee_open(FAR struct file *filep)
{
- FAR struct socket *psock;
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- struct sockaddr_un addr;
-#else
- struct sockaddr_rpmsg addr;
-#endif
+ FAR struct optee_priv_data *priv;
int ret;
- psock = (FAR struct socket *)kmm_zalloc(sizeof(struct socket));
- if (psock == NULL)
+ priv = (FAR struct optee_priv_data *)
Review Comment:
let's ignore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074804683
##
drivers/misc/optee.c:
##
@@ -145,10 +207,398 @@ static bool optee_safe_va_range(FAR void *va, size_t
size)
return false;
}
-#else
+#else /* !CONFIG_ARCH_ADDRENV */
+static uintptr_t optee_va_to_pa(FAR void *va)
+{
+ return (uintptr_t)va;
+}
+
#define optee_safe_va_range(addr, size) (true)
#endif
+/
+ * Name: optee_shm_to_page_list
+ *
+ * Description:
+ * Provide a page list of a shared memory buffer. Secure world doesn't
+ * know about the address environment mapping of NuttX, so physical
+ * pointers are needed when sharing memory. This implementation enables
+ * sharing of physically non-contiguous buffers according to
+ * optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG.
+ * Each entry in the generated page list is an array of the physical,
+ * potentially non-contiguous pages making up the actual buffer to
+ * represent. Note that this representation does not account for the page
+ * offset of the shared memory buffer. The offset is encoded in the
+ * physical address returned in `list_pa_p`.
+ *
+ * Parameters:
+ * shme- Shared memory entry to create a page list for.
+ * list_pa_p - If not NULL, will be set to the page list's physical
+ * address (which is aligned to
+ * OPTEE_MSG_NONCONTIG_PAGE_SIZE) added with shared memory
+ * page offset.
+ *
+ * Returned Values:
+ * A pointer to the kernel virtual address of the page list on success.
+ * NULL on failure. Caller responsible to free the returned list using
+ * `kmm_free()`.
+ *
+ /
+
+FAR void *optee_shm_to_page_list(FAR struct optee_shm_entry *shme,
+ FAR uintptr_t *list_pa_p)
+{
+ const size_t pgsize = OPTEE_MSG_NONCONTIG_PAGE_SIZE;
+ uintptr_t shm_pgoff;
+ uintptr_t shm_page;
+ uint32_t shm_total_pages;
+ uint32_t list_size;
+ FAR void *list;
+ FAR struct page_list_entry *list_entry;
+ uint32_t i;
+
+ shm_pgoff = shme->shm.addr & (pgsize - 1);
+ shm_total_pages = (uint32_t)
+ (ALIGN_UP(shm_pgoff + shme->shm.length, pgsize) / pgsize);
+ list_size = div_round_up(shm_total_pages, PAGES_ARRAY_LEN) * pgsize;
+
+ /* Page list's address should be page aligned, conveniently leaving
+ * log2() zero least-significant bits to use as the page
+ * offset of the shm buffer (added last before return below).
+ */
+
+ list = kmm_memalign(pgsize, list_size);
+ if (!list)
+{
+ return NULL;
+}
+
+ list_entry = (FAR struct page_list_entry *)list;
+ shm_page = ALIGN_DOWN(shme->shm.addr, pgsize);
+ i = 0;
+ while (shm_total_pages)
+{
+ list_entry->pages_array[i++] = optee_va_to_pa((FAR void *)shm_page);
+ shm_page += pgsize;
+ shm_total_pages--;
+
+ if (i == PAGES_ARRAY_LEN)
+{
+ list_entry->next_page_data = optee_va_to_pa(list_entry + 1);
+ list_entry++;
+ i = 0;
+}
+}
+
+ if (list_pa_p)
+{
+ *list_pa_p = optee_va_to_pa(list) | shm_pgoff;
+}
+
+ return list;
+}
+
+/
+ * Name: optee_shm_sec_register
+ *
+ * Description:
+ * Register specified shared memory entry with OP-TEE.
+ *
+ * Parameters:
+ * priv - The driver's private data structure
+ * shme - Pointer to shared memory entry to register. The entry, the
+ * contained shared memory object, or the referenced shared buffer
+ * cannot be NULL.
+ *
+ * Returned Values:
+ * 0 on success, negative error code otherwise.
+ *
+ /
+
+static int optee_shm_sec_register(FAR struct optee_priv_data *priv,
+ FAR struct optee_shm_entry *shme)
+{
+ FAR struct optee_shm_entry *msg_shme;
+ FAR struct optee_msg_arg *msg;
+ FAR void *page_list;
+ uintptr_t page_list_pa;
+ int ret = 0;
+
+ msg = optee_shm_alloc_msg(priv, 1, &msg_shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
+
+ page_list = optee_shm_to_page_list(shme, &page_list_pa);
+ if (page_list == NULL)
+{
+ ret = -ENOMEM;
+ goto errout_with_msg;
+}
+
+ msg->cmd = OPTEE_MSG_CMD_REGISTER_SHM;
+ msg->params[0].attr = OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
+OPTEE_MSG_ATTR_NONCONTIG;
+ msg->params[0].u.tmem.buf_ptr = page_list_pa;
+ msg->params[0].u.tmem.shm_ref = shme->shm.addr;
+ msg->params[0].u.tmem.size = shme->shm.length;
+
+ if (optee_transport_call(priv, msg) || msg->ret)
+{
+ ret = -EINVAL;
+}
+
+ kmm_free(page_list);
+
+errout_with_msg:
+ optee_shm_free(msg_shme);
+
+ return ret;
+}
+
+/**
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074773932
##
drivers/misc/optee.c:
##
@@ -145,10 +207,398 @@ static bool optee_safe_va_range(FAR void *va, size_t
size)
return false;
}
-#else
+#else /* !CONFIG_ARCH_ADDRENV */
+static uintptr_t optee_va_to_pa(FAR void *va)
+{
+ return (uintptr_t)va;
+}
+
#define optee_safe_va_range(addr, size) (true)
#endif
+/
+ * Name: optee_shm_to_page_list
+ *
+ * Description:
+ * Provide a page list of a shared memory buffer. Secure world doesn't
+ * know about the address environment mapping of NuttX, so physical
+ * pointers are needed when sharing memory. This implementation enables
+ * sharing of physically non-contiguous buffers according to
+ * optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG.
+ * Each entry in the generated page list is an array of the physical,
+ * potentially non-contiguous pages making up the actual buffer to
+ * represent. Note that this representation does not account for the page
+ * offset of the shared memory buffer. The offset is encoded in the
+ * physical address returned in `list_pa_p`.
+ *
+ * Parameters:
+ * shme- Shared memory entry to create a page list for.
+ * list_pa_p - If not NULL, will be set to the page list's physical
+ * address (which is aligned to
+ * OPTEE_MSG_NONCONTIG_PAGE_SIZE) added with shared memory
+ * page offset.
+ *
+ * Returned Values:
+ * A pointer to the kernel virtual address of the page list on success.
+ * NULL on failure. Caller responsible to free the returned list using
+ * `kmm_free()`.
+ *
+ /
+
+FAR void *optee_shm_to_page_list(FAR struct optee_shm_entry *shme,
+ FAR uintptr_t *list_pa_p)
+{
+ const size_t pgsize = OPTEE_MSG_NONCONTIG_PAGE_SIZE;
+ uintptr_t shm_pgoff;
+ uintptr_t shm_page;
+ uint32_t shm_total_pages;
+ uint32_t list_size;
+ FAR void *list;
+ FAR struct page_list_entry *list_entry;
+ uint32_t i;
+
+ shm_pgoff = shme->shm.addr & (pgsize - 1);
+ shm_total_pages = (uint32_t)
+ (ALIGN_UP(shm_pgoff + shme->shm.length, pgsize) / pgsize);
+ list_size = div_round_up(shm_total_pages, PAGES_ARRAY_LEN) * pgsize;
Review Comment:
This is wrong. It is meant to be:
```
( / )
* sizeof(struct optee_page_list_entry)
```
The `* pgsize` is meant to be equivalent to `* sizeof(struct
optee_page_list_entry)`. I'll change to that for readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074799936 ## include/nuttx/tee.h: ## @@ -198,6 +198,10 @@ struct tee_ioctl_shm_register_fd_data #define TEE_IOCTL_LOGIN_REE_KERNEL 0x8000 +/* Macro to help calculate the total size of n params */ + +#define TEE_IOCTL_PARAM_SIZE(x)(sizeof(struct tee_ioctl_param) * (x)) Review Comment: why not fix, the macro could be length than 80 chars. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074799133 ## drivers/misc/optee.h: ## @@ -0,0 +1,77 @@ +/ + * drivers/misc/optee.h + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + / + +#ifndef __DRIVERS_MISC_OPTEE_H +#define __DRIVERS_MISC_OPTEE_H + +/ + * Included Files + / + +#include +#include +#include + +#include "optee_msg.h" + +/ + * Pre-processor Definitions + / + +#define OPTEE_SERVER_PATH "optee" +#define OPTEE_MAX_PARAM_NUM6 + +/ + * Public Types + / + +struct optee_priv_data; + +struct optee_priv_data Review Comment: Ok, I just saw your change later. let's keep your current design, but remove the callback pointer. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074781958 ## drivers/misc/optee.h: ## @@ -0,0 +1,77 @@ +/ + * drivers/misc/optee.h + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + / + +#ifndef __DRIVERS_MISC_OPTEE_H +#define __DRIVERS_MISC_OPTEE_H + +/ + * Included Files + / + +#include +#include +#include + +#include "optee_msg.h" + +/ + * Pre-processor Definitions + / + +#define OPTEE_SERVER_PATH "optee" +#define OPTEE_MAX_PARAM_NUM6 + +/ + * Public Types + / + +struct optee_priv_data; + +struct optee_priv_data Review Comment: I put it there because of its common part (the shm list). If `optee_priv_data` is transport-specific then we can't have common alloc/free'ing of shared memory implemented in optee.c (e.g. see optee.c#`optee_close()`). Are you sure? If so, do you mean perhaps to move all shared memory specifics into optee_smc.c instead? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074781958 ## drivers/misc/optee.h: ## @@ -0,0 +1,77 @@ +/ + * drivers/misc/optee.h + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + / + +#ifndef __DRIVERS_MISC_OPTEE_H +#define __DRIVERS_MISC_OPTEE_H + +/ + * Included Files + / + +#include +#include +#include + +#include "optee_msg.h" + +/ + * Pre-processor Definitions + / + +#define OPTEE_SERVER_PATH "optee" +#define OPTEE_MAX_PARAM_NUM6 + +/ + * Public Types + / + +struct optee_priv_data; + +struct optee_priv_data Review Comment: I put it there because of its common part (the shm list). If `optee_priv_data` is transport-specific then we can't have common alloc/free'ing of shared memory implemented in optee.c during open/close. Are you sure? If so, do you mean perhaps to move all shared memory specifics into optee_smc.c instead? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074777223
##
drivers/misc/optee.c:
##
@@ -145,10 +207,398 @@ static bool optee_safe_va_range(FAR void *va, size_t
size)
return false;
}
-#else
+#else /* !CONFIG_ARCH_ADDRENV */
+static uintptr_t optee_va_to_pa(FAR void *va)
+{
+ return (uintptr_t)va;
+}
+
#define optee_safe_va_range(addr, size) (true)
#endif
+/
+ * Name: optee_shm_to_page_list
+ *
+ * Description:
+ * Provide a page list of a shared memory buffer. Secure world doesn't
+ * know about the address environment mapping of NuttX, so physical
+ * pointers are needed when sharing memory. This implementation enables
+ * sharing of physically non-contiguous buffers according to
+ * optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG.
+ * Each entry in the generated page list is an array of the physical,
+ * potentially non-contiguous pages making up the actual buffer to
+ * represent. Note that this representation does not account for the page
+ * offset of the shared memory buffer. The offset is encoded in the
+ * physical address returned in `list_pa_p`.
+ *
+ * Parameters:
+ * shme- Shared memory entry to create a page list for.
+ * list_pa_p - If not NULL, will be set to the page list's physical
+ * address (which is aligned to
+ * OPTEE_MSG_NONCONTIG_PAGE_SIZE) added with shared memory
+ * page offset.
+ *
+ * Returned Values:
+ * A pointer to the kernel virtual address of the page list on success.
+ * NULL on failure. Caller responsible to free the returned list using
+ * `kmm_free()`.
+ *
+ /
+
+FAR void *optee_shm_to_page_list(FAR struct optee_shm_entry *shme,
+ FAR uintptr_t *list_pa_p)
+{
+ const size_t pgsize = OPTEE_MSG_NONCONTIG_PAGE_SIZE;
+ uintptr_t shm_pgoff;
+ uintptr_t shm_page;
+ uint32_t shm_total_pages;
+ uint32_t list_size;
+ FAR void *list;
+ FAR struct page_list_entry *list_entry;
+ uint32_t i;
+
+ shm_pgoff = shme->shm.addr & (pgsize - 1);
+ shm_total_pages = (uint32_t)
+ (ALIGN_UP(shm_pgoff + shme->shm.length, pgsize) / pgsize);
+ list_size = div_round_up(shm_total_pages, PAGES_ARRAY_LEN) * pgsize;
+
+ /* Page list's address should be page aligned, conveniently leaving
+ * log2() zero least-significant bits to use as the page
+ * offset of the shm buffer (added last before return below).
+ */
+
+ list = kmm_memalign(pgsize, list_size);
+ if (!list)
+{
+ return NULL;
+}
+
+ list_entry = (FAR struct page_list_entry *)list;
+ shm_page = ALIGN_DOWN(shme->shm.addr, pgsize);
+ i = 0;
+ while (shm_total_pages)
+{
+ list_entry->pages_array[i++] = optee_va_to_pa((FAR void *)shm_page);
+ shm_page += pgsize;
+ shm_total_pages--;
+
+ if (i == PAGES_ARRAY_LEN)
+{
+ list_entry->next_page_data = optee_va_to_pa(list_entry + 1);
+ list_entry++;
+ i = 0;
+}
+}
+
+ if (list_pa_p)
+{
+ *list_pa_p = optee_va_to_pa(list) | shm_pgoff;
+}
+
+ return list;
+}
+
+/
+ * Name: optee_shm_sec_register
+ *
+ * Description:
+ * Register specified shared memory entry with OP-TEE.
+ *
+ * Parameters:
+ * priv - The driver's private data structure
+ * shme - Pointer to shared memory entry to register. The entry, the
+ * contained shared memory object, or the referenced shared buffer
+ * cannot be NULL.
+ *
+ * Returned Values:
+ * 0 on success, negative error code otherwise.
+ *
+ /
+
+static int optee_shm_sec_register(FAR struct optee_priv_data *priv,
+ FAR struct optee_shm_entry *shme)
+{
+ FAR struct optee_shm_entry *msg_shme;
+ FAR struct optee_msg_arg *msg;
+ FAR void *page_list;
+ uintptr_t page_list_pa;
+ int ret = 0;
+
+ msg = optee_shm_alloc_msg(priv, 1, &msg_shme);
+ if (msg == NULL)
+{
+ return -ENOMEM;
+}
+
+ page_list = optee_shm_to_page_list(shme, &page_list_pa);
+ if (page_list == NULL)
+{
+ ret = -ENOMEM;
+ goto errout_with_msg;
+}
+
+ msg->cmd = OPTEE_MSG_CMD_REGISTER_SHM;
+ msg->params[0].attr = OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
+OPTEE_MSG_ATTR_NONCONTIG;
+ msg->params[0].u.tmem.buf_ptr = page_list_pa;
+ msg->params[0].u.tmem.shm_ref = shme->shm.addr;
+ msg->params[0].u.tmem.size = shme->shm.length;
+
+ if (optee_transport_call(priv, msg) || msg->ret)
+{
+ ret = -EINVAL;
+}
+
+ kmm_free(page_list);
+
+errout_with_msg:
+ optee_shm_free(msg_shme);
+
+ return ret;
+}
+
+/*
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074773932
##
drivers/misc/optee.c:
##
@@ -145,10 +207,398 @@ static bool optee_safe_va_range(FAR void *va, size_t
size)
return false;
}
-#else
+#else /* !CONFIG_ARCH_ADDRENV */
+static uintptr_t optee_va_to_pa(FAR void *va)
+{
+ return (uintptr_t)va;
+}
+
#define optee_safe_va_range(addr, size) (true)
#endif
+/
+ * Name: optee_shm_to_page_list
+ *
+ * Description:
+ * Provide a page list of a shared memory buffer. Secure world doesn't
+ * know about the address environment mapping of NuttX, so physical
+ * pointers are needed when sharing memory. This implementation enables
+ * sharing of physically non-contiguous buffers according to
+ * optee_msg.h#OPTEE_MSG_ATTR_NONCONTIG.
+ * Each entry in the generated page list is an array of the physical,
+ * potentially non-contiguous pages making up the actual buffer to
+ * represent. Note that this representation does not account for the page
+ * offset of the shared memory buffer. The offset is encoded in the
+ * physical address returned in `list_pa_p`.
+ *
+ * Parameters:
+ * shme- Shared memory entry to create a page list for.
+ * list_pa_p - If not NULL, will be set to the page list's physical
+ * address (which is aligned to
+ * OPTEE_MSG_NONCONTIG_PAGE_SIZE) added with shared memory
+ * page offset.
+ *
+ * Returned Values:
+ * A pointer to the kernel virtual address of the page list on success.
+ * NULL on failure. Caller responsible to free the returned list using
+ * `kmm_free()`.
+ *
+ /
+
+FAR void *optee_shm_to_page_list(FAR struct optee_shm_entry *shme,
+ FAR uintptr_t *list_pa_p)
+{
+ const size_t pgsize = OPTEE_MSG_NONCONTIG_PAGE_SIZE;
+ uintptr_t shm_pgoff;
+ uintptr_t shm_page;
+ uint32_t shm_total_pages;
+ uint32_t list_size;
+ FAR void *list;
+ FAR struct page_list_entry *list_entry;
+ uint32_t i;
+
+ shm_pgoff = shme->shm.addr & (pgsize - 1);
+ shm_total_pages = (uint32_t)
+ (ALIGN_UP(shm_pgoff + shme->shm.length, pgsize) / pgsize);
+ list_size = div_round_up(shm_total_pages, PAGES_ARRAY_LEN) * pgsize;
Review Comment:
This is wrong. The `* pgsize` is meant to be equivalent to `* sizeof(struct
optee_page_list_entry)`. I'll change to that for readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074746657
##
drivers/misc/optee.c:
##
@@ -125,52 +115,24 @@ static const struct file_operations g_optee_ops =
static int optee_open(FAR struct file *filep)
{
- FAR struct socket *psock;
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- struct sockaddr_un addr;
-#else
- struct sockaddr_rpmsg addr;
-#endif
+ FAR struct optee_priv_data *priv;
int ret;
- psock = (FAR struct socket *)kmm_zalloc(sizeof(struct socket));
- if (psock == NULL)
+ priv = (FAR struct optee_priv_data *)
+kmm_zalloc(sizeof(struct optee_priv_data));
+ if (priv == NULL)
{
return -ENOMEM;
}
-#ifdef CONFIG_DEV_OPTEE_LOCAL
- ret = psock_socket(AF_UNIX, SOCK_STREAM, 0, psock);
-#else
- ret = psock_socket(AF_RPMSG, SOCK_STREAM, 0, psock);
-#endif
+ ret = optee_transport_open(priv);
if (ret < 0)
{
- kmm_free(psock);
Review Comment:
I did not. See
[optee_socket.c#131](https://github.com/apache/nuttx/pull/16309/commits/2c5e54e253d4b1d175a7240a1c37d5945c1a717d#diff-9378743f90d8f42d4ddd8948d29bca9086689127b4c9bdc3ed026878a4bd5d41R131).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
gpoulios commented on code in PR #16309: URL: https://github.com/apache/nuttx/pull/16309#discussion_r2074743563 ## include/nuttx/tee.h: ## @@ -198,6 +198,10 @@ struct tee_ioctl_shm_register_fd_data #define TEE_IOCTL_LOGIN_REE_KERNEL 0x8000 +/* Macro to help calculate the total size of n params */ + +#define TEE_IOCTL_PARAM_SIZE(x)(sizeof(struct tee_ioctl_param) * (x)) Review Comment: It doesn't fit. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/misc/optee.c: Add an SMC driver for arm [nuttx]
xiaoxiang781216 commented on code in PR #16309:
URL: https://github.com/apache/nuttx/pull/16309#discussion_r2073947475
##
include/nuttx/tee.h:
##
@@ -198,6 +198,10 @@ struct tee_ioctl_shm_register_fd_data
#define TEE_IOCTL_LOGIN_REE_KERNEL 0x8000
+/* Macro to help calculate the total size of n params */
+
+#define TEE_IOCTL_PARAM_SIZE(x)(sizeof(struct tee_ioctl_param) * (x))
Review Comment:
align the 3rd column with other lines
##
drivers/misc/optee.c:
##
@@ -99,6 +104,51 @@ static const struct file_operations g_optee_ops =
* Private Functions
/
+/
+ * Name: optee_safe_va_range
+ *
+ * Description:
+ * Check whether provided virtual address range belongs to user-owned
+ * memory. If this function is called from a kernel thread, it returns
+ * true. If this function is called in a build without CONFIG_ARCH_ADDRENV
+ * it always returns true.
+ *
+ * Parameters:
+ * va- Beginning of address range to check.
+ * size - Size of memory to check.
+ *
+ * Returned Values:
+ * True if the provided address range belongs to the user or the caller is
+ * a kernel thread. False otherwise.
+ *
+ /
+
+ #ifdef CONFIG_ARCH_ADDRENV
+static bool optee_safe_va_range(FAR void *va, size_t size)
+{
+ FAR struct tcb_s *tcb;
+ uint8_t ttype;
+
+ tcb = nxsched_self();
+ ttype = tcb->flags & TCB_FLAG_TTYPE_MASK;
+
+ if (ttype == TCB_FLAG_TTYPE_KERNEL)
+{
+ return true;
+}
+
+ if (up_addrenv_user_vaddr((uintptr_t)va) &&
+ up_addrenv_user_vaddr((uintptr_t)va + size - 1))
+{
+ return true;
+}
+
+ return false;
+}
+#else
+#define optee_safe_va_range(addr, size) (true)
Review Comment:
`# define optee_safe_va_range(addr, size) (true)`
##
drivers/misc/optee.c:
##
@@ -24,16 +24,19 @@
* Included Files
/
-#include
-
#include
+#include
+#include
Review Comment:
remove nuttx/config.h
##
drivers/misc/optee.h:
##
@@ -0,0 +1,77 @@
+/
+ * drivers/misc/optee.h
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+#ifndef __DRIVERS_MISC_OPTEE_H
+#define __DRIVERS_MISC_OPTEE_H
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+
+#include "optee_msg.h"
+
+/
+ * Pre-processor Definitions
+ /
+
+#define OPTEE_SERVER_PATH "optee"
+#define OPTEE_MAX_PARAM_NUM6
+
+/
+ * Public Types
+ /
+
+struct optee_priv_data;
+
+struct optee_priv_data
+{
+ FAR void *transport;
+};
+
+/
+ * Public Functions Definitions
+ /
+
+#undef EXTERN
+#if defined(__cplusplus)
+#define EXTERN extern "C"
+extern "C"
+{
+#else
+#define EXTERN extern
+#endif
+
+int optee_transport_init(void);
+int optee_transport_open(FAR struct optee_priv_data *priv);
+int optee_transport_close(FAR struct optee_priv_data *priv);
Review Comment:
let's change `int` to `void`
```
void optee_transport_close(FAR void *priv);
```
##
drivers/misc/optee.c:
##
@@ -145,10 +207,398 @@ static bool optee_safe_va_range(FAR void *va, size_t
size)
return false;
}
-#else
+#else /* !CONFIG_ARCH_ADDRENV */
+st
