- Linaro-mm-sig - lists.linaro.org

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. >

8 months, 3 weeks

1
0
0 0

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

8 months, 3 weeks

1
0
0 0

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 > >

8 months, 3 weeks

1
0
0 0

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 > >

8 months, 3 weeks

1
0
0 0

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

8 months, 3 weeks

1
0
0 0

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

8 months, 3 weeks

1
0
0 0

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

8 months, 3 weeks

1
0
0 0

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

8 months, 3 weeks

1
0
0 0

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 > >

8 months, 3 weeks

1
0
0 0

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 > >

8 months, 3 weeks

1
0
0 0

Re: [PATCH v9 3/9] tee: implement protected DMA-heap

by Jens Wiklander

On Fri, May 23, 2025 at 3:03 PM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > + Robin > > Jens, > > I suppose you missed to add Robin who has earlier reviewed this patch. Yes, you're right, sorry. Thanks for catching this. > > On Tue, May 20, 2025 at 05:16:46PM +0200, Jens Wiklander wrote: > > Implement DMA heap for protected DMA-buf allocation in the TEE > > subsystem. > > > > Restricted memory refers to memory buffers behind a hardware enforced > > s/Restricted/Protected/ > > > firewall. It is not accessible to the kernel during normal circumstances > > but rather only accessible to certain hardware IPs or CPUs executing in > > higher or differently privileged mode than the kernel itself. This > > interface allows to allocate and manage such protected memory buffers > > via interaction with a TEE implementation. > > > > The protected memory is allocated for a specific use-case, like Secure > > Video Playback, Trusted UI, or Secure Video Recording where certain > > hardware devices can access the memory. > > > > The DMA-heaps are enabled explicitly by the TEE backend driver. The TEE > > backend drivers needs to implement protected memory pool to manage the > > protected memory. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/Makefile | 1 + > > drivers/tee/tee_heap.c | 487 ++++++++++++++++++++++++++++++++++++++ > > drivers/tee/tee_private.h | 6 + > > include/linux/tee_core.h | 65 +++++ > > 4 files changed, 559 insertions(+) > > create mode 100644 drivers/tee/tee_heap.c > > > > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile > > index 5488cba30bd2..949a6a79fb06 100644 > > --- a/drivers/tee/Makefile > > +++ b/drivers/tee/Makefile > > @@ -1,6 +1,7 @@ > > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_TEE) += tee.o > > tee-objs += tee_core.o > > +tee-objs += tee_heap.o > > tee-objs += tee_shm.o > > tee-objs += tee_shm_pool.o > > obj-$(CONFIG_OPTEE) += optee/ > > diff --git a/drivers/tee/tee_heap.c b/drivers/tee/tee_heap.c > > new file mode 100644 > > index 000000000000..a332805f9f26 > > --- /dev/null > > +++ b/drivers/tee/tee_heap.c > > @@ -0,0 +1,487 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2025, Linaro Limited > > + */ > > + > > +#include <linux/dma-buf.h> > > +#include <linux/dma-heap.h> > > +#include <linux/genalloc.h> > > +#include <linux/module.h> > > +#include <linux/scatterlist.h> > > +#include <linux/slab.h> > > +#include <linux/tee_core.h> > > +#include <linux/xarray.h> > > + > > +#include "tee_private.h" > > + > > +struct tee_dma_heap { > > + struct dma_heap *heap; > > + enum tee_dma_heap_id id; > > + struct tee_protmem_pool *pool; > > + struct tee_device *teedev; > > + /* Protects pool and teedev above */ > > + struct mutex mu; > > +}; > > + > > +struct tee_heap_buffer { > > + struct tee_protmem_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_protmem_static_pool { > > + struct tee_protmem_pool pool; > > + struct gen_pool *gen_pool; > > + phys_addr_t pa_base; > > + void *base; > > +}; > > + > > +#if IS_ENABLED(CONFIG_DMABUF_HEAPS) > > +static DEFINE_XARRAY_ALLOC(tee_dma_heap); > > + > > +static int copy_sg_table(struct sg_table *dst, struct sg_table *src) > > +{ > > + struct scatterlist *dst_sg; > > + struct scatterlist *src_sg; > > + int ret; > > + int i; > > + > > + ret = sg_alloc_table(dst, src->orig_nents, GFP_KERNEL); > > + if (ret) > > + return ret; > > + > > + dst_sg = dst->sgl; > > + for_each_sgtable_sg(src, src_sg, i) { > > + sg_set_page(dst_sg, sg_page(src_sg), src_sg->length, > > + src_sg->offset); > > + dst_sg = sg_next(dst_sg); > > + } > > + > > + return 0; > > +} > > + > > +static int tee_heap_attach(struct dma_buf *dmabuf, > > + struct dma_buf_attachment *attachment) > > +{ > > + struct tee_heap_buffer *buf = dmabuf->priv; > > + struct tee_heap_attachment *a; > > + int ret; > > + > > + a = kzalloc(sizeof(*a), GFP_KERNEL); > > + if (!a) > > + return -ENOMEM; > > + > > + ret = copy_sg_table(&a->table, &buf->table); > > + if (ret) { > > + kfree(a); > > + return ret; > > + } > > + > > + a->dev = attachment->dev; > > + attachment->priv = a; > > + > > + return 0; > > +} > > + > > +static void tee_heap_detach(struct dma_buf *dmabuf, > > + struct dma_buf_attachment *attachment) > > +{ > > + struct tee_heap_attachment *a = attachment->priv; > > + > > + sg_free_table(&a->table); > > + kfree(a); > > +} > > + > > +static struct sg_table * > > +tee_heap_map_dma_buf(struct dma_buf_attachment *attachment, > > + enum dma_data_direction direction) > > +{ > > + struct tee_heap_attachment *a = attachment->priv; > > + int ret; > > + > > + ret = dma_map_sgtable(attachment->dev, &a->table, direction, > > + DMA_ATTR_SKIP_CPU_SYNC); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return &a->table; > > +} > > + > > +static void tee_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, > > + struct sg_table *table, > > + enum dma_data_direction direction) > > +{ > > + struct tee_heap_attachment *a = attachment->priv; > > + > > + WARN_ON(&a->table != table); > > + > > + dma_unmap_sgtable(attachment->dev, table, direction, > > + DMA_ATTR_SKIP_CPU_SYNC); > > +} > > + > > +static void tee_heap_buf_free(struct dma_buf *dmabuf) > > +{ > > + struct tee_heap_buffer *buf = dmabuf->priv; > > + struct tee_device *teedev = buf->teedev; > > + > > + buf->pool->ops->free(buf->pool, &buf->table); > > + tee_device_put(teedev); > > +} > > + > > +static const struct dma_buf_ops tee_heap_buf_ops = { > > + .attach = tee_heap_attach, > > + .detach = tee_heap_detach, > > + .map_dma_buf = tee_heap_map_dma_buf, > > + .unmap_dma_buf = tee_heap_unmap_dma_buf, > > + .release = tee_heap_buf_free, > > +}; > > + > > +static struct dma_buf *tee_dma_heap_alloc(struct dma_heap *heap, > > + unsigned long len, u32 fd_flags, > > + u64 heap_flags) > > +{ > > + struct tee_dma_heap *h = dma_heap_get_drvdata(heap); > > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > > + struct tee_device *teedev = NULL; > > + struct tee_heap_buffer *buf; > > + struct tee_protmem_pool *pool; > > + struct dma_buf *dmabuf; > > + int rc; > > + > > + mutex_lock(&h->mu); > > + if (tee_device_get(h->teedev)) { > > + teedev = h->teedev; > > + pool = h->pool; > > + } > > + mutex_unlock(&h->mu); > > + > > + if (!teedev) > > + return ERR_PTR(-EINVAL); > > + > > + buf = kzalloc(sizeof(*buf), GFP_KERNEL); > > + if (!buf) { > > + dmabuf = ERR_PTR(-ENOMEM); > > + goto err; > > + } > > + buf->size = len; > > + buf->pool = pool; > > + buf->teedev = teedev; > > + > > + rc = pool->ops->alloc(pool, &buf->table, len, &buf->offs); > > + if (rc) { > > + dmabuf = ERR_PTR(rc); > > + goto err_kfree; > > + } > > + > > + exp_info.ops = &tee_heap_buf_ops; > > + exp_info.size = len; > > + exp_info.priv = buf; > > + exp_info.flags = fd_flags; > > + dmabuf = dma_buf_export(&exp_info); > > + if (IS_ERR(dmabuf)) > > + goto err_protmem_free; > > + > > + return dmabuf; > > + > > +err_protmem_free: > > + pool->ops->free(pool, &buf->table); > > +err_kfree: > > + kfree(buf); > > +err: > > + tee_device_put(h->teedev); > > + return dmabuf; > > +} > > + > > +static const struct dma_heap_ops tee_dma_heap_ops = { > > + .allocate = tee_dma_heap_alloc, > > +}; > > + > > +static const char *heap_id_2_name(enum tee_dma_heap_id id) > > +{ > > + switch (id) { > > + case TEE_DMA_HEAP_SECURE_VIDEO_PLAY: > > + return "protected,secure-video"; > > + case TEE_DMA_HEAP_TRUSTED_UI: > > + return "protected,trusted-ui"; > > + case TEE_DMA_HEAP_SECURE_VIDEO_RECORD: > > + return "protected,secure-video-record"; > > + default: > > + return NULL; > > + } > > +} > > + > > +static int alloc_dma_heap(struct tee_device *teedev, enum tee_dma_heap_id id, > > + struct tee_protmem_pool *pool) > > +{ > > + struct dma_heap_export_info exp_info = { > > + .ops = &tee_dma_heap_ops, > > + .name = heap_id_2_name(id), > > + }; > > + struct tee_dma_heap *h; > > + int rc; > > + > > + if (!exp_info.name) > > + return -EINVAL; > > + > > + if (xa_reserve(&tee_dma_heap, id, GFP_KERNEL)) { > > + if (!xa_load(&tee_dma_heap, id)) > > + return -EEXIST; > > + return -ENOMEM; > > + } > > + > > + h = kzalloc(sizeof(*h), GFP_KERNEL); > > + if (!h) > > + return -ENOMEM; > > + h->id = id; > > + h->teedev = teedev; > > + h->pool = pool; > > + mutex_init(&h->mu); > > + > > + exp_info.priv = h; > > + h->heap = dma_heap_add(&exp_info); > > + if (IS_ERR(h->heap)) { > > + rc = PTR_ERR(h->heap); > > + kfree(h); > > + > > + return rc; > > + } > > + > > + /* "can't fail" due to the call to xa_reserve() above */ > > + return WARN(xa_store(&tee_dma_heap, id, h, GFP_KERNEL), > > + "xa_store() failed"); > > I think this can rather be simplified to: > > return WARN_ON(xa_is_err(xa_store(&tee_dma_heap, id, h, GFP_KERNEL))); OK > > > +} > > + > > +int tee_device_register_dma_heap(struct tee_device *teedev, > > + enum tee_dma_heap_id id, > > + struct tee_protmem_pool *pool) > > +{ > > + struct tee_dma_heap *h; > > + int rc; > > + > > + h = xa_load(&tee_dma_heap, id); > > + if (h) { > > + mutex_lock(&h->mu); > > + if (h->teedev) { > > + rc = -EBUSY; > > + } else { > > + h->teedev = teedev; > > + h->pool = pool; > > + rc = 0; > > + } > > + mutex_unlock(&h->mu); > > + } else { > > + rc = alloc_dma_heap(teedev, id, pool); > > + } > > + > > + if (rc) > > + dev_err(&teedev->dev, "can't register DMA heap id %d (%s)\n", > > + id, heap_id_2_name(id)); > > + > > + return rc; > > +} > > +EXPORT_SYMBOL_GPL(tee_device_register_dma_heap); > > + > > +void tee_device_unregister_all_dma_heaps(struct tee_device *teedev) > > +{ > > + struct tee_protmem_pool *pool; > > + struct tee_dma_heap *h; > > + u_long i; > > + > > + xa_for_each(&tee_dma_heap, i, h) { > > + if (h) { > > + pool = NULL; > > + mutex_lock(&h->mu); > > + if (h->teedev == teedev) { > > + pool = h->pool; > > + h->teedev = NULL; > > + h->pool = NULL; > > + } > > + mutex_unlock(&h->mu); > > + if (pool) > > + pool->ops->destroy_pool(pool); > > + } > > + } > > +} > > +EXPORT_SYMBOL_GPL(tee_device_unregister_all_dma_heaps); > > + > > +int tee_heap_update_from_dma_buf(struct tee_device *teedev, > > + struct dma_buf *dmabuf, size_t *offset, > > + struct tee_shm *shm, > > + struct tee_shm **parent_shm) > > +{ > > + struct tee_heap_buffer *buf; > > + int rc; > > + > > + /* The DMA-buf must be from our heap */ > > + if (dmabuf->ops != &tee_heap_buf_ops) > > + return -EINVAL; > > + > > + buf = dmabuf->priv; > > + /* The buffer must be from the same teedev */ > > + if (buf->teedev != teedev) > > + return -EINVAL; > > + > > + shm->size = buf->size; > > + > > + rc = buf->pool->ops->update_shm(buf->pool, &buf->table, buf->offs, shm, > > + parent_shm); > > + if (!rc && *parent_shm) > > + *offset = buf->offs; > > + > > + return rc; > > +} > > +#else > > +int tee_device_register_dma_heap(struct tee_device *teedev __always_unused, > > + enum tee_dma_heap_id id __always_unused, > > + struct tee_protmem_pool *pool __always_unused) > > +{ > > + return -EINVAL; > > +} > > +EXPORT_SYMBOL_GPL(tee_device_register_dma_heap); > > + > > +void > > +tee_device_unregister_all_dma_heaps(struct tee_device *teedev __always_unused) > > +{ > > +} > > +EXPORT_SYMBOL_GPL(tee_device_unregister_all_dma_heaps); > > + > > +int tee_heap_update_from_dma_buf(struct tee_device *teedev __always_unused, > > + struct dma_buf *dmabuf __always_unused, > > + size_t *offset __always_unused, > > + struct tee_shm *shm __always_unused, > > + struct tee_shm **parent_shm __always_unused) > > +{ > > + return -EINVAL; > > +} > > +#endif > > + > > +static struct tee_protmem_static_pool * > > +to_protmem_static_pool(struct tee_protmem_pool *pool) > > +{ > > + return container_of(pool, struct tee_protmem_static_pool, pool); > > +} > > + > > +static int protmem_pool_op_static_alloc(struct tee_protmem_pool *pool, > > + struct sg_table *sgt, size_t size, > > + size_t *offs) > > +{ > > + struct tee_protmem_static_pool *stp = to_protmem_static_pool(pool); > > + phys_addr_t pa; > > + int ret; > > + > > + pa = gen_pool_alloc(stp->gen_pool, size); > > + if (!pa) > > + return -ENOMEM; > > + > > + ret = sg_alloc_table(sgt, 1, GFP_KERNEL); > > + if (ret) { > > + gen_pool_free(stp->gen_pool, pa, size); > > + return ret; > > + } > > + > > + sg_set_page(sgt->sgl, phys_to_page(pa), size, 0); > > Did you missed pfn_valid() check from prior v8 review comments? No, I tried it, but pfn_valid() didn't like the address, so I had to find a way to fix it. tee_protmem_static_pool_alloc(), below, calls memremap() on the range, and that should make pfn_valid() redundant. > > > + *offs = pa - stp->pa_base; > > + > > + return 0; > > +} > > + > > +static void protmem_pool_op_static_free(struct tee_protmem_pool *pool, > > + struct sg_table *sgt) > > +{ > > + struct tee_protmem_static_pool *stp = to_protmem_static_pool(pool); > > + struct scatterlist *sg; > > + int i; > > + > > + for_each_sgtable_sg(sgt, sg, i) > > + gen_pool_free(stp->gen_pool, sg_phys(sg), sg->length); > > + sg_free_table(sgt); > > +} > > + > > +static int protmem_pool_op_static_update_shm(struct tee_protmem_pool *pool, > > + struct sg_table *sgt, size_t offs, > > + struct tee_shm *shm, > > + struct tee_shm **parent_shm) > > +{ > > + struct tee_protmem_static_pool *stp = to_protmem_static_pool(pool); > > + > > + shm->paddr = stp->pa_base + offs; > > + *parent_shm = NULL; > > + > > + return 0; > > +} > > + > > +static void protmem_pool_op_static_destroy_pool(struct tee_protmem_pool *pool) > > +{ > > + struct tee_protmem_static_pool *stp = to_protmem_static_pool(pool); > > + > > + gen_pool_destroy(stp->gen_pool); > > + memunmap(stp->base); > > + kfree(stp); > > +} > > + > > +static struct tee_protmem_pool_ops protmem_pool_ops_static = { > > + .alloc = protmem_pool_op_static_alloc, > > + .free = protmem_pool_op_static_free, > > + .update_shm = protmem_pool_op_static_update_shm, > > + .destroy_pool = protmem_pool_op_static_destroy_pool, > > +}; > > + > > +struct tee_protmem_pool *tee_protmem_static_pool_alloc(phys_addr_t paddr, > > + size_t size) > > +{ > > + const size_t page_mask = PAGE_SIZE - 1; > > + struct tee_protmem_static_pool *stp; > > + int rc; > > + > > + /* Check it's page aligned */ > > + if ((paddr | size) & page_mask) > > + return ERR_PTR(-EINVAL); > > + > > + stp = kzalloc(sizeof(*stp), GFP_KERNEL); > > + if (!stp) > > + return ERR_PTR(-ENOMEM); > > + > > + /* > > + * Map the memory as uncached to make sure the kernel can work with > > + * __pfn_to_page() and friends since that's needed when passing the > > + * protected DMA-buf to a device. The memory should otherwise not > > + * be touched by the kernel since it's likely to cause an external > > + * abort due to the protection status. > > + */ > > + stp->base = memremap(paddr, size, MEMREMAP_WC); > > + if (!stp->base) { > > + rc = -EINVAL; > > + goto err_free; > > + } > > + > > + stp->gen_pool = gen_pool_create(PAGE_SHIFT, -1); > > + if (!stp->gen_pool) { > > + rc = -ENOMEM; > > + goto err_unmap; > > + } > > + > > + rc = gen_pool_add(stp->gen_pool, paddr, size, -1); > > + if (rc) > > + goto err_free_pool; > > + > > + stp->pool.ops = &protmem_pool_ops_static; > > + stp->pa_base = paddr; > > + return &stp->pool; > > + > > +err_free_pool: > > + gen_pool_destroy(stp->gen_pool); > > +err_unmap: > > + memunmap(stp->base); > > +err_free: > > + kfree(stp); > > + > > + return ERR_PTR(rc); > > +} > > +EXPORT_SYMBOL_GPL(tee_protmem_static_pool_alloc); > > diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h > > index 9bc50605227c..6c6ff5d5eed2 100644 > > --- a/drivers/tee/tee_private.h > > +++ b/drivers/tee/tee_private.h > > @@ -8,6 +8,7 @@ > > #include <linux/cdev.h> > > #include <linux/completion.h> > > #include <linux/device.h> > > +#include <linux/dma-buf.h> > > #include <linux/kref.h> > > #include <linux/mutex.h> > > #include <linux/types.h> > > @@ -24,4 +25,9 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size); > > struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx, > > unsigned long addr, size_t length); > > > > +int tee_heap_update_from_dma_buf(struct tee_device *teedev, > > + struct dma_buf *dmabuf, size_t *offset, > > + struct tee_shm *shm, > > + struct tee_shm **parent_shm); > > + > > #endif /*TEE_PRIVATE_H*/ > > diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h > > index a38494d6b5f4..b8b99c97e00c 100644 > > --- a/include/linux/tee_core.h > > +++ b/include/linux/tee_core.h > > @@ -8,9 +8,11 @@ > > > > #include <linux/cdev.h> > > #include <linux/device.h> > > +#include <linux/dma-buf.h> > > #include <linux/idr.h> > > #include <linux/kref.h> > > #include <linux/list.h> > > +#include <linux/scatterlist.h> > > #include <linux/tee.h> > > #include <linux/tee_drv.h> > > #include <linux/types.h> > > @@ -30,6 +32,12 @@ > > #define TEE_DEVICE_FLAG_REGISTERED 0x1 > > #define TEE_MAX_DEV_NAME_LEN 32 > > > > +enum tee_dma_heap_id { > > + TEE_DMA_HEAP_SECURE_VIDEO_PLAY = 1, > > + TEE_DMA_HEAP_TRUSTED_UI, > > + TEE_DMA_HEAP_SECURE_VIDEO_RECORD, > > +}; > > + > > /** > > * struct tee_device - TEE Device representation > > * @name: name of device > > @@ -116,6 +124,36 @@ struct tee_desc { > > u32 flags; > > }; > > > > +/** > > + * struct tee_protmem_pool - protected memory pool > > + * @ops: operations > > + * > > + * This is an abstract interface where this struct is expected to be > > + * embedded in another struct specific to the implementation. > > + */ > > +struct tee_protmem_pool { > > + const struct tee_protmem_pool_ops *ops; > > +}; > > + > > +/** > > + * struct tee_protmem_pool_ops - protected memory pool operations > > + * @alloc: called when allocating protected memory > > + * @free: called when freeing protected memory > > + * @update_shm: called when registering a dma-buf to update the @shm > > + * with physical address of the buffer or to return the > > + * @parent_shm of the memory pool > > + * @destroy_pool: called when destroying the pool > > + */ > > +struct tee_protmem_pool_ops { > > + int (*alloc)(struct tee_protmem_pool *pool, struct sg_table *sgt, > > + size_t size, size_t *offs); > > + void (*free)(struct tee_protmem_pool *pool, struct sg_table *sgt); > > + int (*update_shm)(struct tee_protmem_pool *pool, struct sg_table *sgt, > > + size_t offs, struct tee_shm *shm, > > + struct tee_shm **parent_shm); > > + void (*destroy_pool)(struct tee_protmem_pool *pool); > > +}; > > + > > /** > > * tee_device_alloc() - Allocate a new struct tee_device instance > > * @teedesc: Descriptor for this driver > > @@ -154,6 +192,11 @@ int tee_device_register(struct tee_device *teedev); > > */ > > void tee_device_unregister(struct tee_device *teedev); > > > > +int tee_device_register_dma_heap(struct tee_device *teedev, > > + enum tee_dma_heap_id id, > > + struct tee_protmem_pool *pool); > > +void tee_device_unregister_all_dma_heaps(struct tee_device *teedev); > > + > > /** > > * tee_device_set_dev_groups() - Set device attribute groups > > * @teedev: Device to register > > @@ -229,6 +272,28 @@ static inline void tee_shm_pool_free(struct tee_shm_pool *pool) > > pool->ops->destroy_pool(pool); > > } > > > > +/** > > + * tee_protmem_static_pool_alloc() - Create a protected memory manager > > + * @paddr: Physical address of start of pool > > + * @size: Size in bytes of the pool > > + * > > + * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure. > > s/tee_shm_pool/tee_protmem_pool/ I'll fix it. > > Rest looks fine to me. Thanks, Jens > > -Sumit > > > + */ > > +struct tee_protmem_pool *tee_protmem_static_pool_alloc(phys_addr_t paddr, > > + size_t size); > > + > > +/** > > + * tee_protmem_pool_free() - Free a protected memory pool > > + * @pool: The protected memory pool to free > > + * > > + * There must be no remaining protected memory allocated from this pool > > + * when this function is called. > > + */ > > +static inline void tee_protmem_pool_free(struct tee_protmem_pool *pool) > > +{ > > + pool->ops->destroy_pool(pool); > > +} > > + > > /** > > * tee_get_drvdata() - Return driver_data pointer > > * @returns the driver_data pointer supplied to tee_register(). > > -- > > 2.43.0 > >

8 months, 3 weeks

1
0
0 0

Re: [PATCH v3 01/10] dt-bindings: npu: rockchip,rknn: Add bindings

by Rob Herring (Arm)

On Fri, 16 May 2025 18:53:15 +0200, Tomeu Vizoso wrote: > Add the bindings for the Neural Processing Unit IP from Rockchip. > > v2: > - Adapt to new node structure (one node per core, each with its own > IOMMU) > - Several misc. fixes from Sebastian Reichel > > v3: > - Split register block in its constituent subblocks, and only require > the ones that the kernel would ever use (Nicolas Frattaroli) > - Group supplies (Rob Herring) > - Explain the way in which the top core is special (Rob Herring) > > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > Signed-off-by: Sebastian Reichel <sebastian.reichel(a)collabora.com> > --- > .../bindings/npu/rockchip,rknn-core.yaml | 162 +++++++++++++++++++++ > 1 file changed, 162 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml: properties:reg-names: 'oneOf' conditional failed, one must be fixed: [{'const': 'pc'}, {'const': 'cna'}, {'const': 'core'}] is too long [{'const': 'pc'}, {'const': 'cna'}, {'const': 'core'}] is too short False schema does not allow 3 1 was expected 3 is greater than the maximum of 2 hint: "minItems" is only needed if less than the "items" list length from schema $id: http://devicetree.org/meta-schemas/items.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/npu/rockchip,rknn-core.example.dtb: npu-core@fdab0000 (rockchip,rk3588-rknn-core-top): compatible: 'oneOf' conditional failed, one must be fixed: ['rockchip,rk3588-rknn-core-top', 'rockchip,rknn-core-top'] is too long 'rockchip,rk3588-rknn-core-top' is not one of ['rockchip,rk3588-rknn-core'] from schema $id: http://devicetree.org/schemas/npu/rockchip,rknn-core.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/npu/rockchip,rknn-core.example.dtb: npu-core@fdab0000 (rockchip,rk3588-rknn-core-top): reg: [[0, 4255842304, 0, 36864]] is too short from schema $id: http://devicetree.org/schemas/npu/rockchip,rknn-core.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/npu/rockchip,rknn-core.example.dtb: npu-core@fdac0000 (rockchip,rk3588-rknn-core): compatible: 'oneOf' conditional failed, one must be fixed: ['rockchip,rk3588-rknn-core', 'rockchip,rknn-core'] is too long 'rockchip,rk3588-rknn-core' is not one of ['rockchip,rk3588-rknn-core-top'] from schema $id: http://devicetree.org/schemas/npu/rockchip,rknn-core.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/npu/rockchip,rknn-core.example.dtb: npu-core@fdac0000 (rockchip,rk3588-rknn-core): reg: [[0, 4255907840, 0, 36864]] is too short from schema $id: http://devicetree.org/schemas/npu/rockchip,rknn-core.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250516-6-1… The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

8 months, 4 weeks

2
1
0 0

[PATCH v9 0/9] TEE subsystem for protected dma-buf allocations

by Jens Wiklander

Hi, This patch set allocates the protected 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 protection for the memory used for the DMA-bufs. The DMA-heap uses a protected memory pool provided by the backend TEE driver, allowing it to choose how to allocate the protected 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) have been identified so far to serve as examples of what can be expected. The use-cases have predefined DMA-heap names, "protected,secure-video", "protected,trusted-ui", and "protected,secure-video-record". The backend driver registers protected memory pools for the use-cases it supports. Each use-case has its own protected memory pool since different use-cases require isolation from different parts of the system. A protected memory pool can be based on a static carveout instantiated while probing the TEE backend driver, or dynamically allocated from CMA (dma_alloc_pages()) and made protected 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-v9 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 protected DMA-bufs which are consumed by the kmssink. The primitive QEMU tests from previous patch sets can be tested on RockPi in the same way using: xtest --sdp-basic The primitive tests 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-v9 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 to SPMC_AT_EL=n to test without FF-A using the original SMC ABI instead. Please remember to do %make arm-tf-clean for TF-A to be rebuilt properly using the new configuration. https://optee.readthedocs.io/en/latest/building/prerequisites.html list dependencies required to build the above. The primitive 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 V8: * Using dma_alloc_pages() instead of cma_alloc() so the direct dependency on CMA can be removed together with the patches "cma: export cma_alloc() and cma_release()" and "dma-contiguous: export dma_contiguous_default_area". The patch * Renaming the patch "tee: add tee_shm_alloc_cma_phys_mem()" to "tee: add tee_shm_alloc_dma_mem()" * Setting DMA mask for the OP-TEE TEE device based on input from the secure world instead of relying on the parent device so following patches are removed: "tee: tee_device_alloc(): copy dma_mask from parent device" and "optee: pass parent device to tee_device_alloc()". * Adding Sumit Garg's R-B to "tee: refactor params_from_user()" * In the patch "tee: implement protected DMA-heap", map the physical memory passed to tee_protmem_static_pool_alloc(). Changes since V7: * Adding "dma-buf: dma-heap: export declared functions", "cma: export cma_alloc() and cma_release()", and "dma-contiguous: export dma_contiguous_default_area" to export the symbols needed to keep the TEE subsystem as a load module. * Removing CONFIG_TEE_DMABUF_HEAP and CONFIG_TEE_CMA since they aren't needed any longer. * Addressing review comments in "optee: sync secure world ABI headers" * Better align protected memory pool initialization between the smc-abi and ffa-abi parts of the optee driver. * Removing the patch "optee: account for direction while converting parameters" Changes since V6: * Restricted memory is now known as protected memory since to use the same term as https://docs.vulkan.org/guide/latest/protected.html. Update all patches to consistently use protected memory. * In "tee: implement protected DMA-heap" add the hidden config option TEE_DMABUF_HEAP to tell if the DMABUF_HEAPS functions are available for the TEE subsystem * Adding "tee: refactor params_from_user()", broken out from the patch "tee: new ioctl to a register tee_shm from a dmabuf file descriptor" * For "tee: new ioctl to a register tee_shm from a dmabuf file descriptor": - Update commit message to mention protected memory - Remove and open code tee_shm_get_parent_shm() in param_from_user_memref() * In "tee: add tee_shm_alloc_cma_phys_mem" add the hidden config option TEE_CMA to tell if the CMA functions are available for the TEE subsystem * For "tee: tee_device_alloc(): copy dma_mask from parent device" and "optee: pass parent device to tee_device_alloc", added Reviewed-by: Sumit Garg <sumit.garg(a)kernel.org> 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 (8): optee: sync secure world ABI headers dma-buf: dma-heap: export declared functions tee: implement protected DMA-heap tee: refactor params_from_user() tee: add tee_shm_alloc_dma_mem() optee: support protected memory allocation optee: FF-A: dynamic protected memory allocation optee: smc abi: dynamic protected memory allocation drivers/dma-buf/dma-heap.c | 3 + drivers/tee/Makefile | 1 + drivers/tee/optee/Makefile | 1 + drivers/tee/optee/core.c | 10 + drivers/tee/optee/ffa_abi.c | 147 ++++++++- drivers/tee/optee/optee_ffa.h | 27 +- drivers/tee/optee/optee_msg.h | 84 +++++- drivers/tee/optee/optee_private.h | 15 +- drivers/tee/optee/optee_smc.h | 37 ++- drivers/tee/optee/protmem.c | 332 ++++++++++++++++++++ drivers/tee/optee/smc_abi.c | 113 ++++++- drivers/tee/tee_core.c | 155 +++++++--- drivers/tee/tee_heap.c | 487 ++++++++++++++++++++++++++++++ drivers/tee/tee_private.h | 16 + drivers/tee/tee_shm.c | 183 ++++++++++- include/linux/tee_core.h | 71 +++++ include/linux/tee_drv.h | 10 + include/uapi/linux/tee.h | 31 ++ 18 files changed, 1655 insertions(+), 68 deletions(-) create mode 100644 drivers/tee/optee/protmem.c create mode 100644 drivers/tee/tee_heap.c base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e -- 2.43.0

8 months, 4 weeks

3
14
0 0

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

by Christian König

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? 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.

8 months, 4 weeks

1
0
0 0

[PATCH v2 00/12] Rust abstractions for shmem-backed GEM objects

by Lyude Paul

This is the next version of the shmem backed GEM objects series originally from Asahi, previously posted by Daniel Almeida. Along with bindings for shmem backed GEM objects, it also adds a few features that various users like Tyr and Asahi are interested in: * The ability to pass custom arguments to new GEM objects (needed by Tyr) * OpaqueObject (to enable the use of custom private GEM objects, which I believe asahi wanted) And replaces some of the hand-rolled API bindings (sg_table mainly) with some of the WIP patch series for adding kernel-wide bindings. It also addresses the comments from the code review of the last version of this patch series. Currently doesn't apply on an upstream branch, but should very soon as all of the dependencies in this series are on a mailing list already. The current branch this can be applied on top of is here: https://gitlab.freedesktop.org/lyudess/linux/-/commits/rust%2Fgem-shmem-base Which is based on top of nova/nova-next with the following patch series applied: * My (hopefully final) gem bindings cleanup: https://lkml.org/lkml/2025/5/20/1541 * Benno's derive Zeroable series: https://lkml.org/lkml/2025/5/20/1446 * Abdiel's sg_table series: https://lwn.net/Articles/1020986/ Also, there is one FIXES patch on top of Abdiel's work to fix some iterator bugs. These fixes have already been mentioned on the mailing list and should not be needed for their V2 version Asahi Lina (3): rust: helpers: Add bindings/wrappers for dma_resv_lock rust: drm: gem: shmem: Add DRM shmem helper abstraction rust: drm: gem: shmem: Add share_dma_resv to ObjectConfig Lyude Paul (9): rust: drm: gem: Add raw_dma_resv() function drm/gem/shmem: Extract drm_gem_shmem_init() from drm_gem_shmem_create() drm/gem/shmem: Extract drm_gem_shmem_release() from drm_gem_shmem_free() rust: gem: Introduce BaseDriverObject::Args rust: drm: gem: Add OpaqueObject rust: drm: gem: Introduce OwnedSGTable rust: Add dma_buf stub bindings rust: drm: gem: Add export() callback rust: drm: gem: Add BaseObject::prime_export() drivers/gpu/drm/drm_gem_shmem_helper.c | 98 +++++-- drivers/gpu/drm/nova/gem.rs | 6 +- include/drm/drm_gem_shmem_helper.h | 2 + rust/bindings/bindings_helper.h | 4 + rust/helpers/dma-resv.c | 13 + rust/helpers/drm.c | 48 +++- rust/helpers/helpers.c | 1 + rust/kernel/dma_buf.rs | 39 +++ rust/kernel/drm/gem/mod.rs | 187 ++++++++++++- rust/kernel/drm/gem/shmem.rs | 370 +++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 11 files changed, 727 insertions(+), 42 deletions(-) create mode 100644 rust/helpers/dma-resv.c create mode 100644 rust/kernel/dma_buf.rs create mode 100644 rust/kernel/drm/gem/shmem.rs -- 2.49.0

8 months, 4 weeks

2
6
0 0

Re: [PATCH v3 4/5] drm/panfrost: show device-wide list of DRM GEM objects over DebugFS

by Steven Price

On 20/05/2025 18:44, Adrián Larumbe wrote: > This change is essentially a Panfrost port of commit a3707f53eb3f > ("drm/panthor: show device-wide list of DRM GEM objects over DebugFS"). > > The DebugFS file is almost the same as in Panthor, minus the GEM object > usage flags, since Panfrost has no kernel-only BO's. > > Two additional GEM state flags which are displayed but aren't relevant > to Panthor are 'Purged' and 'Purgeable', since Panfrost implements an > explicit shrinker and a madvise ioctl to flag objects as reclaimable. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> Reviewed-by: Steven Price <steven.price(a)arm.com> > --- > drivers/gpu/drm/panfrost/panfrost_device.c | 5 + > drivers/gpu/drm/panfrost/panfrost_device.h | 15 +++ > drivers/gpu/drm/panfrost/panfrost_drv.c | 35 ++++++ > drivers/gpu/drm/panfrost/panfrost_gem.c | 134 +++++++++++++++++++++ > drivers/gpu/drm/panfrost/panfrost_gem.h | 47 ++++++++ > 5 files changed, 236 insertions(+) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c > index 5d35076b2e6d..04bec27449cb 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > @@ -209,6 +209,11 @@ int panfrost_device_init(struct panfrost_device *pfdev) > > spin_lock_init(&pfdev->cycle_counter.lock); > > +#ifdef CONFIG_DEBUG_FS > + mutex_init(&pfdev->debugfs.gems_lock); > + INIT_LIST_HEAD(&pfdev->debugfs.gems_list); > +#endif > + > err = panfrost_pm_domain_init(pfdev); > if (err) > return err; > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h > index dcff70f905cd..077525a3ad68 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.h > +++ b/drivers/gpu/drm/panfrost/panfrost_device.h > @@ -111,6 +111,17 @@ struct panfrost_compatible { > u8 gpu_quirks; > }; > > +/** > + * struct panfrost_device_debugfs - Device-wide DebugFS tracking structures > + */ > +struct panfrost_device_debugfs { > + /** @gems_list: Device-wide list of GEM objects owned by at least one file. */ > + struct list_head gems_list; > + > + /** @gems_lock: Serializes access to the device-wide list of GEM objects. */ > + struct mutex gems_lock; > +}; > + > struct panfrost_device { > struct device *dev; > struct drm_device *ddev; > @@ -164,6 +175,10 @@ struct panfrost_device { > atomic_t use_count; > spinlock_t lock; > } cycle_counter; > + > +#ifdef CONFIG_DEBUG_FS > + struct panfrost_device_debugfs debugfs; > +#endif > }; > > struct panfrost_mmu { > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 179fbaa1cd0c..f576cb215898 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -13,6 +13,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <drm/panfrost_drm.h> > +#include <drm/drm_debugfs.h> > #include <drm/drm_drv.h> > #include <drm/drm_ioctl.h> > #include <drm/drm_syncobj.h> > @@ -659,6 +660,37 @@ static const struct file_operations panfrost_drm_driver_fops = { > .show_fdinfo = drm_show_fdinfo, > }; > > +#ifdef CONFIG_DEBUG_FS > +static int panthor_gems_show(struct seq_file *m, void *data) > +{ > + struct drm_info_node *node = m->private; > + struct drm_device *dev = node->minor->dev; > + struct panfrost_device *pfdev = dev->dev_private; > + > + panfrost_gem_debugfs_print_bos(pfdev, m); > + > + return 0; > +} > + > +static struct drm_info_list panthor_debugfs_list[] = { > + {"gems", panthor_gems_show, 0, NULL}, > +}; > + > +static int panthor_gems_debugfs_init(struct drm_minor *minor) > +{ > + drm_debugfs_create_files(panthor_debugfs_list, > + ARRAY_SIZE(panthor_debugfs_list), > + minor->debugfs_root, minor); > + > + return 0; > +} > + > +static void panfrost_debugfs_init(struct drm_minor *minor) > +{ > + panthor_gems_debugfs_init(minor); > +} > +#endif > + > /* > * Panfrost driver version: > * - 1.0 - initial interface > @@ -683,6 +715,9 @@ static const struct drm_driver panfrost_drm_driver = { > > .gem_create_object = panfrost_gem_create_object, > .gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table, > +#ifdef CONFIG_DEBUG_FS > + .debugfs_init = panfrost_debugfs_init, > +#endif > }; > > static int panfrost_probe(struct platform_device *pdev) > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > index 04483d5fb45d..bb73f2a68a12 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > @@ -12,6 +12,36 @@ > #include "panfrost_gem.h" > #include "panfrost_mmu.h" > > +#ifdef CONFIG_DEBUG_FS > +static void panfrost_gem_debugfs_bo_add(struct panfrost_device *pfdev, > + struct panfrost_gem_object *bo) > +{ > + bo->debugfs.creator.tgid = current->group_leader->pid; > + get_task_comm(bo->debugfs.creator.process_name, current->group_leader); > + > + mutex_lock(&pfdev->debugfs.gems_lock); > + list_add_tail(&bo->debugfs.node, &pfdev->debugfs.gems_list); > + mutex_unlock(&pfdev->debugfs.gems_lock); > +} > + > +static void panfrost_gem_debugfs_bo_rm(struct panfrost_gem_object *bo) > +{ > + struct panfrost_device *pfdev = bo->base.base.dev->dev_private; > + > + if (list_empty(&bo->debugfs.node)) > + return; > + > + mutex_lock(&pfdev->debugfs.gems_lock); > + list_del_init(&bo->debugfs.node); > + mutex_unlock(&pfdev->debugfs.gems_lock); > +} > +#else > +static void panfrost_gem_debugfs_bo_add(struct panfrost_device *pfdev, > + struct panfrost_gem_object *bo) > +{} > +static void panfrost_gem_debugfs_bo_rm(struct panfrost_gem_object *bo) {} > +#endif > + > /* Called DRM core on the last userspace/kernel unreference of the > * BO. > */ > @@ -37,6 +67,7 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj) > WARN_ON_ONCE(!list_empty(&bo->mappings.list)); > > kfree_const(bo->label.str); > + panfrost_gem_debugfs_bo_rm(bo); > mutex_destroy(&bo->label.lock); > > if (bo->sgts) { > @@ -266,6 +297,8 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t > obj->base.map_wc = !pfdev->coherent; > mutex_init(&obj->label.lock); > > + panfrost_gem_debugfs_bo_add(pfdev, obj); > + > return &obj->base.base; > } > > @@ -354,3 +387,104 @@ panfrost_gem_internal_set_label(struct drm_gem_object *obj, const char *label) > > panfrost_gem_set_label(obj, str); > } > + > +#ifdef CONFIG_DEBUG_FS > +struct gem_size_totals { > + size_t size; > + size_t resident; > + size_t reclaimable; > +}; > + > +struct flag_def { > + u32 flag; > + const char *name; > +}; > + > +static void panfrost_gem_debugfs_print_flag_names(struct seq_file *m) > +{ > + int len; > + int i; > + > + static const struct flag_def gem_state_flags_names[] = { > + {PANFROST_DEBUGFS_GEM_STATE_FLAG_IMPORTED, "imported"}, > + {PANFROST_DEBUGFS_GEM_STATE_FLAG_EXPORTED, "exported"}, > + {PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGED, "purged"}, > + {PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGEABLE, "purgeable"}, > + }; > + > + seq_puts(m, "GEM state flags: "); > + for (i = 0, len = ARRAY_SIZE(gem_state_flags_names); i < len; i++) { > + seq_printf(m, "%s (0x%x)%s", gem_state_flags_names[i].name, > + gem_state_flags_names[i].flag, (i < len - 1) ? ", " : "\n\n"); > + } > +} > + > +static void panfrost_gem_debugfs_bo_print(struct panfrost_gem_object *bo, > + struct seq_file *m, > + struct gem_size_totals *totals) > +{ > + unsigned int refcount = kref_read(&bo->base.base.refcount); > + char creator_info[32] = {}; > + size_t resident_size; > + u32 gem_state_flags = 0; > + > + /* Skip BOs being destroyed. */ > + if (!refcount) > + return; > + > + resident_size = bo->base.pages ? bo->base.base.size : 0; > + > + snprintf(creator_info, sizeof(creator_info), > + "%s/%d", bo->debugfs.creator.process_name, bo->debugfs.creator.tgid); > + seq_printf(m, "%-32s%-16d%-16d%-16zd%-16zd0x%-16lx", > + creator_info, > + bo->base.base.name, > + refcount, > + bo->base.base.size, > + resident_size, > + drm_vma_node_start(&bo->base.base.vma_node)); > + > + if (bo->base.base.import_attach) > + gem_state_flags |= PANFROST_DEBUGFS_GEM_STATE_FLAG_IMPORTED; > + if (bo->base.base.dma_buf) > + gem_state_flags |= PANFROST_DEBUGFS_GEM_STATE_FLAG_EXPORTED; > + > + if (bo->base.madv < 0) > + gem_state_flags |= PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGED; > + else if (bo->base.madv > 0) > + gem_state_flags |= PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGEABLE; > + > + seq_printf(m, "0x%-10x", gem_state_flags); > + > + scoped_guard(mutex, &bo->label.lock) { > + seq_printf(m, "%s\n", bo->label.str ? : ""); > + } > + > + totals->size += bo->base.base.size; > + totals->resident += resident_size; > + if (bo->base.madv > 0) > + totals->reclaimable += resident_size; > +} > + > +void panfrost_gem_debugfs_print_bos(struct panfrost_device *pfdev, > + struct seq_file *m) > +{ > + struct gem_size_totals totals = {0}; > + struct panfrost_gem_object *bo; > + > + panfrost_gem_debugfs_print_flag_names(m); > + > + seq_puts(m, "created-by global-name refcount size resident-size file-offset state label\n"); > + seq_puts(m, "-----------------------------------------------------------------------------------------------------------------------------------\n"); > + > + scoped_guard(mutex, &pfdev->debugfs.gems_lock) { > + list_for_each_entry(bo, &pfdev->debugfs.gems_list, debugfs.node) { > + panfrost_gem_debugfs_bo_print(bo, m, &totals); > + } > + } > + > + seq_puts(m, "===================================================================================================================================\n"); > + seq_printf(m, "Total size: %zd, Total resident: %zd, Total reclaimable: %zd\n", > + totals.size, totals.resident, totals.reclaimable); > +} > +#endif > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h > index 3d87c41ad09d..8de3e76f2717 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.h > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h > @@ -8,9 +8,47 @@ > #include <drm/drm_mm.h> > > struct panfrost_mmu; > +struct panfrost_device; > > #define PANFROST_BO_LABEL_MAXLEN 4096 > > +enum panfrost_debugfs_gem_state_flags { > + /** @PANFROST_DEBUGFS_GEM_STATE_FLAG_IMPORTED: GEM BO is PRIME imported. */ > + PANFROST_DEBUGFS_GEM_STATE_FLAG_IMPORTED = BIT(0), > + > + /** @PANFROST_DEBUGFS_GEM_STATE_FLAG_EXPORTED: GEM BO is PRIME exported. */ > + PANFROST_DEBUGFS_GEM_STATE_FLAG_EXPORTED = BIT(1), > + > + /** @PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGED: GEM BO was reclaimed by the shrinker. */ > + PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGED = BIT(2), > + > + /** > + * @PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGEABLE: GEM BO pages were marked as no longer > + * needed by UM and can be reclaimed by the shrinker. > + */ > + PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGEABLE = BIT(3), > +}; > + > +/** > + * struct panfrost_gem_debugfs - GEM object's DebugFS list information > + */ > +struct panfrost_gem_debugfs { > + /** > + * @node: Node used to insert the object in the device-wide list of > + * GEM objects, to display information about it through a DebugFS file. > + */ > + struct list_head node; > + > + /** @creator: Information about the UM process which created the GEM. */ > + struct { > + /** @creator.process_name: Group leader name in owning thread's process */ > + char process_name[TASK_COMM_LEN]; > + > + /** @creator.tgid: PID of the thread's group leader within its process */ > + pid_t tgid; > + } creator; > +}; > + > struct panfrost_gem_object { > struct drm_gem_shmem_object base; > struct sg_table *sgts; > @@ -59,6 +97,10 @@ struct panfrost_gem_object { > > bool noexec :1; > bool is_heap :1; > + > +#ifdef CONFIG_DEBUG_FS > + struct panfrost_gem_debugfs debugfs; > +#endif > }; > > struct panfrost_gem_mapping { > @@ -108,4 +150,9 @@ void panfrost_gem_shrinker_cleanup(struct drm_device *dev); > void panfrost_gem_set_label(struct drm_gem_object *obj, const char *label); > void panfrost_gem_internal_set_label(struct drm_gem_object *obj, const char *label); > > +#ifdef CONFIG_DEBUG_FS > +void panfrost_gem_debugfs_print_bos(struct panfrost_device *pfdev, > + struct seq_file *m); > +#endif > + > #endif /* __PANFROST_GEM_H__ */

8 months, 4 weeks

1
0
0 0

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

by Maxime Ripard

Hi Jared, On Thu, Apr 24, 2025 at 09:11:24AM -0700, Jared Kangas wrote: > > > struct cma_heap { > > > struct dma_heap *heap; > > > @@ -394,15 +395,26 @@ static int __init __add_cma_heap(struct cma *cma, const char *name) > > > static int __init add_default_cma_heap(void) > > > { > > > struct cma *default_cma = dev_get_cma_area(NULL); > > > + const char *legacy_cma_name; > > > int ret; > > > > > > if (!default_cma) > > > return 0; > > > > > > - ret = __add_cma_heap(default_cma, cma_get_name(default_cma)); > > > + ret = __add_cma_heap(default_cma, DEFAULT_CMA_NAME); > > > if (ret) > > > return ret; > > > > > > + legacy_cma_name = cma_get_name(default_cma); > > > + > > > + if (IS_ENABLED(CONFIG_DMABUF_HEAPS_CMA_LEGACY) && > > > + strcmp(legacy_cma_name, DEFAULT_CMA_NAME)) { > > > + ret = __add_cma_heap(default_cma, legacy_cma_name); > > > + if (ret) > > > + pr_warn("cma_heap: failed to add legacy heap: %pe\n", > > > + ERR_PTR(-ret)); > > > + } > > > + > > > > It would also simplify this part, since you would always create the legacy heap. > > By "always", do you mean removing the strcmp? I added this to guard > against cases where the devicetree node's name clashed with the default > name, given that the DT name isn't necessarily restricted to one of the > current names in use ("linux,cma" or "default-pool"). It seems like the > strcmp would be relevant regardless of the naming choice, but if this is > overly cautious, I can remove it in v3. That's not overly cautious, that's something I overlooked :) You're totally right that we should check for that. We should probably add a more specific error message in that case though Maxime

8 months, 4 weeks

1
0
0 0

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

by Christian König

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? > 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. Regards, Christian.

8 months, 4 weeks

1
0
0 0

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

by Christian König

On 5/21/25 06:17, wangtao wrote: >>> Reducing CPU overhead/power consumption is critical for mobile devices. >>> We need simpler and more efficient dmabuf direct I/O support. >>> >>> As Christian evaluated sendfile performance based on your data, could >>> you confirm whether the cache was cleared? If not, please share the >>> post-cache-clearing test data. Thank you for your support. >> >> Yes sorry, I was out yesterday riding motorcycles. I did not clear the cache for >> the buffered reads, I didn't realize you had. The IO plus the copy certainly >> explains the difference. >> >> Your point about the unlikelihood of any of that data being in the cache also >> makes sense. > [wangtao] Thank you for testing and clarifying. > >> >> I'm not sure it changes anything about the ioctl approach though. >> Another way to do this would be to move the (optional) support for direct IO >> into the exporter via dma_buf_fops and dma_buf_ops. Then normal read() >> syscalls would just work for buffers that support them. >> I know that's more complicated, but at least it doesn't require inventing new >> uapi to do it. >> > [wangtao] Thank you for the discussion. I fully support any method that enables > dmabuf direct I/O. > > I understand using sendfile/splice with regular files for dmabuf > adds an extra CPU copy, preventing zero-copy. For example: > sendfile path: [DISK] → DMA → [page cache] → CPU copy → [memory file]. Yeah, but why can't you work on improving that? > The read() syscall can't pass regular file fd parameters, so I added > an ioctl command. > While copy_file_range() supports two fds (fd_in/fd_out), it blocks cross-fs use. > Even without this restriction, file_out->f_op->copy_file_range > only enables dmabuf direct reads from regular files, not writes. > > Since dmabuf's direct I/O limitation comes from its unique > attachment/map/fence model and lacks suitable syscalls, adding > an ioctl seems necessary. I absolutely don't see that. Both splice and sendfile can take two regular file descriptors. That the underlying fops currently can't do that is not a valid argument for adding new uAPI. It just means that you need to work on improving those fops. As long as nobody proves to me that the existing uAPI isn't sufficient for this use case I will systematically reject any approach to adding new one. Regards, Christian. > When system exporters return a duplicated sg_table via map_dma_buf > (used exclusively like a pages array), they should retain control > over it. > > I welcome all solutions to achieve dmabuf direct I/O! Your feedback > is greatly appreciated. > >> 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 >> >> >> >>> >>>>> >>>>>>> dmabuf buffer read | 51 | 1068 | 1118 >>>>>>> dmabuf direct read | 52 | 297 | 349 >>>>>>> >>>>>>> udmabuf sendfile test steps: >>>>>>> 1. Open data file(1024MB), get back_fd 2. Create memfd(32MB) # >>>>>>> Loop steps 2-6 3. Allocate udmabuf with memfd 4. Call >>>>>>> sendfile(memfd, >>>>>>> back_fd) 5. Close memfd after sendfile 6. Close udmabuf 7. >>>>>>> Close back_fd >>>>>>> >>>>>>>> >>>>>>>> Regards, >>>>>>>> Christian. >>>>>>> >>>>>> >>>

8 months, 4 weeks

1
0
0 0

Re: [PATCH v5 01/10] dt-bindings: npu: rockchip,rknn: Add bindings

by Krzysztof Kozlowski

On Tue, May 20, 2025 at 12:26:54PM GMT, Tomeu Vizoso wrote: > Add the bindings for the Neural Processing Unit IP from Rockchip. > > v2: > - Adapt to new node structure (one node per core, each with its own > IOMMU) > - Several misc. fixes from Sebastian Reichel > > v3: > - Split register block in its constituent subblocks, and only require > the ones that the kernel would ever use (Nicolas Frattaroli) > - Group supplies (Rob Herring) > - Explain the way in which the top core is special (Rob Herring) > > v4: > - Change required node name to npu@ (Rob Herring and Krzysztof Kozlowski) > - Remove unneeded items: (Krzysztof Kozlowski) > - Fix use of minItems/maxItems (Krzysztof Kozlowski) > - Add reg-names to list of required properties (Krzysztof Kozlowski) > - Fix example (Krzysztof Kozlowski) > > v5: > - Rename file to rockchip,rk3588-rknn-core.yaml (Krzysztof Kozlowski) > - Streamline compatible property (Krzysztof Kozlowski) > This is a big patchset, so please slow down and do not send it every day but allow people to actually review the version you post. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> Best regards, Krzysztof

8 months, 4 weeks

1
0
0 0

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

by T.J. Mercier

On Mon, May 19, 2025 at 9:06 PM wangtao <tao.wangtao(a)honor.com> wrote: > > > > > -----Original Message----- > > From: wangtao > > Sent: Monday, May 19, 2025 8:04 PM > > To: 'T.J. Mercier' <tjmercier(a)google.com>; Christian König > > <christian.koenig(a)amd.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> > > Subject: RE: [PATCH 2/2] dmabuf/heaps: implement > > DMA_BUF_IOCTL_RW_FILE for system_heap > > > > > > > > > -----Original Message----- > > > From: T.J. Mercier <tjmercier(a)google.com> > > > Sent: Saturday, May 17, 2025 2:37 AM > > > To: Christian König <christian.koenig(a)amd.com> > > > Cc: wangtao <tao.wangtao(a)honor.com>; 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> > > > Subject: Re: [PATCH 2/2] dmabuf/heaps: implement > > DMA_BUF_IOCTL_RW_FILE > > > for system_heap > > > > > > On Fri, May 16, 2025 at 1:36 AM Christian König > > > <christian.koenig(a)amd.com> > > > wrote: > > > > > > > > On 5/16/25 09:40, wangtao wrote: > > > > > > > > > > > > > > >> -----Original Message----- > > > > >> From: Christian König <christian.koenig(a)amd.com> > > > > >> Sent: Thursday, May 15, 2025 10:26 PM > > > > >> To: wangtao <tao.wangtao(a)honor.com>; sumit.semwal(a)linaro.org; > > > > >> benjamin.gaignard(a)collabora.com; Brian.Starkey(a)arm.com; > > > > >> jstultz(a)google.com; tjmercier(a)google.com > > > > >> Cc: 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> > > > > >> Subject: Re: [PATCH 2/2] dmabuf/heaps: implement > > > > >> DMA_BUF_IOCTL_RW_FILE for system_heap > > > > >> > > > > >> On 5/15/25 16:03, wangtao wrote: > > > > >>> [wangtao] My Test Configuration (CPU 1GHz, 5-test average): > > > > >>> Allocation: 32x32MB buffer creation > > > > >>> - dmabuf 53ms vs. udmabuf 694ms (10X slower) > > > > >>> - Note: shmem shows excessive allocation time > > > > >> > > > > >> Yeah, that is something already noted by others as well. But that > > > > >> is orthogonal. > > > > >> > > > > >>> > > > > >>> Read 1024MB File: > > > > >>> - dmabuf direct 326ms vs. udmabuf direct 461ms (40% slower) > > > > >>> - Note: pin_user_pages_fast consumes majority CPU cycles > > > > >>> > > > > >>> Key function call timing: See details below. > > > > >> > > > > >> Those aren't valid, you are comparing different functionalities here. > > > > >> > > > > >> Please try using udmabuf with sendfile() as confirmed to be > > > > >> working by > > > T.J. > > > > > [wangtao] Using buffer IO with dmabuf file read/write requires one > > > memory copy. > > > > > Direct IO removes this copy to enable zero-copy. The sendfile > > > > > system call reduces memory copies from two (read/write) to one. > > > > > However, with udmabuf, sendfile still keeps at least one copy, failing > > zero-copy. > > > > > > > > > > > > Then please work on fixing this. > > > > > > > > Regards, > > > > Christian. > > > > > > > > > > > > > > > > > > If udmabuf sendfile uses buffer IO (file page cache), read latency > > > > > matches dmabuf buffer read, but allocation time is much longer. > > > > > With Direct IO, the default 16-page pipe size makes it slower than > > > > > buffer > > > IO. > > > > > > > > > > Test data shows: > > > > > udmabuf direct read is much faster than udmabuf sendfile. > > > > > dmabuf direct read outperforms udmabuf direct read by a large margin. > > > > > > > > > > Issue: After udmabuf is mapped via map_dma_buf, apps using memfd > > > > > or udmabuf for Direct IO might cause errors, but there are no > > > > > safeguards to prevent this. > > > > > > > > > > Allocate 32x32MB buffer and read 1024 MB file Test: > > > > > Metric | alloc (ms) | read (ms) | total (ms) > > > > > -----------------------|------------|-----------|----------- > > > > > udmabuf buffer read | 539 | 2017 | 2555 > > > > > udmabuf direct read | 522 | 658 | 1179 > > > > > > I can't reproduce the part where udmabuf direct reads are faster than > > > buffered reads. That's the opposite of what I'd expect. Something > > > seems wrong with those buffered reads. > > > > > > > > udmabuf buffer sendfile| 505 | 1040 | 1546 > > > > > udmabuf direct sendfile| 510 | 2269 | 2780 > > > > > > I can reproduce the 3.5x slower udambuf direct sendfile compared to > > > udmabuf direct read. It's a pretty disappointing result, so it seems > > > like something could be improved there. > > > > > > 1G from ext4 on 6.12.17 | read/sendfile (ms) > > > ------------------------|------------------- > > > udmabuf buffer read | 351 > > > udmabuf direct read | 540 > > > udmabuf buffer sendfile | 255 > > > udmabuf direct sendfile | 1990 > > > > > [wangtao] By the way, did you clear the file cache during testing? > > Looking at your data again, read and sendfile buffers are faster than Direct > > I/O, which suggests the file cache wasn’t cleared. If you didn’t clear the file > > cache, the test results are unfair and unreliable for reference. On embedded > > devices, it’s nearly impossible to maintain stable caching for multi-GB files. If > > such files could be cached, we might as well cache dmabufs directly to save > > time on creating dmabufs and reading file data. > > You can call posix_fadvise(file_fd, 0, len, POSIX_FADV_DONTNEED) after > > opening the file or before closing it to clear the file cache, ensuring actual file > > I/O operations are tested. > > > [wangtao] Please confirm if cache clearing was performed during testing. > I reduced the test scope from 3GB to 1GB. While results without > cache clearing show general alignment, udmabuf buffer read remains > slower than direct read. Comparative data: > > Your test reading 1GB(ext4 on 6.12.17: > Method | read/sendfile (ms) | read vs. (%) > ---------------------------------------------------------- > udmabuf buffer read | 351 | 138% > udmabuf direct read | 540 | 212% > udmabuf buffer sendfile | 255 | 100% > udmabuf direct sendfile | 1990 | 780% > > My 3.5GHz tests (f2fs): > Without cache clearing: > Method | alloc | read | vs. (%) > ----------------------------------------------- > udmabuf buffer read | 140 | 386 | 310% > udmabuf direct read | 151 | 326 | 262% > udmabuf buffer sendfile | 136 | 124 | 100% > udmabuf direct sendfile | 132 | 892 | 717% > dmabuf buffer read | 23 | 154 | 124% > patch direct read | 29 | 271 | 218% > > With cache clearing: > 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% > patch direct read | 29 | 265 | 87% > > Results without cache clearing aren't representative for embedded > mobile devices. Notably, on low-power CPUs @1GHz, sendfile latency > without cache clearing exceeds dmabuf direct I/O read time. > > Without cache clearing: > Method | alloc | read | vs. (%) > ----------------------------------------------- > udmabuf buffer read | 546 | 1745 | 442% > udmabuf direct read | 511 | 704 | 178% > udmabuf buffer sendfile | 496 | 395 | 100% > udmabuf direct sendfile | 498 | 2332 | 591% > dmabuf buffer read | 43 | 453 | 115% > my patch direct read | 49 | 310 | 79% > > With cache clearing: > 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% > my patch direct read | 44 | 310 | 30% > > Reducing CPU overhead/power consumption is critical for mobile devices. > We need simpler and more efficient dmabuf direct I/O support. > > As Christian evaluated sendfile performance based on your data, could > you confirm whether the cache was cleared? If not, please share the > post-cache-clearing test data. Thank you for your support. Yes sorry, I was out yesterday riding motorcycles. I did not clear the cache for the buffered reads, I didn't realize you had. The IO plus the copy certainly explains the difference. Your point about the unlikelihood of any of that data being in the cache also makes sense. I'm not sure it changes anything about the ioctl approach though. Another way to do this would be to move the (optional) support for direct IO into the exporter via dma_buf_fops and dma_buf_ops. Then normal read() syscalls would just work for buffers that support them. I know that's more complicated, but at least it doesn't require inventing new uapi to do it. 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 > > > > > > > > > dmabuf buffer read | 51 | 1068 | 1118 > > > > > dmabuf direct read | 52 | 297 | 349 > > > > > > > > > > udmabuf sendfile test steps: > > > > > 1. Open data file(1024MB), get back_fd 2. Create memfd(32MB) # > > > > > Loop steps 2-6 3. Allocate udmabuf with memfd 4. Call > > > > > sendfile(memfd, > > > > > back_fd) 5. Close memfd after sendfile 6. Close udmabuf 7. Close > > > > > back_fd > > > > > > > > > >> > > > > >> Regards, > > > > >> Christian. > > > > > > > > > >

8 months, 4 weeks

1
0
0 0

[PATCH v4] dma-buf: heaps: Introduce a new heap for reserved memory

by Maxime Ripard

Some reserved memory regions might have particular memory setup or attributes that make them good candidates for heaps. Let's provide a heap type that will create a new heap for each reserved memory region flagged as such. Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Hi, This series is the follow-up of the discussion that John and I had some time ago here: https://lore.kernel.org/all/CANDhNCquJn6bH3KxKf65BWiTYLVqSd9892-xtFDHHqqyrr… The initial problem we were discussing was that I'm currently working on a platform which has a memory layout with ECC enabled. However, enabling the ECC has a number of drawbacks on that platform: lower performance, increased memory usage, etc. So for things like framebuffers, the trade-off isn't great and thus there's a memory region with ECC disabled to allocate from for such use cases. After a suggestion from John, I chose to first start using heap allocations flags to allow for userspace to ask for a particular ECC setup. This is then backed by a new heap type that runs from reserved memory chunks flagged as such, and the existing DT properties to specify the ECC properties. After further discussion, it was considered that flags were not the right solution, and relying on the names of the heaps would be enough to let userspace know the kind of buffer it deals with. Thus, even though the uAPI part of it has been dropped in this second version, we still need a driver to create heaps out of carved-out memory regions. In addition to the original usecase, a similar driver can be found in BSPs from most vendors, so I believe it would be a useful addition to the kernel. I submitted a draft PR to the DT schema for the bindings used in this PR: https://github.com/devicetree-org/dt-schema/pull/138 Let me know what you think, Maxime --- Changes in v4: - Rebased on 6.15-rc7 - Map buffers only when map is actually called, not at allocation time - Deal with restricted-dma-pool and shared-dma-pool - Reword Kconfig options - Properly report dma_map_sgtable failures - Link to v3: https://lore.kernel.org/r/20250407-dma-buf-ecc-heap-v3-0-97cdd36a5f29@kerne… Changes in v3: - Reworked global variable patch - Link to v2: https://lore.kernel.org/r/20250401-dma-buf-ecc-heap-v2-0-043fd006a1af@kerne… Changes in v2: - Add vmap/vunmap operations - Drop ECC flags uapi - Rebase on top of 6.14 - Link to v1: https://lore.kernel.org/r/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kerne… --- drivers/dma-buf/heaps/Kconfig | 8 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/carveout_heap.c | 388 ++++++++++++++++++++++++++++++++++ 3 files changed, 397 insertions(+) diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig index a5eef06c422644e8aadaf5aff2bd9a33c49c1ba3..1ce4f6828d8c06bfdd7bc2e5127707f1778586e6 100644 --- a/drivers/dma-buf/heaps/Kconfig +++ b/drivers/dma-buf/heaps/Kconfig @@ -1,5 +1,13 @@ +config DMABUF_HEAPS_CARVEOUT + bool "DMA-BUF Carveout Heaps" + depends on DMABUF_HEAPS + help + Choose this option to enable the carveout dmabuf heap. The carveout + heap is backed by pages from reserved memory regions flagged as + exportable. If in doubt, say Y. + config DMABUF_HEAPS_SYSTEM bool "DMA-BUF System Heap" depends on DMABUF_HEAPS help Choose this option to enable the system dmabuf heap. The system heap diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile index 974467791032ffb8a7aba17b1407d9a19b3f3b44..b734647ad5c84f449106748160258e372f153df2 100644 --- a/drivers/dma-buf/heaps/Makefile +++ b/drivers/dma-buf/heaps/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_DMABUF_HEAPS_CARVEOUT) += carveout_heap.o obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o diff --git a/drivers/dma-buf/heaps/carveout_heap.c b/drivers/dma-buf/heaps/carveout_heap.c new file mode 100644 index 0000000000000000000000000000000000000000..3fac190545bc6f853c18a614f6d89176ed2d7df6 --- /dev/null +++ b/drivers/dma-buf/heaps/carveout_heap.c @@ -0,0 +1,388 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/dma-buf.h> +#include <linux/dma-heap.h> +#include <linux/genalloc.h> +#include <linux/highmem.h> +#include <linux/of_reserved_mem.h> + +struct carveout_heap_priv { + struct dma_heap *heap; + struct gen_pool *pool; +}; + +struct carveout_heap_buffer_priv { + struct mutex lock; + struct list_head attachments; + + unsigned long num_pages; + struct carveout_heap_priv *heap; + phys_addr_t paddr; + void *vaddr; + unsigned int vmap_cnt; +}; + +struct carveout_heap_attachment { + struct list_head head; + struct sg_table table; + + struct device *dev; + bool mapped; +}; + +static int carveout_heap_attach(struct dma_buf *buf, + struct dma_buf_attachment *attachment) +{ + struct carveout_heap_buffer_priv *priv = buf->priv; + struct carveout_heap_attachment *a; + struct sg_table *sgt; + unsigned long len = priv->num_pages * PAGE_SIZE; + int ret; + + a = kzalloc(sizeof(*a), GFP_KERNEL); + if (!a) + return -ENOMEM; + INIT_LIST_HEAD(&a->head); + a->dev = attachment->dev; + attachment->priv = a; + + sgt = &a->table; + ret = sg_alloc_table(sgt, 1, GFP_KERNEL); + if (ret) + goto err_cleanup_attach; + + sg_set_buf(sgt->sgl, priv->vaddr, len); + + mutex_lock(&priv->lock); + list_add(&a->head, &priv->attachments); + mutex_unlock(&priv->lock); + + return 0; + +err_cleanup_attach: + kfree(a); + return ret; +} + +static void carveout_heap_detach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attachment) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + struct carveout_heap_attachment *a = attachment->priv; + + mutex_lock(&priv->lock); + list_del(&a->head); + mutex_unlock(&priv->lock); + + sg_free_table(&a->table); + kfree(a); +} + +static struct sg_table * +carveout_heap_map_dma_buf(struct dma_buf_attachment *attachment, + enum dma_data_direction direction) +{ + struct carveout_heap_attachment *a = attachment->priv; + struct sg_table *table = &a->table; + int ret; + + ret = dma_map_sgtable(a->dev, table, direction, 0); + if (ret) + return ERR_PTR(ret); + + a->mapped = true; + + return table; +} + +static void carveout_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, + struct sg_table *table, + enum dma_data_direction direction) +{ + struct carveout_heap_attachment *a = attachment->priv; + + a->mapped = false; + dma_unmap_sgtable(a->dev, table, direction, 0); +} + +static int +carveout_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, + enum dma_data_direction direction) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + struct carveout_heap_attachment *a; + unsigned long len = priv->num_pages * PAGE_SIZE; + + mutex_lock(&priv->lock); + + if (priv->vmap_cnt) + invalidate_kernel_vmap_range(priv->vaddr, len); + + list_for_each_entry(a, &priv->attachments, head) { + if (!a->mapped) + continue; + + dma_sync_sgtable_for_cpu(a->dev, &a->table, direction); + } + + mutex_unlock(&priv->lock); + + return 0; +} + +static int +carveout_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, + enum dma_data_direction direction) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + struct carveout_heap_attachment *a; + unsigned long len = priv->num_pages * PAGE_SIZE; + + mutex_lock(&priv->lock); + + if (priv->vmap_cnt) + flush_kernel_vmap_range(priv->vaddr, len); + + list_for_each_entry(a, &priv->attachments, head) { + if (!a->mapped) + continue; + + dma_sync_sgtable_for_device(a->dev, &a->table, direction); + } + + mutex_unlock(&priv->lock); + + return 0; +} + +static int carveout_heap_mmap(struct dma_buf *dmabuf, + struct vm_area_struct *vma) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + unsigned long len = priv->num_pages * PAGE_SIZE; + + return vm_iomap_memory(vma, priv->paddr, len); +} + +static int carveout_heap_vmap(struct dma_buf *dmabuf, struct iosys_map *map) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + unsigned long len = priv->num_pages * PAGE_SIZE; + + mutex_lock(&priv->lock); + + if (!priv->vmap_cnt) { + void *vaddr = memremap(priv->paddr, len, MEMREMAP_WB); + + if (!vaddr) { + mutex_unlock(&priv->lock); + return -ENOMEM; + } + + priv->vaddr = vaddr; + } + + WARN_ON(!priv->vaddr); + iosys_map_set_vaddr(map, priv->vaddr); + priv->vmap_cnt++; + + mutex_unlock(&priv->lock); + + return 0; +} + +static void carveout_heap_vunmap(struct dma_buf *dmabuf, struct iosys_map *map) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + + mutex_lock(&priv->lock); + + priv->vmap_cnt--; + if (!priv->vmap_cnt) { + memunmap(priv->vaddr); + priv->vaddr = NULL; + } + + mutex_unlock(&priv->lock); + + iosys_map_clear(map); +} + +static void carveout_heap_dma_buf_release(struct dma_buf *buf) +{ + struct carveout_heap_buffer_priv *buffer_priv = buf->priv; + struct carveout_heap_priv *heap_priv = buffer_priv->heap; + unsigned long len = buffer_priv->num_pages * PAGE_SIZE; + + gen_pool_free(heap_priv->pool, buffer_priv->paddr, len); + kfree(buffer_priv); +} + +static const struct dma_buf_ops carveout_heap_buf_ops = { + .attach = carveout_heap_attach, + .detach = carveout_heap_detach, + .map_dma_buf = carveout_heap_map_dma_buf, + .unmap_dma_buf = carveout_heap_unmap_dma_buf, + .begin_cpu_access = carveout_heap_dma_buf_begin_cpu_access, + .end_cpu_access = carveout_heap_dma_buf_end_cpu_access, + .mmap = carveout_heap_mmap, + .vmap = carveout_heap_vmap, + .vunmap = carveout_heap_vunmap, + .release = carveout_heap_dma_buf_release, +}; + +static struct dma_buf *carveout_heap_allocate(struct dma_heap *heap, + unsigned long len, + u32 fd_flags, + u64 heap_flags) +{ + struct carveout_heap_priv *heap_priv = dma_heap_get_drvdata(heap); + struct carveout_heap_buffer_priv *buffer_priv; + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); + struct dma_buf *buf; + phys_addr_t paddr; + /* len is guaranteed to be page-aligned by the framework, so we can use it as is. */ + size_t size = len; + int ret; + + buffer_priv = kzalloc(sizeof(*buffer_priv), GFP_KERNEL); + if (!buffer_priv) + return ERR_PTR(-ENOMEM); + + INIT_LIST_HEAD(&buffer_priv->attachments); + mutex_init(&buffer_priv->lock); + + paddr = gen_pool_alloc(heap_priv->pool, size); + if (!paddr) { + ret = -ENOMEM; + goto err_free_buffer_priv; + } + + buffer_priv->paddr = paddr; + buffer_priv->heap = heap_priv; + buffer_priv->num_pages = size >> PAGE_SHIFT; + + /* create the dmabuf */ + exp_info.exp_name = dma_heap_get_name(heap); + exp_info.ops = &carveout_heap_buf_ops; + exp_info.size = size; + exp_info.flags = fd_flags; + exp_info.priv = buffer_priv; + + buf = dma_buf_export(&exp_info); + if (IS_ERR(buf)) { + ret = PTR_ERR(buf); + goto err_free_buffer; + } + + return buf; + +err_free_buffer: + gen_pool_free(heap_priv->pool, paddr, len); +err_free_buffer_priv: + kfree(buffer_priv); + + return ERR_PTR(ret); +} + +static const struct dma_heap_ops carveout_heap_ops = { + .allocate = carveout_heap_allocate, +}; + +static int __init carveout_heap_setup(struct device_node *node) +{ + struct dma_heap_export_info exp_info = {}; + const struct reserved_mem *rmem; + struct carveout_heap_priv *priv; + struct dma_heap *heap; + struct gen_pool *pool; + int ret; + + rmem = of_reserved_mem_lookup(node); + if (!rmem) + return -EINVAL; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + pool = gen_pool_create(PAGE_SHIFT, NUMA_NO_NODE); + if (!pool) { + ret = -ENOMEM; + goto err_cleanup_heap; + } + priv->pool = pool; + + ret = gen_pool_add(pool, rmem->base, rmem->size, NUMA_NO_NODE); + if (ret) + goto err_release_mem_region; + + exp_info.name = node->full_name; + exp_info.ops = &carveout_heap_ops; + exp_info.priv = priv; + + heap = dma_heap_add(&exp_info); + if (IS_ERR(heap)) { + ret = PTR_ERR(heap); + goto err_release_mem_region; + } + priv->heap = heap; + + return 0; + +err_release_mem_region: + gen_pool_destroy(pool); +err_cleanup_heap: + kfree(priv); + return ret; +} + +static int __init carveout_heap_init(void) +{ + struct device_node *rmem_node; + struct device_node *node; + int ret; + + rmem_node = of_find_node_by_path("/reserved-memory"); + if (!rmem_node) + return 0; + + for_each_child_of_node(rmem_node, node) { + /* + * TODO: shared-dma-pools register either a CMA or + * coherent memory region depending on whether the + * reusable property is set. + * + * In order to avoid issues, we would need to use the + * respective allocators if the shared-dma-pool property + * was set. + */ + if (of_device_is_compatible(node, "shared-dma-pool")) { + pr_warn("%pOFn: carveout heap driver doesn't support shared-dma-pools.", + node); + continue; + } + + /* + * TODO: restricted-dma-pools register in swiotlb. In + * order to avoid issues, we would need to use the + * swiotlb if that compatible was set. + */ + if (of_device_is_compatible(node, "restricted-dma-pool")) { + pr_warn("%pOFn: carveout heap driver doesn't support restricted-dma-pools.", + node); + continue; + } + + if (!of_property_read_bool(node, "export")) + continue; + + ret = carveout_heap_setup(node); + if (ret) + return ret; + } + + return 0; +} + +module_init(carveout_heap_init); --- base-commit: a17f109942554a751be2c3b05f898f1b3ae98c78 change-id: 20240515-dma-buf-ecc-heap-28a311d2c94e Best regards, -- Maxime Ripard <mripard(a)kernel.org>

9 months

1
0
0 0

Re: [PATCH v5 08/10] accel/rocket: Add IOCTLs for synchronizing memory accesses

by Lucas Stach

Hi Tomeu, Am Dienstag, dem 20.05.2025 um 12:27 +0200 schrieb Tomeu Vizoso: > The NPU cores have their own access to the memory bus, and this isn't > cache coherent with the CPUs. > > Add IOCTLs so userspace can mark when the caches need to be flushed, and > also when a writer job needs to be waited for before the buffer can be > accessed from the CPU. > > Initially based on the same IOCTLs from the Etnaviv driver. > > v2: > - Don't break UABI by reordering the IOCTL IDs (Jeff Hugo) > > v3: > - Check that padding fields in IOCTLs are zero (Jeff Hugo) > > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > --- > drivers/accel/rocket/rocket_drv.c | 2 + > drivers/accel/rocket/rocket_gem.c | 80 +++++++++++++++++++++++++++++++++++++++ > drivers/accel/rocket/rocket_gem.h | 5 +++ > include/uapi/drm/rocket_accel.h | 37 ++++++++++++++++++ > 4 files changed, 124 insertions(+) > > diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c > index fef9b93372d3f65c41c1ac35a9bfa0c01ee721a5..c06e66939e6c39909fe08bef3c4f301b07bf8fbf 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c > @@ -59,6 +59,8 @@ static const struct drm_ioctl_desc rocket_drm_driver_ioctls[] = { > > ROCKET_IOCTL(CREATE_BO, create_bo), > ROCKET_IOCTL(SUBMIT, submit), > + ROCKET_IOCTL(PREP_BO, prep_bo), > + ROCKET_IOCTL(FINI_BO, fini_bo), > }; > > DEFINE_DRM_ACCEL_FOPS(rocket_accel_driver_fops); > diff --git a/drivers/accel/rocket/rocket_gem.c b/drivers/accel/rocket/rocket_gem.c > index 8a8a7185daac4740081293aae6945c9b2bbeb2dd..cdc5238a93fa5978129dc1ac8ec8de955160dc18 100644 > --- a/drivers/accel/rocket/rocket_gem.c > +++ b/drivers/accel/rocket/rocket_gem.c > @@ -129,3 +129,83 @@ int rocket_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file * > > return ret; > } > + > +static inline enum dma_data_direction rocket_op_to_dma_dir(u32 op) > +{ > + if (op & ROCKET_PREP_READ) > + return DMA_FROM_DEVICE; > + else if (op & ROCKET_PREP_WRITE) > + return DMA_TO_DEVICE; > + else > + return DMA_BIDIRECTIONAL; > +} This has copied over the bug fixed in etnaviv commit 58979ad6330a ("drm/etnaviv: fix DMA direction handling for cached RW buffers") Regards, Lucas

9 months

1
0
0 0

Re: [PATCH v4 05/10] accel/rocket: Add a new driver for Rockchip's NPU

by Krzysztof Kozlowski

On Mon, May 19, 2025 at 03:43:37PM GMT, Tomeu Vizoso wrote: > +#endif > diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c > new file mode 100644 > index 0000000000000000000000000000000000000000..bb469ac87d36249157f4ba9d9f7106ad558309e4 > --- /dev/null > +++ b/drivers/accel/rocket/rocket_device.c > @@ -0,0 +1,39 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright 2024-2025 Tomeu Vizoso <tomeu(a)tomeuvizoso.net> */ > + > +#include <linux/clk.h> > +#include <linux/dev_printk.h> > + > +#include "rocket_device.h" > + > +int rocket_device_init(struct rocket_device *rdev) > +{ > + struct device *dev = rdev->cores[0].dev; > + int err; > + > + rdev->clk_npu = devm_clk_get(dev, "npu"); > + if (IS_ERR(rdev->clk_npu)) { > + err = PTR_ERR(rdev->clk_npu); > + dev_err(dev, "devm_clk_get failed %d for clock npu\n", err); > + return err; > + } That's probe path? so use standard syntax: return dev_err_probe(). One line instead of four. > + > + rdev->pclk = devm_clk_get(dev, "pclk"); > + if (IS_ERR(rdev->pclk)) { > + err = PTR_ERR(rdev->pclk); > + dev_err(dev, "devm_clk_get failed %d for clock pclk\n", err); > + return err; Same here... except that this should be blk API and entire function gets smaller. > + } > + > + /* Initialize core 0 (top) */ > + err = rocket_core_init(&rdev->cores[0]); > + if (err) > + return err; > + > + return 0; > +} ... > +static int rocket_device_runtime_resume(struct device *dev) > +{ > + struct rocket_device *rdev = dev_get_drvdata(dev); > + int core = find_core_for_dev(dev); > + int err = 0; > + > + if (core < 0) > + return -ENODEV; > + > + if (core == 0) { > + err = clk_prepare_enable(rdev->clk_npu); > + if (err) { > + dev_err(dev, "clk_prepare_enable failed %d for clock npu\n", err); > + return err; > + } > + > + err = clk_prepare_enable(rdev->pclk); > + if (err) { > + dev_err(dev, "clk_prepare_enable failed %d for clock pclk\n", err); > + goto error_clk_npu; > + } > + } > + > + err = clk_prepare_enable(rdev->cores[core].a_clk); > + if (err) { > + dev_err(dev, "clk_prepare_enable failed %d for a_clk in core %d\n", err, core); > + goto error_pclk; > + } > + > + err = clk_prepare_enable(rdev->cores[core].h_clk); > + if (err) { > + dev_err(dev, "clk_prepare_enable failed %d for h_clk in core %d\n", err, core); > + goto error_a_clk; > + } All four above calls could be just one call with bulk API. > + > + return 0; > + > +error_a_clk: > + clk_disable_unprepare(rdev->cores[core].a_clk); > + > +error_pclk: > + if (core == 0) > + clk_disable_unprepare(rdev->pclk); > + > +error_clk_npu: > + if (core == 0) > + clk_disable_unprepare(rdev->clk_npu); And all this would be gone... > + > + return err; Best regards, Krzysztof

9 months

1
0
0 0

Re: [PATCH v4 01/10] dt-bindings: npu: rockchip,rknn: Add bindings

by Krzysztof Kozlowski

On Mon, May 19, 2025 at 03:43:33PM GMT, Tomeu Vizoso wrote: > Add the bindings for the Neural Processing Unit IP from Rockchip. > > v2: > - Adapt to new node structure (one node per core, each with its own > IOMMU) > - Several misc. fixes from Sebastian Reichel > > v3: > - Split register block in its constituent subblocks, and only require > the ones that the kernel would ever use (Nicolas Frattaroli) > - Group supplies (Rob Herring) > - Explain the way in which the top core is special (Rob Herring) > > v4: > - Change required node name to npu@ (Rob Herring and Krzysztof Kozlowski) > - Remove unneeded items: (Krzysztof Kozlowski) > - Fix use of minItems/maxItems (Krzysztof Kozlowski) > - Add reg-names to list of required properties (Krzysztof Kozlowski) > - Fix example (Krzysztof Kozlowski) > > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > Signed-off-by: Sebastian Reichel <sebastian.reichel(a)collabora.com> This order of SoB is still odd. You as person sending it should be the last signing person. Are you sure you are using b4 for managing trailers? I would expect it to re-order these on every update and this is already v4. > --- > .../bindings/npu/rockchip,rknn-core.yaml | 149 +++++++++++++++++++++ Filename matching compatible, so rockchip,rk3588-rknn-core.yaml > 1 file changed, 149 insertions(+) > > diff --git a/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml b/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..fafd0b01da215c7396262012988e364ef07ea137 > --- /dev/null > +++ b/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml > @@ -0,0 +1,149 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/npu/rockchip,rknn-core.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Neural Processing Unit IP from Rockchip > + > +maintainers: > + - Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > + > +description: > + Rockchip IP for accelerating inference of neural networks, based on NVIDIA's > + open source NVDLA IP. > + > + There is to be a node per each core in the NPU. In Rockchip's design there > + will be one core that is special and needs to be powered on before any of the > + other cores can be used. This special core is called the top core and should > + have the compatible string that corresponds to top cores. > + > +properties: > + $nodename: > + pattern: '^npu@[a-f0-9]+$' > + > + compatible: > + oneOf: Drop... if you followed my advice you would notice it is not necessary. > + - enum: > + - rockchip,rk3588-rknn-core-top > + - enum: > + - rockchip,rk3588-rknn-core My comments were only partially implemented. This syntax is really not readable and not necessary and I asked to make it part of previous enum. This is just one enum: compatible: enum: - foo - bar Best regards, Krzysztof

9 months

1
0
0 0

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig