Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

45 participants
6399 discussions

Re: [PATCH v6 09/10] optee: FF-A: dynamic restricted memory allocation

by Jens Wiklander

On Tue, Apr 1, 2025 at 12:13 PM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > + MM folks to seek guidance here. > > On Thu, Mar 27, 2025 at 09:07:34AM +0100, Jens Wiklander wrote: > > Hi Sumit, > > > > On Tue, Mar 25, 2025 at 8:42 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > On Wed, Mar 05, 2025 at 02:04:15PM +0100, Jens Wiklander wrote: > > > > Add support in the OP-TEE backend driver dynamic restricted memory > > > > allocation with FF-A. > > > > > > > > The restricted memory pools for dynamically allocated restrict memory > > > > are instantiated when requested by user-space. This instantiation can > > > > fail if OP-TEE doesn't support the requested use-case of restricted > > > > memory. > > > > > > > > Restricted memory pools based on a static carveout or dynamic allocation > > > > can coexist for different use-cases. We use only dynamic allocation with > > > > FF-A. > > > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > > > --- > > > > drivers/tee/optee/Makefile | 1 + > > > > drivers/tee/optee/ffa_abi.c | 143 ++++++++++++- > > > > drivers/tee/optee/optee_private.h | 13 +- > > > > drivers/tee/optee/rstmem.c | 329 ++++++++++++++++++++++++++++++ > > > > 4 files changed, 483 insertions(+), 3 deletions(-) > > > > create mode 100644 drivers/tee/optee/rstmem.c > > > > > > <snip> > > > > > diff --git a/drivers/tee/optee/rstmem.c b/drivers/tee/optee/rstmem.c > > > > new file mode 100644 > > > > index 000000000000..ea27769934d4 > > > > --- /dev/null > > > > +++ b/drivers/tee/optee/rstmem.c > > > > @@ -0,0 +1,329 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-only > > > > +/* > > > > + * Copyright (c) 2025, Linaro Limited > > > > + */ > > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > + > > > > +#include <linux/errno.h> > > > > +#include <linux/genalloc.h> > > > > +#include <linux/slab.h> > > > > +#include <linux/string.h> > > > > +#include <linux/tee_core.h> > > > > +#include <linux/types.h> > > > > +#include "optee_private.h" > > > > + > > > > +struct optee_rstmem_cma_pool { > > > > + struct tee_rstmem_pool pool; > > > > + struct gen_pool *gen_pool; > > > > + struct optee *optee; > > > > + size_t page_count; > > > > + u16 *end_points; > > > > + u_int end_point_count; > > > > + u_int align; > > > > + refcount_t refcount; > > > > + u32 use_case; > > > > + struct tee_shm *rstmem; > > > > + /* Protects when initializing and tearing down this struct */ > > > > + struct mutex mutex; > > > > +}; > > > > + > > > > +static struct optee_rstmem_cma_pool * > > > > +to_rstmem_cma_pool(struct tee_rstmem_pool *pool) > > > > +{ > > > > + return container_of(pool, struct optee_rstmem_cma_pool, pool); > > > > +} > > > > + > > > > +static int init_cma_rstmem(struct optee_rstmem_cma_pool *rp) > > > > +{ > > > > + int rc; > > > > + > > > > + rp->rstmem = tee_shm_alloc_cma_phys_mem(rp->optee->ctx, rp->page_count, > > > > + rp->align); > > > > + if (IS_ERR(rp->rstmem)) { > > > > + rc = PTR_ERR(rp->rstmem); > > > > + goto err_null_rstmem; > > > > + } > > > > + > > > > + /* > > > > + * TODO unmap the memory range since the physical memory will > > > > + * become inaccesible after the lend_rstmem() call. > > > > + */ > > > > > > What's your plan for this TODO? I think we need a CMA allocator here > > > which can allocate un-mapped memory such that any cache speculation > > > won't lead to CPU hangs once the memory restriction comes into picture. > > > > What happens is platform-specific. For some platforms, it might be > > enough to avoid explicit access. Yes, a CMA allocator with unmapped > > memory or where memory can be unmapped is one option. > > Did you get a chance to enable real memory protection on RockPi board? No, I don't think I have access to the needed documentation for the board to set it up for relevant peripherals. > This will atleast ensure that mapped restricted memory without explicit > access works fine. Since otherwise once people start to enable real > memory restriction in OP-TEE, there can be chances of random hang ups > due to cache speculation. A hypervisor in the normal world can also make the memory inaccessible to the kernel. That shouldn't cause any hangups due to cache speculation. Cheers, Jens > > MM folks, > > Basically what we are trying to achieve here is a "no-map" DT behaviour > [1] which is rather dynamic in nature. The use-case here is that a memory > block allocated from CMA can be marked restricted at runtime where we > would like the Linux not being able to directly or indirectly (cache > speculation) access it. Once memory restriction use-case has been > completed, the memory block can be marked as normal and freed for > further CMA allocation. > > It will be apprciated if you can guide us regarding the appropriate APIs > to use for un-mapping/mamping CMA allocations for this use-case. > > [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/rese… > > -Sumit > > > > > > > > > > + rc = rp->optee->ops->lend_rstmem(rp->optee, rp->rstmem, rp->end_points, > > > > + rp->end_point_count, rp->use_case); > > > > + if (rc) > > > > + goto err_put_shm; > > > > + rp->rstmem->flags |= TEE_SHM_DYNAMIC; > > > > + > > > > + rp->gen_pool = gen_pool_create(PAGE_SHIFT, -1); > > > > + if (!rp->gen_pool) { > > > > + rc = -ENOMEM; > > > > + goto err_reclaim; > > > > + } > > > > + > > > > + rc = gen_pool_add(rp->gen_pool, rp->rstmem->paddr, > > > > + rp->rstmem->size, -1); > > > > + if (rc) > > > > + goto err_free_pool; > > > > + > > > > + refcount_set(&rp->refcount, 1); > > > > + return 0; > > > > + > > > > +err_free_pool: > > > > + gen_pool_destroy(rp->gen_pool); > > > > + rp->gen_pool = NULL; > > > > +err_reclaim: > > > > + rp->optee->ops->reclaim_rstmem(rp->optee, rp->rstmem); > > > > +err_put_shm: > > > > + tee_shm_put(rp->rstmem); > > > > +err_null_rstmem: > > > > + rp->rstmem = NULL; > > > > + return rc; > > > > +} > > > > + > > > > +static int get_cma_rstmem(struct optee_rstmem_cma_pool *rp) > > > > +{ > > > > + int rc = 0; > > > > + > > > > + if (!refcount_inc_not_zero(&rp->refcount)) { > > > > + mutex_lock(&rp->mutex); > > > > + if (rp->gen_pool) { > > > > + /* > > > > + * Another thread has already initialized the pool > > > > + * before us, or the pool was just about to be torn > > > > + * down. Either way we only need to increase the > > > > + * refcount and we're done. > > > > + */ > > > > + refcount_inc(&rp->refcount); > > > > + } else { > > > > + rc = init_cma_rstmem(rp); > > > > + } > > > > + mutex_unlock(&rp->mutex); > > > > + } > > > > + > > > > + return rc; > > > > +} > > > > + > > > > +static void release_cma_rstmem(struct optee_rstmem_cma_pool *rp) > > > > +{ > > > > + gen_pool_destroy(rp->gen_pool); > > > > + rp->gen_pool = NULL; > > > > + > > > > + rp->optee->ops->reclaim_rstmem(rp->optee, rp->rstmem); > > > > + rp->rstmem->flags &= ~TEE_SHM_DYNAMIC; > > > > + > > > > + WARN(refcount_read(&rp->rstmem->refcount) != 1, "Unexpected refcount"); > > > > + tee_shm_put(rp->rstmem); > > > > + rp->rstmem = NULL; > > > > +} > > > > + > > > > +static void put_cma_rstmem(struct optee_rstmem_cma_pool *rp) > > > > +{ > > > > + if (refcount_dec_and_test(&rp->refcount)) { > > > > + mutex_lock(&rp->mutex); > > > > + if (rp->gen_pool) > > > > + release_cma_rstmem(rp); > > > > + mutex_unlock(&rp->mutex); > > > > + } > > > > +} > > > > + > > > > +static int rstmem_pool_op_cma_alloc(struct tee_rstmem_pool *pool, > > > > + struct sg_table *sgt, size_t size, > > > > + size_t *offs) > > > > +{ > > > > + struct optee_rstmem_cma_pool *rp = to_rstmem_cma_pool(pool); > > > > + size_t sz = ALIGN(size, PAGE_SIZE); > > > > + phys_addr_t pa; > > > > + int rc; > > > > + > > > > + rc = get_cma_rstmem(rp); > > > > + if (rc) > > > > + return rc; > > > > + > > > > + pa = gen_pool_alloc(rp->gen_pool, sz); > > > > + if (!pa) { > > > > + rc = -ENOMEM; > > > > + goto err_put; > > > > + } > > > > + > > > > + rc = sg_alloc_table(sgt, 1, GFP_KERNEL); > > > > + if (rc) > > > > + goto err_free; > > > > + > > > > + sg_set_page(sgt->sgl, phys_to_page(pa), size, 0); > > > > + *offs = pa - rp->rstmem->paddr; > > > > + > > > > + return 0; > > > > +err_free: > > > > + gen_pool_free(rp->gen_pool, pa, size); > > > > +err_put: > > > > + put_cma_rstmem(rp); > > > > + > > > > + return rc; > > > > +} > > > > + > > > > +static void rstmem_pool_op_cma_free(struct tee_rstmem_pool *pool, > > > > + struct sg_table *sgt) > > > > +{ > > > > + struct optee_rstmem_cma_pool *rp = to_rstmem_cma_pool(pool); > > > > + struct scatterlist *sg; > > > > + int i; > > > > + > > > > + for_each_sgtable_sg(sgt, sg, i) > > > > + gen_pool_free(rp->gen_pool, sg_phys(sg), sg->length); > > > > + sg_free_table(sgt); > > > > + put_cma_rstmem(rp); > > > > +} > > > > + > > > > +static int rstmem_pool_op_cma_update_shm(struct tee_rstmem_pool *pool, > > > > + struct sg_table *sgt, size_t offs, > > > > + struct tee_shm *shm, > > > > + struct tee_shm **parent_shm) > > > > +{ > > > > + struct optee_rstmem_cma_pool *rp = to_rstmem_cma_pool(pool); > > > > + > > > > + *parent_shm = rp->rstmem; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void pool_op_cma_destroy_pool(struct tee_rstmem_pool *pool) > > > > +{ > > > > + struct optee_rstmem_cma_pool *rp = to_rstmem_cma_pool(pool); > > > > + > > > > + mutex_destroy(&rp->mutex); > > > > + kfree(rp); > > > > +} > > > > + > > > > +static struct tee_rstmem_pool_ops rstmem_pool_ops_cma = { > > > > + .alloc = rstmem_pool_op_cma_alloc, > > > > + .free = rstmem_pool_op_cma_free, > > > > + .update_shm = rstmem_pool_op_cma_update_shm, > > > > + .destroy_pool = pool_op_cma_destroy_pool, > > > > +}; > > > > + > > > > +static int get_rstmem_config(struct optee *optee, u32 use_case, > > > > + size_t *min_size, u_int *min_align, > > > > + u16 *end_points, u_int *ep_count) > > > > > > I guess this end points terminology is specific to FF-A ABI. Is there > > > any relevance for this in the common APIs? > > > > Yes, endpoints are specific to FF-A ABI. The list of end-points must > > be presented to FFA_MEM_LEND. We're relying on the secure world to > > know which endpoints are needed for a specific use case. > > > > Cheers, > > Jens > > > > > > > > -Sumit > > > > > > > +{ > > > > + struct tee_param params[2] = { > > > > + [0] = { > > > > + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT, > > > > + .u.value.a = use_case, > > > > + }, > > > > + [1] = { > > > > + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT, > > > > + }, > > > > + }; > > > > + struct optee_shm_arg_entry *entry; > > > > + struct tee_shm *shm_param = NULL; > > > > + struct optee_msg_arg *msg_arg; > > > > + struct tee_shm *shm; > > > > + u_int offs; > > > > + int rc; > > > > + > > > > + if (end_points && *ep_count) { > > > > + params[1].u.memref.size = *ep_count * sizeof(*end_points); > > > > + shm_param = tee_shm_alloc_priv_buf(optee->ctx, > > > > + params[1].u.memref.size); > > > > + if (IS_ERR(shm_param)) > > > > + return PTR_ERR(shm_param); > > > > + params[1].u.memref.shm = shm_param; > > > > + } > > > > + > > > > + msg_arg = optee_get_msg_arg(optee->ctx, ARRAY_SIZE(params), &entry, > > > > + &shm, &offs); > > > > + if (IS_ERR(msg_arg)) { > > > > + rc = PTR_ERR(msg_arg); > > > > + goto out_free_shm; > > > > + } > > > > + msg_arg->cmd = OPTEE_MSG_CMD_GET_RSTMEM_CONFIG; > > > > + > > > > + rc = optee->ops->to_msg_param(optee, msg_arg->params, > > > > + ARRAY_SIZE(params), params, > > > > + false /*!update_out*/); > > > > + if (rc) > > > > + goto out_free_msg; > > > > + > > > > + rc = optee->ops->do_call_with_arg(optee->ctx, shm, offs, false); > > > > + if (rc) > > > > + goto out_free_msg; > > > > + if (msg_arg->ret && msg_arg->ret != TEEC_ERROR_SHORT_BUFFER) { > > > > + rc = -EINVAL; > > > > + goto out_free_msg; > > > > + } > > > > + > > > > + rc = optee->ops->from_msg_param(optee, params, ARRAY_SIZE(params), > > > > + msg_arg->params, true /*update_out*/); > > > > + if (rc) > > > > + goto out_free_msg; > > > > + > > > > + if (!msg_arg->ret && end_points && > > > > + *ep_count < params[1].u.memref.size / sizeof(u16)) { > > > > + rc = -EINVAL; > > > > + goto out_free_msg; > > > > + } > > > > + > > > > + *min_size = params[0].u.value.a; > > > > + *min_align = params[0].u.value.b; > > > > + *ep_count = params[1].u.memref.size / sizeof(u16); > > > > + > > > > + if (msg_arg->ret == TEEC_ERROR_SHORT_BUFFER) { > > > > + rc = -ENOSPC; > > > > + goto out_free_msg; > > > > + } > > > > + > > > > + if (end_points) > > > > + memcpy(end_points, tee_shm_get_va(shm_param, 0), > > > > + params[1].u.memref.size); > > > > + > > > > +out_free_msg: > > > > + optee_free_msg_arg(optee->ctx, entry, offs); > > > > +out_free_shm: > > > > + if (shm_param) > > > > + tee_shm_free(shm_param); > > > > + return rc; > > > > +} > > > > + > > > > +struct tee_rstmem_pool *optee_rstmem_alloc_cma_pool(struct optee *optee, > > > > + enum tee_dma_heap_id id) > > > > +{ > > > > + struct optee_rstmem_cma_pool *rp; > > > > + u32 use_case = id; > > > > + size_t min_size; > > > > + int rc; > > > > + > > > > + rp = kzalloc(sizeof(*rp), GFP_KERNEL); > > > > + if (!rp) > > > > + return ERR_PTR(-ENOMEM); > > > > + rp->use_case = use_case; > > > > + > > > > + rc = get_rstmem_config(optee, use_case, &min_size, &rp->align, NULL, > > > > + &rp->end_point_count); > > > > + if (rc) { > > > > + if (rc != -ENOSPC) > > > > + goto err; > > > > + rp->end_points = kcalloc(rp->end_point_count, > > > > + sizeof(*rp->end_points), GFP_KERNEL); > > > > + if (!rp->end_points) { > > > > + rc = -ENOMEM; > > > > + goto err; > > > > + } > > > > + rc = get_rstmem_config(optee, use_case, &min_size, &rp->align, > > > > + rp->end_points, &rp->end_point_count); > > > > + if (rc) > > > > + goto err_kfree_eps; > > > > + } > > > > + > > > > + rp->pool.ops = &rstmem_pool_ops_cma; > > > > + rp->optee = optee; > > > > + rp->page_count = min_size / PAGE_SIZE; > > > > + mutex_init(&rp->mutex); > > > > + > > > > + return &rp->pool; > > > > + > > > > +err_kfree_eps: > > > > + kfree(rp->end_points); > > > > +err: > > > > + kfree(rp); > > > > + return ERR_PTR(rc); > > > > +} > > > > -- > > > > 2.43.0 > > > >

1 year, 1 month

Re: [PATCH v6 05/10] tee: implement restricted DMA-heap

by Jens Wiklander

On Tue, Apr 1, 2025 at 9:58 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, Mar 25, 2025 at 11:55:46AM +0100, Jens Wiklander wrote: > > Hi Sumit, > > > > <snip> > > > > > > > > > > + > > > > +#include "tee_private.h" > > > > + > > > > +struct tee_dma_heap { > > > > + struct dma_heap *heap; > > > > + enum tee_dma_heap_id id; > > > > + struct tee_rstmem_pool *pool; > > > > + struct tee_device *teedev; > > > > + /* Protects pool and teedev above */ > > > > + struct mutex mu; > > > > +}; > > > > + > > > > +struct tee_heap_buffer { > > > > + struct tee_rstmem_pool *pool; > > > > + struct tee_device *teedev; > > > > + size_t size; > > > > + size_t offs; > > > > + struct sg_table table; > > > > +}; > > > > + > > > > +struct tee_heap_attachment { > > > > + struct sg_table table; > > > > + struct device *dev; > > > > +}; > > > > + > > > > +struct tee_rstmem_static_pool { > > > > + struct tee_rstmem_pool pool; > > > > + struct gen_pool *gen_pool; > > > > + phys_addr_t pa_base; > > > > +}; > > > > + > > > > +#if !IS_MODULE(CONFIG_TEE) && IS_ENABLED(CONFIG_DMABUF_HEAPS) > > > > > > Can this dependency rather be better managed via Kconfig? > > > > This was the easiest yet somewhat flexible solution I could find. If > > you have something better, let's use that instead. > > > > --- a/drivers/tee/optee/Kconfig > +++ b/drivers/tee/optee/Kconfig > @@ -5,6 +5,7 @@ config OPTEE > depends on HAVE_ARM_SMCCC > depends on MMU > depends on RPMB || !RPMB > + select DMABUF_HEAPS > help > This implements the OP-TEE Trusted Execution Environment (TEE) > driver. I wanted to avoid that since there are plenty of use cases where DMABUF_HEAPS aren't needed. This seems to do the job: +config TEE_DMABUF_HEAP + bool + depends on TEE = y && DMABUF_HEAPS We can only use DMABUF_HEAPS if the TEE subsystem is compiled into the kernel. Cheers, Jens

1 year, 1 month

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

by Jens Wiklander

On Tue, Apr 1, 2025 at 9:45 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, Mar 25, 2025 at 09:50:35AM +0100, Jens Wiklander wrote: > > On Tue, Mar 25, 2025 at 6:56 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > On Thu, Mar 20, 2025 at 02:00:57PM +0100, Jens Wiklander wrote: > > > > Hi Sumit, > > > > > > > > On Thu, Mar 20, 2025 at 10:25 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > > > > > Hi Jens, > > > > > > > > > > On Mon, Mar 17, 2025 at 08:42:01AM +0100, Jens Wiklander wrote: > > > > > > 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. > > > > > > > > > > > > > > > > Do we need to update length field for input memrefs when update_out is > > > > > set? I don't see that happening in your existing patch too. > > > > > > > > I'm sorry, I was unclear. The update_out parameter means only the > > > > output fields should be updated, not the attribute, offsets, ids, etc. > > > > That is, the length field for memrefs, and the value fields a, b, c > > > > for value params. Some of the memrefs aren't translated one-to-one > > > > with SDP, but the length field can and must be updated. > > > > > > Isn't it rather better to add another attribute type to handled SDP > > > special handling? > > > > This isn't special handling, all parameters get the same treatment. > > When updating a parameter after it has been used, this is all that > > needs to be done, regardless of whether it's an SDP buffer. The > > updates we did before this patch were redundant. > > > > This patch was introduced in the v3 of this patch set, but I don't > > think it's strictly needed any longer since SDP buffers are allocated > > differently now. I think it's nice to only update what's needed when > > translating back a parameter (just as in params_to_user() in > > drivers/tee/tee_core.c), but if you don't like it, we can drop this > > patch. > > params_to_user() doesn't take any additional parameter like "update_out" > which is complicating the program flow here. Can't we rather follow > similar practice for {to/from}_msg_param() APIs? I'm afraid something special is needed. handle_rpc_supp_cmd() needs all the fields to be updated, from_msg_param() prepares input and attribute type may have changed when to_msg_param() is called. Cheers, Jens

1 year, 1 month

Re: [PATCH v3 03/11] tee: add TEE_IOCTL_PARAM_ATTR_TYPE_UBUF

by kernel test robot

Hi Amirreza, kernel test robot noticed the following build warnings: [auto build test WARNING on db8da9da41bced445077925f8a886c776a47440c] url: https://github.com/intel-lab-lkp/linux/commits/Amirreza-Zarrabi/tee-allow-a… base: db8da9da41bced445077925f8a886c776a47440c patch link: https://lore.kernel.org/r/20250327-qcom-tee-using-tee-ss-without-mem-obj-v3… patch subject: [PATCH v3 03/11] tee: add TEE_IOCTL_PARAM_ATTR_TYPE_UBUF config: x86_64-randconfig-122-20250329 (https://download.01.org/0day-ci/archive/20250329/202503291204.imMRd3l7-lkp@…) compiler: clang version 20.1.1 (https://github.com/llvm/llvm-project 424c2d9b7e4de40d0804dd374721e6411c27d1d1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250329/202503291204.imMRd3l7-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/202503291204.imMRd3l7-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/tee/tee_core.c:410:48: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected void *[noderef] uaddr @@ got void [noderef] __user * @@ drivers/tee/tee_core.c:410:48: sparse: expected void *[noderef] uaddr drivers/tee/tee_core.c:410:48: sparse: got void [noderef] __user * >> drivers/tee/tee_core.c:413:30: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const [noderef] __user *ptr @@ got void *[noderef] uaddr @@ drivers/tee/tee_core.c:413:30: sparse: expected void const [noderef] __user *ptr drivers/tee/tee_core.c:413:30: sparse: got void *[noderef] uaddr drivers/tee/tee_core.c:802:41: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected void *[noderef] uaddr @@ got void [noderef] __user * @@ drivers/tee/tee_core.c:802:41: sparse: expected void *[noderef] uaddr drivers/tee/tee_core.c:802:41: sparse: got void [noderef] __user * drivers/tee/tee_core.c:805:30: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const [noderef] __user *ptr @@ got void *[noderef] uaddr @@ drivers/tee/tee_core.c:805:30: sparse: expected void const [noderef] __user *ptr drivers/tee/tee_core.c:805:30: sparse: got void *[noderef] uaddr >> drivers/tee/tee_core.c:413:30: sparse: sparse: dereference of noderef expression >> drivers/tee/tee_core.c:413:30: sparse: sparse: dereference of noderef expression drivers/tee/tee_core.c:694:37: sparse: sparse: dereference of noderef expression drivers/tee/tee_core.c:805:30: sparse: sparse: dereference of noderef expression drivers/tee/tee_core.c:805:30: sparse: sparse: dereference of noderef expression vim +410 drivers/tee/tee_core.c 378 379 static int params_from_user(struct tee_context *ctx, struct tee_param *params, 380 size_t num_params, 381 struct tee_ioctl_param __user *uparams) 382 { 383 size_t n; 384 385 for (n = 0; n < num_params; n++) { 386 struct tee_shm *shm; 387 struct tee_ioctl_param ip; 388 389 if (copy_from_user(&ip, uparams + n, sizeof(ip))) 390 return -EFAULT; 391 392 /* All unused attribute bits has to be zero */ 393 if (ip.attr & ~TEE_IOCTL_PARAM_ATTR_MASK) 394 return -EINVAL; 395 396 params[n].attr = ip.attr; 397 switch (ip.attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK) { 398 case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: 399 case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: 400 break; 401 case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: 402 case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: 403 params[n].u.value.a = ip.a; 404 params[n].u.value.b = ip.b; 405 params[n].u.value.c = ip.c; 406 break; 407 case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT: 408 case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: 409 case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > 410 params[n].u.ubuf.uaddr = u64_to_user_ptr(ip.a); 411 params[n].u.ubuf.size = ip.b; 412 > 413 if (!access_ok(params[n].u.ubuf.uaddr, 414 params[n].u.ubuf.size)) 415 return -EFAULT; 416 417 break; 418 case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: 419 case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: 420 case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: 421 /* 422 * If a NULL pointer is passed to a TA in the TEE, 423 * the ip.c IOCTL parameters is set to TEE_MEMREF_NULL 424 * indicating a NULL memory reference. 425 */ 426 if (ip.c != TEE_MEMREF_NULL) { 427 /* 428 * If we fail to get a pointer to a shared 429 * memory object (and increase the ref count) 430 * from an identifier we return an error. All 431 * pointers that has been added in params have 432 * an increased ref count. It's the callers 433 * responibility to do tee_shm_put() on all 434 * resolved pointers. 435 */ 436 shm = tee_shm_get_from_id(ctx, ip.c); 437 if (IS_ERR(shm)) 438 return PTR_ERR(shm); 439 440 /* 441 * Ensure offset + size does not overflow 442 * offset and does not overflow the size of 443 * the referred shared memory object. 444 */ 445 if ((ip.a + ip.b) < ip.a || 446 (ip.a + ip.b) > shm->size) { 447 tee_shm_put(shm); 448 return -EINVAL; 449 } 450 } else if (ctx->cap_memref_null) { 451 /* Pass NULL pointer to OP-TEE */ 452 shm = NULL; 453 } else { 454 return -EINVAL; 455 } 456 457 params[n].u.memref.shm_offs = ip.a; 458 params[n].u.memref.size = ip.b; 459 params[n].u.memref.shm = shm; 460 break; 461 default: 462 /* Unknown attribute */ 463 return -EINVAL; 464 } 465 } 466 return 0; 467 } 468 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki

1 year, 1 month

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

by kernel test robot

1 year, 1 month

[PATCH v4] drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Add support for exporting a dma_fence fd for a specific point on a timeline. This is needed for vtest/vpipe[1][2] to implement timeline syncobj support, as it needs a way to turn a point on a timeline back into a dma_fence fd. It also closes an odd omission from the syncobj UAPI. [1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/33433 [2] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/805 v2: Add DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE v3: Add unstaged uabi header hunk v4: Also handle IMPORT_SYNC_FILE case Signed-off-by: Rob Clark <robdclark(a)chromium.org> --- drivers/gpu/drm/drm_syncobj.c | 41 ++++++++++++++++++++++++++--------- include/uapi/drm/drm.h | 4 ++++ 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 4f2ab8a7b50f..58d8a9035f7d 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -741,7 +741,7 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private, } static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, - int fd, int handle) + int fd, int handle, u64 point) { struct dma_fence *fence = sync_file_get_fence(fd); struct drm_syncobj *syncobj; @@ -755,14 +755,18 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, return -ENOENT; } - drm_syncobj_replace_fence(syncobj, fence); + if (point) { + drm_syncobj_add_point(syncobj, dma_fence_chain_alloc(), fence, point); + } else { + drm_syncobj_replace_fence(syncobj, fence); + } dma_fence_put(fence); drm_syncobj_put(syncobj); return 0; } static int drm_syncobj_export_sync_file(struct drm_file *file_private, - int handle, int *p_fd) + int handle, u64 point, int *p_fd) { int ret; struct dma_fence *fence; @@ -772,7 +776,7 @@ static int drm_syncobj_export_sync_file(struct drm_file *file_private, if (fd < 0) return fd; - ret = drm_syncobj_find_fence(file_private, handle, 0, 0, &fence); + ret = drm_syncobj_find_fence(file_private, handle, point, 0, &fence); if (ret) goto err_put_fd; @@ -869,6 +873,9 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data, struct drm_file *file_private) { struct drm_syncobj_handle *args = data; + unsigned valid_flags = DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE | + DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE; + u64 point = 0; if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) return -EOPNOTSUPP; @@ -876,13 +883,18 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data, if (args->pad) return -EINVAL; - if (args->flags != 0 && - args->flags != DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE) + if (args->flags != 0 && (args->flags & ~valid_flags)) return -EINVAL; + if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) + point = args->point; + if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE) return drm_syncobj_export_sync_file(file_private, args->handle, - &args->fd); + point, &args->fd); + + if (args->point) + return -EINVAL; return drm_syncobj_handle_to_fd(file_private, args->handle, &args->fd); @@ -893,6 +905,9 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, struct drm_file *file_private) { struct drm_syncobj_handle *args = data; + unsigned valid_flags = DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE | + DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE; + u64 point = 0; if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) return -EOPNOTSUPP; @@ -900,14 +915,20 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, if (args->pad) return -EINVAL; - if (args->flags != 0 && - args->flags != DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE) + if (args->flags != 0 && (args->flags & ~valid_flags)) return -EINVAL; + if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE) + point = args->point; + if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE) return drm_syncobj_import_sync_file_fence(file_private, args->fd, - args->handle); + args->handle, + point); + + if (args->point) + return -EINVAL; return drm_syncobj_fd_to_handle(file_private, args->fd, &args->handle); diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 7fba37b94401..e63a71d3c607 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -905,13 +905,17 @@ struct drm_syncobj_destroy { }; #define DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE (1 << 0) +#define DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE (1 << 1) #define DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE (1 << 0) +#define DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE (1 << 1) struct drm_syncobj_handle { __u32 handle; __u32 flags; __s32 fd; __u32 pad; + + __u64 point; }; struct drm_syncobj_transfer { -- 2.49.0

1 year, 1 month

[PATCH v3] drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs

by Rob Clark

1 year, 1 month

[PATCH v2] drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs

by Rob Clark

1 year, 1 month

Re: [PATCH] drm/nouveau: Prevent signalled fences in pending list

by Christian König

Am 27.03.25 um 09:42 schrieb Philipp Stanner: > Nouveau currently relies on the assumption that dma_fences will only > ever get signalled through nouveau_fence_signal(), which takes care of > removing a signalled fence from the list nouveau_fence_chan.pending. > > This self-imposed rule is violated in nouveau_fence_done(), where > dma_fence_is_signaled() can signal the fence without removing it from > the list. This enables accesses to already signalled fences through the > list, which is a bug. > > Furthermore, it must always be possible to use standard dma_fence > methods an a dma_fence and observe valid behavior. The canonical way of > ensuring that signalling a fence has additional effects is to add those > effects to a callback and register it on the fence. Good catch. > > Move the code from nouveau_fence_signal() into a dma_fence callback. > Register that callback when creating the fence. But that's a really ugly approach. Either nouveau shouldn't implement the signaled callback or make sure that when returning true from the signaled callback the fence is also removed from the pending list. > > Cc: <stable(a)vger.kernel.org> # 4.10+ > Fixes: f54d1867005c ("dma-buf: Rename struct fence to dma_fence") > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > I'm not entirely sure what Fixes-Tag is appropriate. The last time the > line causing the signalled fence in the list was touched is the commit > listed above. Yeah, that's most likely not correct. My educated guess is that the bug was always there just never discovered. Regards, Christian. > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 41 ++++++++++++++++--------- > drivers/gpu/drm/nouveau/nouveau_fence.h | 1 + > 2 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index 7cc84472cece..b2c2241a8803 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -50,24 +50,22 @@ nouveau_fctx(struct nouveau_fence *fence) > return container_of(fence->base.lock, struct nouveau_fence_chan, lock); > } > > -static int > -nouveau_fence_signal(struct nouveau_fence *fence) > +static void > +nouveau_fence_cleanup_cb(struct dma_fence *dfence, struct dma_fence_cb *cb) > { > - int drop = 0; > + struct nouveau_fence_chan *fctx; > + struct nouveau_fence *fence; > + > + fence = container_of(dfence, struct nouveau_fence, base); > + fctx = nouveau_fctx(fence); > > - dma_fence_signal_locked(&fence->base); > list_del(&fence->head); > rcu_assign_pointer(fence->channel, NULL); > > - if (test_bit(DMA_FENCE_FLAG_USER_BITS, &fence->base.flags)) { > - struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > - > - if (!--fctx->notify_ref) > - drop = 1; > - } > + if (test_bit(DMA_FENCE_FLAG_USER_BITS, &fence->base.flags)) > + --fctx->notify_ref; > > dma_fence_put(&fence->base); > - return drop; > } > > static struct nouveau_fence * > @@ -93,7 +91,8 @@ nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error) > if (error) > dma_fence_set_error(&fence->base, error); > > - if (nouveau_fence_signal(fence)) > + dma_fence_signal_locked(&fence->base); > + if (fctx->notify_ref == 0) > nvif_event_block(&fctx->event); > } > fctx->killed = 1; > @@ -131,7 +130,6 @@ static int > nouveau_fence_update(struct nouveau_channel *chan, struct nouveau_fence_chan *fctx) > { > struct nouveau_fence *fence; > - int drop = 0; > u32 seq = fctx->read(chan); > > while (!list_empty(&fctx->pending)) { > @@ -140,10 +138,10 @@ nouveau_fence_update(struct nouveau_channel *chan, struct nouveau_fence_chan *fc > if ((int)(seq - fence->base.seqno) < 0) > break; > > - drop |= nouveau_fence_signal(fence); > + dma_fence_signal_locked(&fence->base); > } > > - return drop; > + return fctx->notify_ref == 0 ? 1 : 0; > } > > static void > @@ -235,6 +233,19 @@ nouveau_fence_emit(struct nouveau_fence *fence) > &fctx->lock, fctx->context, ++fctx->sequence); > kref_get(&fctx->fence_ref); > > + fence->cb.func = nouveau_fence_cleanup_cb; > + /* Adding a callback runs into __dma_fence_enable_signaling(), which will > + * ultimately run into nouveau_fence_no_signaling(), where a WARN_ON > + * would fire because the refcount can be dropped there. > + * > + * Increment the refcount here temporarily to work around that. > + */ > + dma_fence_get(&fence->base); > + ret = dma_fence_add_callback(&fence->base, &fence->cb, nouveau_fence_cleanup_cb); > + dma_fence_put(&fence->base); > + if (ret) > + return ret; > + > ret = fctx->emit(fence); > if (!ret) { > dma_fence_get(&fence->base); > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.h b/drivers/gpu/drm/nouveau/nouveau_fence.h > index 8bc065acfe35..e6b2df7fdc42 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.h > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.h > @@ -10,6 +10,7 @@ struct nouveau_bo; > > struct nouveau_fence { > struct dma_fence base; > + struct dma_fence_cb cb; > > struct list_head head; >

1 year, 1 month

[PATCH v6 00/10] TEE subsystem for restricted dma-buf allocations

by Jens Wiklander

Hi, This patch set allocates the restricted DMA-bufs from a DMA-heap instantiated from the TEE subsystem. The TEE subsystem handles the DMA-buf allocations since it is the TEE (OP-TEE, AMD-TEE, TS-TEE, or perhaps a future QTEE) which sets up the restrictions for the memory used for the DMA-bufs. The DMA-heap uses a restricted memory pool provided by the backend TEE driver, allowing it to choose how to allocate the restricted physical memory. The allocated DMA-bufs must be imported with a new TEE_IOC_SHM_REGISTER_FD before they can be passed as arguments when requesting services from the secure world. Three use-cases (Secure Video Playback, Trusted UI, and Secure Video Recording) has been identified so far to serve as examples of what can be expected. The use-cases has predefined DMA-heap names, "restricted,secure-video", "restricted,trusted-ui", and "restricted,secure-video-record". The backend driver registers restricted memory pools for the use-cases it supports. Each use-case has it's own restricted memory pool since different use-cases requires isolation from different parts of the system. A restricted memory pool can be based on a static carveout instantiated while probing the TEE backend driver, or dynamically allocated from CMA and made restricted as needed by the TEE. This can be tested on a RockPi 4B+ with the following steps: repo init -u https://github.com/jenswi-linaro/manifest.git -m rockpi4.xml \ -b prototype/sdp-v6 repo sync -j8 cd build make toolchains -j$(nproc) make all -j$(nproc) # Copy ../out/rockpi4.img to an SD card and boot the RockPi from that # Connect a monitor to the RockPi # login and at the prompt: gst-launch-1.0 videotestsrc ! \ aesenc key=1f9423681beb9a79215820f6bda73d0f \ iv=e9aa8e834d8d70b7e0d254ff670dd718 serialize-iv=true ! \ aesdec key=1f9423681beb9a79215820f6bda73d0f ! \ kmssink The aesdec module has been hacked to use an OP-TEE TA to decrypt the stream into restricted DMA-bufs which are consumed by the kmssink. The primitive QEMU tests from previous patch set can be tested on RockPi in the same way with: xtest --sdp-basic The primitive test are tested on QEMU with the following steps: repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \ -b prototype/sdp-v6 repo sync -j8 cd build make toolchains -j$(nproc) make SPMC_AT_EL=1 all -j$(nproc) make SPMC_AT_EL=1 run-only # login and at the prompt: xtest --sdp-basic The SPMC_AT_EL=1 parameter configures the build with FF-A and an SPMC at S-EL1 inside OP-TEE. The parameter can be changed into SPMC_AT_EL=n to test without FF-A using the original SMC ABI instead. Please remember to do %rm -rf ../trusted-firmware-a/build/qemu for TF-A to be rebuilt properly using the new configuration. https://optee.readthedocs.io/en/latest/building/prerequisites.html list dependencies needed to build the above. The tests are pretty basic, mostly checking that a Trusted Application in the secure world can access and manipulate the memory. There are also some negative tests for out of bounds buffers etc. Thanks, Jens Changes since V5: * Removing "tee: add restricted memory allocation" and "tee: add TEE_IOC_RSTMEM_FD_INFO" * Adding "tee: implement restricted DMA-heap", "tee: new ioctl to a register tee_shm from a dmabuf file descriptor", "tee: add tee_shm_alloc_cma_phys_mem()", "optee: pass parent device to tee_device_alloc()", and "tee: tee_device_alloc(): copy dma_mask from parent device" * The two TEE driver OPs "rstmem_alloc()" and "rstmem_free()" are replaced with a struct tee_rstmem_pool abstraction. * Replaced the the TEE_IOC_RSTMEM_ALLOC user space API with the DMA-heap API Changes since V4: * Adding the patch "tee: add TEE_IOC_RSTMEM_FD_INFO" needed by the GStreamer demo * Removing the dummy CPU access and mmap functions from the dma_buf_ops * Fixing a compile error in "optee: FF-A: dynamic restricted memory allocation" reported by kernel test robot <lkp(a)intel.com> Changes since V3: * Make the use_case and flags field in struct tee_shm u32's instead of u16's * Add more description for TEE_IOC_RSTMEM_ALLOC in the header file * Import namespace DMA_BUF in module tee, reported by lkp(a)intel.com * Added a note in the commit message for "optee: account for direction while converting parameters" why it's needed * Factor out dynamic restricted memory allocation from "optee: support restricted memory allocation" into two new commits "optee: FF-A: dynamic restricted memory allocation" and "optee: smc abi: dynamic restricted memory allocation" * Guard CMA usage with #ifdef CONFIG_CMA, effectively disabling dynamic restricted memory allocate if CMA isn't configured Changes since the V2 RFC: * Based on v6.12 * Replaced the flags for SVP and Trusted UID memory with a u32 field with unique id for each use case * Added dynamic allocation of restricted memory pools * Added OP-TEE ABI both with and without FF-A for dynamic restricted memory * Added support for FF-A with FFA_LEND Changes since the V1 RFC: * Based on v6.11 * Complete rewrite, replacing the restricted heap with TEE_IOC_RSTMEM_ALLOC Changes since Olivier's post [2]: * Based on Yong Wu's post [1] where much of dma-buf handling is done in the generic restricted heap * Simplifications and cleanup * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap support" * Replaced the word "secure" with "restricted" where applicable Etienne Carriere (1): tee: new ioctl to a register tee_shm from a dmabuf file descriptor Jens Wiklander (9): tee: tee_device_alloc(): copy dma_mask from parent device optee: pass parent device to tee_device_alloc() optee: account for direction while converting parameters optee: sync secure world ABI headers tee: implement restricted DMA-heap tee: add tee_shm_alloc_cma_phys_mem() optee: support restricted memory allocation optee: FF-A: dynamic restricted memory allocation optee: smc abi: dynamic restricted memory allocation drivers/tee/Makefile | 1 + drivers/tee/optee/Makefile | 1 + drivers/tee/optee/call.c | 10 +- drivers/tee/optee/core.c | 1 + drivers/tee/optee/ffa_abi.c | 194 +++++++++++- drivers/tee/optee/optee_ffa.h | 27 +- drivers/tee/optee/optee_msg.h | 65 ++++- drivers/tee/optee/optee_private.h | 55 +++- drivers/tee/optee/optee_smc.h | 71 ++++- drivers/tee/optee/rpc.c | 31 +- drivers/tee/optee/rstmem.c | 329 +++++++++++++++++++++ drivers/tee/optee/smc_abi.c | 190 ++++++++++-- drivers/tee/tee_core.c | 147 +++++++--- drivers/tee/tee_heap.c | 470 ++++++++++++++++++++++++++++++ drivers/tee/tee_private.h | 7 + drivers/tee/tee_shm.c | 199 ++++++++++++- include/linux/tee_core.h | 67 +++++ include/linux/tee_drv.h | 10 + include/uapi/linux/tee.h | 29 ++ 19 files changed, 1781 insertions(+), 123 deletions(-) create mode 100644 drivers/tee/optee/rstmem.c create mode 100644 drivers/tee/tee_heap.c base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6 -- 2.43.0

1 year, 1 month

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig