Linaro-mm-sig June 2024

linaro-mm-sig@lists.linaro.org

26 participants
61 discussions

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > > > > > > > I think it would be better to have the user configurable > > private > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > requirement can get their private information from dma-buf > > directly > > > > > > and > > > > > > no need to change or add the interface. > > > > > > > > What's your opinion in this point? > > > > > > Well of hand I don't see the need for that. > > > > > > What happens if you get a non-secure buffer imported in your > > secure > > > > > device? > > > > > > > > We use the same mediatek-drm driver for secure and non-secure > > buffer. > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > the > > > > normal flow with normal hardware settings. > > > > > > > > We use different configurations to make hardware have different > > > > permission to access the buffer it should access. > > > > > > > > So if we can't get the information of "the buffer is allocated > > from > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > won't > > > > be able to configure the hardware correctly. > > > > > > Why can't you get this information from userspace? > > > > Same reason amd and i915/xe also pass this around internally in the > > kernel, it's just that for those gpus the render and kms node are the > > same > > driver so this is easy. > > The reason I ask is that encryption here looks just like another parameter for the buffer, e.g. like format, stride, tilling etc.. So instead of this during buffer import: mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", 10)); mtk_gem->dma_addr = sg_dma_address(sg->sgl); mtk_gem->size = attach->dmabuf->size; mtk_gem->sg = sg; You can trivially say during use hey this buffer is encrypted. At least that's my 10 mile high view, maybe I'm missing some extensive key exchange or something like that. > > > But on arm you have split designs everywhere and dma-buf > > import/export, so > > something else is needed. And neither current kms uapi nor > > protocols/extensions have provisions for this (afaik) because it > > works on > > the big gpus, and on android it's just hacked up with backchannels. > > > > So yeah essentially I think we probably need something like this, as > > much > > as it sucks. I see it somewhat similar to handling pcip2pdma > > limitations > > in the kernel too. > > > > Not sure where/how it should be handled though, and maybe I've missed > > something around protocols, in which case I guess we should add some > > secure buffer flags to the ADDFB2 ioctl. > > Thanks for your hint, I'll try to add the secure flag to the ADDFB2 > ioctl. If it works, I'll send the patch. Yeah, exactly what I would suggest as well. I'm not an expert for that part, but as far as I know we already have bunch of device specific tilling flags in there. Adding an MTK_ENCRYPTED flag should be trivial. Regards, Christian. > > Regards, > Jason-JH.Lin > > > -Sima > > ************* MEDIATEK Confidentiality Notice ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!

1 day, 11 hours

dma_buf_detach lockdep splat

by Thomas Hellström

Hi! I'm seeing the below lockdep splat 1) with the xe driver in an imported dma-buf object destruction path. It's not because we hold the dma_resv lock at that point, but rather because we hold *another* dma_resv lock at that point, and the dma_resv detach happens when the object is idle, in this case it was idle at the final put(), and dma_buf_detach() is called in the putting process. Holding another dma-buf lock might happen as part of drm_exec_unlock_all, or simply if the wider vm dma_resv was held at object put time, so it's not an uncommon pattern, even if the drm_exec instance can be fixed by putting all bos after unlocking them all. Two solutions coming to mind here: 1) Provide a dma_buf_detach_locked() 2) Have TTM always take the delayed delete path for imported dma-buf objects. I'd prefer 1) since I think the correct place to call this is in the TTM callback delete_mem_notify() where the bo is already locked, and I figure non-TTM gem backends may come to suffer from the same problem. Opinions, suggestions? [1] [ 99.136161] ============================================ [ 99.136162] WARNING: possible recursive locking detected [ 99.136163] 6.10.0-rc2+ #6 Tainted: G U [ 99.136165] -------------------------------------------- [ 99.136166] glxgears:sh0/4675 is trying to acquire lock: [ 99.136167] ffff9967dcdd91a8 (reservation_ww_class_mutex){+.+.}- {3:3}, at: dma_buf_detach+0x3b/0xf0 [ 99.136184] but task is already holding lock: [ 99.136186] ffff9967d8c145a8 (reservation_ww_class_mutex){+.+.}- {3:3}, at: drm_exec_lock_obj+0x49/0x2b0 [drm_exec] [ 99.136191] other info that might help us debug this: [ 99.136192] Possible unsafe locking scenario: [ 99.136194] CPU0 [ 99.136194] ---- [ 99.136195] lock(reservation_ww_class_mutex); [ 99.136197] lock(reservation_ww_class_mutex); [ 99.136199] *** DEADLOCK *** [ 99.136199] May be due to missing lock nesting notation [ 99.136200] 5 locks held by glxgears:sh0/4675: [ 99.136202] #0: ffff9967d8c104c8 (&xef->vm.lock){+.+.}-{3:3}, at: xe_file_close+0xde/0x1c0 [xe] [ 99.136272] #1: ffff9967d5bb7480 (&vm->lock){++++}-{3:3}, at: xe_vm_close_and_put+0x161/0x9b0 [xe] [ 99.136350] #2: ffff9967ef88a970 (&val->lock){.+.+}-{3:3}, at: xe_validation_ctx_init+0x6d/0x70 [xe] [ 99.136440] #3: ffffbd6a085577b8 (reservation_ww_class_acquire){+.+.}-{0:0}, at: xe_vma_destroy_unlocked+0x7f/0xe0 [xe] [ 99.136546] #4: ffff9967d8c145a8 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x49/0x2b0 [drm_exec] [ 99.136552] stack backtrace: [ 99.136553] CPU: 10 PID: 4675 Comm: glxgears:sh0 Tainted: G U 6.10.0-rc2+ #6 [ 99.136555] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 2001 02/01/2023 [ 99.136557] Call Trace: [ 99.136558] <TASK> [ 99.136560] dump_stack_lvl+0x77/0xb0 [ 99.136564] __lock_acquire+0x1232/0x2160 [ 99.136569] lock_acquire+0xcb/0x2d0 [ 99.136570] ? dma_buf_detach+0x3b/0xf0 [ 99.136574] ? __lock_acquire+0x417/0x2160 [ 99.136577] __ww_mutex_lock.constprop.0+0xd0/0x13b0 [ 99.136580] ? dma_buf_detach+0x3b/0xf0 [ 99.136584] ? dma_buf_detach+0x3b/0xf0 [ 99.136588] ? ww_mutex_lock+0x2b/0x90 [ 99.136590] ww_mutex_lock+0x2b/0x90 [ 99.136592] dma_buf_detach+0x3b/0xf0 [ 99.136595] drm_prime_gem_destroy+0x2f/0x40 [drm] [ 99.136638] xe_ttm_bo_destroy+0x32/0x220 [xe] [ 99.136734] ? __mutex_unlock_slowpath+0x3a/0x290 [ 99.136738] drm_exec_unlock_all+0xa1/0xd0 [drm_exec] [ 99.136741] drm_exec_fini+0x12/0xb0 [drm_exec] [ 99.136743] xe_validation_ctx_fini+0x15/0x40 [xe] [ 99.136848] xe_vma_destroy_unlocked+0xb1/0xe0 [xe] [ 99.136954] xe_vm_close_and_put+0x41a/0x9b0 [xe] [ 99.137056] ? xa_find+0xe3/0x1e0 [ 99.137060] xe_file_close+0x10a/0x1c0 [xe] [ 99.137157] drm_file_free+0x22a/0x280 [drm] [ 99.137193] drm_release_noglobal+0x22/0x70 [drm] [ 99.137227] __fput+0xf1/0x2d0 [ 99.137231] task_work_run+0x59/0x90 [ 99.137235] do_exit+0x330/0xb40 [ 99.137238] do_group_exit+0x36/0xa0 [ 99.137241] get_signal+0xbd2/0xbe0 [ 99.137245] arch_do_signal_or_restart+0x3e/0x240 [ 99.137249] syscall_exit_to_user_mode+0x1e7/0x290 [ 99.137252] do_syscall_64+0xa1/0x180 [ 99.137255] ? _raw_spin_unlock+0x23/0x40 [ 99.137257] ? look_up_lock_class+0x6f/0x120 [ 99.137261] ? __lock_acquire+0x417/0x2160 [ 99.137264] ? lock_acquire+0xcb/0x2d0 [ 99.137266] ? __set_task_comm+0x28/0x1e0 [ 99.137268] ? find_held_lock+0x2b/0x80 [ 99.137271] ? __set_task_comm+0xe1/0x1e0 [ 99.137273] ? lock_release+0xca/0x290 [ 99.137277] ? __do_sys_prctl+0x245/0xab0 [ 99.137279] ? lockdep_hardirqs_on_prepare+0xde/0x190 [ 99.137281] ? syscall_exit_to_user_mode+0xb0/0x290 [ 99.137284] ? do_syscall_64+0xa1/0x180 [ 99.137286] ? cpuset_cpus_allowed+0x36/0x140 [ 99.137289] ? find_held_lock+0x2b/0x80 [ 99.137291] ? find_held_lock+0x2b/0x80 [ 99.137294] ? __sched_setaffinity+0x78/0x240 [ 99.137297] ? kfree+0xe2/0x310 [ 99.137301] ? kfree+0x202/0x310 [ 99.137303] ? __sched_setaffinity+0x78/0x240 [ 99.137305] ? __x64_sys_sched_setaffinity+0x69/0xb0 [ 99.137307] ? kfree+0xe2/0x310 [ 99.137310] ? lockdep_hardirqs_on_prepare+0xde/0x190 [ 99.137312] ? syscall_exit_to_user_mode+0xb0/0x290 [ 99.137315] ? do_syscall_64+0xa1/0x180 [ 99.137317] ? trace_hardirqs_off+0x4b/0xc0 [ 99.137321] ? clear_bhb_loop+0x45/0xa0 [ 99.137325] ? clear_bhb_loop+0x45/0xa0 [ 99.137327] ? clear_bhb_loop+0x45/0xa0 [ 99.137330] entry_SYSCALL_64_after_hwframe+0x76/0x7e [ 99.137333] RIP: 0033:0x7fda70ee6169 [ 99.137351] Code: Unable to access opcode bytes at 0x7fda70ee613f. [ 99.137352] RSP: 002b:00007fda5fdffc80 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca [ 99.137354] RAX: fffffffffffffe00 RBX: 0000000000000000 RCX: 00007fda70ee6169 [ 99.137356] RDX: 0000000000000000 RSI: 0000000000000189 RDI: 0000564a96f45b30 [ 99.137358] RBP: 00007fda5fdffcb0 R08: 0000000000000000 R09: 00000000ffffffff [ 99.137359] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 [ 99.137360] R13: 0000000000000000 R14: 0000000000000000 R15: 0000564a96f45b30 [ 99.137365] </TASK>

1 day, 14 hours

[PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags

by Maxime Ripard

Hi, This series is the follow-up of the discussion that John and I had a few months 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 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. We could also easily extend this mechanism to support more flags, or through a new ioctl to discover which flags a given heap supports. 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 Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Maxime Ripard (8): dma-buf: heaps: Introduce a new heap for reserved memory of: Add helper to retrieve ECC memory bits dma-buf: heaps: Import uAPI header dma-buf: heaps: Add ECC protection flags dma-buf: heaps: system: Remove global variable dma-buf: heaps: system: Handle ECC flags dma-buf: heaps: cma: Handle ECC flags dma-buf: heaps: carveout: Handle ECC flags drivers/dma-buf/dma-heap.c | 4 + drivers/dma-buf/heaps/Kconfig | 8 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/carveout_heap.c | 330 ++++++++++++++++++++++++++++++++++ drivers/dma-buf/heaps/cma_heap.c | 10 ++ drivers/dma-buf/heaps/system_heap.c | 29 ++- include/linux/dma-heap.h | 2 + include/linux/of.h | 25 +++ include/uapi/linux/dma-heap.h | 5 +- 9 files changed, 407 insertions(+), 7 deletions(-) --- base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6 change-id: 20240515-dma-buf-ecc-heap-28a311d2c94e Best regards, -- Maxime Ripard <mripard(a)kernel.org>

1 day, 17 hours

[PATCH v5 0/9] dma-buf: heaps: Add restricted heap

by Yong Wu

The purpose of this patchset is for MediaTek secure video playback, and also to enable other potential uses of this in the future. The 'restricted dma-heap' will be used to allocate dma_buf objects that reference memory in the secure world that is inaccessible/unmappable by the non-secure (i.e. kernel/userspace) world. That memory will be used by the secure/ trusted world to store secure information (i.e. decrypted media content). The dma_bufs allocated from the kernel will be passed to V4L2 for video decoding (as input and output). They will also be used by the drm system for rendering of the content. This patchset adds two MediaTek restricted heaps and they will be used in v4l2[1] and drm[2]. 1) restricted_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path). The buffer is reserved for the secure world after bootup and it is used for vcodec's ES/working buffer; 2) restricted_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is dynamically reserved for the secure world and will be got when we start playing secure videos. Once the security video playing is complete, the CMA will be released. This heap is used for the vcodec's frame buffer. [1] https://lore.kernel.org/linux-mediatek/20240412090851.24999-1-yunfei.dong@m… [2] https://lore.kernel.org/linux-mediatek/20240403102701.369-1-shawn.sung@medi… Change note: v5: 1) Reconstruct TEE commands to allow the kernel to obtain the PA of the TEE buffer to initialize a valid sg table. 2) Previously, PA was hidden from the kernel. Then the kernel checks if this is restricted buffer by "if (sg_page(sg) == NULL)". In this version, we will add a new explicit interface (sg_dma_is_restricted) for users to determine whether this is a restricted buffer. 3) some words improve, like using "rheap". Rebase on v6.9-rc7. v4: https://lore.kernel.org/linux-mediatek/20240112092014.23999-1-yong.wu@media… 1) Rename the heap name from "secure" to "restricted". suggested from Simon/Pekka. There are still several "secure" string in MTK file since we use ARM platform in which we call this "secure world"/ "secure command". v3: https://lore.kernel.org/linux-mediatek/20231212024607.3681-1-yong.wu@mediat… 1) Separate the secure heap to a common file(secure_heap.c) and mtk special file (secure_heap_mtk.c), and put all the tee related code into our special file. 2) About dt-binding, Add "mediatek," prefix since this is Mediatek TEE firmware definition. 3) Remove the normal CMA heap which is a draft for qcom. Rebase on v6.7-rc1. v2: https://lore.kernel.org/linux-mediatek/20231111111559.8218-1-yong.wu@mediat… 1) Move John's patches into the vcodec patchset since they use the new dma heap interface directly. https://lore.kernel.org/linux-mediatek/20231106120423.23364-1-yunfei.dong@m… 2) Reword the dt-binding description. 3) Rename the heap name from mtk_svp to secure_mtk_cm. This means the current vcodec/DRM upstream code doesn't match this. 4) Add a normal CMA heap. currently it should be a draft version. 5) Regarding the UUID, I still use hard code, but put it in a private data which allow the others could set their own UUID. What's more, UUID is necessary for the session with TEE. If we don't have it, we can't communicate with the TEE, including the get_uuid interface, which tries to make uuid more generic, not working. If there is other way to make UUID more general, please free to tell me. v1: https://lore.kernel.org/linux-mediatek/20230911023038.30649-1-yong.wu@media… Base on v6.6-rc1. Yong Wu (9): dt-bindings: reserved-memory: Add mediatek,dynamic-restricted-region scatterlist: Add a flag for the restricted memory lib/scatterlist: Add sg_dup_table dma-buf: heaps: Initialize a restricted heap dma-buf: heaps: restricted_heap: Add private heap ops dma-buf: heaps: restricted_heap: Add dma_ops dma-buf: heaps: restricted_heap: Add MediaTek restricted heap and heap_init dma-buf: heaps: restricted_heap_mtk: Add TEE memory service call dma_buf: heaps: restricted_heap_mtk: Add a new CMA heap .../mediatek,dynamic-restricted-region.yaml | 43 ++ drivers/dma-buf/heaps/Kconfig | 16 + drivers/dma-buf/heaps/Makefile | 4 +- drivers/dma-buf/heaps/restricted_heap.c | 219 +++++++++ drivers/dma-buf/heaps/restricted_heap.h | 45 ++ drivers/dma-buf/heaps/restricted_heap_mtk.c | 423 ++++++++++++++++++ drivers/dma-buf/heaps/system_heap.c | 27 +- include/linux/scatterlist.h | 36 ++ lib/scatterlist.c | 26 ++ 9 files changed, 812 insertions(+), 27 deletions(-) create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-restricted-region.yaml create mode 100644 drivers/dma-buf/heaps/restricted_heap.c create mode 100644 drivers/dma-buf/heaps/restricted_heap.h create mode 100644 drivers/dma-buf/heaps/restricted_heap_mtk.c -- 2.18.0

1 day, 19 hours

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 27.06.24 um 05:17 schrieb Jason-JH Lin (林睿祥): > > On Wed, 2024-06-26 at 12:49 +0200, Christian König wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > In the step 3), we need to verify the dma-buf is allocated from > > > > > "restricted_mtk_cma", but there is no way to pass the > > > secure flag > > > > > or > > > > > private data from userspace to the import interface in DRM > > > driver. > > > > > > > > Why do you need to verify that? > > > > > > I need to know the imported buffer is allocated from restricted cma > > > and > > > mark it as a secure buffer in mediatek-drm driver. Then, I will add > > > some configuration to the hardware if the buffer is secure buffer, > > > so > > > that it can get the permission to access the secure buffer. > > > > Yeah so far that makes sense. This is basically what other drivers do > > with secure buffers as well. > > > > But why do you want the kernel to transport that information? Usually > > drivers get the information from userspace what to do with a buffer. > > > > In other words the format, stride, tilling and also if it's a secure > > buffer or not comes from userspace. > > > > Thanks for your clear explanation. > I think this is what I want, but I can't find any DRM interface to pass > the secure flag from userspace. Well stuff like that is usually device driver specific. So you should probably use something device specific which tells the driver that this buffer is encrypted. > > What the hardware usually handles internally is things like > > encryption keys, but you eventually get the information where to look > > for the key from userspace as well. > > > > Handling inside the kernel would only be necessary if userspace could > > for example crash the system with invalid parameters. But for > > encryption that is usually not the case. > > > > Yes, that's true. > > > > > > > > > > So I can only verify it like this now: > > > > > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct > > > > > drm_device > > > > > *dev, struct dma_buf_attachment *attach, struct sg_table *sg) > > > > > { > > > > > struct mtk_gem_obj *mtk_gem; > > > > > > > > > > /* check if the entries in the sg_table are contiguous */ > > > > > if (drm_prime_get_contiguous_size(sg) < > > > attach->dmabuf->size) { > > > > > DRM_ERROR("sg_table is not contiguous"); > > > > > return ERR_PTR(-EINVAL); > > > > > } > > > > > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > > > > > if (IS_ERR(mtk_gem)) > > > > > return ERR_CAST(mtk_gem); > > > > > > > > > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, > > > > > "restricted", > > > > > 10)); > > > > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > > > > mtk_gem->size = attach->dmabuf->size; > > > > > mtk_gem->sg = sg; > > > > > > > > > > return &mtk_gem->base; > > > > > } > > > > > > > > Complete NAK from my side to that approach. Importing of a DMA- > > > buf > > > > should be independent of the exporter. > > > > > > > > What you could do is to provide the secure buffer from a device > > > and > > > > not a device heap. > > > > > > > > > > You mean I should allocate buffer in mediate-drm driver not > > > userspace? > > > > Well that depends. The question is if you have multiple drivers which > > needs to work with this secure buffer? > > > > If yes then you should have a general allocation heap for it. If no > > then the buffers could as well be allocated from the driver interface > > directly. > > > > Yes, this buffer needs work with GPU and DRM drivers, so this general > "restricted_mtk_cma" will allocated in userspace, then being passed to > GPU and DRM. Well do you really need a separate heap for that? In other words is only a certain part of the system memory capable of being encrypted? Or would the "normal" CMA heap do as well and you only need to setup your hardware properly for encryption? Additional to that in most other drivers buffer sharing and encryption are two separate things. In other words other drivers do something like this: 1. Allocate the buffer. 2. Import the buffer using DRM_IOCTL_PRIME_FD_TO_HANDLE. 3. Set additional buffer properties, e.g. format, stride, tilling, if it's secure, which encryption key to use, where to map etc... So as far as I can see the problem you are facing is that you try to mangle everything into DRM_IOCTL_PRIME_FD_TO_HANDLE. Why not make that a separate IOCTL? I mean we intentionally don't provide things like format, stride, tilling etc.. to DRM_IOCTL_PRIME_FD_TO_HANDLE. Encryption is just another of those parameters. Regards, Christian. > > > > I just have modified this to userspace by the comment here: > > > > > > > https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.36… > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > > > > > > > > > I think it would be better to have the user configurable > > > private > > > > > information in dma-buf, so all the drivers who have the same > > > > > requirement can get their private information from dma-buf > > > directly > > > > > and > > > > > no need to change or add the interface. > > > > > > > > > > What's your opinion in this point? > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > What happens if you get a non-secure buffer imported in your > > > secure > > > > device? > > > > > > We use the same mediatek-drm driver for secure and non-secure > > > buffer. > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > the > > > normal flow with normal hardware settings. > > > > > > We use different configurations to make hardware have different > > > permission to access the buffer it should access. > > > > > > So if we can't get the information of "the buffer is allocated from > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > won't > > > be able to configure the hardware correctly. > > > > Why can't you get this information from userspace? > > As I mentioned here: > > https://patchwork.kernel.org/project/linux-mediatek/cover/20240525232928.55… > > I tried some DRM interfaces using buffer FD and arg->flag as > parameters, but it didn't work. So I ask for your help here. > > But I think I should find DRM maintainer to add the secure flag to DRM > interface now. > > Regards, > Jason-JH.Lin > > ************* MEDIATEK Confidentiality Notice ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!

3 days, 1 hour

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > In the step 3), we need to verify the dma-buf is allocated from > > > "restricted_mtk_cma", but there is no way to pass the secure flag > > > or > > > private data from userspace to the import interface in DRM driver. > > > > Why do you need to verify that? > > I need to know the imported buffer is allocated from restricted cma and > mark it as a secure buffer in mediatek-drm driver. Then, I will add > some configuration to the hardware if the buffer is secure buffer, so > that it can get the permission to access the secure buffer. Yeah so far that makes sense. This is basically what other drivers do with secure buffers as well. But why do you want the kernel to transport that information? Usually drivers get the information from userspace what to do with a buffer. In other words the format, stride, tilling and also if it's a secure buffer or not comes from userspace. What the hardware usually handles internally is things like encryption keys, but you eventually get the information where to look for the key from userspace as well. Handling inside the kernel would only be necessary if userspace could for example crash the system with invalid parameters. But for encryption that is usually not the case. > > > > > So I can only verify it like this now: > > > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct > > > drm_device > > > *dev, struct dma_buf_attachment *attach, struct sg_table *sg) > > > { > > > struct mtk_gem_obj *mtk_gem; > > > > > > /* check if the entries in the sg_table are contiguous */ > > > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { > > > DRM_ERROR("sg_table is not contiguous"); > > > return ERR_PTR(-EINVAL); > > > } > > > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > > > if (IS_ERR(mtk_gem)) > > > return ERR_CAST(mtk_gem); > > > > > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, > > > "restricted", > > > 10)); > > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > > mtk_gem->size = attach->dmabuf->size; > > > mtk_gem->sg = sg; > > > > > > return &mtk_gem->base; > > > } > > > > Complete NAK from my side to that approach. Importing of a DMA-buf > > should be independent of the exporter. > > > > What you could do is to provide the secure buffer from a device and > > not a device heap. > > > > You mean I should allocate buffer in mediate-drm driver not userspace? Well that depends. The question is if you have multiple drivers which needs to work with this secure buffer? If yes then you should have a general allocation heap for it. If no then the buffers could as well be allocated from the driver interface directly. > I just have modified this to userspace by the comment here: > > https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.36… > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > > > > > I think it would be better to have the user configurable private > > > information in dma-buf, so all the drivers who have the same > > > requirement can get their private information from dma-buf directly > > > and > > > no need to change or add the interface. > > > > > > What's your opinion in this point? > > > > Well of hand I don't see the need for that. > > > > What happens if you get a non-secure buffer imported in your secure > > device? > > We use the same mediatek-drm driver for secure and non-secure buffer. > If non-secure buffer imported to mediatek-drm driver, it's go to the > normal flow with normal hardware settings. > > We use different configurations to make hardware have different > permission to access the buffer it should access. > > So if we can't get the information of "the buffer is allocated from > restricted_mtk_cma" when importing the buffer into the driver, we won't > be able to configure the hardware correctly. Why can't you get this information from userspace? Regards, Christian. > > Regards, > Jason-JH.Lin > > > > > Regards, > > Christian. > > > > > Regards, > > > Jason-JH.Lin > > > > > > > Regards, > > > > Christian. > > > > > > ************* MEDIATEK Confidentiality Notice > > > ******************** > > > The information contained in this e-mail message (including any > > > attachments) may be confidential, proprietary, privileged, or > > > otherwise > > > exempt from disclosure under applicable laws. It is intended to be > > > conveyed only to the designated recipient(s). Any use, > > > dissemination, > > > distribution, printing, retaining or copying of this e-mail > > > (including its > > > attachments) by unintended recipient(s) is strictly prohibited and > > > may > > > be unlawful. If you are not an intended recipient of this e-mail, > > > or believe > > > > > > that you have received this e-mail in error, please notify the > > > sender > > > immediately (by replying to this e-mail), delete any and all copies > > > of > > > this e-mail (including any attachments) from your system, and do > > > not > > > disclose the content of this e-mail to any other person. Thank you! > > > > ************* MEDIATEK Confidentiality Notice > ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!

3 days, 14 hours

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 19/06/2024 16:46, Alexandre Mergnat wrote: > Add the audio codec sub-device. This sub-device is used to set the > optional voltage values according to the hardware. > The properties are: > - Setup of microphone bias voltage. > - Setup of the speaker pin pull-down. > > Also, add the audio power supply property which is dedicated for > the audio codec sub-device. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > index 37423c2e0fdf..d95307393e75 100644 > --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > @@ -37,6 +37,32 @@ properties: > "#interrupt-cells": > const: 2 > > + vaud28-supply: > + description: 2.8 volt supply phandle for the audio codec > + > + audio-codec: > + type: object Still not much improved. You do not have any resources there, so these should go to the parent node. Best regards, Krzysztof

3 days, 23 hours

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 26/06/2024 10:30, Alexandre Mergnat wrote: > > > On 25/06/2024 15:44, Krzysztof Kozlowski wrote: >> On 25/06/2024 11:23, Alexandre Mergnat wrote: >>> >>> >>> On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >>>> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>>>> Add the audio codec sub-device. This sub-device is used to set the >>>>> optional voltage values according to the hardware. >>>>> The properties are: >>>>> - Setup of microphone bias voltage. >>>>> - Setup of the speaker pin pull-down. >>>>> >>>>> Also, add the audio power supply property which is dedicated for >>>>> the audio codec sub-device. >>>>> >>>>> Signed-off-by: Alexandre Mergnat<amergnat(a)baylibre.com> >>>>> --- >>>>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>>>> 1 file changed, 33 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> index 37423c2e0fdf..d95307393e75 100644 >>>>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> @@ -37,6 +37,32 @@ properties: >>>>> "#interrupt-cells": >>>>> const: 2 >>>>> >>>>> + vaud28-supply: >>>>> + description: 2.8 volt supply phandle for the audio codec >>>>> + >>>>> + audio-codec: >>>>> + type: object >>>> Still not much improved. You do not have any resources there, so these >>>> should go to the parent node. >>> >>> Hi Krzysztof, >>> >>> vaud28-supply seems to be a mistake that I forward port. >>> In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" >>> sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as >>> power-supply. >>> >>> pwrap { >>> pmic { >>> ... >>> audio-codec { >>> ... >>> >>> Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of >>> PMIC-audio-codec subsystem. Then: >>> - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a >>> power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) >>> - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to >>> Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec >>> substystem, beside mediatek,micbias0-microvolt >> >> I don't understand why do we talk again about supply. My comment was not >> under the supply. > > Because your word are: > " > And now you should see how odd it looks. Supplies are part of entire > chip, not subblock, even if they supply dedicated domain within that chip. > > That's why I asked to put it in the parent node. > " > > My bad, I forgot to link you the old message in my previous answer [1] > > [1] https://lore.kernel.org/all/6d21da37-8be7-467c-8878-d57af0b0201b@kernel.org… And you implemented this, so why do we talk again about it? It is already solved, isn't it? Since previous version? Best regards, Krzysztof

3 days, 23 hours

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 25/06/2024 11:23, Alexandre Mergnat wrote: > > > On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>> Add the audio codec sub-device. This sub-device is used to set the >>> optional voltage values according to the hardware. >>> The properties are: >>> - Setup of microphone bias voltage. >>> - Setup of the speaker pin pull-down. >>> >>> Also, add the audio power supply property which is dedicated for >>> the audio codec sub-device. >>> >>> Signed-off-by: Alexandre Mergnat<amergnat(a)baylibre.com> >>> --- >>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>> 1 file changed, 33 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> index 37423c2e0fdf..d95307393e75 100644 >>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> @@ -37,6 +37,32 @@ properties: >>> "#interrupt-cells": >>> const: 2 >>> >>> + vaud28-supply: >>> + description: 2.8 volt supply phandle for the audio codec >>> + >>> + audio-codec: >>> + type: object >> Still not much improved. You do not have any resources there, so these >> should go to the parent node. > > Hi Krzysztof, > > vaud28-supply seems to be a mistake that I forward port. > In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" > sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as > power-supply. > > pwrap { > pmic { > ... > audio-codec { > ... > > Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of > PMIC-audio-codec subsystem. Then: > - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a > power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) > - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to > Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec > substystem, beside mediatek,micbias0-microvolt I don't understand why do we talk again about supply. My comment was not under the supply. Best regards, Krzysztof

4 days, 18 hours

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 25.06.24 um 13:02 schrieb Jason-JH Lin (林睿祥): > > Hi Christian, > > On Tue, 2024-05-21 at 20:36 +0200, Christian König wrote: > > Am 20.05.24 um 09:58 schrieb Yong Wu (吴勇): > > > On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: > > > > > > > > External email : Please do not click links or open attachments > > > > until > > > > you have verified the sender or the content. > > > > Am 15.05.24 um 13:23 schrieb Yong Wu: > > > > > Introduce a FLAG for the restricted memory which means the > > > > > memory > > > > > > > > is > > > > > protected by TEE or hypervisor, then it's inaccessiable for > > > > > kernel. > > > > > > > > > > Currently we don't use sg_dma_unmark_restricted, thus this > > > > > > > > interface > > > > > has not been added. > > > > > > > > Why should that be part of the scatterlist? It doesn't seem to > > > > affect > > > > any of it's functionality. > > > > > > > > As far as I can see the scatterlist shouldn't be the transport of > > > > this > > > > kind of information. > > > > > > Thanks for the review. I will remove this. > > > > > > In our user scenario, DRM will import these buffers and check if > > > this > > > is a restricted buffer. If yes, it will use secure GCE takes over. > > > > > > If this judgment is not suitable to be placed in scatterlist. I > > > don't > > > know if it is ok to limit this inside dma-buf. Adding such an > > > interface: > > > > > > static bool dma_buf_is_restricted(struct dma_buf *dmabuf) > > > { > > > return !strncmp(dmabuf->exp_name, "restricted", 10); > > > } > > > > No, usually stuff like that doesn't belong into DMA buf either. > > > > Question here really is who controls the security status of the > > memory > > backing the buffer? > > > > In other words who tells the exporter that it should allocate and > > fill a > > buffer with encrypted data? > > > > If that is userspace then that is part of the format information and > > it > > is also userspace who should tell the importer that it needs to work > > with encrypted data. > > > > The kernel is intentionally not involved in stuff like that. > > > > Here is the expected protected content buffer flow in DRM: > 1) userspace allocates a dma-buf FD from the "restricted_mtk_cma" by > DMA_HEAP_IOCTL_ALLOC. > 2) userspace imports that dma-buf into the device using prime for the > drm_file. > 3) userspace uses the already implemented driver import code for the > special cases of protected content buffer. What is so special on that case? > > In the step 3), we need to verify the dma-buf is allocated from > "restricted_mtk_cma", but there is no way to pass the secure flag or > private data from userspace to the import interface in DRM driver. Why do you need to verify that? > > So I can only verify it like this now: > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device > *dev, struct dma_buf_attachment *attach, struct sg_table *sg) > { > struct mtk_gem_obj *mtk_gem; > > /* check if the entries in the sg_table are contiguous */ > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { > DRM_ERROR("sg_table is not contiguous"); > return ERR_PTR(-EINVAL); > } > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > if (IS_ERR(mtk_gem)) > return ERR_CAST(mtk_gem); > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", > 10)); > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > mtk_gem->size = attach->dmabuf->size; > mtk_gem->sg = sg; > > return &mtk_gem->base; > } Complete NAK from my side to that approach. Importing of a DMA-buf should be independent of the exporter. What you could do is to provide the secure buffer from a device and not a device heap. > I think I have the same problem as the ECC_FLAG mention in: > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > I think it would be better to have the user configurable private > information in dma-buf, so all the drivers who have the same > requirement can get their private information from dma-buf directly and > no need to change or add the interface. > > What's your opinion in this point? Well of hand I don't see the need for that. What happens if you get a non-secure buffer imported in your secure device? Regards, Christian. > > Regards, > Jason-JH.Lin > > > Regards, > > Christian. > > ************* MEDIATEK Confidentiality Notice > ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!

4 days, 20 hours

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig June 2024