Linaro-mm-sig March 2025

linaro-mm-sig@lists.linaro.org

16 participants
38 discussions

Re: [PATCH] Replace nested max() with single max3()

by Christian König

Am 15.03.25 um 05:40 schrieb feng.wei8(a)zte.com.cn: > From: FengWei <feng.wei8(a)zte.com.cn> > > Use max3() macro instead of nesting max() to simplify the return > statement. Please add a "dma-buf: heaps: " prefix to the subject line, apart from that looks good to me. Regards, Christian. > > Signed-off-by: FengWei <feng.wei8(a)zte.com.cn> > --- > drivers/dma-buf/dma-heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index 3cbe87d4a464..96cb9ab5731a 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c > @@ -147,7 +147,7 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd, > in_size = 0; > if ((ucmd & kcmd & IOC_OUT) == 0) > out_size = 0; > - ksize = max(max(in_size, out_size), drv_size); > + ksize = max3(in_size, out_size, drv_size); > > /* If necessary, allocate buffer for ioctl argument */ > if (ksize > sizeof(stack_kdata)) {

2 months, 1 week

Re: [PATCH v4] dma-buf: heap: Replace nested max() with single max3()

by Krzysztof Kozlowski

On 17/03/2025 03:37, feng.wei8(a)zte.com.cn wrote: > From: FengWei <feng.wei8(a)zte.com.cn> > > Use max3() macro instead of nesting max() to simplify the return > statement. > > Signed-off-by: FengWei <feng.wei8(a)zte.com.cn> > --- > v3 -> v4 > fix the format of this patch. > drivers/dma-buf/dma-heap.c | 2 +- You sent five versions per day of the same patch. Look what was in v3: v2 -> v3 fix the format of this patch So you are doing the same over and over and sending it to us? Srsly, ZTE, slow down and be sure you follow the process BEFORE you send flood of patches like that and learn on the go. Best regards, Krzysztof

2 months, 1 week

Re: [PATCH v6 03/10] optee: account for direction while converting parameters

by Jens Wiklander

Hi Sumit, On Thu, Mar 13, 2025 at 11:41 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > Hi Jens, > > On Wed, Mar 05, 2025 at 02:04:09PM +0100, Jens Wiklander wrote: > > The OP-TEE backend driver has two internal function pointers to convert > > between the subsystem type struct tee_param and the OP-TEE type struct > > optee_msg_param. > > > > The conversion is done from one of the types to the other, which is then > > involved in some operation and finally converted back to the original > > type. When converting to prepare the parameters for the operation, all > > fields must be taken into account, but then converting back, it's enough > > to update only out-values and out-sizes. So, an update_out parameter is > > added to the conversion functions to tell if all or only some fields > > must be copied. > > > > This is needed in a later patch where it might get confusing when > > converting back in from_msg_param() callback since an allocated > > restricted SHM can be using the sec_world_id of the used restricted > > memory pool and that doesn't translate back well. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/optee/call.c | 10 ++-- > > drivers/tee/optee/ffa_abi.c | 43 +++++++++++++---- > > drivers/tee/optee/optee_private.h | 42 +++++++++++------ > > drivers/tee/optee/rpc.c | 31 +++++++++---- > > drivers/tee/optee/smc_abi.c | 76 +++++++++++++++++++++++-------- > > 5 files changed, 144 insertions(+), 58 deletions(-) > > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c > > index 16eb953e14bb..f1533b894726 100644 > > --- a/drivers/tee/optee/call.c > > +++ b/drivers/tee/optee/call.c > > @@ -400,7 +400,8 @@ int optee_open_session(struct tee_context *ctx, > > export_uuid(msg_arg->params[1].u.octets, &client_uuid); > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params + 2, > > - arg->num_params, param); > > + arg->num_params, param, > > + false /*!update_out*/); > > if (rc) > > goto out; > > > > @@ -427,7 +428,8 @@ int optee_open_session(struct tee_context *ctx, > > } > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > - msg_arg->params + 2)) { > > + msg_arg->params + 2, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_COMMUNICATION; > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > /* Close session again to avoid leakage */ > > @@ -541,7 +543,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > msg_arg->cancel_id = arg->cancel_id; > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params, arg->num_params, > > - param); > > + param, false /*!update_out*/); > > if (rc) > > goto out; > > > > @@ -551,7 +553,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > } > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > - msg_arg->params)) { > > + msg_arg->params, true /*update_out*/)) { > > msg_arg->ret = TEEC_ERROR_COMMUNICATION; > > msg_arg->ret_origin = TEEC_ORIGIN_COMMS; > > } > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c > > index 4ca1d5161b82..e4b08cd195f3 100644 > > --- a/drivers/tee/optee/ffa_abi.c > > +++ b/drivers/tee/optee/ffa_abi.c > > @@ -122,15 +122,21 @@ static int optee_shm_rem_ffa_handle(struct optee *optee, u64 global_id) > > */ > > > > static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > - u32 attr, const struct optee_msg_param *mp) > > + u32 attr, const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm = NULL; > > u64 offs_high = 0; > > u64 offs_low = 0; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_FMEM_INPUT) > > + return; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_FMEM_INPUT; > > - p->u.memref.size = mp->u.fmem.size; > > > > if (mp->u.fmem.global_id != OPTEE_MSG_FMEM_INVALID_GLOBAL_ID) > > shm = optee_shm_from_ffa_handle(optee, mp->u.fmem.global_id); > > @@ -141,6 +147,8 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > offs_high = mp->u.fmem.offs_high; > > } > > p->u.memref.shm_offs = offs_low | offs_high << 32; > > +out: > > + p->u.memref.size = mp->u.fmem.size; > > } > > > > /** > > @@ -150,12 +158,14 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > * @params: subsystem internal parameter representation > > * @num_params: number of elements in the parameter arrays > > * @msg_params: OPTEE_MSG parameters > > + * @update_out: update parameter for output only > > * > > * Returns 0 on success or <0 on failure > > */ > > static int optee_ffa_from_msg_param(struct optee *optee, > > struct tee_param *params, size_t num_params, > > - const struct optee_msg_param *msg_params) > > + const struct optee_msg_param *msg_params, > > + bool update_out) > > { > > size_t n; > > > > @@ -166,18 +176,20 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > > > switch (attr) { > > case OPTEE_MSG_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&p->u, 0, sizeof(p->u)); > > break; > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > - optee_from_msg_param_value(p, attr, mp); > > + optee_from_msg_param_value(p, attr, mp, update_out); > > break; > > case OPTEE_MSG_ATTR_TYPE_FMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_FMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_FMEM_INOUT: > > - from_msg_param_ffa_mem(optee, p, attr, mp); > > + from_msg_param_ffa_mem(optee, p, attr, mp, update_out); > > break; > > default: > > return -EINVAL; > > @@ -188,10 +200,16 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > } > > > > static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > struct tee_shm *shm = p->u.memref.shm; > > > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_FMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > @@ -211,6 +229,7 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > memset(&mp->u, 0, sizeof(mp->u)); > > mp->u.fmem.global_id = OPTEE_MSG_FMEM_INVALID_GLOBAL_ID; > > } > > +out: > > mp->u.fmem.size = p->u.memref.size; > > > > return 0; > > @@ -222,13 +241,15 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > * @optee: main service struct > > * @msg_params: OPTEE_MSG parameters > > * @num_params: number of elements in the parameter arrays > > - * @params: subsystem itnernal parameter representation > > + * @params: subsystem internal parameter representation > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_ffa_to_msg_param(struct optee *optee, > > struct optee_msg_param *msg_params, > > size_t num_params, > > - const struct tee_param *params) > > + const struct tee_param *params, > > + bool update_out) > > { > > size_t n; > > > > @@ -238,18 +259,20 @@ static int optee_ffa_to_msg_param(struct optee *optee, > > > > switch (p->attr) { > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&mp->u, 0, sizeof(mp->u)); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > - optee_to_msg_param_value(mp, p); > > + optee_to_msg_param_value(mp, p, update_out); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > - if (to_msg_param_ffa_mem(mp, p)) > > + if (to_msg_param_ffa_mem(mp, p, update_out)) > > return -EINVAL; > > break; > > default: > > Can we rather handle it as follows to improve code readability and > maintainence long term? Ditto for all other places. > > static int optee_ffa_to_msg_param(struct optee *optee, > struct optee_msg_param *msg_params, > size_t num_params, > const struct tee_param *params, > bool update_out) > { > size_t n; > > for (n = 0; n < num_params; n++) { > const struct tee_param *p = params + n; > struct optee_msg_param *mp = msg_params + n; > > if (update_out && (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_NONE || > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT || > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT)) > continue; You're missing updating the length field for memrefs. Cheers, Jens > > switch (p->attr) { > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > memset(&mp->u, 0, sizeof(mp->u)); > break; > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > optee_to_msg_param_value(mp, p); > break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > if (to_msg_param_ffa_mem(mp, p)) > return -EINVAL; > break; > default: > return -EINVAL; > } > } > > return 0; > } > > -Sumit > > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h > > index dc0f355ef72a..20eda508dbac 100644 > > --- a/drivers/tee/optee/optee_private.h > > +++ b/drivers/tee/optee/optee_private.h > > @@ -185,10 +185,12 @@ struct optee_ops { > > bool system_thread); > > int (*to_msg_param)(struct optee *optee, > > struct optee_msg_param *msg_params, > > - size_t num_params, const struct tee_param *params); > > + size_t num_params, const struct tee_param *params, > > + bool update_out); > > int (*from_msg_param)(struct optee *optee, struct tee_param *params, > > size_t num_params, > > - const struct optee_msg_param *msg_params); > > + const struct optee_msg_param *msg_params, > > + bool update_out); > > }; > > > > /** > > @@ -316,23 +318,35 @@ void optee_release(struct tee_context *ctx); > > void optee_release_supp(struct tee_context *ctx); > > > > static inline void optee_from_msg_param_value(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > - p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > - attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > - p->u.value.a = mp->u.value.a; > > - p->u.value.b = mp->u.value.b; > > - p->u.value.c = mp->u.value.c; > > + if (!update_out) > > + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > + attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > + > > + if (attr == OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT || > > + attr == OPTEE_MSG_ATTR_TYPE_VALUE_INOUT || !update_out) { > > + p->u.value.a = mp->u.value.a; > > + p->u.value.b = mp->u.value.b; > > + p->u.value.c = mp->u.value.c; > > + } > > } > > > > static inline void optee_to_msg_param_value(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, > > + bool update_out) > > { > > - mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > > - TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > > - mp->u.value.a = p->u.value.a; > > - mp->u.value.b = p->u.value.b; > > - mp->u.value.c = p->u.value.c; > > + if (!update_out) > > + mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > > + > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > > + p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT || !update_out) { > > + mp->u.value.a = p->u.value.a; > > + mp->u.value.b = p->u.value.b; > > + mp->u.value.c = p->u.value.c; > > + } > > } > > > > void optee_cq_init(struct optee_call_queue *cq, int thread_count); > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c > > index ebbbd42b0e3e..580e6b9b0606 100644 > > --- a/drivers/tee/optee/rpc.c > > +++ b/drivers/tee/optee/rpc.c > > @@ -63,7 +63,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > > } > > > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params)) > > + arg->params, false /*!update_out*/)) > > goto bad; > > > > for (i = 0; i < arg->num_params; i++) { > > @@ -107,7 +107,8 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > > } else { > > params[3].u.value.a = msg.len; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) > > + arg->num_params, params, > > + true /*update_out*/)) > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > else > > arg->ret = TEEC_SUCCESS; > > @@ -188,6 +189,7 @@ static void handle_rpc_func_cmd_wait(struct optee_msg_arg *arg) > > static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > > struct optee_msg_arg *arg) > > { > > + bool update_out = false; > > struct tee_param *params; > > > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > @@ -200,15 +202,21 @@ static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > > } > > > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params)) { > > + arg->params, update_out)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > goto out; > > } > > > > arg->ret = optee_supp_thrd_req(ctx, arg->cmd, arg->num_params, params); > > > > + /* > > + * Special treatment for OPTEE_RPC_CMD_SHM_ALLOC since input is a > > + * value type, but the output is a memref type. > > + */ > > + if (arg->cmd != OPTEE_RPC_CMD_SHM_ALLOC) > > + update_out = true; > > if (optee->ops->to_msg_param(optee, arg->params, arg->num_params, > > - params)) > > + params, update_out)) > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > out: > > kfree(params); > > @@ -270,7 +278,7 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > @@ -280,7 +288,8 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > > params[0].u.value.b = 0; > > params[0].u.value.c = 0; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > } > > @@ -324,7 +333,7 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > @@ -358,7 +367,8 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > > params[0].u.value.b = rdev->descr.capacity; > > params[0].u.value.c = rdev->descr.reliable_wr_count; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > } > > @@ -384,7 +394,7 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT || > > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > @@ -401,7 +411,8 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > > goto out; > > } > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > goto out; > > } > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c > > index 165fadd9abc9..cfdae266548b 100644 > > --- a/drivers/tee/optee/smc_abi.c > > +++ b/drivers/tee/optee/smc_abi.c > > @@ -81,20 +81,26 @@ static int optee_cpuhp_disable_pcpu_irq(unsigned int cpu) > > */ > > > > static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm; > > phys_addr_t pa; > > int rc; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_TMEM_INPUT) > > + return 0; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_TMEM_INPUT; > > - p->u.memref.size = mp->u.tmem.size; > > shm = (struct tee_shm *)(unsigned long)mp->u.tmem.shm_ref; > > if (!shm) { > > p->u.memref.shm_offs = 0; > > p->u.memref.shm = NULL; > > - return 0; > > + goto out; > > } > > > > rc = tee_shm_get_pa(shm, 0, &pa); > > @@ -103,18 +109,25 @@ static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > > > p->u.memref.shm_offs = mp->u.tmem.buf_ptr - pa; > > p->u.memref.shm = shm; > > - > > +out: > > + p->u.memref.size = mp->u.tmem.size; > > return 0; > > } > > > > static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_RMEM_INPUT) > > + return; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; > > - p->u.memref.size = mp->u.rmem.size; > > shm = (struct tee_shm *)(unsigned long)mp->u.rmem.shm_ref; > > > > if (shm) { > > @@ -124,6 +137,8 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > p->u.memref.shm_offs = 0; > > p->u.memref.shm = NULL; > > } > > +out: > > + p->u.memref.size = mp->u.rmem.size; > > } > > > > /** > > @@ -133,11 +148,13 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > * @params: subsystem internal parameter representation > > * @num_params: number of elements in the parameter arrays > > * @msg_params: OPTEE_MSG parameters > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > size_t num_params, > > - const struct optee_msg_param *msg_params) > > + const struct optee_msg_param *msg_params, > > + bool update_out) > > { > > int rc; > > size_t n; > > @@ -149,25 +166,27 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > > > switch (attr) { > > case OPTEE_MSG_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&p->u, 0, sizeof(p->u)); > > break; > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > - optee_from_msg_param_value(p, attr, mp); > > + optee_from_msg_param_value(p, attr, mp, update_out); > > break; > > case OPTEE_MSG_ATTR_TYPE_TMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT: > > - rc = from_msg_param_tmp_mem(p, attr, mp); > > + rc = from_msg_param_tmp_mem(p, attr, mp, update_out); > > if (rc) > > return rc; > > break; > > case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT: > > - from_msg_param_reg_mem(p, attr, mp); > > + from_msg_param_reg_mem(p, attr, mp, update_out); > > break; > > > > default: > > @@ -178,20 +197,25 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > } > > > > static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > int rc; > > phys_addr_t pa; > > > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_TMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > mp->u.tmem.shm_ref = (unsigned long)p->u.memref.shm; > > - mp->u.tmem.size = p->u.memref.size; > > > > if (!p->u.memref.shm) { > > mp->u.tmem.buf_ptr = 0; > > - return 0; > > + goto out; > > } > > > > rc = tee_shm_get_pa(p->u.memref.shm, p->u.memref.shm_offs, &pa); > > @@ -201,19 +225,27 @@ static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > > mp->u.tmem.buf_ptr = pa; > > mp->attr |= OPTEE_MSG_ATTR_CACHE_PREDEFINED << > > OPTEE_MSG_ATTR_CACHE_SHIFT; > > - > > +out: > > + mp->u.tmem.size = p->u.memref.size; > > return 0; > > } > > > > static int to_msg_param_reg_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > mp->u.rmem.shm_ref = (unsigned long)p->u.memref.shm; > > - mp->u.rmem.size = p->u.memref.size; > > mp->u.rmem.offs = p->u.memref.shm_offs; > > +out: > > + mp->u.rmem.size = p->u.memref.size; > > return 0; > > } > > > > @@ -223,11 +255,13 @@ static int to_msg_param_reg_mem(struct optee_msg_param *mp, > > * @msg_params: OPTEE_MSG parameters > > * @num_params: number of elements in the parameter arrays > > * @params: subsystem itnernal parameter representation > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_to_msg_param(struct optee *optee, > > struct optee_msg_param *msg_params, > > - size_t num_params, const struct tee_param *params) > > + size_t num_params, const struct tee_param *params, > > + bool update_out) > > { > > int rc; > > size_t n; > > @@ -238,21 +272,23 @@ static int optee_to_msg_param(struct optee *optee, > > > > switch (p->attr) { > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&mp->u, 0, sizeof(mp->u)); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > - optee_to_msg_param_value(mp, p); > > + optee_to_msg_param_value(mp, p, update_out); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > if (tee_shm_is_dynamic(p->u.memref.shm)) > > - rc = to_msg_param_reg_mem(mp, p); > > + rc = to_msg_param_reg_mem(mp, p, update_out); > > else > > - rc = to_msg_param_tmp_mem(mp, p); > > + rc = to_msg_param_tmp_mem(mp, p, update_out); > > if (rc) > > return rc; > > break; > > -- > > 2.43.0 > >

2 months, 1 week

Re: [PATCH] dma-buf: Replace nested max() with single max3()

by T.J. Mercier

On Fri, Mar 14, 2025 at 10:02 PM <feng.wei8(a)zte.com.cn> wrote: > > From: FengWei <feng.wei8(a)zte.com.cn> > > Use max3() macro instead of nesting max() to simplify the return > statement. > > Signed-off-by: FengWei <feng.wei8(a)zte.com.cn> The commit subject should include "dma-buf: heaps:" as a prefix. With that: Reviewed-by: T.J. Mercier <tjmercier(a)google.com> > --- > drivers/dma-buf/dma-heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index 3cbe87d4a464..96cb9ab5731a 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c > @@ -147,7 +147,7 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd, > in_size = 0; > if ((ucmd & kcmd & IOC_OUT) == 0) > out_size = 0; > - ksize = max(max(in_size, out_size), drv_size); > + ksize = max3(in_size, out_size, drv_size); > > /* If necessary, allocate buffer for ioctl argument */ > if (ksize > sizeof(stack_kdata)) { > -- > 2.25.1

2 months, 1 week

Re: [PATCH v5 RESEND 1/2] dmaengine: qcom: gpi: Add GPI Block event interrupt support

by Vinod Koul

On 12-02-25, 17:35, Jyothi Kumar Seerapu wrote: > GSI hardware generates an interrupt for each transfer completion. > For multiple messages within a single transfer, this results in > N interrupts for N messages, leading to significant software > interrupt latency. > > To mitigate this latency, utilize Block Event Interrupt (BEI) mechanism. > Enabling BEI instructs the GSI hardware to prevent interrupt generation > and BEI is disabled when an interrupt is necessary. > > When using BEI, consider splitting a single multi-message transfer into > chunks of 8 messages internally and so interrupts are not expected for > the first 7 message completions, only the last message triggers > an interrupt, indicating the completion of 8 messages. > > This BEI mechanism enhances overall transfer efficiency. That sounds good but I dont like the idea that we add a custom interface for this. Please use DMA_PREP_INTERRUPT instead. Adding this flag should trigger N interrupts, absence of this should lead to Block events only -- ~Vinod

2 months, 1 week

[PATCH v4] Documentation: dma-buf: heaps: Add heap name definitions

by Maxime Ripard

Following a recent discussion at last Plumbers, John Stultz, Sumit Sewal, TJ Mercier and I came to an agreement that we should document what the dma-buf heaps names are expected to be, and what the buffers attributes you'll get should be documented. Let's create that doc to make sure those attributes and names are guaranteed going forward. Reviewed-by: T.J. Mercier <tjmercier(a)google.com> Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Changes from v3: - Add TJ RB - Fix a few typos. Changes from v2: - Remove exhaustive list of names for platforms, and just mention the alternatives. - Add MAINTAINERS entry Changes from v1: - Add the mention that the cma / reserved heap is optional. --- Documentation/userspace-api/dma-buf-heaps.rst | 25 +++++++++++++++++++ Documentation/userspace-api/index.rst | 1 + MAINTAINERS | 1 + 3 files changed, 27 insertions(+) create mode 100644 Documentation/userspace-api/dma-buf-heaps.rst diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst new file mode 100644 index 000000000000..535f49047ce6 --- /dev/null +++ b/Documentation/userspace-api/dma-buf-heaps.rst @@ -0,0 +1,25 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================== +Allocating dma-buf using heaps +============================== + +Dma-buf Heaps are a way for userspace to allocate dma-buf objects. They are +typically used to allocate buffers from a specific allocation pool, or to share +buffers across frameworks. + +Heaps +===== + +A heap represents a specific allocator. The Linux kernel currently supports the +following heaps: + + - The ``system`` heap allocates virtually contiguous, cacheable, buffers. + + - The ``cma`` heap allocates physically contiguous, cacheable, + buffers. Only present if a CMA region is present. Such a region is + usually created either through the kernel commandline through the + `cma` parameter, a memory region Device-Tree node with the + `linux,cma-default` property set, or through the `CMA_SIZE_MBYTES` or + `CMA_SIZE_PERCENTAGE` Kconfig options. Depending on the platform, it + might be called ``reserved``, ``linux,cma``, or ``default-pool``. diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst index b1395d94b3fd..9cbe4390c872 100644 --- a/Documentation/userspace-api/index.rst +++ b/Documentation/userspace-api/index.rst @@ -42,10 +42,11 @@ Devices and I/O .. toctree:: :maxdepth: 1 accelerators/ocxl + dma-buf-heaps dma-buf-alloc-exchange gpio/index iommufd media/index dcdbas diff --git a/MAINTAINERS b/MAINTAINERS index 8e0736dc2ee0..ef0364e65aef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6911,10 +6911,11 @@ R: T.J. Mercier <tjmercier(a)google.com> L: linux-media(a)vger.kernel.org L: dri-devel(a)lists.freedesktop.org L: linaro-mm-sig(a)lists.linaro.org (moderated for non-subscribers) S: Maintained T: git https://gitlab.freedesktop.org/drm/misc/kernel.git +F: Documentation/userspace-api/dma-buf-heaps.rst F: drivers/dma-buf/dma-heap.c F: drivers/dma-buf/heaps/* F: include/linux/dma-heap.h F: include/uapi/linux/dma-heap.h F: tools/testing/selftests/dmabuf-heaps/ -- 2.48.1

2 months, 2 weeks

[PATCH v3] Documentation: dma-buf: heaps: Add heap name definitions

by Maxime Ripard

Following a recent discussion at last Plumbers, John Stultz, Sumit Sewal, TJ Mercier and I came to an agreement that we should document what the dma-buf heaps names are expected to be, and what the buffers attributes you'll get should be documented. Let's create that doc to make sure those attributes and names are guaranteed going forward. Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Changes from v2: - Remove exhaustive list of names for platforms, and just mention the alternatives. - Add MAINTAINERS entry Changes from v1: - Add the mention that the cma / reserved heap is optional. --- Documentation/userspace-api/dma-buf-heaps.rst | 25 +++++++++++++++++++ Documentation/userspace-api/index.rst | 1 + MAINTAINERS | 1 + 3 files changed, 27 insertions(+) create mode 100644 Documentation/userspace-api/dma-buf-heaps.rst diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst new file mode 100644 index 000000000000..5b92d69646f6 --- /dev/null +++ b/Documentation/userspace-api/dma-buf-heaps.rst @@ -0,0 +1,25 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================== +Allocating dma-buf using heaps +============================== + +Dma-buf Heaps are a way for userspace to allocate dma-buf objects. They are +typically used to allocate buffers from a specific allocation pool, or to share +buffers across frameworks. + +Heaps +===== + +A heap represent a specific allocator. The Linux kernel currently supports the +following heaps: + + - The ``system`` heap allocates virtually contiguous, cacheable, buffers + + - The ``cma`` heap allocates physically contiguous, cacheable, + buffers. Only present if a CMA region is present. Such a region is + usually created either through the kernel commandline through the + `cma` parameter, a memory region Device-Tree node with the + `linux,cma-default` property set, or through the `CMA_SIZE_MBYTES` or + `CMA_SIZE_PERCENTAGE` Kconfig options. Depending on the platform, it + might be called ``reserved``, ``linux,cma``, or ``default-pool``. diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst index b1395d94b3fd..9cbe4390c872 100644 --- a/Documentation/userspace-api/index.rst +++ b/Documentation/userspace-api/index.rst @@ -42,10 +42,11 @@ Devices and I/O .. toctree:: :maxdepth: 1 accelerators/ocxl + dma-buf-heaps dma-buf-alloc-exchange gpio/index iommufd media/index dcdbas diff --git a/MAINTAINERS b/MAINTAINERS index 8e0736dc2ee0..ef0364e65aef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6911,10 +6911,11 @@ R: T.J. Mercier <tjmercier(a)google.com> L: linux-media(a)vger.kernel.org L: dri-devel(a)lists.freedesktop.org L: linaro-mm-sig(a)lists.linaro.org (moderated for non-subscribers) S: Maintained T: git https://gitlab.freedesktop.org/drm/misc/kernel.git +F: Documentation/userspace-api/dma-buf-heaps.rst F: drivers/dma-buf/dma-heap.c F: drivers/dma-buf/heaps/* F: include/linux/dma-heap.h F: include/uapi/linux/dma-heap.h F: tools/testing/selftests/dmabuf-heaps/ -- 2.48.1

2 months, 2 weeks

Re: [PATCH v4 0/6] TEE subsystem for restricted dma-buf allocations

by Sumit Garg

On Fri, 14 Feb 2025 at 21:19, Boris Brezillon <boris.brezillon(a)collabora.com> wrote: > > On Fri, 14 Feb 2025 18:37:14 +0530 > Sumit Garg <sumit.garg(a)linaro.org> wrote: > > > On Fri, 14 Feb 2025 at 15:37, Jens Wiklander <jens.wiklander(a)linaro.org> wrote: > > > > > > Hi, > > > > > > On Thu, Feb 13, 2025 at 6:39 PM Daniel Stone <daniel(a)fooishbar.org> wrote: > > > > > > > > Hi, > > > > > > > > On Thu, 13 Feb 2025 at 15:57, Jens Wiklander <jens.wiklander(a)linaro.org> wrote: > > > > > On Thu, Feb 13, 2025 at 3:05 PM Daniel Stone <daniel(a)fooishbar.org> wrote: > > > > > > But just because TEE is one good backend implementation, doesn't mean > > > > > > it should be the userspace ABI. Why should userspace care that TEE has > > > > > > mediated the allocation instead of it being a predefined range within > > > > > > DT? > > > > > > > > > > The TEE may very well use a predefined range that part is abstracted > > > > > with the interface. > > > > > > > > Of course. But you can also (and this has been shipped on real > > > > devices) handle this without any per-allocation TEE needs by simply > > > > allocating from a memory range which is predefined within DT. > > > > > > > > From the userspace point of view, why should there be one ABI to > > > > allocate memory from a predefined range which is delivered by DT to > > > > the kernel, and one ABI to allocate memory from a predefined range > > > > which is mediated by TEE? > > > > > > We need some way to specify the protection profile (or use case as > > > I've called it in the ABI) required for the buffer. Whether it's > > > defined in DT seems irrelevant. > > > > > > > > > > > > > What advantage > > > > > > does userspace get from having to have a different codepath to get a > > > > > > different handle to memory? What about x86? > > > > > > > > > > > > I think this proposal is looking at it from the wrong direction. > > > > > > Instead of working upwards from the implementation to userspace, start > > > > > > with userspace and work downwards. The interesting property to focus > > > > > > on is allocating memory, not that EL1 is involved behind the scenes. > > > > > > > > > > From what I've gathered from earlier discussions, it wasn't much of a > > > > > problem for userspace to handle this. If the kernel were to provide it > > > > > via a different ABI, how would it be easier to implement in the > > > > > kernel? I think we need an example to understand your suggestion. > > > > > > > > It is a problem for userspace, because we need to expose acceptable > > > > parameters for allocation through the entire stack. If you look at the > > > > dmabuf documentation in the kernel for how buffers should be allocated > > > > and exchanged, you can see the negotiation flow for modifiers. This > > > > permeates through KMS, EGL, Vulkan, Wayland, GStreamer, and more. > > > > > > What dma-buf properties are you referring to? > > > dma_heap_ioctl_allocate() accepts a few flags for the resulting file > > > descriptor and no flags for the heap itself. > > > > > > > > > > > Standardising on heaps allows us to add those in a similar way. > > > > > > How would you solve this with heaps? Would you use one heap for each > > > protection profile (use case), add heap_flags, or do a bit of both? > > I would say one heap per-profile. > And then it would have a per vendor multiplication factor as each vendor enforces memory restriction in a platform specific manner which won't scale. > > > > Christian gave an historical background here [1] as to why that hasn't > > worked in the past with DMA heaps given the scalability issues. > > > > [1] https://lore.kernel.org/dri-devel/e967e382-6cca-4dee-8333-39892d532f71@gmai… > > Hm, I fail to see where Christian dismiss the dma-heaps solution in > this email. He even says: > > > If the memory is not physically attached to any device, but rather just > memory attached to the CPU or a system wide memory controller then > expose the memory as DMA-heap with specific requirements (e.g. certain > sized pages, contiguous, restricted, encrypted, ...). I am not saying Christian dismissed DMA heaps but rather how scalability is an issue. What we are proposing here is a generic interface via TEE to the firmware/Trusted OS which can perform all the platform specific memory restrictions. This solution will scale across vendors. > > > > > > > > > > If we > > > > have to add different allocation mechanisms, then the complexity > > > > increases, permeating not only into all the different userspace APIs, > > > > but also into the drivers which need to support every different > > > > allocation mechanism even if they have no opinion on it - e.g. Mali > > > > doesn't care in any way whether the allocation comes from a heap or > > > > TEE or ACPI or whatever, it cares only that the memory is protected. > > > > > > > > Does that help? > > > > > > I think you're missing the stage where an unprotected buffer is > > > received and decrypted into a protected buffer. If you use the TEE for > > > decryption or to configure the involved devices for the use case, it > > > makes sense to let the TEE allocate the buffers, too. A TEE doesn't > > > have to be an OS in the secure world, it can be an abstraction to > > > support the use case depending on the design. So the restricted buffer > > > is already allocated before we reach Mali in your example. > > > > > > Allocating restricted buffers from the TEE subsystem saves us from > > > maintaining proxy dma-buf heaps. > > Honestly, when I look at dma-heap implementations, they seem > to be trivial shells around existing (more complex) allocators, and the > boiler plate [1] to expose a dma-heap is relatively small. The dma-buf > implementation, you already have, so we're talking about a hundred > lines of code to maintain, which shouldn't be significantly more than > what you have for the new ioctl() to be honest. It will rather be redundant vendor specific code under DMA heaps calling into firmware/Trusted OS to enforce memory restrictions as you can look into Mediatek example [1]. With TEE subsystem managing that it won't be the case as we will provide a common abstraction for the communication with underlying firmware/Trusted OS. [1] https://lore.kernel.org/linux-arm-kernel/20240515112308.10171-1-yong.wu@med… > And I'll insist on what > Daniel said, it's a small price to pay to have a standard interface to > expose to userspace. If dma-heaps are not used for this kind things, I > honestly wonder what they will be used for... Let's try not to forcefully find a use-case for DMA heaps when there is a better alternative available. I am still failing to see why you don't consider following as a standardised user-space interface: "When user-space has to work with restricted memory, ask TEE device to allocate it" -Sumit

2 months, 2 weeks

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig March 2025