xiaoxiang781216 commented on code in PR #16356:
URL: https://github.com/apache/nuttx/pull/16356#discussion_r2083392213
##########
drivers/misc/optee.c:
##########
@@ -890,21 +941,32 @@ optee_ioctl_shm_alloc(FAR struct tee_ioctl_shm_alloc_data
*data)
if (ftruncate(memfd, data->size) < 0)
{
+ ret = get_errno();
goto err;
}
- data->id = (uintptr_t)mmap(NULL, data->size, PROT_READ | PROT_WRITE,
- MAP_SHARED, memfd, 0);
- if (data->id == (uintptr_t)MAP_FAILED)
+ kaddr = mmap(NULL, data->size, PROT_READ | PROT_WRITE, MAP_SHARED,
Review Comment:
mmap always return the pointer belong to userspace, not kernel address, so
it can't be used inside kernel or pass to optee.
##########
drivers/misc/optee.c:
##########
@@ -925,20 +987,27 @@ optee_ioctl_shm_register(FAR struct optee_priv_data *priv,
return -EFAULT;
}
- if (rdata->flags & TEE_SHM_ALLOC)
+ if (rdata->flags)
{
return -EINVAL;
}
ret = optee_shm_alloc(priv, (FAR void *)(uintptr_t)rdata->addr,
Review Comment:
note: we can't use rdata->addr directly in kernel space
##########
tools/nxstyle.c:
##########
@@ -636,6 +637,16 @@ static const char *g_white_content_list[] =
"inflateEnd",
"zError",
+ /* Ref:
+ * apps/tee/libteec/optee_client/libteec/include/tee_client_api.h
+ */
+
+ "timeLow",
Review Comment:
ditto
##########
drivers/misc/Kconfig:
##########
@@ -39,6 +39,17 @@ config DEV_RPMSG_SERVER
default n
depends on RPMSG
+config OPTEE_OPENVELA_COMPAT
+ bool "Enable driver compatibility with openvela OP-TEE OS"
Review Comment:
let's remove prompt string, to disable the user enable it from defconfig
##########
drivers/misc/optee.c:
##########
@@ -555,23 +555,22 @@ static int optee_close(FAR struct file *filep)
return 0;
}
+#ifdef CONFIG_OPTEE_OPENVELA_COMPAT
Review Comment:
@hujun260 please review this change, let's try our best to use the same
method to pass the share memory and remove CONFIG_OPTEE_OPENVELA_COMPAT.
##########
tools/nxstyle.c:
##########
@@ -212,6 +212,7 @@ static const char *g_white_prefix[] =
"luaL_", /* Ref: apps/interpreters/lua/lua-5.x.x/src/lauxlib.h */
"V4L2_", /* Ref: include/sys/video_controls.h */
"Ifx", /* Ref: arch/tricore/src */
+ "TEEC_", /* Ref:
apps/tee/libteec/optee_client/libteec/include/tee_client_api.h */
Review Comment:
let's keep the sort
##########
drivers/misc/optee.c:
##########
@@ -890,21 +941,32 @@ optee_ioctl_shm_alloc(FAR struct tee_ioctl_shm_alloc_data
*data)
if (ftruncate(memfd, data->size) < 0)
{
+ ret = get_errno();
goto err;
}
- data->id = (uintptr_t)mmap(NULL, data->size, PROT_READ | PROT_WRITE,
- MAP_SHARED, memfd, 0);
- if (data->id == (uintptr_t)MAP_FAILED)
+ kaddr = mmap(NULL, data->size, PROT_READ | PROT_WRITE, MAP_SHARED,
Review Comment:
if the kernel/optee access is required, we have to implement optee_shm_mmap
and `return file_allocate(..., g_optee_shm_ops)`, not depends on memfd at all.
##########
drivers/misc/optee.h:
##########
@@ -46,17 +45,19 @@
* Public Types
****************************************************************************/
-struct optee_shm_entry
+struct optee_priv_data
{
- struct list_node node;
- struct tee_ioctl_shm_register_data shm;
+ uintptr_t alignment; /* Transport-specified message alignment */
+ FAR struct idr_s *shms; /* An RB tree of all shm entries */
};
-struct optee_priv_data
+struct optee_shm_entry
{
- uintptr_t alignment; /* Transport-specified message alignment */
- struct list_node shm_list; /* A list of all shm registered */
- spinlock_t lock; /* Lock used to guard list accesses */
+ FAR struct optee_priv_data *priv;
+ int32_t id;
Review Comment:
move after line 59 to avoid the padding
##########
drivers/misc/optee.c:
##########
@@ -568,20 +628,7 @@ static int optee_to_msg_param(FAR struct optee_msg_param
*mparams,
case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT:
case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
- mp->attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT + p->attr -
- TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
- if (p->c != TEE_MEMREF_NULL)
- {
- mp->u.rmem.shm_ref = p->c;
- }
- else
- {
- mp->u.rmem.shm_ref = 0;
- }
-
- mp->u.rmem.size = p->b;
- mp->u.rmem.offs = p->a;
- break;
+ return optee_memref_to_msg_param(priv, mp, p);
Review Comment:
why not continue to the next entry
##########
drivers/misc/optee.c:
##########
@@ -533,7 +538,16 @@ static int optee_close(FAR struct file *filep)
idr_for_each_entry(priv->shms, shm, id)
{
- optee_shm_free(shm);
+ if (shm->fd > -1)
+ {
+ /* The user did not call close(), do it here */
+
+ nx_close(shm->fd);
Review Comment:
but, vfs layer will call close for ALL opened file during exit, so we don't
this kind of defense programming.
--
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: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
