Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

8 participants
2915 discussions

Patch "drm/gem: Internally test import_attach for imported objects" has been added to the 6.6-stable tree

by gregkh＠linuxfoundation.org

This is a note to let you know that I've just added the patch titled drm/gem: Internally test import_attach for imported objects to the 6.6-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… The filename of the patch is: drm-gem-internally-test-import_attach-for-imported-objects.patch and it can be found in the queue-6.6 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let <stable(a)vger.kernel.org> know about it. From 8260731ccad0451207b45844bb66eb161a209218 Mon Sep 17 00:00:00 2001 From: Thomas Zimmermann <tzimmermann(a)suse.de> Date: Wed, 16 Apr 2025 08:57:45 +0200 Subject: drm/gem: Internally test import_attach for imported objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From: Thomas Zimmermann <tzimmermann(a)suse.de> commit 8260731ccad0451207b45844bb66eb161a209218 upstream. Test struct drm_gem_object.import_attach to detect imported objects. During object clenanup, the dma_buf field might be NULL. Testing it in an object's free callback then incorrectly does a cleanup as for native objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that clears the dma_buf field in drm_gem_object_exported_dma_buf_free(). v3: - only test for import_attach (Boris) v2: - use import_attach.dmabuf instead of dma_buf (Christian) Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper") Reported-by: Andy Yan <andyshrk(a)163.com> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andysh… Tested-by: Andy Yan <andyshrk(a)163.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: David Airlie <airlied(a)gmail.com> Cc: Simona Vetter <simona(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Reviewed-by: Simona Vetter <simona.vetter(a)ffwll.ch> Link: https://lore.kernel.org/r/20250416065820.26076-1-tzimmermann@suse.de Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- include/drm/drm_gem.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -567,8 +567,7 @@ static inline bool drm_gem_object_is_sha */ static inline bool drm_gem_is_imported(const struct drm_gem_object *obj) { - /* The dma-buf's priv field points to the original GEM object. */ - return obj->dma_buf && (obj->dma_buf->priv != obj); + return !!obj->import_attach; } #ifdef CONFIG_LOCKDEP Patches currently in stable-queue which might be from tzimmermann(a)suse.de are queue-6.6/drm-gem-internally-test-import_attach-for-imported-objects.patch queue-6.6/drm-ast-find-vbios-mode-from-regular-display-size.patch queue-6.6/drm-gem-test-for-imported-gem-buffers-with-helper.patch queue-6.6/drm-atomic-clarify-the-rules-around-drm_atomic_state.patch

1 month, 1 week

Patch "drm/gem: Internally test import_attach for imported objects" has been added to the 6.12-stable tree

by gregkh＠linuxfoundation.org

This is a note to let you know that I've just added the patch titled drm/gem: Internally test import_attach for imported objects to the 6.12-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… The filename of the patch is: drm-gem-internally-test-import_attach-for-imported-objects.patch and it can be found in the queue-6.12 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let <stable(a)vger.kernel.org> know about it. From 8260731ccad0451207b45844bb66eb161a209218 Mon Sep 17 00:00:00 2001 From: Thomas Zimmermann <tzimmermann(a)suse.de> Date: Wed, 16 Apr 2025 08:57:45 +0200 Subject: drm/gem: Internally test import_attach for imported objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From: Thomas Zimmermann <tzimmermann(a)suse.de> commit 8260731ccad0451207b45844bb66eb161a209218 upstream. Test struct drm_gem_object.import_attach to detect imported objects. During object clenanup, the dma_buf field might be NULL. Testing it in an object's free callback then incorrectly does a cleanup as for native objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that clears the dma_buf field in drm_gem_object_exported_dma_buf_free(). v3: - only test for import_attach (Boris) v2: - use import_attach.dmabuf instead of dma_buf (Christian) Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper") Reported-by: Andy Yan <andyshrk(a)163.com> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andysh… Tested-by: Andy Yan <andyshrk(a)163.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: David Airlie <airlied(a)gmail.com> Cc: Simona Vetter <simona(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Reviewed-by: Simona Vetter <simona.vetter(a)ffwll.ch> Link: https://lore.kernel.org/r/20250416065820.26076-1-tzimmermann@suse.de Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- include/drm/drm_gem.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -580,8 +580,7 @@ static inline bool drm_gem_object_is_sha */ static inline bool drm_gem_is_imported(const struct drm_gem_object *obj) { - /* The dma-buf's priv field points to the original GEM object. */ - return obj->dma_buf && (obj->dma_buf->priv != obj); + return !!obj->import_attach; } #ifdef CONFIG_LOCKDEP Patches currently in stable-queue which might be from tzimmermann(a)suse.de are queue-6.12/drm-gem-internally-test-import_attach-for-imported-objects.patch queue-6.12/drm-ast-find-vbios-mode-from-regular-display-size.patch queue-6.12/drm-gem-test-for-imported-gem-buffers-with-helper.patch queue-6.12/drm-atomic-clarify-the-rules-around-drm_atomic_state.patch

1 month, 1 week

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

1 month, 1 week

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

by Jens Wiklander

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

1 month, 1 week

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

by Jens Wiklander

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

1 month, 1 week

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

by Jens Wiklander

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

1 month, 1 week

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

by Jens Wiklander

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

1 month, 1 week

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

by Maxime Ripard

Hi, On Thu, May 22, 2025 at 12:14:18PM -0700, Jared Kangas wrote: > The CMA heap's name in devtmpfs can vary depending on how the heap is > defined. Its name defaults to "reserved", but if a CMA area is defined > in the devicetree, the heap takes on the devicetree node's name, such as > "default-pool" or "linux,cma". To simplify naming, unconditionally name > it "default_cma_region", but keep a legacy node in place backed by the > same underlying allocator for backwards compatibility. > > Signed-off-by: Jared Kangas <jkangas(a)redhat.com> > --- > Documentation/userspace-api/dma-buf-heaps.rst | 7 +++++-- > drivers/dma-buf/heaps/Kconfig | 10 ++++++++++ > drivers/dma-buf/heaps/cma_heap.c | 20 ++++++++++++++++++- > 3 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst > index 23bd0bd7b0654..1dfe5e7acd5a3 100644 > --- a/Documentation/userspace-api/dma-buf-heaps.rst > +++ b/Documentation/userspace-api/dma-buf-heaps.rst > @@ -21,5 +21,8 @@ following heaps: > usually created either through the kernel commandline through the > ``cma`` parameter, a memory region Device-Tree node with the > ``linux,cma-default`` property set, or through the ``CMA_SIZE_MBYTES`` or > - ``CMA_SIZE_PERCENTAGE`` Kconfig options. Depending on the platform, it > - might be called ``reserved``, ``linux,cma``, or ``default-pool``. > + ``CMA_SIZE_PERCENTAGE`` Kconfig options. The heap's name in devtmpfs is > + ``default_cma_region``. For backwards compatibility, when the > + ``DMABUF_HEAPS_CMA_LEGACY`` Kconfig option is set, a duplicate node is > + created following legacy naming conventions; the legacy name might be > + ``reserved``, ``linux,cma``, or ``default-pool``. > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig > index a5eef06c42264..bb369b38b001a 100644 > --- a/drivers/dma-buf/heaps/Kconfig > +++ b/drivers/dma-buf/heaps/Kconfig > @@ -12,3 +12,13 @@ config DMABUF_HEAPS_CMA > Choose this option to enable dma-buf CMA heap. This heap is backed > by the Contiguous Memory Allocator (CMA). If your system has these > regions, you should say Y here. > + > +config DMABUF_HEAPS_CMA_LEGACY > + bool "Legacy DMA-BUF CMA Heap" > + default y > + depends on DMABUF_HEAPS_CMA > + help > + Add a duplicate CMA-backed dma-buf heap with legacy naming derived > + from the CMA area's devicetree node, or "reserved" if the area is not > + defined in the devicetree. This uses the same underlying allocator as > + CONFIG_DMABUF_HEAPS_CMA. > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > index e998d8ccd1dc6..dfeccafc6ae3c 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -9,6 +9,9 @@ > * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/ > * Andrew F. Davis <afd(a)ti.com> > */ > + > +#define pr_fmt(fmt) "cma_heap: " fmt > + > #include <linux/cma.h> > #include <linux/dma-buf.h> > #include <linux/dma-heap.h> > @@ -22,6 +25,7 @@ > #include <linux/slab.h> > #include <linux/vmalloc.h> > > +#define DEFAULT_CMA_NAME "default_cma_region" > > struct cma_heap { > struct dma_heap *heap; > @@ -394,15 +398,29 @@ static int __init __add_cma_heap(struct cma *cma, const char *name) > static int __init add_default_cma_heap(void) > { > struct cma *default_cma = dev_get_cma_area(NULL); > + const char *legacy_cma_name; > int ret; > > if (!default_cma) > return 0; > > - ret = __add_cma_heap(default_cma, cma_get_name(default_cma)); > + ret = __add_cma_heap(default_cma, DEFAULT_CMA_NAME); > if (ret) > return ret; > > + if (IS_ENABLED(CONFIG_DMABUF_HEAPS_CMA_LEGACY)) { > + legacy_cma_name = cma_get_name(default_cma); > + if (!strcmp(legacy_cma_name, DEFAULT_CMA_NAME)) { > + pr_warn("legacy name and default name are the same, skipping legacy heap\n"); > + return 0; > + } > + > + ret = __add_cma_heap(default_cma, legacy_cma_name); > + if (ret) > + pr_warn("failed to add legacy heap: %pe\n", > + ERR_PTR(-ret)); Are you sure about the -ret? ret should already be a negative number if it failed? With that fixed, Reviewed-by: Maxime Ripard <mripard(a)kernel.org> Maxime

1 month, 1 week

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

by Maxime Ripard

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

1 month, 1 week

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

by Maxime Ripard

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

1 month, 1 week

← Newer
1
...
5
6
7
8
9
10
11
...
292
Older →

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig