Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

6 participants
3123 discussions

Re: [PATCH 2/2] dmabuf/heaps: implement DMA_BUF_IOCTL_RW_FILE for system_heap

by Christian König

On 5/27/25 16:35, wangtao wrote: >> -----Original Message----- >> From: Christian König <christian.koenig(a)amd.com> >> Sent: Thursday, May 22, 2025 7:58 PM >> To: wangtao <tao.wangtao(a)honor.com>; T.J. Mercier >> <tjmercier(a)google.com> >> Cc: sumit.semwal(a)linaro.org; benjamin.gaignard(a)collabora.com; >> Brian.Starkey(a)arm.com; jstultz(a)google.com; linux-media(a)vger.kernel.org; >> dri-devel(a)lists.freedesktop.org; linaro-mm-sig(a)lists.linaro.org; linux- >> kernel(a)vger.kernel.org; wangbintian(BintianWang) >> <bintian.wang(a)honor.com>; yipengxiang <yipengxiang(a)honor.com>; liulu >> 00013167 <liulu.liu(a)honor.com>; hanfeng 00012985 <feng.han(a)honor.com>; >> amir73il(a)gmail.com >> Subject: Re: [PATCH 2/2] dmabuf/heaps: implement >> DMA_BUF_IOCTL_RW_FILE for system_heap >> >> On 5/22/25 10:02, wangtao wrote: >>>> -----Original Message----- >>>> From: Christian König <christian.koenig(a)amd.com> >>>> Sent: Wednesday, May 21, 2025 7:57 PM >>>> To: wangtao <tao.wangtao(a)honor.com>; T.J. Mercier >>>> <tjmercier(a)google.com> >>>> Cc: sumit.semwal(a)linaro.org; benjamin.gaignard(a)collabora.com; >>>> Brian.Starkey(a)arm.com; jstultz(a)google.com; >>>> linux-media(a)vger.kernel.org; dri-devel(a)lists.freedesktop.org; >>>> linaro-mm-sig(a)lists.linaro.org; linux- kernel(a)vger.kernel.org; >>>> wangbintian(BintianWang) <bintian.wang(a)honor.com>; yipengxiang >>>> <yipengxiang(a)honor.com>; liulu >>>> 00013167 <liulu.liu(a)honor.com>; hanfeng 00012985 >>>> <feng.han(a)honor.com>; amir73il(a)gmail.com >>>> Subject: Re: [PATCH 2/2] dmabuf/heaps: implement >>>> DMA_BUF_IOCTL_RW_FILE for system_heap >>>> >>>> On 5/21/25 12:25, wangtao wrote: >>>>> [wangtao] I previously explained that >>>>> read/sendfile/splice/copy_file_range >>>>> syscalls can't achieve dmabuf direct IO zero-copy. >>>> >>>> And why can't you work on improving those syscalls instead of >>>> creating a new IOCTL? >>>> >>> [wangtao] As I mentioned in previous emails, these syscalls cannot >>> achieve dmabuf zero-copy due to technical constraints. >> >> Yeah, and why can't you work on removing those technical constrains? >> >> What is blocking you from improving the sendfile system call or proposing a >> patch to remove the copy_file_range restrictions? > [wangtao] Since sendfile/splice can't eliminate CPU copies, I skipped cross-FS checks > in copy_file_range when copying memory/disk files. It will probably be a longer discussion, but I think that having the FS people take a look as well is clearly mandatory. If Linus or anybody else of those maintainers then say that this isn't going to fly either we can still look into alternatives. Thanks, Christian. > Will send new patches after completing shmem/udmabuf callback. > Thank you for your attention to this issue. > > UFS 4.0 device @4GB/s, Arm64 CPU @1GHz: > | Metrics |Creat(us)|Close(us)| I/O(us) |I/O(MB/s)| Vs.% > |--------------------------|---------|---------|---------|---------|------- > | 0) dmabuf buffer read | 46898 | 4804 | 1173661 | 914 | 100% > | 1) udmabuf buffer read | 593844 | 337111 | 2144681 | 500 | 54% > | 2) memfd buffer read | 1029 | 305322 | 2215859 | 484 | 52% > | 3) memfd direct read | 562 | 295239 | 1019913 | 1052 | 115% > | 4) memfd buffer sendfile | 785 | 299026 | 1431304 | 750 | 82% > | 5) memfd direct sendfile | 718 | 296307 | 2622270 | 409 | 44% > | 6) memfd buffer splice | 981 | 299694 | 1573710 | 682 | 74% > | 7) memfd direct splice | 890 | 302509 | 1269757 | 845 | 92% > | 8) memfd buffer c_f_r | 33 | 4432 | N/A | N/A | N/A > | 9) memfd direct c_f_r | 27 | 4421 | N/A | N/A | N/A > |10) memfd buffer sendfile | 595797 | 423105 | 1242494 | 864 | 94% > |11) memfd direct sendfile | 593758 | 357921 | 2344001 | 458 | 50% > |12) memfd buffer splice | 623221 | 356212 | 1117507 | 960 | 105% > |13) memfd direct splice | 587059 | 345484 | 857103 | 1252 | 136% > |14) udmabuf buffer c_f_r | 22725 | 10248 | N/A | N/A | N/A > |15) udmabuf direct c_f_r | 20120 | 9952 | N/A | N/A | N/A > |16) dmabuf buffer c_f_r | 46517 | 4708 | 857587 | 1252 | 136% > |17) dmabuf direct c_f_r | 47339 | 4661 | 284023 | 3780 | 413% > >> >> Regards, >> Christian. >> >> Could you >>> specify the technical points, code, or principles that need >>> optimization? >>> >>> Let me explain again why these syscalls can't work: >>> 1. read() syscall >>> - dmabuf fops lacks read callback implementation. Even if implemented, >>> file_fd info cannot be transferred >>> - read(file_fd, dmabuf_ptr, len) with remap_pfn_range-based mmap >>> cannot access dmabuf_buf pages, forcing buffer-mode reads >>> >>> 2. sendfile() syscall >>> - Requires CPU copy from page cache to memory file(tmpfs/shmem): >>> [DISK] --DMA--> [page cache] --CPU copy--> [MEMORY file] >>> - CPU overhead (both buffer/direct modes involve copies): >>> 55.08% do_sendfile >>> |- 55.08% do_splice_direct >>> |-|- 55.08% splice_direct_to_actor >>> |-|-|- 22.51% copy_splice_read >>> |-|-|-|- 16.57% f2fs_file_read_iter >>> |-|-|-|-|- 15.12% __iomap_dio_rw >>> |-|-|- 32.33% direct_splice_actor >>> |-|-|-|- 32.11% iter_file_splice_write >>> |-|-|-|-|- 28.42% vfs_iter_write >>> |-|-|-|-|-|- 28.42% do_iter_write >>> |-|-|-|-|-|-|- 28.39% shmem_file_write_iter >>> |-|-|-|-|-|-|-|- 24.62% generic_perform_write >>> |-|-|-|-|-|-|-|-|- 18.75% __pi_memmove >>> >>> 3. splice() requires one end to be a pipe, incompatible with regular files or >> dmabuf. >>> >>> 4. copy_file_range() >>> - Blocked by cross-FS restrictions (Amir's commit 868f9f2f8e00) >>> - Even without this restriction, Even without restrictions, implementing >>> the copy_file_range callback in dmabuf fops would only allow dmabuf >> read >>> from regular files. This is because copy_file_range relies on >>> file_out->f_op->copy_file_range, which cannot support dmabuf >> write >>> operations to regular files. >>> >>> Test results confirm these limitations: >>> T.J. Mercier's 1G from ext4 on 6.12.20 | read/sendfile (ms) w/ 3 > >>> drop_caches >>> ------------------------|------------------- >>> udmabuf buffer read | 1210 >>> udmabuf direct read | 671 >>> udmabuf buffer sendfile | 1096 >>> udmabuf direct sendfile | 2340 >>> >>> My 3GHz CPU tests (cache cleared): >>> Method | alloc | read | vs. (%) >>> ----------------------------------------------- >>> udmabuf buffer read | 135 | 546 | 180% >>> udmabuf direct read | 159 | 300 | 99% >>> udmabuf buffer sendfile | 134 | 303 | 100% >>> udmabuf direct sendfile | 141 | 912 | 301% >>> dmabuf buffer read | 22 | 362 | 119% >>> my patch direct read | 29 | 265 | 87% >>> >>> My 1GHz CPU tests (cache cleared): >>> Method | alloc | read | vs. (%) >>> ----------------------------------------------- >>> udmabuf buffer read | 552 | 2067 | 198% >>> udmabuf direct read | 540 | 627 | 60% >>> udmabuf buffer sendfile | 497 | 1045 | 100% udmabuf direct sendfile | >>> 527 | 2330 | 223% >>> dmabuf buffer read | 40 | 1111 | 106% >>> patch direct read | 44 | 310 | 30% >>> >>> Test observations align with expectations: >>> 1. dmabuf buffer read requires slow CPU copies 2. udmabuf direct read >>> achieves zero-copy but has page retrieval >>> latency from vaddr >>> 3. udmabuf buffer sendfile suffers CPU copy overhead 4. udmabuf direct >>> sendfile combines CPU copies with frequent DMA >>> operations due to small pipe buffers 5. dmabuf buffer read also >>> requires CPU copies 6. My direct read patch enables zero-copy with >>> better performance >>> on low-power CPUs >>> 7. udmabuf creation time remains problematic (as you’ve noted). >>> >>>>> My focus is enabling dmabuf direct I/O for [regular file] <--DMA--> >>>>> [dmabuf] zero-copy. >>>> >>>> Yeah and that focus is wrong. You need to work on a general solution >>>> to the issue and not specific to your problem. >>>> >>>>> Any API achieving this would work. Are there other uAPIs you think >>>>> could help? Could you recommend experts who might offer suggestions? >>>> >>>> Well once more: Either work on sendfile or copy_file_range or >>>> eventually splice to make it what you want to do. >>>> >>>> When that is done we can discuss with the VFS people if that approach >>>> is feasible. >>>> >>>> But just bypassing the VFS review by implementing a DMA-buf specific >>>> IOCTL is a NO-GO. That is clearly not something you can do in any way. >>> [wangtao] The issue is that only dmabuf lacks Direct I/O zero-copy >>> support. Tmpfs/shmem already work with Direct I/O zero-copy. As >>> explained, existing syscalls or generic methods can't enable dmabuf >>> direct I/O zero-copy, which is why I propose adding an IOCTL command. >>> >>> I respect your perspective. Could you clarify specific technical >>> aspects, code requirements, or implementation principles for modifying >>> sendfile() or copy_file_range()? This would help advance our discussion. >>> >>> Thank you for engaging in this dialogue. >>> >>>> >>>> Regards, >>>> Christian. >

5 months, 1 week

Re: [PATCH v9 8/9] optee: FF-A: dynamic protected memory allocation

by Jens Wiklander

On Mon, May 26, 2025 at 10:09 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, May 20, 2025 at 05:16:51PM +0200, Jens Wiklander wrote: > > Add support in the OP-TEE backend driver dynamic protected memory > > allocation with FF-A. > > > > The protected memory pools for dynamically allocated protected memory > > are instantiated when requested by user-space. This instantiation can > > fail if OP-TEE doesn't support the requested use-case of protected > > 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> > > --- [...] > > +static int optee_ffa_protmem_pool_init(struct optee *optee, u32 sec_caps) > > +{ > > + enum tee_dma_heap_id id = TEE_DMA_HEAP_SECURE_VIDEO_PLAY; > > + struct tee_protmem_pool *pool; > > + int rc = 0; > > + > > + if (sec_caps & OPTEE_FFA_SEC_CAP_PROTMEM) { > > + pool = optee_protmem_alloc_dyn_pool(optee, id); > > + if (IS_ERR(pool)) > > + return PTR_ERR(pool); > > + > > + rc = tee_device_register_dma_heap(optee->teedev, id, pool); > > + if (rc) > > + pool->ops->destroy_pool(pool); > > + } > > + > > + return rc; > > +} > > + > > static int optee_ffa_probe(struct ffa_device *ffa_dev) > > { > > const struct ffa_notifier_ops *notif_ops; > > @@ -918,7 +1057,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) > > optee); > > if (IS_ERR(teedev)) { > > rc = PTR_ERR(teedev); > > - goto err_free_pool; > > + goto err_free_shm_pool; > > } > > optee->teedev = teedev; > > > > @@ -965,6 +1104,9 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) > > rc); > > } > > > > + if (optee_ffa_protmem_pool_init(optee, sec_caps)) > > Let's add a Kconfig check for DMABUF heaps support here as well. I prefer complaining in the log if there's something wrong with the configuration. > > > + pr_info("Protected memory service not available\n"); > > + [...] > > +static int init_dyn_protmem(struct optee_protmem_dyn_pool *rp) > > +{ > > + int rc; > > + > > + rp->protmem = tee_shm_alloc_dma_mem(rp->optee->ctx, rp->page_count); > > + if (IS_ERR(rp->protmem)) { > > + rc = PTR_ERR(rp->protmem); > > + goto err_null_protmem; > > + } > > + > > + /* > > + * TODO unmap the memory range since the physical memory will > > + * become inaccesible after the lend_protmem() call. > > Let's ellaborate this comment to also say that unmap isn't strictly > needed here in case a platform supports hypervisor in EL2 which can > perform unmapping as part for memory lending to secure world as that > will avoid any cache pre-fetch of memory lent to secure world. > > With that I can live with this as a ToDo in kernel which can be > implemented once we see platforms requiring this change to happen. OK, I'll add something. [...] > > + > > +struct tee_protmem_pool *optee_protmem_alloc_dyn_pool(struct optee *optee, > > + enum tee_dma_heap_id id) > > +{ > > + struct optee_protmem_dyn_pool *rp; > > + u32 use_case = id; > > Here we can get rid of redundant extra variable with s/id/use_case/. OK, I'll update. Cheers, Jens

5 months, 1 week

Re: [PATCH v9 9/9] optee: smc abi: dynamic protected memory allocation

by Jens Wiklander

On Mon, May 26, 2025 at 10:13 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, May 20, 2025 at 05:16:52PM +0200, Jens Wiklander wrote: > > Add support in the OP-TEE backend driver for dynamic protected memory > > allocation using the SMC ABI. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/optee/smc_abi.c | 102 ++++++++++++++++++++++++++++++------ > > 1 file changed, 85 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c > > index f3cae8243785..6b3fbe7f0909 100644 > > --- a/drivers/tee/optee/smc_abi.c > > +++ b/drivers/tee/optee/smc_abi.c > > @@ -965,6 +965,70 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx, > > return rc; > > } > > > > +static int optee_smc_lend_protmem(struct optee *optee, struct tee_shm *protmem, > > + u16 *end_points, unsigned int ep_count, > > + u32 use_case) > > +{ > > + struct optee_shm_arg_entry *entry; > > + struct optee_msg_arg *msg_arg; > > + struct tee_shm *shm; > > + u_int offs; > > + int rc; > > + > > + msg_arg = optee_get_msg_arg(optee->ctx, 2, &entry, &shm, &offs); > > + if (IS_ERR(msg_arg)) > > + return PTR_ERR(msg_arg); > > + > > + msg_arg->cmd = OPTEE_MSG_CMD_LEND_PROTMEM; > > + msg_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > + msg_arg->params[0].u.value.a = use_case; > > + msg_arg->params[1].attr = OPTEE_MSG_ATTR_TYPE_TMEM_INPUT; > > + msg_arg->params[1].u.tmem.buf_ptr = protmem->paddr; > > + msg_arg->params[1].u.tmem.size = protmem->size; > > + msg_arg->params[1].u.tmem.shm_ref = (u_long)protmem; > > + > > + rc = optee->ops->do_call_with_arg(optee->ctx, shm, offs, false); > > + if (rc) > > + goto out; > > + if (msg_arg->ret != TEEC_SUCCESS) { > > + rc = -EINVAL; > > + goto out; > > + } > > + protmem->sec_world_id = (u_long)protmem; > > + > > +out: > > + optee_free_msg_arg(optee->ctx, entry, offs); > > + return rc; > > +} > > + > > +static int optee_smc_reclaim_protmem(struct optee *optee, > > + struct tee_shm *protmem) > > +{ > > + struct optee_shm_arg_entry *entry; > > + struct optee_msg_arg *msg_arg; > > + struct tee_shm *shm; > > + u_int offs; > > + int rc; > > + > > + msg_arg = optee_get_msg_arg(optee->ctx, 1, &entry, &shm, &offs); > > + if (IS_ERR(msg_arg)) > > + return PTR_ERR(msg_arg); > > + > > + msg_arg->cmd = OPTEE_MSG_CMD_RECLAIM_PROTMEM; > > + msg_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; > > + msg_arg->params[0].u.rmem.shm_ref = (u_long)protmem; > > + > > + rc = optee->ops->do_call_with_arg(optee->ctx, shm, offs, false); > > + if (rc) > > + goto out; > > + if (msg_arg->ret != TEEC_SUCCESS) > > + rc = -EINVAL; > > + > > +out: > > + optee_free_msg_arg(optee->ctx, entry, offs); > > + return rc; > > +} > > + > > /* > > * 5. Asynchronous notification > > */ > > @@ -1216,6 +1280,8 @@ static const struct optee_ops optee_ops = { > > .do_call_with_arg = optee_smc_do_call_with_arg, > > .to_msg_param = optee_to_msg_param, > > .from_msg_param = optee_from_msg_param, > > + .lend_protmem = optee_smc_lend_protmem, > > + .reclaim_protmem = optee_smc_reclaim_protmem, > > }; > > > > static int enable_async_notif(optee_invoke_fn *invoke_fn) > > @@ -1586,11 +1652,14 @@ static inline int optee_load_fw(struct platform_device *pdev, > > > > static int optee_protmem_pool_init(struct optee *optee) > > { > > + bool protm = optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_PROTMEM; > > + bool dyn_protm = optee->smc.sec_caps & > > + OPTEE_SMC_SEC_CAP_DYNAMIC_PROTMEM; > > enum tee_dma_heap_id heap_id = TEE_DMA_HEAP_SECURE_VIDEO_PLAY; > > - struct tee_protmem_pool *pool; > > - int rc; > > + struct tee_protmem_pool *pool = ERR_PTR(-EINVAL); > > + int rc = -EINVAL; > > > > - if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_PROTMEM) { > > + if (protm) { > > union { > > struct arm_smccc_res smccc; > > struct optee_smc_get_protmem_config_result result; > > @@ -1598,26 +1667,26 @@ static int optee_protmem_pool_init(struct optee *optee) > > > > optee->smc.invoke_fn(OPTEE_SMC_GET_PROTMEM_CONFIG, 0, 0, 0, 0, > > 0, 0, 0, &res.smccc); > > - if (res.result.status != OPTEE_SMC_RETURN_OK) { > > - pr_err("Secure Data Path service not available\n"); > > - return 0; > > - } > > - rc = optee_set_dma_mask(optee, res.result.pa_width); > > + if (res.result.status == OPTEE_SMC_RETURN_OK) > > + rc = optee_set_dma_mask(optee, res.result.pa_width); > > This change should be folded in patch 7/9. OK > > > if (!rc) > > pool = tee_protmem_static_pool_alloc(res.result.start, > > res.result.size); > > - if (IS_ERR(pool)) > > - return PTR_ERR(pool); > > + } > > > > + if (dyn_protm && IS_ERR(pool)) > > + pool = optee_protmem_alloc_dyn_pool(optee, heap_id); > > + > > + if (!IS_ERR(pool)) { > > rc = tee_device_register_dma_heap(optee->teedev, heap_id, pool); > > if (rc) > > - goto err; > > + pool->ops->destroy_pool(pool); > > } > > > > + if (protm || dyn_protm) > > + return rc; > > + > > return 0; > > -err: > > - pool->ops->destroy_pool(pool); > > - return rc; > > } > > > > static int optee_probe(struct platform_device *pdev) > > @@ -1788,9 +1857,8 @@ static int optee_probe(struct platform_device *pdev) > > pr_info("Asynchronous notifications enabled\n"); > > } > > > > - rc = optee_protmem_pool_init(optee); > > - if (rc) > > - goto err_notif_uninit; > > + if (optee_protmem_pool_init(optee)) > > + pr_info("Protected memory service not available\n"); > > This change can be folded in patch 7/9. Yes, that makes sense. Cheers, Jens > > Rest looks good to me. > > -Sumit > > > > > /* > > * Ensure that there are no pre-existing shm objects before enabling > > -- > > 2.43.0 > >

5 months, 1 week

Re: [PATCH v9 7/9] optee: support protected memory allocation

by Jens Wiklander

On Mon, May 26, 2025 at 9:33 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, May 20, 2025 at 05:16:50PM +0200, Jens Wiklander wrote: > > Add support in the OP-TEE backend driver for protected memory > > allocation. The support is limited to only the SMC ABI and for secure > > video buffers. > > > > OP-TEE is probed for the range of protected physical memory and a > > memory pool allocator is initialized if OP-TEE have support for such > > memory. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/optee/core.c | 10 +++++++ > > drivers/tee/optee/optee_private.h | 2 ++ > > drivers/tee/optee/smc_abi.c | 45 +++++++++++++++++++++++++++++-- > > 3 files changed, 55 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c > > index c75fddc83576..4b14a7ac56f9 100644 > > --- a/drivers/tee/optee/core.c > > +++ b/drivers/tee/optee/core.c > > @@ -56,6 +56,15 @@ int optee_rpmb_intf_rdev(struct notifier_block *intf, unsigned long action, > > return 0; > > } > > > > +int optee_set_dma_mask(struct optee *optee, u_int pa_width) > > +{ > > + u64 mask = DMA_BIT_MASK(min(64, pa_width)); > > + > > + optee->teedev->dev.dma_mask = &optee->teedev->dev.coherent_dma_mask; > > + > > + return dma_set_mask_and_coherent(&optee->teedev->dev, mask); > > +} > > + > > static void optee_bus_scan(struct work_struct *work) > > { > > WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP)); > > @@ -181,6 +190,7 @@ void optee_remove_common(struct optee *optee) > > tee_device_unregister(optee->supp_teedev); > > tee_device_unregister(optee->teedev); > > > > + tee_device_unregister_all_dma_heaps(optee->teedev); > > tee_shm_pool_free(optee->pool); > > optee_supp_uninit(&optee->supp); > > mutex_destroy(&optee->call_queue.mutex); > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h > > index dc0f355ef72a..5e3c34802121 100644 > > --- a/drivers/tee/optee/optee_private.h > > +++ b/drivers/tee/optee/optee_private.h > > @@ -272,6 +272,8 @@ struct optee_call_ctx { > > > > extern struct blocking_notifier_head optee_rpmb_intf_added; > > > > +int optee_set_dma_mask(struct optee *optee, u_int pa_width); > > + > > int optee_notif_init(struct optee *optee, u_int max_key); > > void optee_notif_uninit(struct optee *optee); > > int optee_notif_wait(struct optee *optee, u_int key, u32 timeout); > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c > > index f0c3ac1103bb..f3cae8243785 100644 > > --- a/drivers/tee/optee/smc_abi.c > > +++ b/drivers/tee/optee/smc_abi.c > > @@ -1584,6 +1584,42 @@ static inline int optee_load_fw(struct platform_device *pdev, > > } > > #endif > > > > +static int optee_protmem_pool_init(struct optee *optee) > > +{ > > + enum tee_dma_heap_id heap_id = TEE_DMA_HEAP_SECURE_VIDEO_PLAY; > > + struct tee_protmem_pool *pool; > > + int rc; > > + > > + if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_PROTMEM) { > > + union { > > + struct arm_smccc_res smccc; > > + struct optee_smc_get_protmem_config_result result; > > + } res; > > + > > + optee->smc.invoke_fn(OPTEE_SMC_GET_PROTMEM_CONFIG, 0, 0, 0, 0, > > + 0, 0, 0, &res.smccc); > > + if (res.result.status != OPTEE_SMC_RETURN_OK) { > > + pr_err("Secure Data Path service not available\n"); > > + return 0; > > + } > > + rc = optee_set_dma_mask(optee, res.result.pa_width); > > + if (!rc) > > + pool = tee_protmem_static_pool_alloc(res.result.start, > > + res.result.size); > > + if (IS_ERR(pool)) > > + return PTR_ERR(pool); > > + > > + rc = tee_device_register_dma_heap(optee->teedev, heap_id, pool); > > + if (rc) > > + goto err; > > + } > > + > > + return 0; > > +err: > > + pool->ops->destroy_pool(pool); > > + return rc; > > +} > > + > > static int optee_probe(struct platform_device *pdev) > > { > > optee_invoke_fn *invoke_fn; > > @@ -1679,7 +1715,7 @@ static int optee_probe(struct platform_device *pdev) > > optee = kzalloc(sizeof(*optee), GFP_KERNEL); > > if (!optee) { > > rc = -ENOMEM; > > - goto err_free_pool; > > + goto err_free_shm_pool; > > } > > > > optee->ops = &optee_ops; > > @@ -1752,6 +1788,10 @@ static int optee_probe(struct platform_device *pdev) > > pr_info("Asynchronous notifications enabled\n"); > > } > > > > + rc = optee_protmem_pool_init(optee); > > Here we should do a Kconfig check for CONFIG_DMABUF_HEAPS so that we > don't proceed any further with initialization. Why? If OP-TEE is configured for protected memory but the kernel isn't, something isn't right, and a print could be useful. Cheers, Jens > > Rest looks good to me. > > -Sumit > > > + if (rc) > > + goto err_notif_uninit; > > + > > /* > > * Ensure that there are no pre-existing shm objects before enabling > > * the shm cache so that there's no chance of receiving an invalid > > @@ -1787,6 +1827,7 @@ static int optee_probe(struct platform_device *pdev) > > optee_disable_shm_cache(optee); > > optee_smc_notif_uninit_irq(optee); > > optee_unregister_devices(); > > + tee_device_unregister_all_dma_heaps(optee->teedev); > > err_notif_uninit: > > optee_notif_uninit(optee); > > err_close_ctx: > > @@ -1803,7 +1844,7 @@ static int optee_probe(struct platform_device *pdev) > > tee_device_unregister(optee->teedev); > > err_free_optee: > > kfree(optee); > > -err_free_pool: > > +err_free_shm_pool: > > tee_shm_pool_free(pool); > > if (memremaped_shm) > > memunmap(memremaped_shm); > > -- > > 2.43.0 > >

5 months, 1 week

Re: [PATCH v9 6/9] tee: add tee_shm_alloc_dma_mem()

by Jens Wiklander

On Mon, May 26, 2025 at 11:33 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Mon, May 26, 2025 at 11:21:47AM +0200, Jens Wiklander wrote: > > On Mon, May 26, 2025 at 9:22 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > On Tue, May 20, 2025 at 05:16:49PM +0200, Jens Wiklander wrote: > > > > Add tee_shm_alloc_dma_mem() to allocate DMA memory. The memory is > > > > represented by a tee_shm object using the new flag TEE_SHM_DMA_MEM to > > > > identify it as DMA memory. The allocated memory will later be lent to > > > > the TEE to be used as protected memory. > > > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > > > --- > > > > drivers/tee/tee_shm.c | 74 ++++++++++++++++++++++++++++++++++++++-- > > > > include/linux/tee_core.h | 5 +++ > > > > 2 files changed, 77 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > > > > index e1ed52ee0a16..92a6a35e1a1e 100644 > > > > --- a/drivers/tee/tee_shm.c > > > > +++ b/drivers/tee/tee_shm.c > > > > @@ -5,6 +5,8 @@ > > > > #include <linux/anon_inodes.h> > > > > #include <linux/device.h> > > > > #include <linux/dma-buf.h> > > > > +#include <linux/dma-mapping.h> > > > > +#include <linux/highmem.h> > > > > #include <linux/idr.h> > > > > #include <linux/io.h> > > > > #include <linux/mm.h> > > > > @@ -13,9 +15,14 @@ > > > > #include <linux/tee_core.h> > > > > #include <linux/uaccess.h> > > > > #include <linux/uio.h> > > > > -#include <linux/highmem.h> > > > > #include "tee_private.h" > > > > > > > > +struct tee_shm_dma_mem { > > > > + struct tee_shm shm; > > > > + dma_addr_t dma_addr; > > > > + struct page *page; > > > > +}; > > > > + > > > > static void shm_put_kernel_pages(struct page **pages, size_t page_count) > > > > { > > > > size_t n; > > > > @@ -49,7 +56,14 @@ static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) > > > > struct tee_shm *parent_shm = NULL; > > > > void *p = shm; > > > > > > > > - if (shm->flags & TEE_SHM_DMA_BUF) { > > > > + if (shm->flags & TEE_SHM_DMA_MEM) { > > > > + struct tee_shm_dma_mem *dma_mem; > > > > + > > > > + dma_mem = container_of(shm, struct tee_shm_dma_mem, shm); > > > > + p = dma_mem; > > > > + dma_free_pages(&teedev->dev, shm->size, dma_mem->page, > > > > + dma_mem->dma_addr, DMA_BIDIRECTIONAL); > > > > > > Although the kernel bot already found a randconfig issue, it looks like > > > we need to add Kconfig dependencies like HAS_DMA, DMA_CMA etc. > > > > > > Also, I was thinking if we should rather add a new TEE subsystem > > > specific Kconfig option like: TEE_DMABUF_HEAPS which can then be used to > > > select whatever dependency is needed as well as act as a gating Kconfig > > > for relevant features. > > > > You mean something like this? > > > > --- a/drivers/tee/Kconfig > > +++ b/drivers/tee/Kconfig > > @@ -13,6 +13,14 @@ menuconfig TEE > > > > if TEE > > > > +config TEE_DMABUF_HEAPS > > + bool > > + depends on HAS_DMA && DMABUF_HEAPS > > Yeah this looks fine to me but needs to be tested if DMA_CMA is a > dependency here too. Why? It can work without CMA for small allocations. > > > + > > +config TEE_STATIC_PROTMEM_POOL > > + bool > > + depends on HAS_IOMEM && TEE_DMABUF_HEAPS > > The static and dynamic protected memory pools should get auto enabled if > TEE_DMABUF_HEAPS is enabled since they are pre-requisite to provide the > protected heaps support. Something like: > > +config TEE_STATIC_PROTMEM_POOL > + bool > + default y if TEE_DMABUF_HEAPS > + depends on HAS_IOMEM Right, I'll update as needed. Cheers, Jens

5 months, 1 week

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

by Maxime Ripard

Hi, On Thu, May 22, 2025 at 12:14:18PM -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, unconditionally name > it "default_cma_region", but keep a legacy node in place backed by the > same underlying allocator for backwards compatibility. > > Signed-off-by: Jared Kangas <jkangas(a)redhat.com> > --- > Documentation/userspace-api/dma-buf-heaps.rst | 7 +++++-- > drivers/dma-buf/heaps/Kconfig | 10 ++++++++++ > drivers/dma-buf/heaps/cma_heap.c | 20 ++++++++++++++++++- > 3 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst > index 23bd0bd7b0654..1dfe5e7acd5a3 100644 > --- a/Documentation/userspace-api/dma-buf-heaps.rst > +++ b/Documentation/userspace-api/dma-buf-heaps.rst > @@ -21,5 +21,8 @@ following heaps: > 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_SIZE_PERCENTAGE`` Kconfig options. The heap's name in devtmpfs is > + ``default_cma_region``. 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``. > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig > index a5eef06c42264..bb369b38b001a 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 "Legacy 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..dfeccafc6ae3c 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -9,6 +9,9 @@ > * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/ > * Andrew F. Davis <afd(a)ti.com> > */ > + > +#define pr_fmt(fmt) "cma_heap: " fmt > + > #include <linux/cma.h> > #include <linux/dma-buf.h> > #include <linux/dma-heap.h> > @@ -22,6 +25,7 @@ > #include <linux/slab.h> > #include <linux/vmalloc.h> > > +#define DEFAULT_CMA_NAME "default_cma_region" > > struct cma_heap { > struct dma_heap *heap; > @@ -394,15 +398,29 @@ 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; > > + if (IS_ENABLED(CONFIG_DMABUF_HEAPS_CMA_LEGACY)) { > + legacy_cma_name = cma_get_name(default_cma); > + if (!strcmp(legacy_cma_name, DEFAULT_CMA_NAME)) { > + pr_warn("legacy name and default name are the same, skipping legacy heap\n"); > + return 0; > + } > + > + ret = __add_cma_heap(default_cma, legacy_cma_name); > + if (ret) > + pr_warn("failed to add legacy heap: %pe\n", > + ERR_PTR(-ret)); Are you sure about the -ret? ret should already be a negative number if it failed? With that fixed, Reviewed-by: Maxime Ripard <mripard(a)kernel.org> Maxime

5 months, 1 week

Re: [PATCH v3 2/3] dma-buf: heaps: Parameterize heap name in __add_cma_heap()

by Maxime Ripard

On Thu, 22 May 2025 12:14:17 -0700, Jared Kangas wrote: > Prepare for the introduction of a fixed-name CMA heap by replacing the > unused void pointer parameter in __add_cma_heap() with the heap name. > > Signed-off-by: Jared Kangas <jkangas(a)redhat.com> Reviewed-by: Maxime Ripard <mripard(a)kernel.org> Thanks! Maxime

5 months, 1 week

Re: [PATCH v3 1/3] Documentation: dma-buf: heaps: Fix code markup

by Maxime Ripard

On Thu, 22 May 2025 12:14:16 -0700, Jared Kangas wrote: > Code snippets should be wrapped in double backticks to follow > reStructuredText semantics; the use of single backticks uses the > :title-reference: role by default, which isn't quite what we want. > Add double backticks to code snippets to fix this. > > > [ ... ] Reviewed-by: Maxime Ripard <mripard(a)kernel.org> Thanks! Maxime

5 months, 1 week

Re: [PATCH v9 6/9] tee: add tee_shm_alloc_dma_mem()

by Jens Wiklander

On Mon, May 26, 2025 at 9:22 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, May 20, 2025 at 05:16:49PM +0200, Jens Wiklander wrote: > > Add tee_shm_alloc_dma_mem() to allocate DMA memory. The memory is > > represented by a tee_shm object using the new flag TEE_SHM_DMA_MEM to > > identify it as DMA memory. The allocated memory will later be lent to > > the TEE to be used as protected memory. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/tee_shm.c | 74 ++++++++++++++++++++++++++++++++++++++-- > > include/linux/tee_core.h | 5 +++ > > 2 files changed, 77 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > > index e1ed52ee0a16..92a6a35e1a1e 100644 > > --- a/drivers/tee/tee_shm.c > > +++ b/drivers/tee/tee_shm.c > > @@ -5,6 +5,8 @@ > > #include <linux/anon_inodes.h> > > #include <linux/device.h> > > #include <linux/dma-buf.h> > > +#include <linux/dma-mapping.h> > > +#include <linux/highmem.h> > > #include <linux/idr.h> > > #include <linux/io.h> > > #include <linux/mm.h> > > @@ -13,9 +15,14 @@ > > #include <linux/tee_core.h> > > #include <linux/uaccess.h> > > #include <linux/uio.h> > > -#include <linux/highmem.h> > > #include "tee_private.h" > > > > +struct tee_shm_dma_mem { > > + struct tee_shm shm; > > + dma_addr_t dma_addr; > > + struct page *page; > > +}; > > + > > static void shm_put_kernel_pages(struct page **pages, size_t page_count) > > { > > size_t n; > > @@ -49,7 +56,14 @@ static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) > > struct tee_shm *parent_shm = NULL; > > void *p = shm; > > > > - if (shm->flags & TEE_SHM_DMA_BUF) { > > + if (shm->flags & TEE_SHM_DMA_MEM) { > > + struct tee_shm_dma_mem *dma_mem; > > + > > + dma_mem = container_of(shm, struct tee_shm_dma_mem, shm); > > + p = dma_mem; > > + dma_free_pages(&teedev->dev, shm->size, dma_mem->page, > > + dma_mem->dma_addr, DMA_BIDIRECTIONAL); > > Although the kernel bot already found a randconfig issue, it looks like > we need to add Kconfig dependencies like HAS_DMA, DMA_CMA etc. > > Also, I was thinking if we should rather add a new TEE subsystem > specific Kconfig option like: TEE_DMABUF_HEAPS which can then be used to > select whatever dependency is needed as well as act as a gating Kconfig > for relevant features. You mean something like this? --- a/drivers/tee/Kconfig +++ b/drivers/tee/Kconfig @@ -13,6 +13,14 @@ menuconfig TEE if TEE +config TEE_DMABUF_HEAPS + bool + depends on HAS_DMA && DMABUF_HEAPS + +config TEE_STATIC_PROTMEM_POOL + bool + depends on HAS_IOMEM && TEE_DMABUF_HEAPS + Cheers, Jens > > -Sumit > > > + } else if (shm->flags & TEE_SHM_DMA_BUF) { > > struct tee_shm_dmabuf_ref *ref; > > > > ref = container_of(shm, struct tee_shm_dmabuf_ref, shm); > > @@ -306,6 +320,62 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size) > > } > > EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf); > > > > +/** > > + * tee_shm_alloc_dma_mem() - Allocate DMA memory as shared memory object > > + * @ctx: Context that allocates the shared memory > > + * @page_count: Number of pages > > + * > > + * The allocated memory is expected to be lent (made inaccessible to the > > + * kernel) to the TEE while it's used and returned (accessible to the > > + * kernel again) before it's freed. > > + * > > + * This function should normally only be used internally in the TEE > > + * drivers. > > + * > > + * @returns a pointer to 'struct tee_shm' > > + */ > > +struct tee_shm *tee_shm_alloc_dma_mem(struct tee_context *ctx, > > + size_t page_count) > > +{ > > + struct tee_device *teedev = ctx->teedev; > > + struct tee_shm_dma_mem *dma_mem; > > + dma_addr_t dma_addr; > > + struct page *page; > > + > > + if (!tee_device_get(teedev)) > > + return ERR_PTR(-EINVAL); > > + > > + page = dma_alloc_pages(&teedev->dev, page_count * PAGE_SIZE, > > + &dma_addr, DMA_BIDIRECTIONAL, GFP_KERNEL); > > + if (!page) > > + goto err_put_teedev; > > + > > + dma_mem = kzalloc(sizeof(*dma_mem), GFP_KERNEL); > > + if (!dma_mem) > > + goto err_free_pages; > > + > > + refcount_set(&dma_mem->shm.refcount, 1); > > + dma_mem->shm.ctx = ctx; > > + dma_mem->shm.paddr = page_to_phys(page); > > + dma_mem->dma_addr = dma_addr; > > + dma_mem->page = page; > > + dma_mem->shm.size = page_count * PAGE_SIZE; > > + dma_mem->shm.flags = TEE_SHM_DMA_MEM; > > + > > + teedev_ctx_get(ctx); > > + > > + return &dma_mem->shm; > > + > > +err_free_pages: > > + dma_free_pages(&teedev->dev, page_count * PAGE_SIZE, page, dma_addr, > > + DMA_BIDIRECTIONAL); > > +err_put_teedev: > > + tee_device_put(teedev); > > + > > + return ERR_PTR(-ENOMEM); > > +} > > +EXPORT_SYMBOL_GPL(tee_shm_alloc_dma_mem); > > + > > int tee_dyn_shm_alloc_helper(struct tee_shm *shm, size_t size, size_t align, > > int (*shm_register)(struct tee_context *ctx, > > struct tee_shm *shm, > > diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h > > index 02c07f661349..925690e1020b 100644 > > --- a/include/linux/tee_core.h > > +++ b/include/linux/tee_core.h > > @@ -29,6 +29,8 @@ > > #define TEE_SHM_POOL BIT(2) /* Memory allocated from pool */ > > #define TEE_SHM_PRIV BIT(3) /* Memory private to TEE driver */ > > #define TEE_SHM_DMA_BUF BIT(4) /* Memory with dma-buf handle */ > > +#define TEE_SHM_DMA_MEM BIT(5) /* Memory allocated with */ > > + /* dma_alloc_pages() */ > > > > #define TEE_DEVICE_FLAG_REGISTERED 0x1 > > #define TEE_MAX_DEV_NAME_LEN 32 > > @@ -310,6 +312,9 @@ void *tee_get_drvdata(struct tee_device *teedev); > > */ > > struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size); > > > > +struct tee_shm *tee_shm_alloc_dma_mem(struct tee_context *ctx, > > + size_t page_count); > > + > > int tee_dyn_shm_alloc_helper(struct tee_shm *shm, size_t size, size_t align, > > int (*shm_register)(struct tee_context *ctx, > > struct tee_shm *shm, > > -- > > 2.43.0 > >

5 months, 2 weeks

Re: [PATCH v9 5/9] tee: new ioctl to a register tee_shm from a dmabuf file descriptor

by Jens Wiklander

On Fri, May 23, 2025 at 3:31 PM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, May 20, 2025 at 05:16:48PM +0200, Jens Wiklander wrote: > > From: Etienne Carriere <etienne.carriere(a)foss.st.com> > > > > Add a userspace API to create a tee_shm object that refers to a dmabuf > > reference. > > > > Userspace registers the dmabuf file descriptor as in a tee_shm object. > > The registration is completed with a tee_shm returned file descriptor. > > > > Userspace is free to close the dmabuf file descriptor after it has been > > registered since all the resources are now held via the new tee_shm > > object. > > > > Closing the tee_shm file descriptor will eventually release all > > resources used by the tee_shm object when all references are released. > > > > The new IOCTL, TEE_IOC_SHM_REGISTER_FD, supports dmabuf references to > > physically contiguous memory buffers. Dmabuf references acquired from > > the TEE DMA-heap can be used as protected memory for Secure Video Path > > and such use cases. It depends on the TEE and the TEE driver if dmabuf > > references acquired by other means can be used. > > > > A new tee_shm flag is added to identify tee_shm objects built from a > > registered dmabuf, TEE_SHM_DMA_BUF. > > > > Signed-off-by: Etienne Carriere <etienne.carriere(a)foss.st.com> > > Signed-off-by: Olivier Masse <olivier.masse(a)nxp.com> > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/tee_core.c | 63 +++++++++++++++++++++- > > drivers/tee/tee_private.h | 10 ++++ > > drivers/tee/tee_shm.c | 111 ++++++++++++++++++++++++++++++++++++-- > > include/linux/tee_core.h | 1 + > > include/linux/tee_drv.h | 10 ++++ > > include/uapi/linux/tee.h | 31 +++++++++++ > > 6 files changed, 221 insertions(+), 5 deletions(-) > > With below minor comments fixed, feel free to add: > > Reviewed-by: Sumit Garg <sumit.garg(a)oss.qualcomm.com> > > > > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c > > index 5259b8223c27..0e9d9e5872a4 100644 > > --- a/drivers/tee/tee_core.c > > +++ b/drivers/tee/tee_core.c > > @@ -353,11 +353,49 @@ tee_ioctl_shm_register(struct tee_context *ctx, > > return ret; > > } > > > > +static int > > +tee_ioctl_shm_register_fd(struct tee_context *ctx, > > + struct tee_ioctl_shm_register_fd_data __user *udata) > > +{ > > + struct tee_ioctl_shm_register_fd_data data; > > + struct tee_shm *shm; > > + long ret; > > + > > + if (copy_from_user(&data, udata, sizeof(data))) > > + return -EFAULT; > > + > > + /* Currently no input flags are supported */ > > + if (data.flags) > > + return -EINVAL; > > + > > + shm = tee_shm_register_fd(ctx, data.fd); > > + if (IS_ERR(shm)) > > + return -EINVAL; > > + > > + data.id = shm->id; > > + data.flags = shm->flags; > > + data.size = shm->size; > > + > > + if (copy_to_user(udata, &data, sizeof(data))) > > + ret = -EFAULT; > > + else > > + ret = tee_shm_get_fd(shm); > > + > > + /* > > + * When user space closes the file descriptor the shared memory > > + * should be freed or if tee_shm_get_fd() failed then it will > > + * be freed immediately. > > + */ > > + tee_shm_put(shm); > > + return ret; > > +} > > + > > static int param_from_user_memref(struct tee_context *ctx, > > struct tee_param_memref *memref, > > struct tee_ioctl_param *ip) > > { > > struct tee_shm *shm; > > + size_t offs = 0; > > > > /* > > * If a NULL pointer is passed to a TA in the TEE, > > @@ -388,6 +426,26 @@ static int param_from_user_memref(struct tee_context *ctx, > > tee_shm_put(shm); > > return -EINVAL; > > } > > + > > + if (shm->flags & TEE_SHM_DMA_BUF) { > > + struct tee_shm_dmabuf_ref *ref; > > + > > + ref = container_of(shm, struct tee_shm_dmabuf_ref, shm); > > + if (ref->parent_shm) { > > + /* > > + * The shm already has one reference to > > + * ref->parent_shm so we are clear of 0. > > + * We're getting another reference since > > + * this shm will be used in the parameter > > + * list instead of the shm we got with > > + * tee_shm_get_from_id() above. > > + */ > > + refcount_inc(&ref->parent_shm->refcount); > > + tee_shm_put(shm); > > + shm = ref->parent_shm; > > + offs = ref->offset; > > + } > > + } > > } else if (ctx->cap_memref_null) { > > /* Pass NULL pointer to OP-TEE */ > > shm = NULL; > > @@ -395,7 +453,7 @@ static int param_from_user_memref(struct tee_context *ctx, > > return -EINVAL; > > } > > > > - memref->shm_offs = ip->a; > > + memref->shm_offs = ip->a + offs; > > Is this an issue being detected now? If yes then shouldn't it be a > separate fix eligible for backport? No, this is to add an eventual offset for a TEE_SHM_DMA_BUF, introduced above. > > > memref->size = ip->b; > > memref->shm = shm; > > > > @@ -841,6 +899,8 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > return tee_ioctl_shm_alloc(ctx, uarg); > > case TEE_IOC_SHM_REGISTER: > > return tee_ioctl_shm_register(ctx, uarg); > > + case TEE_IOC_SHM_REGISTER_FD: > > + return tee_ioctl_shm_register_fd(ctx, uarg); > > case TEE_IOC_OPEN_SESSION: > > return tee_ioctl_open_session(ctx, uarg); > > case TEE_IOC_INVOKE: > > @@ -1300,3 +1360,4 @@ MODULE_AUTHOR("Linaro"); > > MODULE_DESCRIPTION("TEE Driver"); > > MODULE_VERSION("1.0"); > > MODULE_LICENSE("GPL v2"); > > +MODULE_IMPORT_NS("DMA_BUF"); > > diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h > > index 6c6ff5d5eed2..308467705da6 100644 > > --- a/drivers/tee/tee_private.h > > +++ b/drivers/tee/tee_private.h > > @@ -13,6 +13,16 @@ > > #include <linux/mutex.h> > > #include <linux/types.h> > > > > +/* extra references appended to shm object for registered shared memory */ > > +struct tee_shm_dmabuf_ref { > > + struct tee_shm shm; > > + size_t offset; > > + struct dma_buf *dmabuf; > > + struct dma_buf_attachment *attach; > > + struct sg_table *sgt; > > + struct tee_shm *parent_shm; > > +}; > > + > > int tee_shm_get_fd(struct tee_shm *shm); > > > > bool tee_device_get(struct tee_device *teedev); > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > > index daf6e5cfd59a..e1ed52ee0a16 100644 > > --- a/drivers/tee/tee_shm.c > > +++ b/drivers/tee/tee_shm.c > > @@ -4,6 +4,7 @@ > > */ > > #include <linux/anon_inodes.h> > > #include <linux/device.h> > > +#include <linux/dma-buf.h> > > #include <linux/idr.h> > > #include <linux/io.h> > > #include <linux/mm.h> > > @@ -45,7 +46,23 @@ static void release_registered_pages(struct tee_shm *shm) > > > > static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) > > { > > - if (shm->flags & TEE_SHM_POOL) { > > + struct tee_shm *parent_shm = NULL; > > + void *p = shm; > > + > > + if (shm->flags & TEE_SHM_DMA_BUF) { > > + struct tee_shm_dmabuf_ref *ref; > > + > > + ref = container_of(shm, struct tee_shm_dmabuf_ref, shm); > > + parent_shm = ref->parent_shm; > > + p = ref; > > + if (ref->attach) { > > + dma_buf_unmap_attachment(ref->attach, ref->sgt, > > + DMA_BIDIRECTIONAL); > > + > > + dma_buf_detach(ref->dmabuf, ref->attach); > > + } > > + dma_buf_put(ref->dmabuf); > > + } else if (shm->flags & TEE_SHM_POOL) { > > teedev->pool->ops->free(teedev->pool, shm); > > } else if (shm->flags & TEE_SHM_DYNAMIC) { > > int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm); > > @@ -57,9 +74,10 @@ static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) > > release_registered_pages(shm); > > } > > > > - teedev_ctx_put(shm->ctx); > > + if (shm->ctx) > > + teedev_ctx_put(shm->ctx); > > redundant change? Yes, I'll remove it. > > > > > - kfree(shm); > > + kfree(p); > > > > tee_device_put(teedev); > > } > > @@ -169,7 +187,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size) > > * tee_client_invoke_func(). The memory allocated is later freed with a > > * call to tee_shm_free(). > > * > > - * @returns a pointer to 'struct tee_shm' > > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure > > */ > > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size) > > { > > @@ -179,6 +197,91 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size) > > } > > EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf); > > > > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd) > > +{ > > + struct tee_shm_dmabuf_ref *ref; > > + int rc; > > + > > + if (!tee_device_get(ctx->teedev)) > > + return ERR_PTR(-EINVAL); > > + > > + teedev_ctx_get(ctx); > > + > > + ref = kzalloc(sizeof(*ref), GFP_KERNEL); > > + if (!ref) { > > + rc = -ENOMEM; > > + goto err_put_tee; > > + } > > + > > + refcount_set(&ref->shm.refcount, 1); > > + ref->shm.ctx = ctx; > > + ref->shm.id = -1; > > + ref->shm.flags = TEE_SHM_DMA_BUF; > > + > > + ref->dmabuf = dma_buf_get(fd); > > + if (IS_ERR(ref->dmabuf)) { > > + rc = PTR_ERR(ref->dmabuf); > > + goto err_kfree_ref; > > + } > > + > > + rc = tee_heap_update_from_dma_buf(ctx->teedev, ref->dmabuf, > > + &ref->offset, &ref->shm, > > + &ref->parent_shm); > > + if (!rc) > > + goto out; > > + if (rc != -EINVAL) > > + goto err_put_dmabuf; > > + > > + ref->attach = dma_buf_attach(ref->dmabuf, &ctx->teedev->dev); > > + if (IS_ERR(ref->attach)) { > > + rc = PTR_ERR(ref->attach); > > + goto err_put_dmabuf; > > + } > > + > > + ref->sgt = dma_buf_map_attachment(ref->attach, DMA_BIDIRECTIONAL); > > + if (IS_ERR(ref->sgt)) { > > + rc = PTR_ERR(ref->sgt); > > + goto err_detach; > > + } > > + > > + if (sg_nents(ref->sgt->sgl) != 1) { > > + rc = -EINVAL; > > + goto err_unmap_attachement; > > + } > > + > > + ref->shm.paddr = page_to_phys(sg_page(ref->sgt->sgl)); > > + ref->shm.size = ref->sgt->sgl->length; > > + > > +out: > > + mutex_lock(&ref->shm.ctx->teedev->mutex); > > + ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref->shm, > > + 1, 0, GFP_KERNEL); > > + mutex_unlock(&ref->shm.ctx->teedev->mutex); > > + if (ref->shm.id < 0) { > > + rc = ref->shm.id; > > + if (ref->attach) > > + goto err_unmap_attachement; > > + goto err_put_dmabuf; > > + } > > + > > + return &ref->shm; > > + > > +err_unmap_attachement: > > + dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL); > > +err_detach: > > + dma_buf_detach(ref->dmabuf, ref->attach); > > +err_put_dmabuf: > > + dma_buf_put(ref->dmabuf); > > +err_kfree_ref: > > + kfree(ref); > > +err_put_tee: > > + teedev_ctx_put(ctx); > > + tee_device_put(ctx->teedev); > > + > > + return ERR_PTR(rc); > > +} > > +EXPORT_SYMBOL_GPL(tee_shm_register_fd); > > + > > /** > > * tee_shm_alloc_priv_buf() - Allocate shared memory for a privately shared > > * kernel buffer > > diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h > > index b8b99c97e00c..02c07f661349 100644 > > --- a/include/linux/tee_core.h > > +++ b/include/linux/tee_core.h > > @@ -28,6 +28,7 @@ > > #define TEE_SHM_USER_MAPPED BIT(1) /* Memory mapped in user space */ > > #define TEE_SHM_POOL BIT(2) /* Memory allocated from pool */ > > #define TEE_SHM_PRIV BIT(3) /* Memory private to TEE driver */ > > +#define TEE_SHM_DMA_BUF BIT(4) /* Memory with dma-buf handle */ > > > > #define TEE_DEVICE_FLAG_REGISTERED 0x1 > > #define TEE_MAX_DEV_NAME_LEN 32 > > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h > > index a54c203000ed..824f1251de60 100644 > > --- a/include/linux/tee_drv.h > > +++ b/include/linux/tee_drv.h > > @@ -116,6 +116,16 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size); > > struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx, > > void *addr, size_t length); > > > > +/** > > + * tee_shm_register_fd() - Register shared memory from file descriptor > > + * > > + * @ctx: Context that allocates the shared memory > > + * @fd: Shared memory file descriptor reference > > + * > > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure > > + */ > > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd); > > + > > /** > > * tee_shm_free() - Free shared memory > > * @shm: Handle to shared memory to free > > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h > > index d0430bee8292..8ec5f46fbfbe 100644 > > --- a/include/uapi/linux/tee.h > > +++ b/include/uapi/linux/tee.h > > @@ -118,6 +118,37 @@ struct tee_ioctl_shm_alloc_data { > > #define TEE_IOC_SHM_ALLOC _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 1, \ > > struct tee_ioctl_shm_alloc_data) > > > > +/** > > + * struct tee_ioctl_shm_register_fd_data - Shared memory registering argument > > + * @fd: [in] File descriptor identifying dmabuf reference > > + * @size: [out] Size of referenced memory > > + * @flags: [in] Flags to/from allocation. > > + * @id: [out] Identifier of the shared memory > > + * > > + * The flags field should currently be zero as input. Updated by the call > > + * with actual flags as defined by TEE_IOCTL_SHM_* above. > > + * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD below. > > + */ > > +struct tee_ioctl_shm_register_fd_data { > > + __s64 fd; > > + __u64 size; > > + __u32 flags; > > + __s32 id; > > +}; > > + > > +/** > > + * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file descriptor > > + * > > + * Returns a file descriptor on success or < 0 on failure > > + * > > + * The returned file descriptor refers to the shared memory object in the > > + * kernel. The supplied file deccriptor can be closed if it's not needed > > + * for other purposes. The shared memory is freed when the descriptor is > > + * closed. > > + */ > > +#define TEE_IOC_SHM_REGISTER_FD _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 8, \ > > + struct tee_ioctl_shm_register_fd_data) > > Please add this IOCTL in correct sequence order. OK, I'll fix it. Thanks, Jens > > -Sumit > > > + > > /** > > * struct tee_ioctl_buf_data - Variable sized buffer > > * @buf_ptr: [in] A __user pointer to a buffer > > -- > > 2.43.0 > >

5 months, 2 weeks

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig