Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

13 participants
3023 discussions

Re: [PATCH v8 03/14] optee: account for direction while converting parameters

by Jens Wiklander

Hi Sumit, On Wed, May 7, 2025 at 2:42 PM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > Hi Jens, > > On Fri, May 02, 2025 at 11:59:17AM +0200, 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(-) > > > > Thinking about it a bit more, as you mentioned in v6 that this patch > isn't strictly required for this series, can we drop this from this > series and rather followup with a separate patch? If you agree then I > can give a try on simplifying this patch? Sure. Who knows, perhaps it will be ready before a version of the patch is accepted? Cheers, Jens > > -Sumit > > > 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: > > 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 > >

4 months, 1 week

Re: [PATCH v4 08/11] tee: add Qualcomm TEE driver

by kernel test robot

Hi Amirreza, kernel test robot noticed the following build warnings: [auto build test WARNING on 33035b665157558254b3c21c3f049fd728e72368] url: https://github.com/intel-lab-lkp/linux/commits/Amirreza-Zarrabi/tee-allow-a… base: 33035b665157558254b3c21c3f049fd728e72368 patch link: https://lore.kernel.org/r/20250428-qcom-tee-using-tee-ss-without-mem-obj-v4… patch subject: [PATCH v4 08/11] tee: add Qualcomm TEE driver config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250507/202505071540.hAeEOUWt-lkp@…) compiler: sh4-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250507/202505071540.hAeEOUWt-lkp@…) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp(a)intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202505071540.hAeEOUWt-lkp@intel.com/ All warnings (new ones prefixed by >>): >> Warning: drivers/tee/qcomtee/async.c:101 function parameter 'async_msg' not described in 'async_release' >> Warning: drivers/tee/qcomtee/async.c:101 Excess function parameter 'msg' description in 'async_release' Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for DRM_AUX_HPD_BRIDGE Depends on [n]: HAS_IOMEM [=y] && DRM [=n] && DRM_BRIDGE [=n] && OF [=y] Selected by [m]: - UCSI_HUAWEI_GAOKUN [=m] && USB_SUPPORT [=y] && TYPEC [=m] && TYPEC_UCSI [=m] && EC_HUAWEI_GAOKUN [=m] -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki

4 months, 1 week

[PATCH 0/3] media: fix incorrect use of dma_sync_sg_*() calls

by Marek Szyprowski

Dear All, This patchset fixes the incorrect use of dma_sync_sg_*() calls in media and related drivers. They are replaced with much safer dma_sync_sgtable_*() variants, which take care of passing the proper number of elements for the sync operation. Best regards Marek Szyprowski, PhD Samsung R&D Institute Poland Patch summary: Marek Szyprowski (3): media: videobuf2: use sgtable-based scatterlist wrappers udmabuf: use sgtable-based scatterlist wrappers omap3isp:: use sgtable-based scatterlist wrappers drivers/dma-buf/udmabuf.c | 5 ++--- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 4 ++-- drivers/media/platform/ti/omap3isp/ispccdc.c | 8 ++++---- drivers/media/platform/ti/omap3isp/ispstat.c | 6 ++---- 4 files changed, 10 insertions(+), 13 deletions(-) -- 2.34.1

4 months, 1 week

Re: [PATCH v2 2/2] dma-buf: heaps: Give default CMA heap a fixed name

by Maxime Ripard

Hi Jared, Thanks for working on this On Tue, Apr 22, 2025 at 12:19:39PM -0700, Jared Kangas wrote: > The CMA heap's name in devtmpfs can vary depending on how the heap is > defined. Its name defaults to "reserved", but if a CMA area is defined > in the devicetree, the heap takes on the devicetree node's name, such as > "default-pool" or "linux,cma". To simplify naming, just name it > "default_cma", and keep a legacy node in place backed by the same > underlying structure for backwards compatibility. > > Signed-off-by: Jared Kangas <jkangas(a)redhat.com> > --- > Documentation/userspace-api/dma-buf-heaps.rst | 11 +++++++---- > drivers/dma-buf/heaps/Kconfig | 10 ++++++++++ > drivers/dma-buf/heaps/cma_heap.c | 14 +++++++++++++- > 3 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst > index 535f49047ce64..577de813ba461 100644 > --- a/Documentation/userspace-api/dma-buf-heaps.rst > +++ b/Documentation/userspace-api/dma-buf-heaps.rst > @@ -19,7 +19,10 @@ following heaps: > - 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``. > + ``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. The heap's name in devtmpfs is > + ``default_cma``. For backwards compatibility, when the > + ``DMABUF_HEAPS_CMA_LEGACY`` Kconfig option is set, a duplicate node is > + created following legacy naming conventions; the legacy name might be > + ``reserved``, ``linux,cma``, or ``default-pool``. It looks like, in addition to documenting the new naming, you also changed all the backticks to double backticks. Why did you do so? It seems mostly unrelated to that patch, so it would be better in a separate patch. > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig > index a5eef06c42264..83f3770fa820a 100644 > --- a/drivers/dma-buf/heaps/Kconfig > +++ b/drivers/dma-buf/heaps/Kconfig > @@ -12,3 +12,13 @@ config DMABUF_HEAPS_CMA > Choose this option to enable dma-buf CMA heap. This heap is backed > by the Contiguous Memory Allocator (CMA). If your system has these > regions, you should say Y here. > + > +config DMABUF_HEAPS_CMA_LEGACY > + bool "DMA-BUF CMA Heap" > + default y > + depends on DMABUF_HEAPS_CMA > + help > + Add a duplicate CMA-backed dma-buf heap with legacy naming derived > + from the CMA area's devicetree node, or "reserved" if the area is not > + defined in the devicetree. This uses the same underlying allocator as > + CONFIG_DMABUF_HEAPS_CMA. > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > index e998d8ccd1dc6..cd742c961190d 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -22,6 +22,7 @@ > #include <linux/slab.h> > #include <linux/vmalloc.h> > > +#define DEFAULT_CMA_NAME "default_cma" I appreciate this is kind of bikeshed-color territory, but I think "cma" would be a better option here. There's nothing "default" about it. > struct cma_heap { > struct dma_heap *heap; > @@ -394,15 +395,26 @@ static int __init __add_cma_heap(struct cma *cma, const char *name) > static int __init add_default_cma_heap(void) > { > struct cma *default_cma = dev_get_cma_area(NULL); > + const char *legacy_cma_name; > int ret; > > if (!default_cma) > return 0; > > - ret = __add_cma_heap(default_cma, cma_get_name(default_cma)); > + ret = __add_cma_heap(default_cma, DEFAULT_CMA_NAME); > if (ret) > return ret; > > + legacy_cma_name = cma_get_name(default_cma); > + > + if (IS_ENABLED(CONFIG_DMABUF_HEAPS_CMA_LEGACY) && > + strcmp(legacy_cma_name, DEFAULT_CMA_NAME)) { > + ret = __add_cma_heap(default_cma, legacy_cma_name); > + if (ret) > + pr_warn("cma_heap: failed to add legacy heap: %pe\n", > + ERR_PTR(-ret)); > + } > + It would also simplify this part, since you would always create the legacy heap. Maxime

4 months, 1 week

Re: [PATCH v3] drm/gem: Internally test import_attach for imported objects

by Thomas Zimmermann

Hi Am 05.05.25 um 09:29 schrieb Boris Brezillon: > On Wed, 30 Apr 2025 11:07:17 +0200 > Simona Vetter <simona.vetter(a)ffwll.ch> wrote: > >> On Wed, Apr 16, 2025 at 11:38:03AM +0200, Simona Vetter wrote: >>> On Wed, Apr 16, 2025 at 08:57:45AM +0200, Thomas Zimmermann wrote: >>>> Test struct drm_gem_object.import_attach to detect imported objects. >>>> >>>> During object clenanup, the dma_buf field might be NULL. Testing it in >>>> an object's free callback then incorrectly does a cleanup as for native >>>> objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that >>>> clears the dma_buf field in drm_gem_object_exported_dma_buf_free(). >>>> >>>> v3: >>>> - only test for import_attach (Boris) >>>> v2: >>>> - use import_attach.dmabuf instead of dma_buf (Christian) >>>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> >>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper") >>>> Reported-by: Andy Yan <andyshrk(a)163.com> >>>> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andysh… >>>> Tested-by: Andy Yan <andyshrk(a)163.com> >>>> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> >>>> Cc: Anusha Srivatsa <asrivats(a)redhat.com> >>>> Cc: Christian König <christian.koenig(a)amd.com> >>>> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> >>>> Cc: Maxime Ripard <mripard(a)kernel.org> >>>> Cc: David Airlie <airlied(a)gmail.com> >>>> Cc: Simona Vetter <simona(a)ffwll.ch> >>>> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> >>>> Cc: "Christian König" <christian.koenig(a)amd.com> >>>> Cc: dri-devel(a)lists.freedesktop.org >>>> Cc: linux-media(a)vger.kernel.org >>>> Cc: linaro-mm-sig(a)lists.linaro.org >>> Reviewed-by: Simona Vetter <simona.vetter(a)ffwll.ch> >> Also quick doc request: We do have a bit of overview documentation for >> prime here about specifically this lifetime fun, and why there's a chain >> of references and hence a distinction between imported foreign dma-buf and >> re-imported native dma-buf: >> >> https://dri.freedesktop.org/docs/drm/gpu/drm-mm.html#reference-counting-for… >> >> I think it would be good to augment this with more links to functions >> (like this one recently added and fixed in this patch here) and struct >> members to that overview. And maybe also link from key function and struct >> functions back to that overview doc. Otherwise I think the next person >> will get confused by this rather tricky code again and break a corner >> cases. > BTW, could we also backmerge 6.15-rc5 into drm-misc-next so the fix is > also present in drm-misc-next? drm-misc-next is now at -rc5 Best regards Thomas -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)

4 months, 1 week

[PATCH AUTOSEL 6.6 115/294] drm/gem: Test for imported GEM buffers with helper

by Sasha Levin

From: Thomas Zimmermann <tzimmermann(a)suse.de> [ Upstream commit b57aa47d39e94dc47403a745e2024664e544078c ] Add drm_gem_is_imported() that tests if a GEM object's buffer has been imported. Update the GEM code accordingly. GEM code usually tests for imports if import_attach has been set in struct drm_gem_object. But attaching a dma-buf on import requires a DMA-capable importer device, which is not the case for many serial busses like USB or I2C. The new helper tests if a GEM object's dma-buf has been created from the GEM object. Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Reviewed-by: Anusha Srivatsa <asrivats(a)redhat.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/20250226172457.217725-2-tzimm… Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/gpu/drm/drm_gem.c | 4 ++-- include/drm/drm_gem.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 44a948b80ee14..deb93f78ce344 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -322,7 +322,7 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, return -ENOENT; /* Don't allow imported objects to be mapped */ - if (obj->import_attach) { + if (drm_gem_is_imported(obj)) { ret = -EINVAL; goto out; } @@ -1155,7 +1155,7 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent, drm_vma_node_start(&obj->vma_node)); drm_printf_indent(p, indent, "size=%zu\n", obj->size); drm_printf_indent(p, indent, "imported=%s\n", - str_yes_no(obj->import_attach)); + str_yes_no(drm_gem_is_imported(obj))); if (obj->funcs->print_info) obj->funcs->print_info(p, indent, obj); diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 7c2ec139c464a..fbfccb96dd17b 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -35,6 +35,7 @@ */ #include <linux/kref.h> +#include <linux/dma-buf.h> #include <linux/dma-resv.h> #include <linux/list.h> #include <linux/mutex.h> @@ -557,6 +558,19 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje return (obj->handle_count > 1) || obj->dma_buf; } +/** + * drm_gem_is_imported() - Tests if GEM object's buffer has been imported + * @obj: the GEM object + * + * Returns: + * True if the GEM object's buffer has been imported, false otherwise + */ +static inline bool drm_gem_is_imported(const struct drm_gem_object *obj) +{ + /* The dma-buf's priv field points to the original GEM object. */ + return obj->dma_buf && (obj->dma_buf->priv != obj); +} + #ifdef CONFIG_LOCKDEP /** * drm_gem_gpuva_set_lock() - Set the lock protecting accesses to the gpuva list. -- 2.39.5

4 months, 1 week

[PATCH AUTOSEL 6.12 277/486] drm/amdgpu: remove all KFD fences from the BO on release

by Sasha Levin

From: Christian König <christian.koenig(a)amd.com> [ Upstream commit cb0de06d1b0afb2d0c600ad748069f5ce27730ec ] Remove all KFD BOs from the private dma_resv object. This prevents the KFD from being evict unecessarily when an exported BO is released. Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: James Zhu <James.Zhu(a)amd.com> Reviewed-by: Felix Kuehling <felix.kuehling(a)amd.com> Reviewed-and-tested-by: James Zhu <James.Zhu(a)amd.com> Signed-off-by: Alex Deucher <alexander.deucher(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 5 +- .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 52 ++++++++----------- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 38 ++++++++------ 3 files changed, 47 insertions(+), 48 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index f9d1194484423..581fe1a48f376 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h @@ -192,7 +192,7 @@ int kfd_debugfs_kfd_mem_limits(struct seq_file *m, void *data); #if IS_ENABLED(CONFIG_HSA_AMD) bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm); struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f); -int amdgpu_amdkfd_remove_fence_on_pt_pd_bos(struct amdgpu_bo *bo); +void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo); int amdgpu_amdkfd_evict_userptr(struct mmu_interval_notifier *mni, unsigned long cur_seq, struct kgd_mem *mem); int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo, @@ -212,9 +212,8 @@ struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f) } static inline -int amdgpu_amdkfd_remove_fence_on_pt_pd_bos(struct amdgpu_bo *bo) +void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo) { - return 0; } static inline diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index fa572ba7f9fc1..1465b3adacb0a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -370,40 +370,32 @@ static int amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo, return 0; } -int amdgpu_amdkfd_remove_fence_on_pt_pd_bos(struct amdgpu_bo *bo) +/** + * amdgpu_amdkfd_remove_all_eviction_fences - Remove all eviction fences + * @bo: the BO where to remove the evictions fences from. + * + * This functions should only be used on release when all references to the BO + * are already dropped. We remove the eviction fence from the private copy of + * the dma_resv object here since that is what is used during release to + * determine of the BO is idle or not. + */ +void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo) { - struct amdgpu_bo *root = bo; - struct amdgpu_vm_bo_base *vm_bo; - struct amdgpu_vm *vm; - struct amdkfd_process_info *info; - struct amdgpu_amdkfd_fence *ef; - int ret; - - /* we can always get vm_bo from root PD bo.*/ - while (root->parent) - root = root->parent; + struct dma_resv *resv = &bo->tbo.base._resv; + struct dma_fence *fence, *stub; + struct dma_resv_iter cursor; - vm_bo = root->vm_bo; - if (!vm_bo) - return 0; + dma_resv_assert_held(resv); - vm = vm_bo->vm; - if (!vm) - return 0; - - info = vm->process_info; - if (!info || !info->eviction_fence) - return 0; - - ef = container_of(dma_fence_get(&info->eviction_fence->base), - struct amdgpu_amdkfd_fence, base); - - BUG_ON(!dma_resv_trylock(bo->tbo.base.resv)); - ret = amdgpu_amdkfd_remove_eviction_fence(bo, ef); - dma_resv_unlock(bo->tbo.base.resv); + stub = dma_fence_get_stub(); + dma_resv_for_each_fence(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP, fence) { + if (!to_amdgpu_amdkfd_fence(fence)) + continue; - dma_fence_put(&ef->base); - return ret; + dma_resv_replace_fences(resv, fence->context, stub, + DMA_RESV_USAGE_BOOKKEEP); + } + dma_fence_put(stub); } static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 4c4bdc4f51b29..fc588ef598c09 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -1246,28 +1246,36 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo) if (abo->kfd_bo) amdgpu_amdkfd_release_notify(abo); - /* We only remove the fence if the resv has individualized. */ - WARN_ON_ONCE(bo->type == ttm_bo_type_kernel - && bo->base.resv != &bo->base._resv); - if (bo->base.resv == &bo->base._resv) - amdgpu_amdkfd_remove_fence_on_pt_pd_bos(abo); + /* + * We lock the private dma_resv object here and since the BO is about to + * be released nobody else should have a pointer to it. + * So when this locking here fails something is wrong with the reference + * counting. + */ + if (WARN_ON_ONCE(!dma_resv_trylock(&bo->base._resv))) + return; + + amdgpu_amdkfd_remove_all_eviction_fences(abo); if (!bo->resource || bo->resource->mem_type != TTM_PL_VRAM || !(abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE) || adev->in_suspend || drm_dev_is_unplugged(adev_to_drm(adev))) - return; + goto out; - if (WARN_ON_ONCE(!dma_resv_trylock(bo->base.resv))) - return; + r = dma_resv_reserve_fences(&bo->base._resv, 1); + if (r) + goto out; - r = amdgpu_fill_buffer(abo, 0, bo->base.resv, &fence, true); - if (!WARN_ON(r)) { - amdgpu_vram_mgr_set_cleared(bo->resource); - amdgpu_bo_fence(abo, fence, false); - dma_fence_put(fence); - } + r = amdgpu_fill_buffer(abo, 0, &bo->base._resv, &fence, true); + if (WARN_ON(r)) + goto out; + + amdgpu_vram_mgr_set_cleared(bo->resource); + dma_resv_add_fence(&bo->base._resv, fence, DMA_RESV_USAGE_KERNEL); + dma_fence_put(fence); - dma_resv_unlock(bo->base.resv); +out: + dma_resv_unlock(&bo->base._resv); } /** -- 2.39.5

4 months, 1 week

[PATCH AUTOSEL 6.12 192/486] drm/gem: Test for imported GEM buffers with helper

by Sasha Levin

From: Thomas Zimmermann <tzimmermann(a)suse.de> [ Upstream commit b57aa47d39e94dc47403a745e2024664e544078c ] Add drm_gem_is_imported() that tests if a GEM object's buffer has been imported. Update the GEM code accordingly. GEM code usually tests for imports if import_attach has been set in struct drm_gem_object. But attaching a dma-buf on import requires a DMA-capable importer device, which is not the case for many serial busses like USB or I2C. The new helper tests if a GEM object's dma-buf has been created from the GEM object. Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Reviewed-by: Anusha Srivatsa <asrivats(a)redhat.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/20250226172457.217725-2-tzimm… Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/gpu/drm/drm_gem.c | 4 ++-- include/drm/drm_gem.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 149b8e25da5bb..426d0867882df 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -322,7 +322,7 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, return -ENOENT; /* Don't allow imported objects to be mapped */ - if (obj->import_attach) { + if (drm_gem_is_imported(obj)) { ret = -EINVAL; goto out; } @@ -1152,7 +1152,7 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent, drm_vma_node_start(&obj->vma_node)); drm_printf_indent(p, indent, "size=%zu\n", obj->size); drm_printf_indent(p, indent, "imported=%s\n", - str_yes_no(obj->import_attach)); + str_yes_no(drm_gem_is_imported(obj))); if (obj->funcs->print_info) obj->funcs->print_info(p, indent, obj); diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index d8b86df2ec0da..70c0f8c83629d 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -35,6 +35,7 @@ */ #include <linux/kref.h> +#include <linux/dma-buf.h> #include <linux/dma-resv.h> #include <linux/list.h> #include <linux/mutex.h> @@ -570,6 +571,19 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje return (obj->handle_count > 1) || obj->dma_buf; } +/** + * drm_gem_is_imported() - Tests if GEM object's buffer has been imported + * @obj: the GEM object + * + * Returns: + * True if the GEM object's buffer has been imported, false otherwise + */ +static inline bool drm_gem_is_imported(const struct drm_gem_object *obj) +{ + /* The dma-buf's priv field points to the original GEM object. */ + return obj->dma_buf && (obj->dma_buf->priv != obj); +} + #ifdef CONFIG_LOCKDEP /** * drm_gem_gpuva_set_lock() - Set the lock protecting accesses to the gpuva list. -- 2.39.5

4 months, 1 week

[PATCH AUTOSEL 6.14 628/642] drm/gem: Internally test import_attach for imported objects

by Sasha Levin

From: Thomas Zimmermann <tzimmermann(a)suse.de> [ Upstream commit 8260731ccad0451207b45844bb66eb161a209218 ] Test struct drm_gem_object.import_attach to detect imported objects. During object clenanup, the dma_buf field might be NULL. Testing it in an object's free callback then incorrectly does a cleanup as for native objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that clears the dma_buf field in drm_gem_object_exported_dma_buf_free(). v3: - only test for import_attach (Boris) v2: - use import_attach.dmabuf instead of dma_buf (Christian) Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper") Reported-by: Andy Yan <andyshrk(a)163.com> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andysh… Tested-by: Andy Yan <andyshrk(a)163.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: David Airlie <airlied(a)gmail.com> Cc: Simona Vetter <simona(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Reviewed-by: Simona Vetter <simona.vetter(a)ffwll.ch> Link: https://lore.kernel.org/r/20250416065820.26076-1-tzimmermann@suse.de Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- include/drm/drm_gem.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 2bf893eabb4b2..bcd54020d6ba5 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -585,8 +585,7 @@ static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje */ static inline bool drm_gem_is_imported(const struct drm_gem_object *obj) { - /* The dma-buf's priv field points to the original GEM object. */ - return obj->dma_buf && (obj->dma_buf->priv != obj); + return !!obj->import_attach; } #ifdef CONFIG_LOCKDEP -- 2.39.5

4 months, 1 week

[PATCH AUTOSEL 6.14 355/642] drm/amdgpu: remove all KFD fences from the BO on release

by Sasha Levin

From: Christian König <christian.koenig(a)amd.com> [ Upstream commit cb0de06d1b0afb2d0c600ad748069f5ce27730ec ] Remove all KFD BOs from the private dma_resv object. This prevents the KFD from being evict unecessarily when an exported BO is released. Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: James Zhu <James.Zhu(a)amd.com> Reviewed-by: Felix Kuehling <felix.kuehling(a)amd.com> Reviewed-and-tested-by: James Zhu <James.Zhu(a)amd.com> Signed-off-by: Alex Deucher <alexander.deucher(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 5 +- .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 52 ++++++++----------- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 38 ++++++++------ 3 files changed, 47 insertions(+), 48 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index 8af67f18500a7..2f48dc5747aa2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h @@ -192,7 +192,7 @@ int kfd_debugfs_kfd_mem_limits(struct seq_file *m, void *data); #if IS_ENABLED(CONFIG_HSA_AMD) bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm); struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f); -int amdgpu_amdkfd_remove_fence_on_pt_pd_bos(struct amdgpu_bo *bo); +void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo); int amdgpu_amdkfd_evict_userptr(struct mmu_interval_notifier *mni, unsigned long cur_seq, struct kgd_mem *mem); int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo, @@ -212,9 +212,8 @@ struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f) } static inline -int amdgpu_amdkfd_remove_fence_on_pt_pd_bos(struct amdgpu_bo *bo) +void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo) { - return 0; } static inline diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 70224b9f54f2f..c0aaa72b6c210 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -370,40 +370,32 @@ static int amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo, return 0; } -int amdgpu_amdkfd_remove_fence_on_pt_pd_bos(struct amdgpu_bo *bo) +/** + * amdgpu_amdkfd_remove_all_eviction_fences - Remove all eviction fences + * @bo: the BO where to remove the evictions fences from. + * + * This functions should only be used on release when all references to the BO + * are already dropped. We remove the eviction fence from the private copy of + * the dma_resv object here since that is what is used during release to + * determine of the BO is idle or not. + */ +void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo) { - struct amdgpu_bo *root = bo; - struct amdgpu_vm_bo_base *vm_bo; - struct amdgpu_vm *vm; - struct amdkfd_process_info *info; - struct amdgpu_amdkfd_fence *ef; - int ret; - - /* we can always get vm_bo from root PD bo.*/ - while (root->parent) - root = root->parent; + struct dma_resv *resv = &bo->tbo.base._resv; + struct dma_fence *fence, *stub; + struct dma_resv_iter cursor; - vm_bo = root->vm_bo; - if (!vm_bo) - return 0; + dma_resv_assert_held(resv); - vm = vm_bo->vm; - if (!vm) - return 0; - - info = vm->process_info; - if (!info || !info->eviction_fence) - return 0; - - ef = container_of(dma_fence_get(&info->eviction_fence->base), - struct amdgpu_amdkfd_fence, base); - - BUG_ON(!dma_resv_trylock(bo->tbo.base.resv)); - ret = amdgpu_amdkfd_remove_eviction_fence(bo, ef); - dma_resv_unlock(bo->tbo.base.resv); + stub = dma_fence_get_stub(); + dma_resv_for_each_fence(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP, fence) { + if (!to_amdgpu_amdkfd_fence(fence)) + continue; - dma_fence_put(&ef->base); - return ret; + dma_resv_replace_fences(resv, fence->context, stub, + DMA_RESV_USAGE_BOOKKEEP); + } + dma_fence_put(stub); } static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 00752e3f9d8ab..0b9987781f762 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -1295,28 +1295,36 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo) if (abo->kfd_bo) amdgpu_amdkfd_release_notify(abo); - /* We only remove the fence if the resv has individualized. */ - WARN_ON_ONCE(bo->type == ttm_bo_type_kernel - && bo->base.resv != &bo->base._resv); - if (bo->base.resv == &bo->base._resv) - amdgpu_amdkfd_remove_fence_on_pt_pd_bos(abo); + /* + * We lock the private dma_resv object here and since the BO is about to + * be released nobody else should have a pointer to it. + * So when this locking here fails something is wrong with the reference + * counting. + */ + if (WARN_ON_ONCE(!dma_resv_trylock(&bo->base._resv))) + return; + + amdgpu_amdkfd_remove_all_eviction_fences(abo); if (!bo->resource || bo->resource->mem_type != TTM_PL_VRAM || !(abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE) || adev->in_suspend || drm_dev_is_unplugged(adev_to_drm(adev))) - return; + goto out; - if (WARN_ON_ONCE(!dma_resv_trylock(bo->base.resv))) - return; + r = dma_resv_reserve_fences(&bo->base._resv, 1); + if (r) + goto out; - r = amdgpu_fill_buffer(abo, 0, bo->base.resv, &fence, true); - if (!WARN_ON(r)) { - amdgpu_vram_mgr_set_cleared(bo->resource); - amdgpu_bo_fence(abo, fence, false); - dma_fence_put(fence); - } + r = amdgpu_fill_buffer(abo, 0, &bo->base._resv, &fence, true); + if (WARN_ON(r)) + goto out; + + amdgpu_vram_mgr_set_cleared(bo->resource); + dma_resv_add_fence(&bo->base._resv, fence, DMA_RESV_USAGE_KERNEL); + dma_fence_put(fence); - dma_resv_unlock(bo->base.resv); +out: + dma_resv_unlock(&bo->base._resv); } /** -- 2.39.5

4 months, 1 week

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig