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

Re: [PATCH v6 1/4] drm/panthor: Introduce BO labeling

by Liviu Dudau

On Wed, Apr 09, 2025 at 10:22:19PM +0100, Adrián Larumbe wrote: > Add a new character string Panthor BO field, and a function that allows > setting it from within the driver. > > Driver takes care of freeing the string when it's replaced or no longer > needed at object destruction time, but allocating it is the responsibility > of callers. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> Best regards, Liviu > --- > drivers/gpu/drm/panthor/panthor_gem.c | 39 +++++++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_gem.h | 17 ++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c > index 8244a4e6c2a2..af0ac17f357f 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.c > +++ b/drivers/gpu/drm/panthor/panthor_gem.c > @@ -2,6 +2,7 @@ > /* Copyright 2019 Linaro, Ltd, Rob Herring <robh(a)kernel.org> */ > /* Copyright 2023 Collabora ltd. */ > > +#include <linux/cleanup.h> > #include <linux/dma-buf.h> > #include <linux/dma-mapping.h> > #include <linux/err.h> > @@ -18,6 +19,14 @@ static void panthor_gem_free_object(struct drm_gem_object *obj) > struct panthor_gem_object *bo = to_panthor_bo(obj); > struct drm_gem_object *vm_root_gem = bo->exclusive_vm_root_gem; > > + /* > + * Label might have been allocated with kstrdup_const(), > + * we need to take that into account when freeing the memory > + */ > + kfree_const(bo->label.str); > + > + mutex_destroy(&bo->label.lock); > + > drm_gem_free_mmap_offset(&bo->base.base); > mutex_destroy(&bo->gpuva_list_lock); > drm_gem_shmem_free(&bo->base); > @@ -196,6 +205,7 @@ struct drm_gem_object *panthor_gem_create_object(struct drm_device *ddev, size_t > obj->base.map_wc = !ptdev->coherent; > mutex_init(&obj->gpuva_list_lock); > drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock); > + mutex_init(&obj->label.lock); > > return &obj->base.base; > } > @@ -247,3 +257,32 @@ panthor_gem_create_with_handle(struct drm_file *file, > > return ret; > } > + > +void > +panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label) > +{ > + struct panthor_gem_object *bo = to_panthor_bo(obj); > + const char *old_label; > + > + scoped_guard(mutex, &bo->label.lock) { > + old_label = bo->label.str; > + bo->label.str = label; > + } > + > + kfree(old_label); > +} > + > +void > +panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label) > +{ > + const char *str; > + > + str = kstrdup_const(label, GFP_KERNEL); > + if (!str) { > + /* Failing to allocate memory for a label isn't a fatal condition */ > + drm_warn(bo->obj->dev, "Not enough memory to allocate BO label"); > + return; > + } > + > + panthor_gem_bo_set_label(bo->obj, str); > +} > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index 1a363bb814f4..af0d77338860 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -46,6 +46,20 @@ struct panthor_gem_object { > > /** @flags: Combination of drm_panthor_bo_flags flags. */ > u32 flags; > + > + /** > + * @label: BO tagging fields. The label can be assigned within the > + * driver itself or through a specific IOCTL. > + */ > + struct { > + /** > + * @label.str: Pointer to NULL-terminated string, > + */ > + const char *str; > + > + /** @lock.str: Protects access to the @label.str field. */ > + struct mutex lock; > + } label; > }; > > /** > @@ -91,6 +105,9 @@ panthor_gem_create_with_handle(struct drm_file *file, > struct panthor_vm *exclusive_vm, > u64 *size, u32 flags, uint32_t *handle); > > +void panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label); > +void panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label); > + > static inline u64 > panthor_kernel_bo_gpuva(struct panthor_kernel_bo *bo) > { > -- > 2.48.1 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯

8 months, 1 week

1
0
0 0

Re: [PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list

by Christian König

Am 10.04.25 um 11:24 schrieb Philipp Stanner: > Nouveau currently relies on the assumption that dma_fences will only > ever get signaled through nouveau_fence_signal(), which takes care of > removing a signaled fence from the list nouveau_fence_chan.pending. > > This self-imposed rule is violated in nouveau_fence_done(), where > dma_fence_is_signaled() (somewhat surprisingly, considering its name) > can signal the fence without removing it from the list. This enables > accesses to already signaled fences through the list, which is a bug. > > In particular, it can race with nouveau_fence_context_kill(), which > would then attempt to set an error code on an already signaled fence, > which is illegal. > > In nouveau_fence_done(), the call to nouveau_fence_update() already > ensures to signal all ready fences. Thus, the signaling potentially > performed by dma_fence_is_signaled() is actually not necessary. Ah, I now got what you are trying to do here! But that won't help. The problem is it is perfectly valid for somebody external (e.g. other driver, TTM etc...) to call dma_fence_is_signaled() on a nouveau fence. This will then in turn still signal the fence and leave it on the pending list and creating the problem you have. Regards, Christian. > > Replace the call to dma_fence_is_signaled() with > nouveau_fence_base_is_signaled(). > > Cc: <stable(a)vger.kernel.org> # 4.10+, precise commit not to be determined > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index 7cc84472cece..33535987d8ed 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -274,7 +274,7 @@ nouveau_fence_done(struct nouveau_fence *fence) > nvif_event_block(&fctx->event); > spin_unlock_irqrestore(&fctx->lock, flags); > } > - return dma_fence_is_signaled(&fence->base); > + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags); > } > > static long

8 months, 1 week

1
0
0 0

Re: [PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list

by Christian König

Am 10.04.25 um 14:21 schrieb Danilo Krummrich: > On Thu, Apr 10, 2025 at 02:13:34PM +0200, Christian König wrote: >> Am 10.04.25 um 11:24 schrieb Philipp Stanner: >>> Nouveau currently relies on the assumption that dma_fences will only >>> ever get signaled through nouveau_fence_signal(), which takes care of >>> removing a signaled fence from the list nouveau_fence_chan.pending. >>> >>> This self-imposed rule is violated in nouveau_fence_done(), where >>> dma_fence_is_signaled() (somewhat surprisingly, considering its name) >>> can signal the fence without removing it from the list. This enables >>> accesses to already signaled fences through the list, which is a bug. >>> >>> In particular, it can race with nouveau_fence_context_kill(), which >>> would then attempt to set an error code on an already signaled fence, >>> which is illegal. >>> >>> In nouveau_fence_done(), the call to nouveau_fence_update() already >>> ensures to signal all ready fences. Thus, the signaling potentially >>> performed by dma_fence_is_signaled() is actually not necessary. >>> >>> Replace the call to dma_fence_is_signaled() with >>> nouveau_fence_base_is_signaled(). >>> >>> Cc: <stable(a)vger.kernel.org> # 4.10+, precise commit not to be determined >>> Signed-off-by: Philipp Stanner <phasta(a)kernel.org> >>> --- >>> drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c >>> index 7cc84472cece..33535987d8ed 100644 >>> --- a/drivers/gpu/drm/nouveau/nouveau_fence.c >>> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c >>> @@ -274,7 +274,7 @@ nouveau_fence_done(struct nouveau_fence *fence) >>> nvif_event_block(&fctx->event); >>> spin_unlock_irqrestore(&fctx->lock, flags); >>> } >>> - return dma_fence_is_signaled(&fence->base); >>> + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags); >> See the code above that: >> >> if (fence->base.ops == &nouveau_fence_ops_legacy || >> fence->base.ops == &nouveau_fence_ops_uevent) { > I think this check is a bit pointless given that fence is already a struct > nouveau_fence. :) Oh, good point. I totally missed that. In this case that indeed doesn't make any sense at all. (Unless somebody just blindly upcasted the structure, but I really hope that this isn't the case here). Regards, Christian.

8 months, 1 week

1
0
0 0

Re: [PATCH 0/3] drm/nouveau: Fix & improve nouveau_fence_done()

by Christian König

Am 10.04.25 um 11:51 schrieb Philipp Stanner: > On Thu, 2025-04-10 at 11:24 +0200, Philipp Stanner wrote: >> Contains two patches improving nouveau_fence_done(), and one >> addressing >> an actual bug (race): > Oops, that's the wrong calltrace. Here we go: > > [ 85.791794] Call Trace: [ 85.791796] <TASK> [ 85.791797] ? nouveau_fence_context_kill (/home/imperator/linux/./include/linux/dma-fence.h:587 (discriminator 9) /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_fence.c:94 (discriminator 9)) nouveau [ 85.791874] ? __warn.cold (/home/imperator/linux/kernel/panic.c:748) [ 85.791878] ? nouveau_fence_context_kill (/home/imperator/linux/./include/linux/dma-fence.h:587 (discriminator 9) /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_fence.c:94 (discriminator 9)) nouveau [ 85.791950] ? report_bug (/home/imperator/linux/lib/bug.c:180 /home/imperator/linux/lib/bug.c:219) [ 85.791953] ? handle_bug (/home/imperator/linux/arch/x86/kernel/traps.c:260) [ 85.791956] ? exc_invalid_op (/home/imperator/linux/arch/x86/kernel/traps.c:309 (discriminator 1)) [ 85.791957] ? asm_exc_invalid_op (/home/imperator/linux/./arch/x86/include/asm/idtentry.h:621) [ 85.791960] ? nouveau_fence_context_kill (/home/imperator/linux/./include/linux/dma-fence.h:587 (discriminator 9) /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_fence.c:94 (discriminator 9)) nouveau [ 85.792028] drm_sched_fini.cold (/home/imperator/linux/./include/trace/../../drivers/gpu/drm/scheduler/gpu_scheduler_trace.h:72 (discriminator 1)) gpu_sched [ 85.792033] ? drm_sched_entity_kill.part.0 (/home/imperator/linux/drivers/gpu/drm/scheduler/sched_entity.c:243 (discriminator 2)) gpu_sched [ 85.792037] nouveau_sched_destroy (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_sched.c:509 /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_sched.c:518) nouveau [ 85.792122] nouveau_abi16_chan_fini.isra.0 (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_abi16.c:188) nouveau [ 85.792191] nouveau_abi16_fini (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_abi16.c:224 (discriminator 3)) nouveau [ 85.792263] nouveau_drm_postclose (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_drm.c:1240) nouveau [ 85.792349] drm_file_free (/home/imperator/linux/drivers/gpu/drm/drm_file.c:255) [ 85.792353] drm_release (/home/imperator/linux/./arch/x86/include/asm/atomic.h:67 (discriminator 1) /home/imperator/linux/./include/linux/atomic/atomic-arch-fallback.h:2278 (discriminator 1) /home/imperator/linux/./include/linux/atomic/atomic-instrumented.h:1384 (discriminator 1) /home/imperator/linux/drivers/gpu/drm/drm_file.c:428 (discriminator 1)) [ 85.792355] __fput (/home/imperator/linux/fs/file_table.c:464) [ 85.792357] task_work_run (/home/imperator/linux/kernel/task_work.c:227) [ 85.792360] do_exit (/home/imperator/linux/kernel/exit.c:939) [ 85.792362] do_group_exit (/home/imperator/linux/kernel/exit.c:1069) [ 85.792364] get_signal (/home/imperator/linux/kernel/signal.c:3036) [ 85.792366] arch_do_signal_or_restart (/home/imperator/linux/./arch/x86/include/asm/syscall.h:38 /home/imperator/linux/arch/x86/kernel/signal.c:264 /home/imperator/linux/arch/x86/kernel/signal.c:339) [ 85.792369] syscall_exit_to_user_mode (/home/imperator/linux/kernel/entry/common.c:113 /home/imperator/linux/./include/linux/entry-common.h:329 /home/imperator/linux/kernel/entry/common.c:207 /home/imperator/linux/kernel/entry/common.c:218) [ 85.792372] do_syscall_64 (/home/imperator/linux/./arch/x86/include/asm/cpufeature.h:172 /home/imperator/linux/arch/x86/entry/common.c:98) [ 85.792373] ? syscall_exit_to_user_mode_prepare (/home/imperator/linux/./include/linux/audit.h:357 /home/imperator/linux/kernel/entry/common.c:166 /home/imperator/linux/kernel/entry/common.c:200) [ 85.792376] ? syscall_exit_to_user_mode (/home/imperator/linux/./arch/x86/include/asm/paravirt.h:686 /home/imperator/linux/./include/linux/entry-common.h:232 /home/imperator/linux/kernel/entry/common.c:206 /home/imperator/linux/kernel/entry/common.c:218) [ 85.792377] ? do_syscall_64 (/home/imperator/linux/./arch/x86/include/asm/cpufeature.h:172 /home/imperator/linux/arch/x86/entry/common.c:98) [ 85.792378] entry_SYSCALL_64_after_hwframe (/home/imperator/linux/arch/x86/entry/entry_64.S:130) [ 85.792381] RIP: 0033:0x7ff950b6af70 [ 85.792383] Code: Unable to access opcode bytes at 0x7ff950b6af46. objdump: '/tmp/tmp.sfPRl5k2te.o': No such file Code starting with the faulting instruction =========================================== [ 85.792383] RSP: 002b:00007ff93cdfb6f0 EFLAGS: 00000293 ORIG_RAX: 000000000000010f [ 85.792385] RAX: fffffffffffffdfe RBX: 000055d386d61870 RCX: 00007ff950b6af70 [ 85.792386] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 00007ff928000b90 [ 85.792387] RBP: 00007ff93cdfb740 R08: 0000000000000008 R09: 0000000000000000 [ 85.792388] R10: 0000000000000000 R11: 0000000000000293 R12: 0000000000000001 [ 85.792388] R13: 0000000000000000 R14: 0000000000000000 R15: 00007ff951b10b40 [ 85.792390] </TASK> [ 85.792391] ---[ end trace 0000000000000000 ]--- I think I understand the problem now as well, but that backtrace is completely mangled in the mail. It would be nice if you could send that out again. Thanks, Christian. > > By the way, for reference: > I did try whether it could be done to have nouveau_fence_signal() > incorporated into nouveau_fence_update() and nouveau_fence_done(). > This, however, would then cause a race with the list_del() in > nouveau_fence_no_signaling(), WARNing because of the list poison. > > So the "solution" space is: > * A cleanup callback on the dma_fence. > * Keeping the current race or > * replacing it with another race with another function. > * Just preventing nouveau_fence_done() from signaling fences other > than through nouveau_fence_update/signal > > The later seems clearly like the cleanest solution to me. Alternative > would be a work-intensive rework of all the misdesigns broken in > nouveau_fence.c > > > P. > >> [ 39.848463] WARNING: CPU: 21 PID: 1734 at >> drivers/gpu/drm/nouveau/nouveau_fence.c:509 >> nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848551] Modules linked in: snd_seq_dummy snd_hrtimer >> nf_conntrack_netbios_ns nf_conntrack_broadcast nft_fib_inet >> nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_ine >> t nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat >> nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set >> nf_tables qrtr sunrpc snd_sof_pci_intel_ >> tgl snd_sof_pci_intel_cnl snd_sof_intel_hda_generic snd_sof_pci >> snd_sof_xtensa_dsp snd_sof_intel_hda_common snd_soc_hdac_hda >> snd_sof_intel_hda snd_sof snd_sof_utils snd >> _soc_acpi_intel_match snd_soc_acpi snd_soc_acpi_intel_sdca_quirks >> snd_sof_intel_hda_mlink snd_soc_sdca snd_soc_avs snd_ctl_led >> snd_soc_hda_codec intel_rapl_msr snd_hda_ >> codec_realtek snd_hda_ext_core intel_rapl_common >> snd_hda_codec_generic snd_soc_core snd_hda_scodec_component >> intel_uncore_frequency intel_uncore_frequency_common snd_hd >> a_codec_hdmi intel_ifs snd_compress i10nm_edac skx_edac_common nfit >> snd_hda_intel snd_intel_dspcfg libnvdimm snd_hda_codec binfmt_misc >> snd_hwdep snd_hda_core snd_seq sn >> d_seq_device dell_wmi >> [ 39.848575] dell_pc x86_pkg_temp_thermal spi_nor platform_profile >> sparse_keymap intel_powerclamp dax_hmem snd_pcm cxl_acpi coretemp >> cxl_port iTCO_wdt mtd rapl intel >> _pmc_bxt pmt_telemetry cxl_core dell_wmi_sysman pmt_class >> iTCO_vendor_support snd_timer isst_if_mmio vfat intel_cstate >> dell_smbios dcdbas fat dell_wmi_ddv dell_smm_hwmo >> n dell_wmi_descriptor firmware_attributes_class wmi_bmof intel_uncore >> einj pcspkr isst_if_mbox_pci atlantic snd isst_if_common intel_vsec >> e1000e macsec mei_me i2c_i801 >> spi_intel_pci soundcore i2c_smbus spi_intel mei joydev loop nfnetlink >> zram nouveau drm_ttm_helper ttm polyval_clmulni iaa_crypto gpu_sched >> polyval_generic rtsx_pci_sdmm >> c ghash_clmulni_intel i2c_algo_bit mmc_core drm_gpuvm sha512_ssse3 >> nvme drm_exec drm_display_helper sha256_ssse3 idxd sha1_ssse3 cec >> nvme_core idxd_bus rtsx_pci nvme_au >> th pinctrl_alderlake ip6_tables ip_tables fuse >> [ 39.848603] CPU: 21 UID: 42 PID: 1734 Comm: gnome-shell Tainted: >> G W 6.14.0-rc4+ #11 >> [ 39.848605] Tainted: [W]=WARN >> [ 39.848606] Hardware name: Dell Inc. Precision 7960 Tower/01G0M6, >> BIOS 2.7.0 12/17/2024 >> [ 39.848607] RIP: 0010:nouveau_fence_no_signaling+0xac/0xd0 >> [nouveau] >> [ 39.848688] Code: db 74 17 48 8d 7b 38 b8 ff ff ff ff f0 0f c1 43 >> 38 83 f8 01 74 29 85 c0 7e 17 31 c0 5b 5d c3 cc cc cc cc e8 76 b2 c5 >> f0 eb 96 <0f> 0b e9 67 ff ff f >> f be 03 00 00 00 e8 83 76 33 f1 31 c0 eb dd e8 >> [ 39.848690] RSP: 0018:ff1cc1ffc5c039f0 EFLAGS: 00010046 >> [ 39.848691] RAX: 0000000000000001 RBX: ff175a3b504da980 RCX: >> ff175a3b4801e008 >> [ 39.848692] RDX: ff175a3b43e7bad0 RSI: ffffffffc09d3fda RDI: >> ff175a3b504da980 >> [ 39.848693] RBP: ff175a3b504da9c0 R08: ffffffffc09e39df R09: >> 0000000000000001 >> [ 39.848694] R10: 0000000000000001 R11: 0000000000000000 R12: >> ff175a3b6d97de00 >> [ 39.848695] R13: 0000000000000246 R14: ff1cc1ffc5c03c60 R15: >> 0000000000000001 >> [ 39.848696] FS: 00007fc5477846c0(0000) GS:ff175a5a50280000(0000) >> knlGS:0000000000000000 >> [ 39.848698] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> [ 39.848699] CR2: 000055cb7613d1a8 CR3: 000000012e5ce004 CR4: >> 0000000000f71ef0 >> [ 39.848700] DR0: 0000000000000000 DR1: 0000000000000000 DR2: >> 0000000000000000 >> [ 39.848701] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: >> 0000000000000400 >> [ 39.848702] PKRU: 55555554 >> [ 39.848703] Call Trace: >> [ 39.848704] <TASK> >> [ 39.848705] ? nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848782] ? __warn.cold+0x93/0xfa >> [ 39.848785] ? nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848861] ? report_bug+0xff/0x140 >> [ 39.848863] ? handle_bug+0x58/0x90 >> [ 39.848865] ? exc_invalid_op+0x17/0x70 >> [ 39.848866] ? asm_exc_invalid_op+0x1a/0x20 >> [ 39.848870] ? nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848943] nouveau_fence_enable_signaling+0x32/0x80 [nouveau] >> [ 39.849016] ? __pfx_nouveau_fence_cleanup_cb+0x10/0x10 [nouveau] >> [ 39.849088] __dma_fence_enable_signaling+0x33/0xc0 >> [ 39.849090] dma_fence_add_callback+0x4b/0xd0 >> [ 39.849093] nouveau_fence_emit+0xa3/0x260 [nouveau] >> [ 39.849166] nouveau_fence_new+0x7d/0xf0 [nouveau] >> [ 39.849242] nouveau_gem_ioctl_pushbuf+0xe8f/0x1300 [nouveau] >> [ 39.849338] ? __pfx_nouveau_gem_ioctl_pushbuf+0x10/0x10 [nouveau] >> [ 39.849431] drm_ioctl_kernel+0xad/0x100 >> [ 39.849433] drm_ioctl+0x288/0x550 >> [ 39.849435] ? __pfx_nouveau_gem_ioctl_pushbuf+0x10/0x10 [nouveau] >> [ 39.849526] nouveau_drm_ioctl+0x57/0xb0 [nouveau] >> [ 39.849620] __x64_sys_ioctl+0x94/0xc0 >> [ 39.849621] do_syscall_64+0x82/0x160 >> [ 39.849623] ? drm_ioctl+0x2b7/0x550 >> [ 39.849625] ? __pfx_nouveau_gem_ioctl_pushbuf+0x10/0x10 [nouveau] >> [ 39.849719] ? ktime_get_mono_fast_ns+0x38/0xd0 >> [ 39.849721] ? __pm_runtime_suspend+0x69/0xc0 >> [ 39.849724] ? syscall_exit_to_user_mode_prepare+0x15e/0x1a0 >> [ 39.849726] ? syscall_exit_to_user_mode+0x10/0x200 >> [ 39.849729] ? do_syscall_64+0x8e/0x160 >> [ 39.849730] ? exc_page_fault+0x7e/0x1a0 >> [ 39.849733] entry_SYSCALL_64_after_hwframe+0x76/0x7e >> [ 39.849735] RIP: 0033:0x7fc5576fe0ad >> [ 39.849736] Code: 04 25 28 00 00 00 48 89 45 c8 31 c0 48 8d 45 10 >> c7 45 b0 10 00 00 00 48 89 45 b8 48 8d 45 d0 48 89 45 c0 b8 10 00 00 >> 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1a 48 8b 45 c8 64 48 2b 04 25 28 >> 00 00 00 >> [ 39.849737] RSP: 002b:00007ffc002688a0 EFLAGS: 00000246 ORIG_RAX: >> 0000000000000010 >> [ 39.849739] RAX: ffffffffffffffda RBX: 000055cb74e316c0 RCX: >> 00007fc5576fe0ad >> [ 39.849740] RDX: 00007ffc00268960 RSI: 00000000c0406481 RDI: >> 000000000000000e >> [ 39.849741] RBP: 00007ffc002688f0 R08: 0000000000000000 R09: >> 000055cb74e35560 >> [ 39.849742] R10: 0000000000000014 R11: 0000000000000246 R12: >> 00007ffc00268960 >> [ 39.849744] R13: 00000000c0406481 R14: 000000000000000e R15: >> 000055cb74e3cd10 >> [ 39.849746] </TASK> >> [ 39.849746] ---[ end trace 0000000000000000 ]--- >> [ 39.849776] ------------[ cut here ]------------ >> >> >> This is the first WARN_ON() in dma_fence_set_error(), called by >> nouveau_fence_context_kill(). >> >> It's rare, but it is a bug, or rather: the archetype of a race, since >> (as Christian pointed out) nouveau_fence_update() later at some point >> will remove the signaled fence (by signaling it again). >> >> >> P. >> >> >> Philipp Stanner (3): >> drm/nouveau: Prevent signaled fences in pending list >> drm/nouveau: Remove surplus if-branch >> drm/nouveau: Add helper to check base fence >> >> drivers/gpu/drm/nouveau/nouveau_fence.c | 32 ++++++++++++++--------- >> -- >> 1 file changed, 18 insertions(+), 14 deletions(-) >>

8 months, 1 week

1
0
0 0

Re: [PATCH 2/3] drm/nouveau: Remove surplus if-branch

by Christian König

Am 10.04.25 um 11:24 schrieb Philipp Stanner: > nouveau_fence_done() contains an if-branch which checks for the > existence of either of two fence backend ops. Those two are the only > backend ops existing in Nouveau, however; and at least one backend ops > must be in use for the entire driver to be able to work. The if branch > is, therefore, surplus. > > Remove the if-branch. What happens here is that nouveau checks if the fence comes from itself or some external source. So when you remove that check you potentially illegally uses nouveau_fctx() on a non-nouveau fence. Regards, Christian. > > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index 33535987d8ed..db6f4494405c 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -259,21 +259,19 @@ nouveau_fence_emit(struct nouveau_fence *fence) > bool > nouveau_fence_done(struct nouveau_fence *fence) > { > - if (fence->base.ops == &nouveau_fence_ops_legacy || > - fence->base.ops == &nouveau_fence_ops_uevent) { > - struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > - struct nouveau_channel *chan; > - unsigned long flags; > + struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > + struct nouveau_channel *chan; > + unsigned long flags; > > - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) > - return true; > + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) > + return true; > + > + spin_lock_irqsave(&fctx->lock, flags); > + chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > + if (chan && nouveau_fence_update(chan, fctx)) > + nvif_event_block(&fctx->event); > + spin_unlock_irqrestore(&fctx->lock, flags); > > - spin_lock_irqsave(&fctx->lock, flags); > - chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > - if (chan && nouveau_fence_update(chan, fctx)) > - nvif_event_block(&fctx->event); > - spin_unlock_irqrestore(&fctx->lock, flags); > - } > return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags); > } >

8 months, 1 week

1
0
0 0

Re: [PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list

by Christian König

Am 10.04.25 um 11:24 schrieb Philipp Stanner: > Nouveau currently relies on the assumption that dma_fences will only > ever get signaled through nouveau_fence_signal(), which takes care of > removing a signaled fence from the list nouveau_fence_chan.pending. > > This self-imposed rule is violated in nouveau_fence_done(), where > dma_fence_is_signaled() (somewhat surprisingly, considering its name) > can signal the fence without removing it from the list. This enables > accesses to already signaled fences through the list, which is a bug. > > In particular, it can race with nouveau_fence_context_kill(), which > would then attempt to set an error code on an already signaled fence, > which is illegal. > > In nouveau_fence_done(), the call to nouveau_fence_update() already > ensures to signal all ready fences. Thus, the signaling potentially > performed by dma_fence_is_signaled() is actually not necessary. > > Replace the call to dma_fence_is_signaled() with > nouveau_fence_base_is_signaled(). > > Cc: <stable(a)vger.kernel.org> # 4.10+, precise commit not to be determined > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index 7cc84472cece..33535987d8ed 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -274,7 +274,7 @@ nouveau_fence_done(struct nouveau_fence *fence) > nvif_event_block(&fctx->event); > spin_unlock_irqrestore(&fctx->lock, flags); > } > - return dma_fence_is_signaled(&fence->base); > + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags); See the code above that: if (fence->base.ops == &nouveau_fence_ops_legacy || fence->base.ops == &nouveau_fence_ops_uevent) { .... Nouveau first tests if it's one of it's own fences, and if yes does some special handling. E.g. checking the fence status bits etc... So this dma_fence_is_signaled() is for all non-nouveau fences and then not touching the internal flags is perfectly correct as far as I can see. Regards, Christian. > } > > static long

8 months, 1 week

1
0
0 0

Re: [PATCH 1/2] dma-fence: Rename dma_fence_is_signaled()

by Christian König

Am 09.04.25 um 17:04 schrieb Philipp Stanner: > On Wed, 2025-04-09 at 16:10 +0200, Christian König wrote: >>> I only see improvement by making things more obvious. >>> >>> In any case, how would you call a wrapper that just does >>> test_bit(IS_SIGNALED, …) ? >> Broken, that was very intentionally removed quite shortly after we >> created the framework. >> >> We have a few cases were implementations do check that for their >> fences, but consumers should never be allowed to touch such >> internals. > There is theory and there is practice. In practice, those internals are > being used by Nouveau, i915, Xe, vmgfx and radeon. What do you mean? I only skimmed over the use cases, but as far as I can see those are all valid. You can test the flag if you know what the fence means to you, that is not a problem at all. > So it seems that we failed quite a bit at communicating clearly how the > interface should be used. > > And, to repeat myself, with both name and docu of that function, I > think it is very easy to misunderstand what it's doing. You say that it > shouldn't matter – and maybe that's true, in theory. In practice, it > does matter. In practice, APIs get misused and have side-effects. And > making that harder is desirable. That sounds like I didn't used the right wording. It *must* not matter to the consumer. See the purpose of the DMA-fence framework is to make it irrelevant for the consumer how the provider has implemented it's fences. This means that things like if polling or interrupt driven signaling is used, 32bit vs 64bit seq numbers, etc... should all be hidden by the framework from the consumer of the fences. BTW I'm actually not sure if nouveau has a bug here. As far as I can see nouveau_fence_signal() will be called later eventually and do the necessary cleanup. But on the other hand it wouldn't surprise me if nouveau has a bug with that. The driver has been basically only barely maintained for quite a while. > In any case, I might have to add another such call to Nouveau, because > the solution preferred by you over the callback causes another race. > Certainly one could solve this in a clean way, but someone has to do > the work, and we're talking about more than a few hours here. Well this is not my preferred solution, it's just the technical correct solution as far as I can see. > In any case, be so kind and look at patch 2 and tell me there if you're > at least OK with making the documentation more detailed. As far as I can see that is clearly the wrong place to document that stuff. Regards, Christian. > > P.

8 months, 1 week

1
0
0 0

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

by Jens Wiklander

On Wed, Apr 9, 2025 at 2:50 PM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, Apr 08, 2025 at 03:28:45PM +0200, Jens Wiklander wrote: > > On Tue, Apr 8, 2025 at 11:14 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > On Tue, Apr 01, 2025 at 10:33:04AM +0200, Jens Wiklander wrote: > > > > On Tue, Apr 1, 2025 at 9:58 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > > > > > On Tue, Mar 25, 2025 at 11:55:46AM +0100, Jens Wiklander wrote: > > > > > > Hi Sumit, > > > > > > > > > > > > > > > > <snip> > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > +#include "tee_private.h" > > > > > > > > + > > > > > > > > +struct tee_dma_heap { > > > > > > > > + struct dma_heap *heap; > > > > > > > > + enum tee_dma_heap_id id; > > > > > > > > + struct tee_rstmem_pool *pool; > > > > > > > > + struct tee_device *teedev; > > > > > > > > + /* Protects pool and teedev above */ > > > > > > > > + struct mutex mu; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +struct tee_heap_buffer { > > > > > > > > + struct tee_rstmem_pool *pool; > > > > > > > > + struct tee_device *teedev; > > > > > > > > + size_t size; > > > > > > > > + size_t offs; > > > > > > > > + struct sg_table table; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +struct tee_heap_attachment { > > > > > > > > + struct sg_table table; > > > > > > > > + struct device *dev; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +struct tee_rstmem_static_pool { > > > > > > > > + struct tee_rstmem_pool pool; > > > > > > > > + struct gen_pool *gen_pool; > > > > > > > > + phys_addr_t pa_base; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +#if !IS_MODULE(CONFIG_TEE) && IS_ENABLED(CONFIG_DMABUF_HEAPS) > > > > > > > > > > > > > > Can this dependency rather be better managed via Kconfig? > > > > > > > > > > > > This was the easiest yet somewhat flexible solution I could find. If > > > > > > you have something better, let's use that instead. > > > > > > > > > > > > > > > > --- a/drivers/tee/optee/Kconfig > > > > > +++ b/drivers/tee/optee/Kconfig > > > > > @@ -5,6 +5,7 @@ config OPTEE > > > > > depends on HAVE_ARM_SMCCC > > > > > depends on MMU > > > > > depends on RPMB || !RPMB > > > > > + select DMABUF_HEAPS > > > > > help > > > > > This implements the OP-TEE Trusted Execution Environment (TEE) > > > > > driver. > > > > > > > > I wanted to avoid that since there are plenty of use cases where > > > > DMABUF_HEAPS aren't needed. > > > > > > Yeah, but how the users will figure out the dependency to enable DMA > > > heaps with TEE subsystem. > > > > I hope, without too much difficulty. They are after all looking for a > > way to allocate memory from a DMA heap. > > > > > So it's better we provide a generic kernel > > > Kconfig which enables all the default features. > > > > I disagree, it should be possible to configure without DMABUF_HEAPS if desired. > > It's hard to see a use-case for that additional compile time option. If > you are worried about kernel size then those can be built as modules. On > the other hand the benifit is that we avoid ifdefery and providing sane > TEE defaults where features can be detected and enabled at runtime > instead. My primary concern isn't kernel size, even if it shouldn't be irrelevant. It doesn't seem right to enable features that are not asked for casually. In this case, it's not unreasonable or unexpected that DMABUF_HEAPS must be explicitly enabled in the config if a heap interface is needed. It's the same as before this patch set. > > > > > > > > > > This seems to do the job: > > > > +config TEE_DMABUF_HEAP > > > > + bool > > > > + depends on TEE = y && DMABUF_HEAPS > > > > > > > > We can only use DMABUF_HEAPS if the TEE subsystem is compiled into the kernel. > > > > > > Ah, I see. So we aren't exporting the DMA heaps APIs for TEE subsystem > > > to use. We should do that such that there isn't a hard dependency to > > > compile them into the kernel. > > > > I was saving that for a later patch set as a later problem. We may > > save some time by not doing it now. > > > > But I think it's not a correct way to just reuse internal APIs from DMA > heaps subsystem without exporting them. It can be seen as a inter > subsystem API contract breach. I hope it won't be an issue with DMA heap > maintainers regarding export of those APIs. Fair enough. I'll add a patch in the next patch set for that. I guess the same goes for CMA. Cheers, Jens

8 months, 1 week

1
0
0 0

Re: [PATCH 1/2] dma-fence: Rename dma_fence_is_signaled()

by Christian König

Am 09.04.25 um 16:01 schrieb Philipp Stanner: > On Wed, 2025-04-09 at 15:14 +0200, Christian König wrote: >> Am 09.04.25 um 14:56 schrieb Philipp Stanner: >>> On Wed, 2025-04-09 at 14:51 +0200, Philipp Stanner wrote: >>>> On Wed, 2025-04-09 at 14:39 +0200, Boris Brezillon wrote: >>>>> Hi Philipp, >>>>> >>>>> On Wed, 9 Apr 2025 14:06:37 +0200 >>>>> Philipp Stanner <phasta(a)kernel.org> wrote: >>>>> >>>>>> dma_fence_is_signaled()'s name strongly reads as if this >>>>>> function >>>>>> were >>>>>> intended for checking whether a fence is already signaled. >>>>>> Also >>>>>> the >>>>>> boolean it returns hints at that. >>>>>> >>>>>> The function's behavior, however, is more complex: it can >>>>>> check >>>>>> with a >>>>>> driver callback whether the hardware's sequence number >>>>>> indicates >>>>>> that >>>>>> the fence can already be treated as signaled, although the >>>>>> hardware's / >>>>>> driver's interrupt handler has not signaled it yet. If that's >>>>>> the >>>>>> case, >>>>>> the function also signals the fence. >>>>>> >>>>>> (Presumably) this has caused a bug in Nouveau (unknown >>>>>> commit), >>>>>> where >>>>>> nouveau_fence_done() uses the function to check a fence, >>>>>> which >>>>>> causes a >>>>>> race. >>>>>> >>>>>> Give the function a more obvious name. >>>>> This is just my personal view on this, but I find the new name >>>>> just >>>>> as >>>>> confusing as the old one. It sounds like something is checked, >>>>> but >>>>> it's >>>>> clear what, and then the fence is forcibly signaled like it >>>>> would >>>>> be >>>>> if >>>>> you call drm_fence_signal(). Of course, this clarified by the >>>>> doc, >>>>> but >>>>> given the goal was to make the function name clearly reflect >>>>> what >>>>> it >>>>> does, I'm not convinced it's significantly better. >>>>> >>>>> Maybe dma_fence_check_hw_state_and_propagate(), though it might >>>>> be >>>>> too long of name. Oh well, feel free to ignore this comments if >>>>> a >>>>> majority is fine with the new name. >>>> Yoa, the name isn't perfect (the perfect name describing the >>>> whole >>>> behavior would be >>>> dma_fence_check_if_already_signaled_then_check_hardware_state_and >>>> _pro >>>> pa >>>> gate() ^^' >>>> >>>> My intention here is to have the reader realize "watch out, the >>>> fence >>>> might get signaled here!", which is probably the most important >>>> event >>>> regarding fences, which can race, invoke the callbacks and so on. >>>> >>>> For details readers will then check the documentation. >>>> >>>> But I'm of course open to see if there's a majority for this or >>>> that >>>> name. >>> how about: >>> >>> dma_fence_check_hw_and_signal() ? >> I don't think that renaming the function is a good idea in the first >> place. >> >> What the function does internally is an implementation detail of the >> framework. >> >> For the code using this function it's completely irrelevant if the >> function might also signal the fence, what matters for the caller is >> the returned status of the fence. I think this also counts for the >> dma_fence_is_signaled() documentation. > It does obviously matter. As it's currently implemented, a lot of > important things happen implicitly. Yeah, but that's ok. The code who calls this is the consumer of the interface and so shouldn't need to know this. That's why we have created the DMA fence framework in the first place. For the provider side when a driver or similar implements the interface the relevant documentation is the dma_fence_ops structure. > I only see improvement by making things more obvious. > > In any case, how would you call a wrapper that just does > test_bit(IS_SIGNALED, …) ? Broken, that was very intentionally removed quite shortly after we created the framework. We have a few cases were implementations do check that for their fences, but consumers should never be allowed to touch such internals. Regards, Christian. > > P. > >> What we should improve is the documentation of the dma_fence_ops- >>> enable_signaling and dma_fence_ops->signaled callbacks. >> Especially see the comment about reference counts on enable_signaling >> which is missing on the signaled callback. That is most likely the >> root cause why nouveau implemented enable_signaling correctly but not >> the other one. >> >> But putting that aside I think we should make nails with heads and >> let the framework guarantee that the fences stay alive until they are >> signaled (one way or another). This completely removes the burden to >> keep a reference on unsignaled fences from the drivers / >> implementations and make things more over all more defensive. >> >> Regards, >> Christian. >> >>> P. >>> >>>> P. >>>> >>>> >>>>> Regards, >>>>> >>>>> Boris

8 months, 1 week

1
0
0 0

Re: [PATCH 1/2] dma-fence: Rename dma_fence_is_signaled()

by Christian König

Am 09.04.25 um 14:56 schrieb Philipp Stanner: > On Wed, 2025-04-09 at 14:51 +0200, Philipp Stanner wrote: >> On Wed, 2025-04-09 at 14:39 +0200, Boris Brezillon wrote: >>> Hi Philipp, >>> >>> On Wed, 9 Apr 2025 14:06:37 +0200 >>> Philipp Stanner <phasta(a)kernel.org> wrote: >>> >>>> dma_fence_is_signaled()'s name strongly reads as if this function >>>> were >>>> intended for checking whether a fence is already signaled. Also >>>> the >>>> boolean it returns hints at that. >>>> >>>> The function's behavior, however, is more complex: it can check >>>> with a >>>> driver callback whether the hardware's sequence number indicates >>>> that >>>> the fence can already be treated as signaled, although the >>>> hardware's / >>>> driver's interrupt handler has not signaled it yet. If that's the >>>> case, >>>> the function also signals the fence. >>>> >>>> (Presumably) this has caused a bug in Nouveau (unknown commit), >>>> where >>>> nouveau_fence_done() uses the function to check a fence, which >>>> causes a >>>> race. >>>> >>>> Give the function a more obvious name. >>> This is just my personal view on this, but I find the new name just >>> as >>> confusing as the old one. It sounds like something is checked, but >>> it's >>> clear what, and then the fence is forcibly signaled like it would >>> be >>> if >>> you call drm_fence_signal(). Of course, this clarified by the doc, >>> but >>> given the goal was to make the function name clearly reflect what >>> it >>> does, I'm not convinced it's significantly better. >>> >>> Maybe dma_fence_check_hw_state_and_propagate(), though it might be >>> too long of name. Oh well, feel free to ignore this comments if a >>> majority is fine with the new name. >> Yoa, the name isn't perfect (the perfect name describing the whole >> behavior would be >> dma_fence_check_if_already_signaled_then_check_hardware_state_and_pro >> pa >> gate() ^^' >> >> My intention here is to have the reader realize "watch out, the fence >> might get signaled here!", which is probably the most important event >> regarding fences, which can race, invoke the callbacks and so on. >> >> For details readers will then check the documentation. >> >> But I'm of course open to see if there's a majority for this or that >> name. > how about: > > dma_fence_check_hw_and_signal() ? I don't think that renaming the function is a good idea in the first place. What the function does internally is an implementation detail of the framework. For the code using this function it's completely irrelevant if the function might also signal the fence, what matters for the caller is the returned status of the fence. I think this also counts for the dma_fence_is_signaled() documentation. What we should improve is the documentation of the dma_fence_ops->enable_signaling and dma_fence_ops->signaled callbacks. Especially see the comment about reference counts on enable_signaling which is missing on the signaled callback. That is most likely the root cause why nouveau implemented enable_signaling correctly but not the other one. But putting that aside I think we should make nails with heads and let the framework guarantee that the fences stay alive until they are signaled (one way or another). This completely removes the burden to keep a reference on unsignaled fences from the drivers / implementations and make things more over all more defensive. Regards, Christian. > > P. > >> P. >> >> >>> Regards, >>> >>> Boris

8 months, 1 week

1
0
0 0

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

by David Hildenbrand

On 01.04.25 12:13, Sumit Garg wrote: > + MM folks to seek guidance here. > > On Thu, Mar 27, 2025 at 09:07:34AM +0100, Jens Wiklander wrote: >> Hi Sumit, >> >> On Tue, Mar 25, 2025 at 8:42 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: >>> >>> On Wed, Mar 05, 2025 at 02:04:15PM +0100, Jens Wiklander wrote: >>>> Add support in the OP-TEE backend driver dynamic restricted memory >>>> allocation with FF-A. >>>> >>>> The restricted memory pools for dynamically allocated restrict memory >>>> are instantiated when requested by user-space. This instantiation can >>>> fail if OP-TEE doesn't support the requested use-case of restricted >>>> memory. >>>> >>>> Restricted memory pools based on a static carveout or dynamic allocation >>>> can coexist for different use-cases. We use only dynamic allocation with >>>> FF-A. >>>> >>>> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> >>>> --- >>>> drivers/tee/optee/Makefile | 1 + >>>> drivers/tee/optee/ffa_abi.c | 143 ++++++++++++- >>>> drivers/tee/optee/optee_private.h | 13 +- >>>> drivers/tee/optee/rstmem.c | 329 ++++++++++++++++++++++++++++++ >>>> 4 files changed, 483 insertions(+), 3 deletions(-) >>>> create mode 100644 drivers/tee/optee/rstmem.c >>>> > > <snip> > >>>> diff --git a/drivers/tee/optee/rstmem.c b/drivers/tee/optee/rstmem.c >>>> new file mode 100644 >>>> index 000000000000..ea27769934d4 >>>> --- /dev/null >>>> +++ b/drivers/tee/optee/rstmem.c >>>> @@ -0,0 +1,329 @@ >>>> +// SPDX-License-Identifier: GPL-2.0-only >>>> +/* >>>> + * Copyright (c) 2025, Linaro Limited >>>> + */ >>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>>> + >>>> +#include <linux/errno.h> >>>> +#include <linux/genalloc.h> >>>> +#include <linux/slab.h> >>>> +#include <linux/string.h> >>>> +#include <linux/tee_core.h> >>>> +#include <linux/types.h> >>>> +#include "optee_private.h" >>>> + >>>> +struct optee_rstmem_cma_pool { >>>> + struct tee_rstmem_pool pool; >>>> + struct gen_pool *gen_pool; >>>> + struct optee *optee; >>>> + size_t page_count; >>>> + u16 *end_points; >>>> + u_int end_point_count; >>>> + u_int align; >>>> + refcount_t refcount; >>>> + u32 use_case; >>>> + struct tee_shm *rstmem; >>>> + /* Protects when initializing and tearing down this struct */ >>>> + struct mutex mutex; >>>> +}; >>>> + >>>> +static struct optee_rstmem_cma_pool * >>>> +to_rstmem_cma_pool(struct tee_rstmem_pool *pool) >>>> +{ >>>> + return container_of(pool, struct optee_rstmem_cma_pool, pool); >>>> +} >>>> + >>>> +static int init_cma_rstmem(struct optee_rstmem_cma_pool *rp) >>>> +{ >>>> + int rc; >>>> + >>>> + rp->rstmem = tee_shm_alloc_cma_phys_mem(rp->optee->ctx, rp->page_count, >>>> + rp->align); >>>> + if (IS_ERR(rp->rstmem)) { >>>> + rc = PTR_ERR(rp->rstmem); >>>> + goto err_null_rstmem; >>>> + } >>>> + >>>> + /* >>>> + * TODO unmap the memory range since the physical memory will >>>> + * become inaccesible after the lend_rstmem() call. >>>> + */ >>> >>> What's your plan for this TODO? I think we need a CMA allocator here >>> which can allocate un-mapped memory such that any cache speculation >>> won't lead to CPU hangs once the memory restriction comes into picture. >> >> What happens is platform-specific. For some platforms, it might be >> enough to avoid explicit access. Yes, a CMA allocator with unmapped >> memory or where memory can be unmapped is one option. > > Did you get a chance to enable real memory protection on RockPi board? > This will atleast ensure that mapped restricted memory without explicit > access works fine. Since otherwise once people start to enable real > memory restriction in OP-TEE, there can be chances of random hang ups > due to cache speculation. > > MM folks, > > Basically what we are trying to achieve here is a "no-map" DT behaviour > [1] which is rather dynamic in nature. The use-case here is that a memory > block allocated from CMA can be marked restricted at runtime where we > would like the Linux not being able to directly or indirectly (cache > speculation) access it. Once memory restriction use-case has been > completed, the memory block can be marked as normal and freed for > further CMA allocation. > > It will be apprciated if you can guide us regarding the appropriate APIs > to use for un-mapping/mamping CMA allocations for this use-case. Can we get some more information why that is even required, so we can decide if that is even the right thing to do? :) Who would mark the memory block as restricted and for which purpose? In arch/powerpc/platforms/powernv/memtrace.c we have some arch-specific code to remove the directmap after alloc_contig_pages(). See memtrace_alloc_node(). But it's very arch-specific ... -- Cheers, David / dhildenb

8 months, 1 week

1
0
0 0

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

by Jens Wiklander

On Wed, Apr 9, 2025 at 9:20 AM Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> wrote: > > > > On 4/9/2025 4:41 PM, Jens Wiklander wrote: > > Hi Amirreza, > > > > On Wed, Apr 9, 2025 at 2:28 AM Amirreza Zarrabi > > <amirreza.zarrabi(a)oss.qualcomm.com> wrote: > >> > >> Hi jens, > >> > >> On 4/8/2025 10:19 PM, Jens Wiklander wrote: > >> > >> Hi Amirreza, > >> > >> On Fri, Mar 28, 2025 at 3:48 AM Amirreza Zarrabi > >> <amirreza.zarrabi(a)oss.qualcomm.com> wrote: > >> > >> For drivers that can transfer data to the TEE without using shared > >> memory from client, it is necessary to receive the user address > >> directly, bypassing any processing by the TEE subsystem. Introduce > >> TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT/OUTPUT/INOUT to represent > >> userspace buffers. > >> > >> Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> > >> --- > >> drivers/tee/tee_core.c | 33 +++++++++++++++++++++++++++++++++ > >> include/linux/tee_drv.h | 6 ++++++ > >> include/uapi/linux/tee.h | 22 ++++++++++++++++------ > >> 3 files changed, 55 insertions(+), 6 deletions(-) > >> > >> Is this patch needed now that the QCOMTEE driver supports shared > >> memory? I prefer keeping changes to the ABI to a minimum. > >> > >> Cheers, > >> Jens > >> > >> Unfortunately, this is still required. QTEE supports two types of data transfer: > >> (1) using UBUF and (2) memory objects. Even with memory object support, some APIs still > >> expect to receive data using UBUF. For instance, to load a TA, QTEE offers two interfaces: > >> one where the TA binary is in UBUF and another where the TA binary is in a memory object. > > > > Is this a limitation in the QTEE backend driver or on the secure side? > > Can it be fixed? I don't ask for changes in the ABI to the secure > > world since I assume you haven't made such changes while this patch > > set has evolved. > > > > Cheers, > > Jens > > The secure-side ABI supports passing data using memcpy to the same > buffer that contains the message for QTEE, rather than using a memory > object. Some services tend to use this approach for small data instead > of allocating a memory object. I have no choice but to expose this support. Got it, thanks! It's needed. > > Throughout the patchset, I have not made any change to the ABI but > tried to provide support for the memory object in a separate, > independent commit, distinct from the UBUF. OK Cheers, Jens > > Best regards, > Amir > > > > >> > >> Best Regards, > >> Amir > >> > >> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c > >> index 22cc7d624b0c..bc862a11d437 100644 > >> --- a/drivers/tee/tee_core.c > >> +++ b/drivers/tee/tee_core.c > >> @@ -404,6 +404,17 @@ static int params_from_user(struct tee_context *ctx, struct tee_param *params, > >> params[n].u.value.b = ip.b; > >> params[n].u.value.c = ip.c; > >> break; > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT: > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > >> + params[n].u.ubuf.uaddr = u64_to_user_ptr(ip.a); > >> + params[n].u.ubuf.size = ip.b; > >> + > >> + if (!access_ok(params[n].u.ubuf.uaddr, > >> + params[n].u.ubuf.size)) > >> + return -EFAULT; > >> + > >> + break; > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > >> @@ -472,6 +483,11 @@ static int params_to_user(struct tee_ioctl_param __user *uparams, > >> put_user(p->u.value.c, &up->c)) > >> return -EFAULT; > >> break; > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > >> + if (put_user((u64)p->u.ubuf.size, &up->b)) > >> + return -EFAULT; > >> + break; > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > >> if (put_user((u64)p->u.memref.size, &up->b)) > >> @@ -672,6 +688,13 @@ static int params_to_supp(struct tee_context *ctx, > >> ip.b = p->u.value.b; > >> ip.c = p->u.value.c; > >> break; > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT: > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > >> + ip.a = (u64)p->u.ubuf.uaddr; > >> + ip.b = p->u.ubuf.size; > >> + ip.c = 0; > >> + break; > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > >> @@ -774,6 +797,16 @@ static int params_from_supp(struct tee_param *params, size_t num_params, > >> p->u.value.b = ip.b; > >> p->u.value.c = ip.c; > >> break; > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > >> + p->u.ubuf.uaddr = u64_to_user_ptr(ip.a); > >> + p->u.ubuf.size = ip.b; > >> + > >> + if (!access_ok(params[n].u.ubuf.uaddr, > >> + params[n].u.ubuf.size)) > >> + return -EFAULT; > >> + > >> + break; > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > >> /* > >> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h > >> index ce23fd42c5d4..d773f91c6bdd 100644 > >> --- a/include/linux/tee_drv.h > >> +++ b/include/linux/tee_drv.h > >> @@ -82,6 +82,11 @@ struct tee_param_memref { > >> struct tee_shm *shm; > >> }; > >> > >> +struct tee_param_ubuf { > >> + void * __user uaddr; > >> + size_t size; > >> +}; > >> + > >> struct tee_param_value { > >> u64 a; > >> u64 b; > >> @@ -92,6 +97,7 @@ struct tee_param { > >> u64 attr; > >> union { > >> struct tee_param_memref memref; > >> + struct tee_param_ubuf ubuf; > >> struct tee_param_value value; > >> } u; > >> }; > >> diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h > >> index d0430bee8292..3e9b1ec5dfde 100644 > >> --- a/include/uapi/linux/tee.h > >> +++ b/include/uapi/linux/tee.h > >> @@ -151,6 +151,13 @@ struct tee_ioctl_buf_data { > >> #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT 6 > >> #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT 7 /* input and output */ > >> > >> +/* > >> + * These defines userspace buffer parameters. > >> + */ > >> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT 8 > >> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT 9 > >> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT 10 /* input and output */ > >> + > >> /* > >> * Mask for the type part of the attribute, leaves room for more types > >> */ > >> @@ -186,14 +193,17 @@ struct tee_ioctl_buf_data { > >> /** > >> * struct tee_ioctl_param - parameter > >> * @attr: attributes > >> - * @a: if a memref, offset into the shared memory object, else a value parameter > >> - * @b: if a memref, size of the buffer, else a value parameter > >> + * @a: if a memref, offset into the shared memory object, > >> + * else if a ubuf, address of the user buffer, > >> + * else a value parameter > >> + * @b: if a memref or ubuf, size of the buffer, else a value parameter > >> * @c: if a memref, shared memory identifier, else a value parameter > >> * > >> - * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref or value is used in > >> - * the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value and > >> - * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref. TEE_PARAM_ATTR_TYPE_NONE > >> - * indicates that none of the members are used. > >> + * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref, ubuf, or value is > >> + * used in the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value, > >> + * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref, and TEE_PARAM_ATTR_TYPE_UBUF_* > >> + * indicates ubuf. TEE_PARAM_ATTR_TYPE_NONE indicates that none of the members > >> + * are used. > >> * > >> * Shared memory is allocated with TEE_IOC_SHM_ALLOC which returns an > >> * identifier representing the shared memory object. A memref can reference > >> > >> -- > >> 2.34.1 > >> >

8 months, 1 week

1
0
0 0

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

by Jens Wiklander

Hi Amirreza, On Wed, Apr 9, 2025 at 2:28 AM Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> wrote: > > Hi jens, > > On 4/8/2025 10:19 PM, Jens Wiklander wrote: > > Hi Amirreza, > > On Fri, Mar 28, 2025 at 3:48 AM Amirreza Zarrabi > <amirreza.zarrabi(a)oss.qualcomm.com> wrote: > > For drivers that can transfer data to the TEE without using shared > memory from client, it is necessary to receive the user address > directly, bypassing any processing by the TEE subsystem. Introduce > TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT/OUTPUT/INOUT to represent > userspace buffers. > > Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> > --- > drivers/tee/tee_core.c | 33 +++++++++++++++++++++++++++++++++ > include/linux/tee_drv.h | 6 ++++++ > include/uapi/linux/tee.h | 22 ++++++++++++++++------ > 3 files changed, 55 insertions(+), 6 deletions(-) > > Is this patch needed now that the QCOMTEE driver supports shared > memory? I prefer keeping changes to the ABI to a minimum. > > Cheers, > Jens > > Unfortunately, this is still required. QTEE supports two types of data transfer: > (1) using UBUF and (2) memory objects. Even with memory object support, some APIs still > expect to receive data using UBUF. For instance, to load a TA, QTEE offers two interfaces: > one where the TA binary is in UBUF and another where the TA binary is in a memory object. Is this a limitation in the QTEE backend driver or on the secure side? Can it be fixed? I don't ask for changes in the ABI to the secure world since I assume you haven't made such changes while this patch set has evolved. Cheers, Jens > > Best Regards, > Amir > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c > index 22cc7d624b0c..bc862a11d437 100644 > --- a/drivers/tee/tee_core.c > +++ b/drivers/tee/tee_core.c > @@ -404,6 +404,17 @@ static int params_from_user(struct tee_context *ctx, struct tee_param *params, > params[n].u.value.b = ip.b; > params[n].u.value.c = ip.c; > break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > + params[n].u.ubuf.uaddr = u64_to_user_ptr(ip.a); > + params[n].u.ubuf.size = ip.b; > + > + if (!access_ok(params[n].u.ubuf.uaddr, > + params[n].u.ubuf.size)) > + return -EFAULT; > + > + break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > @@ -472,6 +483,11 @@ static int params_to_user(struct tee_ioctl_param __user *uparams, > put_user(p->u.value.c, &up->c)) > return -EFAULT; > break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > + if (put_user((u64)p->u.ubuf.size, &up->b)) > + return -EFAULT; > + break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > if (put_user((u64)p->u.memref.size, &up->b)) > @@ -672,6 +688,13 @@ static int params_to_supp(struct tee_context *ctx, > ip.b = p->u.value.b; > ip.c = p->u.value.c; > break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > + ip.a = (u64)p->u.ubuf.uaddr; > + ip.b = p->u.ubuf.size; > + ip.c = 0; > + break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > @@ -774,6 +797,16 @@ static int params_from_supp(struct tee_param *params, size_t num_params, > p->u.value.b = ip.b; > p->u.value.c = ip.c; > break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > + p->u.ubuf.uaddr = u64_to_user_ptr(ip.a); > + p->u.ubuf.size = ip.b; > + > + if (!access_ok(params[n].u.ubuf.uaddr, > + params[n].u.ubuf.size)) > + return -EFAULT; > + > + break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > /* > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h > index ce23fd42c5d4..d773f91c6bdd 100644 > --- a/include/linux/tee_drv.h > +++ b/include/linux/tee_drv.h > @@ -82,6 +82,11 @@ struct tee_param_memref { > struct tee_shm *shm; > }; > > +struct tee_param_ubuf { > + void * __user uaddr; > + size_t size; > +}; > + > struct tee_param_value { > u64 a; > u64 b; > @@ -92,6 +97,7 @@ struct tee_param { > u64 attr; > union { > struct tee_param_memref memref; > + struct tee_param_ubuf ubuf; > struct tee_param_value value; > } u; > }; > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h > index d0430bee8292..3e9b1ec5dfde 100644 > --- a/include/uapi/linux/tee.h > +++ b/include/uapi/linux/tee.h > @@ -151,6 +151,13 @@ struct tee_ioctl_buf_data { > #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT 6 > #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT 7 /* input and output */ > > +/* > + * These defines userspace buffer parameters. > + */ > +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT 8 > +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT 9 > +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT 10 /* input and output */ > + > /* > * Mask for the type part of the attribute, leaves room for more types > */ > @@ -186,14 +193,17 @@ struct tee_ioctl_buf_data { > /** > * struct tee_ioctl_param - parameter > * @attr: attributes > - * @a: if a memref, offset into the shared memory object, else a value parameter > - * @b: if a memref, size of the buffer, else a value parameter > + * @a: if a memref, offset into the shared memory object, > + * else if a ubuf, address of the user buffer, > + * else a value parameter > + * @b: if a memref or ubuf, size of the buffer, else a value parameter > * @c: if a memref, shared memory identifier, else a value parameter > * > - * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref or value is used in > - * the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value and > - * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref. TEE_PARAM_ATTR_TYPE_NONE > - * indicates that none of the members are used. > + * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref, ubuf, or value is > + * used in the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value, > + * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref, and TEE_PARAM_ATTR_TYPE_UBUF_* > + * indicates ubuf. TEE_PARAM_ATTR_TYPE_NONE indicates that none of the members > + * are used. > * > * Shared memory is allocated with TEE_IOC_SHM_ALLOC which returns an > * identifier representing the shared memory object. A memref can reference > > -- > 2.34.1 >

8 months, 1 week

1
0
0 0

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

by Jens Wiklander

On Tue, Apr 8, 2025 at 11:20 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, Apr 01, 2025 at 02:26:59PM +0200, Jens Wiklander wrote: > > On Tue, Apr 1, 2025 at 12:13 PM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > + MM folks to seek guidance here. > > > > > > On Thu, Mar 27, 2025 at 09:07:34AM +0100, Jens Wiklander wrote: > > > > Hi Sumit, > > > > > > > > On Tue, Mar 25, 2025 at 8:42 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > > > > > On Wed, Mar 05, 2025 at 02:04:15PM +0100, Jens Wiklander wrote: > > > > > > Add support in the OP-TEE backend driver dynamic restricted memory > > > > > > allocation with FF-A. > > > > > > > > > > > > The restricted memory pools for dynamically allocated restrict memory > > > > > > are instantiated when requested by user-space. This instantiation can > > > > > > fail if OP-TEE doesn't support the requested use-case of restricted > > > > > > memory. > > > > > > > > > > > > Restricted memory pools based on a static carveout or dynamic allocation > > > > > > can coexist for different use-cases. We use only dynamic allocation with > > > > > > FF-A. > > > > > > > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > > > > > --- > > > > > > drivers/tee/optee/Makefile | 1 + > > > > > > drivers/tee/optee/ffa_abi.c | 143 ++++++++++++- > > > > > > drivers/tee/optee/optee_private.h | 13 +- > > > > > > drivers/tee/optee/rstmem.c | 329 ++++++++++++++++++++++++++++++ > > > > > > 4 files changed, 483 insertions(+), 3 deletions(-) > > > > > > create mode 100644 drivers/tee/optee/rstmem.c > > > > > > > > > > > > <snip> > > > > > > > > > diff --git a/drivers/tee/optee/rstmem.c b/drivers/tee/optee/rstmem.c > > > > > > new file mode 100644 > > > > > > index 000000000000..ea27769934d4 > > > > > > --- /dev/null > > > > > > +++ b/drivers/tee/optee/rstmem.c > > > > > > @@ -0,0 +1,329 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0-only > > > > > > +/* > > > > > > + * Copyright (c) 2025, Linaro Limited > > > > > > + */ > > > > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > > > + > > > > > > +#include <linux/errno.h> > > > > > > +#include <linux/genalloc.h> > > > > > > +#include <linux/slab.h> > > > > > > +#include <linux/string.h> > > > > > > +#include <linux/tee_core.h> > > > > > > +#include <linux/types.h> > > > > > > +#include "optee_private.h" > > > > > > + > > > > > > +struct optee_rstmem_cma_pool { > > > > > > + struct tee_rstmem_pool pool; > > > > > > + struct gen_pool *gen_pool; > > > > > > + struct optee *optee; > > > > > > + size_t page_count; > > > > > > + u16 *end_points; > > > > > > + u_int end_point_count; > > > > > > + u_int align; > > > > > > + refcount_t refcount; > > > > > > + u32 use_case; > > > > > > + struct tee_shm *rstmem; > > > > > > + /* Protects when initializing and tearing down this struct */ > > > > > > + struct mutex mutex; > > > > > > +}; > > > > > > + > > > > > > +static struct optee_rstmem_cma_pool * > > > > > > +to_rstmem_cma_pool(struct tee_rstmem_pool *pool) > > > > > > +{ > > > > > > + return container_of(pool, struct optee_rstmem_cma_pool, pool); > > > > > > +} > > > > > > + > > > > > > +static int init_cma_rstmem(struct optee_rstmem_cma_pool *rp) > > > > > > +{ > > > > > > + int rc; > > > > > > + > > > > > > + rp->rstmem = tee_shm_alloc_cma_phys_mem(rp->optee->ctx, rp->page_count, > > > > > > + rp->align); > > > > > > + if (IS_ERR(rp->rstmem)) { > > > > > > + rc = PTR_ERR(rp->rstmem); > > > > > > + goto err_null_rstmem; > > > > > > + } > > > > > > + > > > > > > + /* > > > > > > + * TODO unmap the memory range since the physical memory will > > > > > > + * become inaccesible after the lend_rstmem() call. > > > > > > + */ > > > > > > > > > > What's your plan for this TODO? I think we need a CMA allocator here > > > > > which can allocate un-mapped memory such that any cache speculation > > > > > won't lead to CPU hangs once the memory restriction comes into picture. > > > > > > > > What happens is platform-specific. For some platforms, it might be > > > > enough to avoid explicit access. Yes, a CMA allocator with unmapped > > > > memory or where memory can be unmapped is one option. > > > > > > Did you get a chance to enable real memory protection on RockPi board? > > > > No, I don't think I have access to the needed documentation for the > > board to set it up for relevant peripherals. > > > > > This will atleast ensure that mapped restricted memory without explicit > > > access works fine. Since otherwise once people start to enable real > > > memory restriction in OP-TEE, there can be chances of random hang ups > > > due to cache speculation. > > > > A hypervisor in the normal world can also make the memory inaccessible > > to the kernel. That shouldn't cause any hangups due to cache > > speculation. > > The hypervisor should unmap the memory from EL2 translation tables which > I think should disallow the cache speculation to take place. However, > without hypervisor here the memory remains mapped in normal world which > can lead to cache speculation for restricted buffers. That's why we > should atleast test on one platform with real memory protection enabled > to rule out any assumptions we make. Do I hear a volunteer? ;-) Anyway, this isn't something that can be enabled in the kernel alone. Only platforms where the firmware has been updated will be affected. If this can't be supported on a particular platform, there's still the option with a static carveout. Cheers, Jens > > -Sumit > > > > > Cheers, > > Jens > > > > > > > > MM folks, > > > > > > Basically what we are trying to achieve here is a "no-map" DT behaviour > > > [1] which is rather dynamic in nature. The use-case here is that a memory > > > block allocated from CMA can be marked restricted at runtime where we > > > would like the Linux not being able to directly or indirectly (cache > > > speculation) access it. Once memory restriction use-case has been > > > completed, the memory block can be marked as normal and freed for > > > further CMA allocation. > > > > > > It will be apprciated if you can guide us regarding the appropriate APIs > > > to use for un-mapping/mamping CMA allocations for this use-case. > > > > > > [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/rese… > > > > > > -Sumit

8 months, 1 week

1
0
0 0

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

by Jens Wiklander

On Tue, Apr 8, 2025 at 11:14 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, Apr 01, 2025 at 10:33:04AM +0200, Jens Wiklander wrote: > > On Tue, Apr 1, 2025 at 9:58 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > On Tue, Mar 25, 2025 at 11:55:46AM +0100, Jens Wiklander wrote: > > > > Hi Sumit, > > > > > > > > > > <snip> > > > > > > > > > > > > > > > > > > + > > > > > > +#include "tee_private.h" > > > > > > + > > > > > > +struct tee_dma_heap { > > > > > > + struct dma_heap *heap; > > > > > > + enum tee_dma_heap_id id; > > > > > > + struct tee_rstmem_pool *pool; > > > > > > + struct tee_device *teedev; > > > > > > + /* Protects pool and teedev above */ > > > > > > + struct mutex mu; > > > > > > +}; > > > > > > + > > > > > > +struct tee_heap_buffer { > > > > > > + struct tee_rstmem_pool *pool; > > > > > > + struct tee_device *teedev; > > > > > > + size_t size; > > > > > > + size_t offs; > > > > > > + struct sg_table table; > > > > > > +}; > > > > > > + > > > > > > +struct tee_heap_attachment { > > > > > > + struct sg_table table; > > > > > > + struct device *dev; > > > > > > +}; > > > > > > + > > > > > > +struct tee_rstmem_static_pool { > > > > > > + struct tee_rstmem_pool pool; > > > > > > + struct gen_pool *gen_pool; > > > > > > + phys_addr_t pa_base; > > > > > > +}; > > > > > > + > > > > > > +#if !IS_MODULE(CONFIG_TEE) && IS_ENABLED(CONFIG_DMABUF_HEAPS) > > > > > > > > > > Can this dependency rather be better managed via Kconfig? > > > > > > > > This was the easiest yet somewhat flexible solution I could find. If > > > > you have something better, let's use that instead. > > > > > > > > > > --- a/drivers/tee/optee/Kconfig > > > +++ b/drivers/tee/optee/Kconfig > > > @@ -5,6 +5,7 @@ config OPTEE > > > depends on HAVE_ARM_SMCCC > > > depends on MMU > > > depends on RPMB || !RPMB > > > + select DMABUF_HEAPS > > > help > > > This implements the OP-TEE Trusted Execution Environment (TEE) > > > driver. > > > > I wanted to avoid that since there are plenty of use cases where > > DMABUF_HEAPS aren't needed. > > Yeah, but how the users will figure out the dependency to enable DMA > heaps with TEE subsystem. I hope, without too much difficulty. They are after all looking for a way to allocate memory from a DMA heap. > So it's better we provide a generic kernel > Kconfig which enables all the default features. I disagree, it should be possible to configure without DMABUF_HEAPS if desired. > > > This seems to do the job: > > +config TEE_DMABUF_HEAP > > + bool > > + depends on TEE = y && DMABUF_HEAPS > > > > We can only use DMABUF_HEAPS if the TEE subsystem is compiled into the kernel. > > Ah, I see. So we aren't exporting the DMA heaps APIs for TEE subsystem > to use. We should do that such that there isn't a hard dependency to > compile them into the kernel. I was saving that for a later patch set as a later problem. We may save some time by not doing it now. Cheers, Jens > > -Sumit > > > > > Cheers, > > Jens

8 months, 1 week

1
0
0 0

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

by Jens Wiklander

Hi Amirreza, On Fri, Mar 28, 2025 at 3:48 AM Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> wrote: > > Introduce qcomtee_object, which represents an object in both QTEE and > the kernel. QTEE clients can invoke an instance of qcomtee_object to > access QTEE services. If this invocation produces a new object in QTEE, > an instance of qcomtee_object will be returned. > > Similarly, QTEE can request services from the kernel by issuing a callback > request, which invokes an instance of qcomtee_object in the kernel. > Any subsystem that exposes a service to QTEE should allocate and initialize > an instance of qcomtee_object with a dispatcher callback that is called > when the object is invoked. > > Implement initial support for exporting qcomtee_object to userspace > and QTEE, enabling the invocation of objects hosted in QTEE and userspace > through the TEE subsystem. > > Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> > --- > drivers/tee/Kconfig | 1 + > drivers/tee/Makefile | 1 + > drivers/tee/qcomtee/Kconfig | 10 + > drivers/tee/qcomtee/Makefile | 9 + > drivers/tee/qcomtee/async.c | 160 +++++++ > drivers/tee/qcomtee/call.c | 753 +++++++++++++++++++++++++++++++ > drivers/tee/qcomtee/core.c | 801 +++++++++++++++++++++++++++++++++ > drivers/tee/qcomtee/qcom_scm.c | 38 ++ > drivers/tee/qcomtee/qcomtee_msg.h | 239 ++++++++++ > drivers/tee/qcomtee/qcomtee_private.h | 222 +++++++++ > drivers/tee/qcomtee/release.c | 48 ++ > drivers/tee/qcomtee/shm.c | 149 ++++++ > drivers/tee/qcomtee/user_obj.c | 710 +++++++++++++++++++++++++++++ > include/linux/firmware/qcom/qcom_tee.h | 302 +++++++++++++ > include/uapi/linux/tee.h | 1 + > 15 files changed, 3444 insertions(+) > > diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig > index 61b507c18780..3a995d7f0d74 100644 > --- a/drivers/tee/Kconfig > +++ b/drivers/tee/Kconfig > @@ -16,5 +16,6 @@ if TEE > source "drivers/tee/optee/Kconfig" > source "drivers/tee/amdtee/Kconfig" > source "drivers/tee/tstee/Kconfig" > +source "drivers/tee/qcomtee/Kconfig" > > endif > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile > index 5488cba30bd2..74e987f8f7ea 100644 > --- a/drivers/tee/Makefile > +++ b/drivers/tee/Makefile > @@ -6,3 +6,4 @@ tee-objs += tee_shm_pool.o > obj-$(CONFIG_OPTEE) += optee/ > obj-$(CONFIG_AMDTEE) += amdtee/ > obj-$(CONFIG_ARM_TSTEE) += tstee/ > +obj-$(CONFIG_QCOMTEE) += qcomtee/ > diff --git a/drivers/tee/qcomtee/Kconfig b/drivers/tee/qcomtee/Kconfig > new file mode 100644 > index 000000000000..d180a6d07d33 > --- /dev/null > +++ b/drivers/tee/qcomtee/Kconfig > @@ -0,0 +1,10 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +# Qualcomm Trusted Execution Environment Configuration > +config QCOMTEE > + tristate "Qualcomm TEE Support" > + select QCOM_SCM > + help > + This option enables the Qualcomm Trusted Execution Environment (QTEE) > + driver. It provides an API to access services offered by QTEE and any > + loaded Trusted Applications (TAs), as well as exporting kernel > + services to QTEE. > diff --git a/drivers/tee/qcomtee/Makefile b/drivers/tee/qcomtee/Makefile > new file mode 100644 > index 000000000000..1b14b943e5f5 > --- /dev/null > +++ b/drivers/tee/qcomtee/Makefile > @@ -0,0 +1,9 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +obj-$(CONFIG_QCOMTEE) += qcomtee.o > +qcomtee-objs += async.o > +qcomtee-objs += call.o > +qcomtee-objs += core.o > +qcomtee-objs += qcom_scm.o > +qcomtee-objs += release.o > +qcomtee-objs += shm.o > +qcomtee-objs += user_obj.o > diff --git a/drivers/tee/qcomtee/async.c b/drivers/tee/qcomtee/async.c > new file mode 100644 > index 000000000000..4c880c3441a2 > --- /dev/null > +++ b/drivers/tee/qcomtee/async.c > @@ -0,0 +1,160 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include "qcomtee_private.h" > +#include "qcomtee_msg.h" > + > +#define QCOMTEE_ASYNC_VERSION_1_0 0x00010000U /* Maj: 0x0001, Min: 0x0000. */ > +#define QCOMTEE_ASYNC_VERSION_1_1 0x00010001U /* Maj: 0x0001, Min: 0x0001. */ > +#define QCOMTEE_ASYNC_VERSION_1_2 0x00010002U /* Maj: 0x0001, Min: 0x0002. */ > +#define QCOMTEE_ASYNC_VERSION QCOMTEE_ASYNC_VERSION_1_2 /* Current Version. */ > + > +#define QCOMTEE_ASYNC_VERSION_MAJOR(n) upper_16_bits(n) > +#define QCOMTEE_ASYNC_VERSION_MINOR(n) lower_16_bits(n) > + > +/** > + * struct qcomtee_async_msg_hdr - Asynchronous message header format. > + * @version: current async protocol version of the remote endpoint. > + * @op: async operation. > + * > + * @version specifies the endpoint's (QTEE or driver) supported async protocol. > + * For example, if QTEE sets @version to %QCOMTEE_ASYNC_VERSION_1_1, QTEE > + * handles operations supported in %QCOMTEE_ASYNC_VERSION_1_1 or > + * %QCOMTEE_ASYNC_VERSION_1_0. @op determines the message format. > + */ > +struct qcomtee_async_msg_hdr { > + u32 version; > + u32 op; > +}; > + > +/* Size of an empty async message. */ > +#define QCOMTEE_ASYNC_MSG_ZERO sizeof(struct qcomtee_async_msg_hdr) > + > +/** > + * struct qcomtee_async_release_msg - Release asynchronous message. > + * @hdr: message header as &struct qcomtee_async_msg_hdr. > + * @counts: number of objects in @object_ids. > + * @object_ids: array of object IDs that should be released. > + * > + * Available in Maj = 0x0001, Min >= 0x0000. > + */ > +struct qcomtee_async_release_msg { > + struct qcomtee_async_msg_hdr hdr; > + u32 counts; > + u32 object_ids[] __counted_by(counts); > +}; > + > +/** > + * qcomtee_get_async_buffer() - Get the start of the asynchronous message. > + * @oic: context used for the current invocation. > + * @async_buffer: return buffer to extract from or fill in async messages. > + * > + * If @oic is used for direct object invocation, the whole outbound buffer > + * is available for the async message. If @oic is used for a callback request, > + * the tail of the outbound buffer (after the callback request message) is > + * available for the async message. > + * > + * The start of the async buffer is aligned, see qcomtee_msg_offset_align(). > + */ > +static void qcomtee_get_async_buffer(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_buffer *async_buffer) > +{ > + struct qcomtee_msg_callback *msg; > + unsigned int offset; > + int i; > + > + if (!(oic->flags & QCOMTEE_OIC_FLAG_BUSY)) { > + /* The outbound buffer is empty. Using the whole buffer. */ > + offset = 0; > + } else { > + msg = (struct qcomtee_msg_callback *)oic->out_msg.addr; > + > + /* Start offset in a message for buffer arguments. */ > + offset = qcomtee_msg_buffer_args(struct qcomtee_msg_callback, > + qcomtee_msg_args(msg)); > + > + /* Add size of IB arguments. */ > + qcomtee_msg_for_each_input_buffer(i, msg) > + offset += qcomtee_msg_offset_align(msg->args[i].b.size); > + > + /* Add size of OB arguments. */ > + qcomtee_msg_for_each_output_buffer(i, msg) > + offset += qcomtee_msg_offset_align(msg->args[i].b.size); > + } > + > + async_buffer->addr = oic->out_msg.addr + offset; > + async_buffer->size = oic->out_msg.size - offset; > +} > + > +/** > + * async_release() - Process QTEE async release requests. > + * @oic: context used for the current invocation. > + * @msg: async message for object release. > + * @size: size of the async buffer available. > + * > + * Return: Size of the outbound buffer used when processing @msg. > + */ > +static size_t async_release(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_async_msg_hdr *async_msg, > + size_t size) > +{ > + struct qcomtee_async_release_msg *msg; > + struct qcomtee_object *object; > + int i; > + > + msg = (struct qcomtee_async_release_msg *)async_msg; > + > + for (i = 0; i < msg->counts; i++) { > + object = qcomtee_idx_erase(msg->object_ids[i]); > + qcomtee_object_put(object); > + } > + > + return struct_size(msg, object_ids, msg->counts); > +} > + > +/** > + * qcomtee_fetch_async_reqs() - Fetch and process asynchronous messages. > + * @oic: context used for the current invocation. > + * > + * Calls handlers to process the requested operations in the async message. > + * Currently, only supports async release requests. > + */ > +void qcomtee_fetch_async_reqs(struct qcomtee_object_invoke_ctx *oic) > +{ > + struct qcomtee_async_msg_hdr *async_msg; > + struct qcomtee_buffer async_buffer; > + size_t consumed, used = 0; > + > + qcomtee_get_async_buffer(oic, &async_buffer); > + > + while (async_buffer.size - used > QCOMTEE_ASYNC_MSG_ZERO) { > + async_msg = (struct qcomtee_async_msg_hdr *)(async_buffer.addr + > + used); > + > + if (QCOMTEE_ASYNC_VERSION_MAJOR(async_msg->version) != > + QCOMTEE_ASYNC_VERSION_MAJOR(QCOMTEE_ASYNC_VERSION)) > + goto out; > + > + switch (async_msg->op) { > + case QCOMTEE_MSG_OBJECT_OP_RELEASE: > + consumed = async_release(oic, async_msg, > + async_buffer.size - used); > + break; > + default: > + goto out; > + } > + > + /* Supported operation but unable to parse the message. */ > + if (!consumed) > + goto out; > + > + /* Next async message. */ > + used += qcomtee_msg_offset_align(consumed); > + } > + > +out: > + /* Reset the async buffer so async requests do not loop to QTEE. */ > + memzero_explicit(async_buffer.addr, async_buffer.size); > +} > diff --git a/drivers/tee/qcomtee/call.c b/drivers/tee/qcomtee/call.c > new file mode 100644 > index 000000000000..f7f5c2c0bebb > --- /dev/null > +++ b/drivers/tee/qcomtee/call.c > @@ -0,0 +1,753 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/slab.h> > +#include <linux/tee.h> > +#include <linux/platform_device.h> > + > +#include "qcomtee_private.h" > + > +static int find_qtee_object(struct qcomtee_object **object, unsigned long id, > + struct qcomtee_context_data *ctxdata) > +{ > + int err = 0; > + > + guard(rcu)(); Does the qcomtee_object_get() call need to be RCU protected? > + /* Object release is RCU protected. */ > + *object = idr_find(&ctxdata->qtee_objects_idr, id); > + if (!qcomtee_object_get(*object)) > + err = -EINVAL; > + > + return err; > +} > + > +static void del_qtee_object(unsigned long id, > + struct qcomtee_context_data *ctxdata) > +{ > + struct qcomtee_object *object; > + > + scoped_guard(spinlock, &ctxdata->qtee_lock) > + object = idr_remove(&ctxdata->qtee_objects_idr, id); > + > + qcomtee_object_put(object); > +} > + > +/** > + * qcomtee_context_add_qtee_object() - Add a QTEE object to the context. > + * @param: TEE parameter representing @object. > + * @object: QTEE object. > + * @ctx: context to add the object. > + * > + * It assumes @object is %QCOMTEE_OBJECT_TYPE_TEE and the caller has already > + * issued qcomtee_object_get() for @object. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_context_add_qtee_object(struct tee_param *param, > + struct qcomtee_object *object, > + struct tee_context *ctx) > +{ > + int ret; > + struct qcomtee_context_data *ctxdata = ctx->data; > + > + guard(spinlock)(&ctxdata->qtee_lock); > + ret = idr_alloc(&ctxdata->qtee_objects_idr, object, 0, 0, GFP_KERNEL); > + if (ret < 0) > + return ret; > + Is the spinlock needed when updating param below? > + param->u.objref.id = ret; > + /* QTEE Object: QCOMTEE_OBJREF_FLAG_TEE set. */ > + param->u.objref.flags = QCOMTEE_OBJREF_FLAG_TEE; > + > + return 0; > +} > + > +/* Retrieve the QTEE object added with qcomtee_context_add_qtee_object(). */ > +int qcomtee_context_find_qtee_object(struct qcomtee_object **object, > + struct tee_param *param, > + struct tee_context *ctx) > +{ > + struct qcomtee_context_data *ctxdata = ctx->data; > + > + /* 'qtee_objects_idr' stores QTEE objects only. */ > + if (!(param->u.objref.flags & QCOMTEE_OBJREF_FLAG_TEE)) > + return -EINVAL; > + > + return find_qtee_object(object, param->u.objref.id, ctxdata); > +} > + > +/** > + * qcomtee_context_del_qtee_object() - Delete a QTEE object from the context. > + * @param: TEE parameter representing @object. > + * @ctx: context for deleting the object. > + * > + * The @param has been initialized by qcomtee_context_add_qtee_object(). > + */ > +void qcomtee_context_del_qtee_object(struct tee_param *param, > + struct tee_context *ctx) > +{ > + struct qcomtee_context_data *ctxdata = ctx->data; > + > + /* 'qtee_objects_idr' stores QTEE objects only. */ > + if (param->u.objref.flags & QCOMTEE_OBJREF_FLAG_TEE) > + del_qtee_object(param->u.objref.id, ctxdata); > +} > + > +/** > + * qcomtee_objref_to_arg() - Convert OBJREF parameter to QTEE argument. > + * @arg: QTEE argument. > + * @param: TEE parameter. > + * @ctx: context in which the conversion should happen. > + * > + * It assumes @param is an OBJREF. > + * It does not set @arg.type; the caller should initialize it to a correct > + * &enum qcomtee_arg_type value. It gets the object's refcount in @arg; > + * the caller should manage to put it afterward. > + * > + * For objects that are not in QTEE (i.e. !(param->u.objref.flags & > + * QCOMTEE_OBJREF_FLAG_TEE)), it also calls qcomtee_object_get() to keep a > + * temporary copy for the driver as the release of them are asynchronous and > + * may go away even before returning from the invocation. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_objref_to_arg(struct qcomtee_arg *arg, struct tee_param *param, > + struct tee_context *ctx) > +{ > + struct qcomtee_object *object; > + int err; > + > + /* param is a NULL object: */ > + if (param->u.objref.id == TEE_OBJREF_NULL) { > + arg->o = NULL_QCOMTEE_OBJECT; > + > + return 0; > + } > + > + /* param is a callback object: */ > + if (param->u.objref.flags & QCOMTEE_OBJREF_FLAG_USER) { > + err = qcomtee_user_param_to_object(&object, param, ctx); How about a qcomtee_get_object_from_user_param() that already has increased the refcount? That should balance better with qcomtee_context_find_qtee_object(). > + if (!err) > + qcomtee_object_get(object); > + /* param is a QTEE object: */ > + } else if (param->u.objref.flags & QCOMTEE_OBJREF_FLAG_TEE) { > + err = qcomtee_context_find_qtee_object(&object, param, ctx); > + } else { > + err = -EINVAL; > + } > + > + arg->o = err ? NULL_QCOMTEE_OBJECT : object; I prefer an if statement here instead of the ternary operator > + > + return err; > +} > + > +/** > + * qcomtee_objref_from_arg() - Convert QTEE argument to OBJREF param. > + * @param: TEE parameter. > + * @arg: QTEE argument. > + * @ctx: context in which the conversion should happen. > + * > + * It assumes @arg is of %QCOMTEE_ARG_TYPE_IO or %QCOMTEE_ARG_TYPE_OO. > + * It does not set @param.attr; the caller should initialize it to a > + * correct type. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_objref_from_arg(struct tee_param *param, struct qcomtee_arg *arg, > + struct tee_context *ctx) > +{ > + struct qcomtee_object *object; > + int err; > + > + /* param should be of OBJREF. */ > + if (param->attr != TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_INPUT && > + param->attr != TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_OUTPUT && > + param->attr != TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_INOUT) > + return -EINVAL; > + > + object = arg->o; > + > + switch (typeof_qcomtee_object(object)) { > + case QCOMTEE_OBJECT_TYPE_NULL: > + param->u.objref.id = TEE_OBJREF_NULL; > + err = 0; > + > + break; > + case QCOMTEE_OBJECT_TYPE_CB: > + if (is_qcomtee_user_object(object)) > + err = qcomtee_user_param_from_object(param, object, > + ctx); > + else > + err = -EINVAL; > + > + break; > + case QCOMTEE_OBJECT_TYPE_TEE: > + err = qcomtee_context_add_qtee_object(param, object, ctx); > + > + break; > + case QCOMTEE_OBJECT_TYPE_ROOT: > + default: > + return -EINVAL; > + } > + > + return err; > +} > + > +/** > + * qcomtee_params_to_args() - Convert TEE parameters to QTEE arguments. > + * @u: QTEE arguments. > + * @params: TEE parameters. > + * @num_params: number of elements in the parameter array. > + * @ctx: context in which the conversion should happen. > + * > + * It assumes @u has at least @num_params + 1 entries and has been initialized > + * with %QCOMTEE_ARG_TYPE_INV as &struct qcomtee_arg.type. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +static int qcomtee_params_to_args(struct qcomtee_arg *u, > + struct tee_param *params, int num_params, > + struct tee_context *ctx) > +{ > + int i; > + > + for (i = 0; i < num_params; i++) { > + switch (params[i].attr) { > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + u[i].flags = QCOMTEE_ARG_FLAGS_UADDR; > + u[i].b.uaddr = params[i].u.ubuf.uaddr; > + u[i].b.size = params[i].u.ubuf.size; > + > + if (params[i].attr == > + TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT) > + u[i].type = QCOMTEE_ARG_TYPE_IB; > + else /* TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT */ > + u[i].type = QCOMTEE_ARG_TYPE_OB; > + > + break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_INPUT: > + u[i].type = QCOMTEE_ARG_TYPE_IO; > + if (qcomtee_objref_to_arg(&u[i], &params[i], ctx)) > + goto out_failed; > + > + break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_OUTPUT: > + u[i].type = QCOMTEE_ARG_TYPE_OO; > + u[i].o = NULL_QCOMTEE_OBJECT; > + break; > + default: > + goto out_failed; > + } > + } > + > + return 0; > + > +out_failed: > + /* Undo qcomtee_objref_to_arg(). */ > + for (i--; i >= 0; i--) { > + if (u[i].type != QCOMTEE_ARG_TYPE_IO) > + continue; > + > + qcomtee_user_object_set_notify(u[i].o, false); > + if (typeof_qcomtee_object(u[i].o) == QCOMTEE_OBJECT_TYPE_CB) > + qcomtee_object_put(u[i].o); > + > + qcomtee_object_put(u[i].o); > + } > + > + return -EINVAL; > +} > + > +/** > + * qcomtee_params_from_args() - Convert QTEE arguments to TEE parameters. > + * @params: TEE parameters. > + * @u: QTEE arguments. > + * @num_params: number of elements in the parameter array. > + * @ctx: context in which the conversion should happen. > + * > + * @u should have already been initialized by qcomtee_params_to_args(). > + * This also represents the end of a QTEE invocation that started with > + * qcomtee_params_to_args() by releasing %QCOMTEE_ARG_TYPE_IO objects. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +static int qcomtee_params_from_args(struct tee_param *params, > + struct qcomtee_arg *u, int num_params, > + struct tee_context *ctx) > +{ > + int i, np; > + > + qcomtee_arg_for_each(np, u) { > + if (u[np].type == QCOMTEE_ARG_TYPE_OB) { Can we use switch(u[np].type)? > + /* TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT */ > + params[np].u.value.b = u[np].b.size; Should this be params[np].u.ubuf.size? > + > + } else if (u[np].type == QCOMTEE_ARG_TYPE_IO) { > + /* IEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_INPUT */ > + qcomtee_object_put(u[np].o); > + > + } else if (u[np].type == QCOMTEE_ARG_TYPE_OO) { > + /* TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_OUTPUT */ > + if (qcomtee_objref_from_arg(&params[np], &u[np], ctx)) > + goto out_failed; > + } > + } > + > + return 0; > + > +out_failed: > + /* Undo qcomtee_objref_from_arg(). */ > + for (i = 0; i < np; i++) { > + if (params[i].attr == TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_OUTPUT) > + qcomtee_context_del_qtee_object(&params[i], ctx); > + } > + > + /* Release any IO and OO objects not processed. */ > + for (; u[i].type; i++) { > + if (u[i].type == QCOMTEE_ARG_TYPE_OO || > + u[i].type == QCOMTEE_ARG_TYPE_IO) > + qcomtee_object_put(u[i].o); > + } > + > + return -EINVAL; > +} > + > +/* TEE Device Ops. */ > + > +static int qcomtee_params_check(struct tee_param *params, int num_params) > +{ > + int io = 0, oo = 0, ib = 0, ob = 0; > + int i; > + > + /* QTEE accepts 64 arguments. */ > + if (num_params > QCOMTEE_ARGS_MAX) > + return -EINVAL; > + > + /* Supported parameter types. */ > + for (i = 0; i < num_params; i++) { > + switch (params[i].attr) { > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT: > + ib++; > + break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + ob++; > + break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_INPUT: > + io++; > + break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_OUTPUT: > + oo++; > + break; > + default: > + return -EINVAL; > + } > + } > + > + /* QTEE accepts 16 arguments of each supported types. */ > + if (io > QCOMTEE_ARGS_PER_TYPE || oo > QCOMTEE_ARGS_PER_TYPE || > + ib > QCOMTEE_ARGS_PER_TYPE || ob > QCOMTEE_ARGS_PER_TYPE) > + return -EINVAL; > + > + return 0; > +} > + > +/* Check if an operation on ROOT_QCOMTEE_OBJECT from userspace is permitted. */ > +static int qcomtee_root_object_check(u32 op, struct tee_param *params, > + int num_params) > +{ > + /* Some privileged operations recognized by QTEE. */ > + if (op == QCOMTEE_ROOT_OP_NOTIFY_DOMAIN_CHANGE || > + op == QCOMTEE_ROOT_OP_ADCI_ACCEPT || > + op == QCOMTEE_ROOT_OP_ADCI_SHUTDOWN) > + return -EINVAL; > + > + /* > + * QCOMTEE_ROOT_OP_REGISTER_WITH_CREDENTIALS is to register with QTEE > + * by passing a credential object as input OBJREF. TEE_OBJREF_NULL as a > + * credential object represents a privileged client for QTEE and > + * is used by the kernel only. > + */ > + if (op == QCOMTEE_ROOT_OP_REGISTER_WITH_CREDENTIALS && > + num_params == 2) { > + if (params[0].attr == TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_INPUT && > + params[1].attr == TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_OUTPUT) { > + if (params[0].u.objref.id == TEE_OBJREF_NULL) > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > +/** > + * qcomtee_object_invoke() - Invoke a QTEE object. > + * @ctx: TEE context. > + * @arg: ioctl arguments. > + * @params: parameters for the object. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +static int qcomtee_object_invoke(struct tee_context *ctx, > + struct tee_ioctl_object_invoke_arg *arg, > + struct tee_param *params) > +{ > + struct qcomtee_object_invoke_ctx *oic __free(kfree) = NULL; > + struct qcomtee_context_data *ctxdata = ctx->data; > + struct qcomtee_arg *u __free(kfree) = NULL; > + struct qcomtee_object *object; > + int i, ret, result; > + > + if (qcomtee_params_check(params, arg->num_params)) > + return -EINVAL; > + > + /* First, handle reserved operations: */ > + if (arg->op == QCOMTEE_MSG_OBJECT_OP_RELEASE) { > + del_qtee_object(arg->object, ctxdata); > + > + return 0; > + } > + > + /* Otherwise, invoke a QTEE object: */ > + oic = qcomtee_object_invoke_ctx_alloc(ctx); > + if (!oic) > + return -ENOMEM; > + > + /* +1 for ending QCOMTEE_ARG_TYPE_INV. */ > + u = kcalloc(arg->num_params + 1, sizeof(*u), GFP_KERNEL); > + if (!u) > + return -ENOMEM; > + > + /* Get an object to invoke. */ > + if (arg->object == TEE_OBJREF_NULL) { > + /* Use ROOT if TEE_OBJREF_NULL is invoked. */ > + if (qcomtee_root_object_check(arg->op, params, arg->num_params)) > + return -EINVAL; > + > + object = ROOT_QCOMTEE_OBJECT; > + } else if (find_qtee_object(&object, arg->object, ctxdata)) { > + return -EINVAL; > + } > + > + ret = qcomtee_params_to_args(u, params, arg->num_params, ctx); > + if (ret) > + goto out; > + > + ret = qcomtee_object_do_invoke(oic, object, arg->op, u, &result); > + if (ret) { > + qcomtee_arg_for_each_input_object(i, u) { > + qcomtee_user_object_set_notify(u[i].o, false); > + qcomtee_object_put(u[i].o); > + } > + > + goto out; > + } > + > + if (!result) { > + /* Assume service is UNAVAIL if unable to process the result. */ > + if (qcomtee_params_from_args(params, u, arg->num_params, ctx)) > + result = QCOMTEE_MSG_ERROR_UNAVAIL; > + } else { > + /* > + * qcomtee_params_to_args() gets a copy of IO for the driver to > + * make sure they do not get released while in the middle of > + * invocation. On success (!result), qcomtee_params_from_args() > + * puts them. > + */ > + qcomtee_arg_for_each_input_object(i, u) > + qcomtee_object_put(u[i].o); > + } > + > + arg->ret = result; > +out: > + qcomtee_object_put(object); > + > + return ret; > +} > + > +/** > + * qcomtee_supp_recv() - Wait for a request for the supplicant. > + * @ctx: TEE context. > + * @op: requested operation on the object. > + * @num_params: number of elements in the parameter array. > + * @params: parameters for @op. > + * > + * The first parameter is a meta %TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT. > + * On input, it provides a user buffer. This buffer is used for parameters of > + * type %TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT in qcomtee_cb_params_from_args(). > + * On output, the object ID and request ID are stored in the meta parameter. > + * > + * @num_params is updated to the number of parameters that actually exist > + * in @params on return. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +static int qcomtee_supp_recv(struct tee_context *ctx, u32 *op, u32 *num_params, > + struct tee_param *params) > +{ > + struct qcomtee_user_object_request_data data; > + void __user *uaddr; > + size_t ubuf_size; > + int i, ret; > + > + if (!*num_params) > + return -EINVAL; > + > + /* We expect the first parameter to be an INOUT + meta parameter. */ > + if (params->attr != > + (TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT | TEE_IOCTL_PARAM_ATTR_META)) Why aren't you using TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_*? > + return -EINVAL; > + > + /* Other parameters are none. */ > + for (i = 1; i < *num_params; i++) > + if (params[i].attr) > + return -EINVAL; > + > + if (!IS_ALIGNED(params->u.value.a, 8)) > + return -EINVAL; > + > + /* User buffer and size from meta parameter. */ > + uaddr = u64_to_user_ptr(params->u.value.a); > + ubuf_size = params->u.value.b; Please use params->u.ubuf.uaddr and params->u.ubuf.size instead > + /* Process TEE parameters. +/-1 to ignore the meta parameter. */ > + ret = qcomtee_user_object_select(ctx, params + 1, *num_params - 1, > + uaddr, ubuf_size, &data); > + if (ret) > + return ret; > + > + params->u.value.a = data.object_id; > + params->u.value.b = data.id; params->u.objref.{id, flags}? Should params->attr be updated with TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_*? > + params->u.value.c = 0; > + *op = data.op; > + *num_params = data.np + 1; > + > + return 0; > +} > + > +/** > + * qcomtee_supp_send() - Submit a response for a request. > + * @ctx: TEE context. > + * @errno: return value for the request. > + * @num_params: number of elements in the parameter array. > + * @params: returned parameters. > + * > + * The first parameter is a meta %TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT. > + * It specifies the request ID this response belongs to. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +static int qcomtee_supp_send(struct tee_context *ctx, u32 errno, u32 num_params, > + struct tee_param *params) > +{ > + if (!num_params) > + return -EINVAL; > + > + /* We expect the first parameter to be an OUTPUT + meta parameter. */ > + if (params->attr != (TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT | > + TEE_IOCTL_PARAM_ATTR_META)) > + return -EINVAL; Why is this used if it's to be ignored, making room for forward compatibility? > + > + /* Process TEE parameters. +/-1 to ignore the meta parameter. */ > + return qcomtee_user_object_submit(ctx, params + 1, num_params - 1, > + params->u.value.a, errno); > +} > + > +static int qcomtee_open(struct tee_context *ctx) > +{ > + struct qcomtee_context_data *ctxdata __free(kfree) = NULL; > + > + ctxdata = kzalloc(sizeof(*ctxdata), GFP_KERNEL); > + if (!ctxdata) > + return -ENOMEM; > + > + /* > + * In the QTEE driver, the same context is used to refcount resources > + * shared by QTEE. For example, teedev_ctx_get() is called for any > + * instance of callback objects (see qcomtee_user_param_to_object()). > + * > + * Maintain a copy of teedev for QTEE as it serves as a direct user of > + * this context. The teedev will be released in the context's release(). > + * > + * tee_device_unregister() will remain blocked until all contexts > + * are released. This includes contexts owned by the user, which are > + * closed by teedev_close_context(), as well as those owned by QTEE > + * closed by tee_context_put() in object's release(). > + */ > + if (!tee_device_get(ctx->teedev)) > + return -EINVAL; > + > + idr_init(&ctxdata->qtee_objects_idr); > + spin_lock_init(&ctxdata->qtee_lock); > + idr_init(&ctxdata->reqs_idr); > + INIT_LIST_HEAD(&ctxdata->reqs_list); > + mutex_init(&ctxdata->reqs_lock); > + init_completion(&ctxdata->req_c); > + > + ctx->data = no_free_ptr(ctxdata); > + > + return 0; > +} > + > +/* This is called when the user closes the device. */ > +static void qcomtee_close_context(struct tee_context *ctx) > +{ > + struct qcomtee_context_data *ctxdata = ctx->data; > + struct qcomtee_object *object; > + int id; > + > + /* Process QUEUED or PROCESSING requests. */ > + qcomtee_requests_destroy(ctxdata); > + /* Release QTEE objects. */ > + idr_for_each_entry(&ctxdata->qtee_objects_idr, object, id) > + qcomtee_object_put(object); > +} > + > +/* This is called when the final reference to the context goes away. */ > +static void qcomtee_release(struct tee_context *ctx) > +{ > + struct qcomtee_context_data *ctxdata = ctx->data; > + > + idr_destroy(&ctxdata->qtee_objects_idr); > + idr_destroy(&ctxdata->reqs_idr); > + kfree(ctxdata); > + > + /* There is nothing shared in this context with QTEE. */ > + tee_device_put(ctx->teedev); > +} > + > +static void qcomtee_get_version(struct tee_device *teedev, > + struct tee_ioctl_version_data *vers) > +{ > + struct tee_ioctl_version_data v = { > + .impl_id = TEE_IMPL_ID_QTEE, > + .gen_caps = TEE_GEN_CAP_OBJREF, > + }; > + > + *vers = v; > +} > + > +static const struct tee_driver_ops qcomtee_ops = { > + .get_version = qcomtee_get_version, > + .open = qcomtee_open, > + .close_context = qcomtee_close_context, > + .release = qcomtee_release, > + .object_invoke_func = qcomtee_object_invoke, > + .supp_recv = qcomtee_supp_recv, > + .supp_send = qcomtee_supp_send, > +}; > + > +static const struct tee_desc qcomtee_desc = { > + .name = "qcomtee", > + .ops = &qcomtee_ops, > + .owner = THIS_MODULE, > +}; > + > +static int qcomtee_probe(struct platform_device *pdev) > +{ > + struct workqueue_struct *async_wq; > + struct tee_device *teedev; > + struct tee_shm_pool *pool; > + struct tee_context *ctx; > + struct qcomtee *qcomtee; > + int err; > + > + qcomtee = kzalloc(sizeof(*qcomtee), GFP_KERNEL); > + if (!qcomtee) > + return -ENOMEM; > + > + pool = qcomtee_shm_pool_alloc(); > + if (IS_ERR(pool)) { > + err = PTR_ERR(pool); > + > + goto err_free_qcomtee; > + } > + > + teedev = tee_device_alloc(&qcomtee_desc, NULL, pool, qcomtee); > + if (IS_ERR(teedev)) { > + err = PTR_ERR(teedev); > + > + goto err_pool_destroy; > + } > + > + qcomtee->teedev = teedev; > + qcomtee->pool = pool; > + err = tee_device_register(qcomtee->teedev); > + if (err) > + goto err_unreg_teedev; > + > + platform_set_drvdata(pdev, qcomtee); > + /* Start async wq. */ > + async_wq = alloc_ordered_workqueue("qcomtee_wq", 0); > + if (!async_wq) > + goto err_unreg_teedev; > + > + qcomtee->wq = async_wq; > + /* Driver context used for async operations of teedev. */ > + ctx = teedev_open(qcomtee->teedev); > + if (IS_ERR(ctx)) { > + err = PTR_ERR(ctx); > + > + goto err_dest_wq; > + } > + > + qcomtee->ctx = ctx; > + > + return 0; > + > +err_dest_wq: > + destroy_workqueue(qcomtee->wq); > +err_unreg_teedev: > + tee_device_unregister(qcomtee->teedev); > +err_pool_destroy: > + tee_shm_pool_free(pool); > +err_free_qcomtee: > + kfree(qcomtee); > + > + return err; > +} > + > +/** > + * qcomtee_remove() - Device Removal Routine. > + * @pdev: platform device information struct. > + * > + * It is called by the platform subsystem to alert the driver that it should > + * release the device. > + * > + * QTEE does not provide an API to inform it about a callback object going away. > + * However, when releasing QTEE objects, any callback object sent to QTEE > + * previously would be released by QTEE as part of the object release. > + */ > +static void qcomtee_remove(struct platform_device *pdev) > +{ > + struct qcomtee *qcomtee = platform_get_drvdata(pdev); > + > + teedev_close_context(qcomtee->ctx); > + /* Wait for RELEASE operations to be processed for QTEE objects. */ > + tee_device_unregister(qcomtee->teedev); > + destroy_workqueue(qcomtee->wq); > + tee_shm_pool_free(qcomtee->pool); > + kfree(qcomtee); > +} > + > +static const struct platform_device_id qcomtee_ids[] = { { "qcomtee", 0 }, {} }; > +MODULE_DEVICE_TABLE(platform, qcomtee_ids); > + > +static struct platform_driver qcomtee_platform_driver = { > + .probe = qcomtee_probe, > + .remove = qcomtee_remove, > + .driver = { > + .name = "qcomtee", > + }, > + .id_table = qcomtee_ids, > +}; > + > +module_platform_driver(qcomtee_platform_driver); > + > +MODULE_AUTHOR("Qualcomm"); > +MODULE_DESCRIPTION("QTEE driver"); > +MODULE_VERSION("1.0"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/tee/qcomtee/core.c b/drivers/tee/qcomtee/core.c > new file mode 100644 > index 000000000000..162e32db656a > --- /dev/null > +++ b/drivers/tee/qcomtee/core.c > @@ -0,0 +1,801 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/xarray.h> > + > +#include "qcomtee_msg.h" > +#include "qcomtee_private.h" > + > +/* This is the QTEE root object. */ > +struct qcomtee_object qcomtee_object_root = { > + .name = "root", > + .object_type = QCOMTEE_OBJECT_TYPE_ROOT, > + .info.qtee_id = QCOMTEE_MSG_OBJECT_ROOT, > +}; > +EXPORT_SYMBOL_GPL(qcomtee_object_root); > + > +/* Next argument of type @type after index @i. */ > +int qcomtee_next_arg_type(struct qcomtee_arg *u, int i, > + enum qcomtee_arg_type type) > +{ > + while (u[i].type != QCOMTEE_ARG_TYPE_INV && u[i].type != type) > + i++; > + return i; > +} > + > +/* > + * QTEE expects IDs with the QCOMTEE_MSG_OBJECT_NS_BIT set for objects > + * of the QCOMTEE_OBJECT_TYPE_CB type. > + */ > +#define QCOMTEE_OBJECT_ID_START (QCOMTEE_MSG_OBJECT_NS_BIT + 1) > +#define QCOMTEE_OBJECT_ID_END (UINT_MAX) Should this be U32_MAX considering that QCOMTEE_MSG_OBJECT_NS_BIT is defined as BIT(31)? > + > +#define QCOMTEE_OBJECT_SET(p, type, ...) \ > + __QCOMTEE_OBJECT_SET(p, type, ##__VA_ARGS__, 0UL) > +#define __QCOMTEE_OBJECT_SET(p, type, optr, ...) \ > + do { \ > + (p)->object_type = (type); \ > + (p)->info.qtee_id = (unsigned long)(optr); \ > + } while (0) > + > +static struct qcomtee_object * > +qcomtee_qtee_object_alloc(struct qcomtee_object_invoke_ctx *oic, > + unsigned int object_id) > +{ > + struct qcomtee *qcomtee = tee_get_drvdata(oic->ctx->teedev); > + struct qcomtee_object *object __free(kfree); > + > + object = kzalloc(sizeof(*object), GFP_KERNEL); > + if (!object) > + return NULL_QCOMTEE_OBJECT; > + > + /* If failed, "no-name". */ > + object->name = kasprintf(GFP_KERNEL, "qcomtee-%u", object_id); Where does it become "no-name" if object->name is NULL? Is it worth it with a special case for the unlikely condition that kasprintf() fails while no other memory allocations fail? > + QCOMTEE_OBJECT_SET(object, QCOMTEE_OBJECT_TYPE_TEE, object_id); > + kref_init(&object->refcount); > + /* A QTEE object requires a context for async operations. */ > + object->info.qcomtee_async_ctx = qcomtee->ctx; > + teedev_ctx_get(object->info.qcomtee_async_ctx); > + > + return no_free_ptr(object); > +} > + > +void qcomtee_qtee_object_free(struct qcomtee_object *object) > +{ > + /* See qcomtee_qtee_object_alloc(). */ > + teedev_ctx_put(object->info.qcomtee_async_ctx); > + > + kfree(object->name); > + kfree(object); > +} > + > +static void qcomtee_object_release(struct kref *refcount) > +{ > + struct qcomtee_object *object; > + const char *name; > + > + object = container_of(refcount, struct qcomtee_object, refcount); > + > + synchronize_rcu(); Please add a comment in the code explaining why this is needed. > + > + switch (typeof_qcomtee_object(object)) { > + case QCOMTEE_OBJECT_TYPE_TEE: > + qcomtee_release_tee_object(object); > + > + break; > + case QCOMTEE_OBJECT_TYPE_CB: > + name = object->name; > + > + if (object->ops->release) > + object->ops->release(object); > + > + kfree_const(name); > + > + break; > + case QCOMTEE_OBJECT_TYPE_ROOT: > + case QCOMTEE_OBJECT_TYPE_NULL: > + default: > + break; > + } > +} > + > +/** > + * qcomtee_object_get() - Increase the object's reference count. > + * @object: object to increase the reference count. > + */ > +int qcomtee_object_get(struct qcomtee_object *object) > +{ > + if (object != NULL_QCOMTEE_OBJECT && object != ROOT_QCOMTEE_OBJECT) > + return kref_get_unless_zero(&object->refcount); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(qcomtee_object_get); > + > +/** > + * qcomtee_object_put() - Decrease the object's reference count. > + * @object: object to decrease the reference count. > + */ > +void qcomtee_object_put(struct qcomtee_object *object) > +{ > + if (object != NULL_QCOMTEE_OBJECT && object != ROOT_QCOMTEE_OBJECT) > + kref_put(&object->refcount, qcomtee_object_release); > +} > +EXPORT_SYMBOL_GPL(qcomtee_object_put); > + > +/* > + * ''Local Object Table''. > + * Objects from the kernel exported to QTEE are assigned an ID and stored > + * in xa_qcom_local_objects (also known as the kernel object table). > + * QTEE uses this ID to reference the objects using qcomtee_local_object_get(). > + */ > +static DEFINE_XARRAY_ALLOC(xa_qcom_local_objects); Please move this to the top of this file where the other global variables are kept. Do we have/need any checks to see the unrelated processes doesn't use objects from each other because they manage to guess an id? > + > +static int qcomtee_idx_alloc(u32 *idx, struct qcomtee_object *object) > +{ > + static u32 xa_last_id = QCOMTEE_OBJECT_ID_START; > + > + /* Every ID allocated here has QCOMTEE_MSG_OBJECT_NS_BIT set. */ > + return xa_alloc_cyclic(&xa_qcom_local_objects, idx, object, > + XA_LIMIT(QCOMTEE_OBJECT_ID_START, > + QCOMTEE_OBJECT_ID_END), > + &xa_last_id, GFP_KERNEL); > +} > + > +struct qcomtee_object *qcomtee_idx_erase(u32 idx) > +{ > + if (idx < QCOMTEE_OBJECT_ID_START || idx > QCOMTEE_OBJECT_ID_END) > + return NULL_QCOMTEE_OBJECT; > + > + return xa_erase(&xa_qcom_local_objects, idx); > +} > + > +/** > + * qcomtee_object_id_get() - Get an ID for an object to send to QTEE. > + * @object: object to assign an ID. > + * @object_id: object ID. > + * > + * This is called on the path to QTEE to construct the message; see > + * qcomtee_prepare_msg() and qcomtee_update_msg(). > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +static int qcomtee_object_id_get(struct qcomtee_object *object, > + unsigned int *object_id) > +{ > + u32 idx; > + > + switch (typeof_qcomtee_object(object)) { > + case QCOMTEE_OBJECT_TYPE_CB: > + if (qcomtee_idx_alloc(&idx, object) < 0) > + return -ENOSPC; > + > + *object_id = idx; > + > + break; > + case QCOMTEE_OBJECT_TYPE_ROOT: > + case QCOMTEE_OBJECT_TYPE_TEE: > + *object_id = object->info.qtee_id; > + > + break; > + case QCOMTEE_OBJECT_TYPE_NULL: > + *object_id = QCOMTEE_MSG_OBJECT_NULL; > + > + break; > + } > + > + return 0; > +} > + > +/* Release object ID assigned in qcomtee_object_id_get. */ > +static void qcomtee_object_id_put(unsigned int object_id) > +{ > + qcomtee_idx_erase(object_id); > +} > + > +/** > + * qcomtee_local_object_get() - Get the object referenced by the ID. > + * @object_id: object ID. > + * > + * It is called on the path from QTEE. > + * It is called on behalf of QTEE to obtain an instance of an object > + * for a given ID. It increases the object's reference count on success. > + * > + * Return: On error, returns %NULL_QCOMTEE_OBJECT. > + * On success, returns the object. > + */ > +static struct qcomtee_object *qcomtee_local_object_get(unsigned int object_id) > +{ > + struct qcomtee_object *object; > + > + /* > + * This is not protected by an RCU read lock because we are > + * confident that QTEE does not issue a RELEASE request and > + * qcomtee_local_object_get() concurrently. > + */ I think it would be better to not depend on that. An RCU read lock must be very cheap compared to what we do in this context. > + object = xa_load(&xa_qcom_local_objects, object_id); > + > + qcomtee_object_get(object); > + > + return object; > +} > + > +/** > + * qcomtee_object_user_init() - Initialize an object for the user. > + * @object: object to initialize. > + * @ot: type of object as &enum qcomtee_object_type. > + * @ops: instance of callbacks. > + * @fmt: name assigned to the object. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_object_user_init(struct qcomtee_object *object, > + enum qcomtee_object_type ot, > + struct qcomtee_object_operations *ops, > + const char *fmt, ...) > +{ > + va_list ap; > + int ret; > + > + kref_init(&object->refcount); > + QCOMTEE_OBJECT_SET(object, QCOMTEE_OBJECT_TYPE_NULL); > + > + va_start(ap, fmt); > + switch (ot) { > + case QCOMTEE_OBJECT_TYPE_NULL: > + ret = 0; > + > + break; > + case QCOMTEE_OBJECT_TYPE_CB: > + object->ops = ops; > + if (!object->ops->dispatch) > + return -EINVAL; > + > + /* If failed, "no-name". */ > + object->name = kvasprintf_const(GFP_KERNEL, fmt, ap); > + QCOMTEE_OBJECT_SET(object, QCOMTEE_OBJECT_TYPE_CB); > + > + ret = 0; > + break; > + case QCOMTEE_OBJECT_TYPE_ROOT: > + case QCOMTEE_OBJECT_TYPE_TEE: > + default: > + ret = -EINVAL; > + } > + va_end(ap); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(qcomtee_object_user_init); > + > +/** > + * qcomtee_object_type() - Returns the type of object represented by an ID. > + * @object_id: object ID for the object. > + * > + * This is similar to typeof_qcomtee_object(), but instead of receiving an > + * object as an argument, it receives an object ID. It is used internally > + * on the return path from QTEE. > + * > + * Return: Returns the type of object referenced by @object_id. > + */ > +static enum qcomtee_object_type qcomtee_object_type(unsigned int object_id) > +{ > + if (object_id == QCOMTEE_MSG_OBJECT_NULL) > + return QCOMTEE_OBJECT_TYPE_NULL; > + > + if (object_id & QCOMTEE_MSG_OBJECT_NS_BIT) > + return QCOMTEE_OBJECT_TYPE_CB; > + > + return QCOMTEE_OBJECT_TYPE_TEE; > +} > + > +/** > + * qcomtee_object_qtee_init() - Initialize an object for QTEE. > + * @object: object returned. > + * @object_id: object ID received from QTEE. > + * > + * Return: On failure, returns < 0 and sets @object to %NULL_QCOMTEE_OBJECT. > + * On success, returns 0 > + */ > +static int qcomtee_object_qtee_init(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object **object, > + unsigned int object_id) > +{ > + int ret = 0; > + > + switch (qcomtee_object_type(object_id)) { > + case QCOMTEE_OBJECT_TYPE_NULL: > + *object = NULL_QCOMTEE_OBJECT; > + > + break; > + case QCOMTEE_OBJECT_TYPE_CB: > + *object = qcomtee_local_object_get(object_id); > + if (*object == NULL_QCOMTEE_OBJECT) > + ret = -EINVAL; > + > + break; > + > + default: /* QCOMTEE_OBJECT_TYPE_TEE */ > + *object = qcomtee_qtee_object_alloc(oic, object_id); > + if (*object == NULL_QCOMTEE_OBJECT) > + ret = -ENOMEM; > + > + break; > + } > + > + return ret; > +} > + > +/* > + * ''Marshaling API'' > + * qcomtee_prepare_msg - Prepare the inbound buffer for sending to QTEE > + * qcomtee_update_args - Parse the QTEE response in the inbound buffer > + * qcomtee_prepare_args - Parse the QTEE request from the outbound buffer > + * qcomtee_update_msg - Update the outbound buffer with the response for QTEE > + */ > + > +static int qcomtee_prepare_msg(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object *object, u32 op, > + struct qcomtee_arg *u) > +{ > + struct qcomtee_msg_object_invoke *msg; > + unsigned int object_id; > + int i, ib, ob, io, oo; > + size_t off; > + > + /* Use the input message buffer in 'oic'. */ > + msg = oic->in_msg.addr; > + > + /* Start offset in a message for buffer arguments. */ > + off = qcomtee_msg_buffer_args(struct qcomtee_msg_object_invoke, > + qcomtee_args_len(u)); > + > + /* Get the ID of the object being invoked. */ > + if (qcomtee_object_id_get(object, &object_id)) > + return -ENOSPC; > + > + ib = 0; > + qcomtee_arg_for_each_input_buffer(i, u) { > + void *ptr; > + > + /* Overflow already checked in qcomtee_msg_buffers_alloc(). */ > + msg->args[ib].b.offset = off; > + msg->args[ib].b.size = u[i].b.size; > + > + ptr = qcomtee_msg_offset_to_ptr(msg, off); > + /* Userspace client or kernel client!? */ > + if (!(u[i].flags & QCOMTEE_ARG_FLAGS_UADDR)) > + memcpy(ptr, u[i].b.addr, u[i].b.size); > + else if (copy_from_user(ptr, u[i].b.uaddr, u[i].b.size)) > + return -EINVAL; > + > + off += qcomtee_msg_offset_align(u[i].b.size); > + ib++; > + } > + > + ob = ib; > + qcomtee_arg_for_each_output_buffer(i, u) { > + /* Overflow already checked in qcomtee_msg_buffers_alloc(). */ > + msg->args[ob].b.offset = off; > + msg->args[ob].b.size = u[i].b.size; > + > + off += qcomtee_msg_offset_align(u[i].b.size); > + ob++; > + } > + > + io = ob; > + qcomtee_arg_for_each_input_object(i, u) { > + if (qcomtee_object_id_get(u[i].o, &msg->args[io].o)) { > + /* Put whatever we got. */ > + qcomtee_object_id_put(object_id); > + for (io--; io >= ob; io--) > + qcomtee_object_id_put(msg->args[io].o); > + > + return -ENOSPC; > + } > + > + io++; > + } > + > + oo = io; > + qcomtee_arg_for_each_output_object(i, u) > + oo++; > + > + /* Set object, operation, and argument counts. */ > + qcomtee_msg_init(msg, object_id, op, ib, ob, io, oo); > + > + return 0; > +} > + > +/** > + * qcomtee_update_args() - Parse the QTEE response in the inbound buffer. > + * @u: array of arguments for the invocation. > + * @oic: context to use for the invocation. > + * > + * @u must be the same as the one used in qcomtee_prepare_msg() when > + * initializing the inbound buffer. > + * > + * On failure, it continues processing the QTEE message. The caller should > + * do the necessary cleanup, including calling qcomtee_object_put() > + * on the output objects. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +static int qcomtee_update_args(struct qcomtee_arg *u, > + struct qcomtee_object_invoke_ctx *oic) > +{ > + struct qcomtee_msg_object_invoke *msg; > + int i, ib, ob, io, oo; > + int ret = 0; > + > + /* Use the input message buffer in 'oic'. */ > + msg = oic->in_msg.addr; > + > + ib = 0; > + qcomtee_arg_for_each_input_buffer(i, u) > + ib++; > + > + ob = ib; > + qcomtee_arg_for_each_output_buffer(i, u) { > + void *ptr; > + > + /* QTEE can override the size to a smaller value. */ > + u[i].b.size = msg->args[ob].b.size; > + > + ptr = qcomtee_msg_offset_to_ptr(msg, msg->args[ob].b.offset); > + /* Userspace client or kernel client!? */ > + if (!(u[i].flags & QCOMTEE_ARG_FLAGS_UADDR)) > + memcpy(u[i].b.addr, ptr, u[i].b.size); > + else if (copy_to_user(u[i].b.uaddr, ptr, u[i].b.size)) > + ret = -EINVAL; > + > + ob++; > + } > + > + io = ob; > + qcomtee_arg_for_each_input_object(i, u) > + io++; > + > + oo = io; > + qcomtee_arg_for_each_output_object(i, u) { > + if (qcomtee_object_qtee_init(oic, &u[i].o, msg->args[oo].o)) > + ret = -EINVAL; > + > + oo++; > + } > + > + return ret; > +} > + > +/** > + * qcomtee_prepare_args() - Parse the QTEE request from the outbound buffer. > + * @oic: context to use for the invocation. > + * > + * It initializes &qcomtee_object_invoke_ctx->u based on the QTEE request in > + * the outbound buffer. It sets %QCOMTEE_ARG_TYPE_INV at the end of the array. > + * > + * On failure, it continues processing the QTEE message. The caller should > + * do the necessary cleanup, including calling qcomtee_object_put() > + * on the input objects. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +static int qcomtee_prepare_args(struct qcomtee_object_invoke_ctx *oic) > +{ > + struct qcomtee_msg_callback *msg; > + union qcomtee_msg_arg *arg; > + int i, ret = 0; > + > + /* Use the output message buffer in 'oic'. */ > + msg = oic->out_msg.addr; > + > + qcomtee_msg_for_each_input_buffer(i, msg) { > + arg = &msg->args[i]; > + oic->u[i].b.addr = > + qcomtee_msg_offset_to_ptr(msg, arg->b.offset); > + oic->u[i].b.size = arg->b.size; > + oic->u[i].type = QCOMTEE_ARG_TYPE_IB; > + } > + > + qcomtee_msg_for_each_output_buffer(i, msg) { > + arg = &msg->args[i]; > + oic->u[i].b.addr = > + qcomtee_msg_offset_to_ptr(msg, arg->b.offset); > + oic->u[i].b.size = arg->b.size; > + oic->u[i].type = QCOMTEE_ARG_TYPE_OB; > + } > + > + qcomtee_msg_for_each_input_object(i, msg) { > + if (qcomtee_object_qtee_init(oic, &oic->u[i].o, msg->args[i].o)) > + ret = -EINVAL; > + > + oic->u[i].type = QCOMTEE_ARG_TYPE_IO; > + } > + > + qcomtee_msg_for_each_output_object(i, msg) > + oic->u[i].type = QCOMTEE_ARG_TYPE_OO; > + > + /* End of Arguments. */ > + oic->u[i].type = QCOMTEE_ARG_TYPE_INV; > + > + return ret; > +} > + > +static int qcomtee_update_msg(struct qcomtee_object_invoke_ctx *oic) > +{ > + struct qcomtee_msg_callback *msg; > + int i, ib, ob, io, oo; > + > + /* Use the output message buffer in 'oic'. */ > + msg = oic->out_msg.addr; > + > + ib = 0; > + qcomtee_arg_for_each_input_buffer(i, oic->u) > + ib++; > + > + ob = ib; > + qcomtee_arg_for_each_output_buffer(i, oic->u) { > + /* Only reduce size; never increase it. */ > + if (msg->args[ob].b.size < oic->u[i].b.size) > + return -EINVAL; > + > + msg->args[ob].b.size = oic->u[i].b.size; > + ob++; > + } > + > + io = ob; > + qcomtee_arg_for_each_input_object(i, oic->u) > + io++; > + > + oo = io; > + qcomtee_arg_for_each_output_object(i, oic->u) { > + if (qcomtee_object_id_get(oic->u[i].o, &msg->args[oo].o)) { > + /* Put whatever we got. */ > + for (oo--; oo >= io; oo--) > + qcomtee_object_id_put(msg->args[oo].o); > + > + return -ENOSPC; > + } > + > + oo++; > + } > + > + return 0; > +} > + > +/* Invoke a callback object. */ > +static void qcomtee_cb_object_invoke(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_msg_callback *msg) > +{ > + int i, errno; > + u32 op; > + > + /* Get the object being invoked. */ > + unsigned int object_id = msg->cxt; > + struct qcomtee_object *object; > + > + /* QTEE cannot invoke a NULL object or objects it hosts. */ > + if (qcomtee_object_type(object_id) == QCOMTEE_OBJECT_TYPE_NULL || > + qcomtee_object_type(object_id) == QCOMTEE_OBJECT_TYPE_TEE) { > + errno = -EINVAL; > + goto out; > + } > + > + object = qcomtee_local_object_get(object_id); > + if (object == NULL_QCOMTEE_OBJECT) { > + errno = -EINVAL; > + goto out; > + } > + > + oic->object = object; > + > + /* Filter bits used by transport. */ > + op = msg->op & QCOMTEE_MSG_OBJECT_OP_MASK; > + > + switch (op) { > + case QCOMTEE_MSG_OBJECT_OP_RELEASE: > + qcomtee_object_id_put(object_id); > + qcomtee_object_put(object); > + errno = 0; > + > + break; > + case QCOMTEE_MSG_OBJECT_OP_RETAIN: > + qcomtee_object_get(object); > + errno = 0; > + > + break; > + default: > + errno = qcomtee_prepare_args(oic); > + if (errno) { > + /* Release any object that arrived as input. */ > + qcomtee_arg_for_each_input_buffer(i, oic->u) > + qcomtee_object_put(oic->u[i].o); > + > + break; > + } > + > + errno = object->ops->dispatch(oic, object, op, oic->u); > + if (!errno) { > + /* On success, notify at the appropriate time. */ > + oic->flags |= QCOMTEE_OIC_FLAG_NOTIFY; > + } > + } > + > +out: > + > + oic->errno = errno; > +} > + > +/** > + * qcomtee_qtee_objects_put() - Put the callback objects in the argument array. > + * @u: array of arguments. > + * > + * When qcomtee_object_do_invoke_internal() is successfully invoked, > + * QTEE takes ownership of the callback objects. If the invocation fails, > + * qcomtee_object_do_invoke_internal() calls qcomtee_qtee_objects_put() > + * to mimic the release of callback objects by QTEE. > + */ > +static void qcomtee_qtee_objects_put(struct qcomtee_arg *u) > +{ > + int i; > + > + qcomtee_arg_for_each_input_object(i, u) { > + if (typeof_qcomtee_object(u[i].o) == QCOMTEE_OBJECT_TYPE_CB) > + qcomtee_object_put(u[i].o); > + } > +} > + > +/** > + * qcomtee_object_do_invoke_internal() - Submit an invocation for an object. > + * @oic: context to use for the current invocation. > + * @object: object being invoked. > + * @op: requested operation on the object. > + * @u: array of arguments for the current invocation. > + * @result: result returned from QTEE. > + * > + * The caller is responsible for keeping track of the refcount for each > + * object, including @object. On return, the caller loses ownership of all > + * input objects of type %QCOMTEE_OBJECT_TYPE_CB. > + * > + * Return: On success, returns 0. On error, returns -EAGAIN if invocation > + * failed and the user may retry the invocation, -ENODEV on fatal failure. > + */ > +int qcomtee_object_do_invoke_internal(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object *object, u32 op, > + struct qcomtee_arg *u, int *result) > +{ > + struct qcomtee_msg_callback *cb_msg; > + struct qcomtee_object *qto; > + int i, ret, errno; > + u64 res_type; > + > + /* Allocate inbound and outbound buffers. */ > + if (qcomtee_msg_buffers_alloc(oic, u)) { > + qcomtee_qtee_objects_put(u); > + > + return -EAGAIN; > + } > + > + if (qcomtee_prepare_msg(oic, object, op, u)) { > + qcomtee_qtee_objects_put(u); > + > + ret = -EAGAIN; > + goto out; > + } > + > + /* Use input message buffer in 'oic'. */ > + cb_msg = oic->out_msg.addr; > + > + while (1) { > + if (oic->flags & QCOMTEE_OIC_FLAG_BUSY) { > + errno = oic->errno; > + if (!errno) > + errno = qcomtee_update_msg(oic); > + qcomtee_msg_set_result(cb_msg, errno); > + } > + > + /* Invoke the remote object. */ > + ret = qcomtee_object_invoke_ctx_invoke(oic, result, &res_type); > + > + if (oic->flags & QCOMTEE_OIC_FLAG_BUSY) { > + qto = oic->object; > + if (qto) { > + if (oic->flags & QCOMTEE_OIC_FLAG_NOTIFY) { > + /* Don't care about the exact errno. */ > + if (qto->ops->notify) > + qto->ops->notify(oic, qto, > + errno || ret); > + } > + > + /* Get is in qcomtee_cb_object_invoke(). */ > + qcomtee_object_put(qto); > + } > + > + oic->object = NULL_QCOMTEE_OBJECT; > + oic->flags &= ~(QCOMTEE_OIC_FLAG_BUSY | > + QCOMTEE_OIC_FLAG_NOTIFY); > + } > + > + if (ret) { > + if (!(oic->flags & QCOMTEE_OIC_FLAG_SHARED)) { > + qcomtee_qtee_objects_put(u); > + > + ret = -EAGAIN; > + } else { > + /* > + * On error, there is no clean way to exit. > + * For some reason, we cannot communicate with > + * QTEE, so we cannot notify QTEE about the > + * failure and do further cleanup. > + */ > + ret = -ENODEV; > + } > + > + goto out; > + > + } else { > + /* > + * QTEE obtained ownership of QCOMTEE_OBJECT_TYPE_CB > + * input objects in 'u'. On further failure, QTEE is > + * responsible for releasing them. > + */ > + oic->flags |= QCOMTEE_OIC_FLAG_SHARED; > + } > + > + /* Is it a callback request? */ > + if (res_type != QCOMTEE_RESULT_INBOUND_REQ_NEEDED) { > + if (!*result) { > + ret = qcomtee_update_args(u, oic); > + if (ret) { > + /* Put output objects. Retry. */ > + qcomtee_arg_for_each_output_object(i, u) > + qcomtee_object_put(u[i].o); > + > + ret = -EAGAIN; > + } > + } > + > + break; > + > + } else { > + oic->flags |= QCOMTEE_OIC_FLAG_BUSY; > + > + qcomtee_fetch_async_reqs(oic); > + qcomtee_cb_object_invoke(oic, cb_msg); > + } > + } > + > + qcomtee_fetch_async_reqs(oic); > +out: > + qcomtee_msg_buffers_free(oic); > + > + return ret; > +} > + > +int qcomtee_object_do_invoke(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object *object, u32 op, > + struct qcomtee_arg *u, int *result) > +{ > + /* User can not set bits used by transport. */ > + if (op & ~QCOMTEE_MSG_OBJECT_OP_MASK) > + return -EINVAL; > + > + /* User can only invoke QTEE hosted objects. */ > + if (typeof_qcomtee_object(object) != QCOMTEE_OBJECT_TYPE_TEE && > + typeof_qcomtee_object(object) != QCOMTEE_OBJECT_TYPE_ROOT) > + return -EINVAL; > + > + /* User cannot directly issue these operations to QTEE. */ > + if (op == QCOMTEE_MSG_OBJECT_OP_RELEASE) > + return -EINVAL; > + > + return qcomtee_object_do_invoke_internal(oic, object, op, u, result); > +} > +EXPORT_SYMBOL_GPL(qcomtee_object_do_invoke); > diff --git a/drivers/tee/qcomtee/qcom_scm.c b/drivers/tee/qcomtee/qcom_scm.c > new file mode 100644 > index 000000000000..38a3d080f86d > --- /dev/null > +++ b/drivers/tee/qcomtee/qcom_scm.c > @@ -0,0 +1,38 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/firmware/qcom/qcom_scm.h> > + > +#include "qcomtee_private.h" > + > +int qcomtee_object_invoke_ctx_invoke(struct qcomtee_object_invoke_ctx *oic, > + int *result, u64 *res_type) > +{ > + phys_addr_t out_msg_paddr; > + phys_addr_t in_msg_paddr; > + int ret; > + u64 res; > + > + tee_shm_get_pa(oic->out_shm, 0, &out_msg_paddr); > + tee_shm_get_pa(oic->in_shm, 0, &in_msg_paddr); > + if (!(oic->flags & QCOMTEE_OIC_FLAG_BUSY)) { > + /* Direct QTEE object invocation. */ > + ret = qcom_scm_qtee_invoke_smc(in_msg_paddr, oic->in_msg.size, > + out_msg_paddr, oic->out_msg.size, > + &res, res_type); > + } else { > + /* Submit callback response. */ > + ret = qcom_scm_qtee_callback_response(out_msg_paddr, > + oic->out_msg.size, > + &res, res_type); > + } > + > + if (ret) > + pr_err("QTEE returned with %d.\n", ret); > + else > + *result = (int)res; > + > + return ret; > +} > diff --git a/drivers/tee/qcomtee/qcomtee_msg.h b/drivers/tee/qcomtee/qcomtee_msg.h > new file mode 100644 > index 000000000000..33a6b426153c > --- /dev/null > +++ b/drivers/tee/qcomtee/qcomtee_msg.h > @@ -0,0 +1,239 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef QCOMTEE_MSG_H > +#define QCOMTEE_MSG_H > + > +#include <linux/firmware/qcom/qcom_tee.h> > + > +/** > + * DOC: ''Qualcomm TEE'' (QTEE) Transport Message > + * > + * There are two buffers shared with QTEE: inbound and outbound buffers. > + * The inbound buffer is used for direct object invocation, and the outbound > + * buffer is used to make a request from QTEE to the kernel; i.e., a callback > + * request. > + * > + * The unused tail of the outbound buffer is also used for sending and > + * receiving asynchronous messages. An asynchronous message is independent of > + * the current object invocation (i.e., contents of the inbound buffer) or > + * callback request (i.e., the head of the outbound buffer); see > + * qcomtee_get_async_buffer(). It is used by endpoints (QTEE or kernel) as an > + * optimization to reduce the number of context switches between the secure and > + * non-secure worlds. > + * > + * For instance, QTEE never sends an explicit callback request to release an > + * object in the kernel. Instead, it sends asynchronous release messages in the > + * outbound buffer when QTEE returns from the previous direct object invocation, > + * or appends asynchronous release messages after the current callback request. > + * > + * QTEE supports two types of arguments in a message: buffer and object > + * arguments. Depending on the direction of data flow, they could be input > + * buffer (IO) to QTEE, output buffer (OB) from QTEE, input object (IO) to QTEE, > + * or output object (OO) from QTEE. Object arguments hold object IDs. Buffer > + * arguments hold (offset, size) pairs into the inbound or outbound buffers. > + * > + * QTEE holds an object table for objects it hosts and exposes to the kernel. > + * An object ID is an index to the object table in QTEE. > + * > + * For the direct object invocation message format in the inbound buffer, see > + * &struct qcomtee_msg_object_invoke. For the callback request message format > + * in the outbound buffer, see &struct qcomtee_msg_callback. For the message > + * format for asynchronous messages in the outbound buffer, see > + * &struct qcomtee_async_msg_hdr. > + */ > + > +/** > + * define QCOMTEE_MSG_OBJECT_NS_BIT - Non-secure bit > + * > + * Object ID is a globally unique 32-bit number. IDs referencing objects > + * in the kernel should have %QCOMTEE_MSG_OBJECT_NS_BIT set. > + */ > +#define QCOMTEE_MSG_OBJECT_NS_BIT BIT(31) > + > +/* Static object IDs recognized by QTEE. */ > +#define QCOMTEE_MSG_OBJECT_NULL (0U) > +#define QCOMTEE_MSG_OBJECT_ROOT (1U) > + > +/* Definitions from QTEE as part of the transport protocol. */ > + > +/* qcomtee_msg_arg is an argument as recognized by QTEE. */ > +union qcomtee_msg_arg { > + struct { > + u32 offset; > + u32 size; > + } b; > + u32 o; > +}; > + > +/* BI and BO payloads in QTEE messages should be at 64-bit boundaries. */ > +#define qcomtee_msg_offset_align(o) ALIGN((o), sizeof(u64)) > + > +/* Operations for objects are 32-bit. Transport uses the upper 16 bits. */ > +#define QCOMTEE_MSG_OBJECT_OP_MASK GENMASK(15, 0) > + > +/* Reserved Operation IDs sent to QTEE: */ > +/* QCOMTEE_MSG_OBJECT_OP_RELEASE - Reduces the refcount and releases the object. > + * QCOMTEE_MSG_OBJECT_OP_RETAIN - Increases the refcount. > + * > + * These operation IDs are valid for all objects. > + */ > + > +#define QCOMTEE_MSG_OBJECT_OP_RELEASE (QCOMTEE_MSG_OBJECT_OP_MASK - 0) > +#define QCOMTEE_MSG_OBJECT_OP_RETAIN (QCOMTEE_MSG_OBJECT_OP_MASK - 1) > + > +/* Subset of operations supported by QTEE root object. */ > + > +#define QCOMTEE_ROOT_OP_REGISTER_WITH_CREDENTIALS 5 > +#define QCOMTEE_ROOT_OP_NOTIFY_DOMAIN_CHANGE 4 > +#define QCOMTEE_ROOT_OP_ADCI_ACCEPT 8 > +#define QCOMTEE_ROOT_OP_ADCI_SHUTDOWN 9 > + > +/* Response types as returned from qcomtee_object_invoke_ctx_invoke(). */ > + > +/* The message contains a callback request. */ > +#define QCOMTEE_RESULT_INBOUND_REQ_NEEDED 3 > + > +/** > + * struct qcomtee_msg_object_invoke - Direct object invocation message. > + * @ctx: object ID hosted in QTEE. > + * @op: operation for the object. > + * @counts: number of different types of arguments in @args. > + * @args: array of arguments. > + * > + * @counts consists of 4 * 4-bit fields. Bits 0 - 3 represent the number of > + * input buffers, bits 4 - 7 represent the number of output buffers, > + * bits 8 - 11 represent the number of input objects, and bits 12 - 15 > + * represent the number of output objects. The remaining bits should be zero. > + * > + * The maximum number of arguments of each type is defined by > + * %QCOMTEE_ARGS_PER_TYPE. > + */ > +struct qcomtee_msg_object_invoke { > + u32 cxt; > + u32 op; > + u32 counts; > + union qcomtee_msg_arg args[]; > +}; > + > +/** > + * struct qcomtee_msg_callback - Callback request message. > + * @result: result of operation @op on the object referenced by @cxt. > + * @cxt: object ID hosted in the kernel. > + * @op: operation for the object. > + * @counts: number of different types of arguments in @args. > + * @args: array of arguments. > + * > + * For details of @counts, see &qcomtee_msg_object_invoke.counts. > + */ > +struct qcomtee_msg_callback { > + u32 result; > + u32 cxt; > + u32 op; > + u32 counts; > + union qcomtee_msg_arg args[]; > +}; > + > +/* Offset in the message for the beginning of the buffer argument's contents. */ > +#define qcomtee_msg_buffer_args(t, n) \ > + qcomtee_msg_offset_align(struct_size_t(t, args, n)) > +/* Pointer to the beginning of a buffer argument's content at an offset. */ > +#define qcomtee_msg_offset_to_ptr(m, off) ((void *)&((char *)(m))[(off)]) > + > +/* Some helpers to manage msg.counts. */ > + > +#define QCOMTEE_MSG_NUM_IB(x) ((x) & 0xfU) > +#define QCOMTEE_MSG_NUM_OB(x) (((x) >> 4) & 0xfU) > +#define QCOMTEE_MSG_NUM_IO(x) (((x) >> 8) & 0xfU) > +#define QCOMTEE_MSG_NUM_OO(x) (((x) >> 12) & 0xfU) > + > +#define QCOMTEE_MSG_IDX_IB(x) (0U) > +#define QCOMTEE_MSG_IDX_OB(x) (QCOMTEE_MSG_IDX_IB(x) + QCOMTEE_MSG_NUM_IB(x)) > +#define QCOMTEE_MSG_IDX_IO(x) (QCOMTEE_MSG_IDX_OB(x) + QCOMTEE_MSG_NUM_OB(x)) > +#define QCOMTEE_MSG_IDX_OO(x) (QCOMTEE_MSG_IDX_IO(x) + QCOMTEE_MSG_NUM_IO(x)) > + > +#define qcomtee_msg_for_each(i, c, type) \ > + for (i = QCOMTEE_MSG_IDX_##type(c); \ > + i < (QCOMTEE_MSG_IDX_##type(c) + QCOMTEE_MSG_NUM_##type(c)); i++) > + > +#define qcomtee_msg_for_each_input_buffer(i, m) \ > + qcomtee_msg_for_each(i, (m)->counts, IB) > +#define qcomtee_msg_for_each_output_buffer(i, m) \ > + qcomtee_msg_for_each(i, (m)->counts, OB) > +#define qcomtee_msg_for_each_input_object(i, m) \ > + qcomtee_msg_for_each(i, (m)->counts, IO) > +#define qcomtee_msg_for_each_output_object(i, m) \ > + qcomtee_msg_for_each(i, (m)->counts, OO) > + > +/* Sum of arguments in a message. */ > +#define qcomtee_msg_args(m) \ > + (QCOMTEE_MSG_IDX_OO((m)->counts) + QCOMTEE_MSG_NUM_OO((m)->counts)) > + > +static inline void qcomtee_msg_init(struct qcomtee_msg_object_invoke *msg, > + u32 cxt, u32 op, int in_buffer, > + int out_buffer, int in_object, > + int out_object) > +{ > + msg->counts |= (in_buffer & 0xfU); > + msg->counts |= ((out_buffer - in_buffer) & 0xfU) << 4; > + msg->counts |= ((in_object - out_buffer) & 0xfU) << 8; > + msg->counts |= ((out_object - in_object) & 0xfU) << 12; > + msg->cxt = cxt; > + msg->op = op; > +} > + > +/* Generic error codes. */ > +#define QCOMTEE_MSG_OK 0 /* non-specific success code. */ > +#define QCOMTEE_MSG_ERROR 1 /* non-specific error. */ > +#define QCOMTEE_MSG_ERROR_INVALID 2 /* unsupported/unrecognized request. */ > +#define QCOMTEE_MSG_ERROR_SIZE_IN 3 /* supplied buffer/string too large. */ > +#define QCOMTEE_MSG_ERROR_SIZE_OUT 4 /* supplied output buffer too small. */ > +#define QCOMTEE_MSG_ERROR_USERBASE 10 /* start of user-defined error range. */ > + > +/* Transport layer error codes. */ > +#define QCOMTEE_MSG_ERROR_DEFUNCT -90 /* object no longer exists. */ > +#define QCOMTEE_MSG_ERROR_ABORT -91 /* calling thread must exit. */ > +#define QCOMTEE_MSG_ERROR_BADOBJ -92 /* invalid object context. */ > +#define QCOMTEE_MSG_ERROR_NOSLOTS -93 /* caller's object table full. */ > +#define QCOMTEE_MSG_ERROR_MAXARGS -94 /* too many args. */ > +#define QCOMTEE_MSG_ERROR_MAXDATA -95 /* buffers too large. */ > +#define QCOMTEE_MSG_ERROR_UNAVAIL -96 /* the request could not be processed. */ > +#define QCOMTEE_MSG_ERROR_KMEM -97 /* kernel out of memory. */ > +#define QCOMTEE_MSG_ERROR_REMOTE -98 /* local method sent to remote object. */ > +#define QCOMTEE_MSG_ERROR_BUSY -99 /* Object is busy. */ > +#define QCOMTEE_MSG_ERROR_TIMEOUT -103 /* Call Back Object invocation timed out. */ > + > +static inline void qcomtee_msg_set_result(struct qcomtee_msg_callback *cb_msg, > + int err) > +{ > + if (!err) { > + cb_msg->result = QCOMTEE_MSG_OK; > + } else if (err < 0) { > + /* If err < 0, then it is a transport error. */ > + switch (err) { > + case -ENOMEM: > + cb_msg->result = QCOMTEE_MSG_ERROR_KMEM; > + break; > + case -ENODEV: > + cb_msg->result = QCOMTEE_MSG_ERROR_DEFUNCT; > + break; > + case -ENOSPC: > + case -EBUSY: > + cb_msg->result = QCOMTEE_MSG_ERROR_BUSY; > + break; > + case -EBADF: > + case -EINVAL: > + cb_msg->result = QCOMTEE_MSG_ERROR_UNAVAIL; > + break; > + default: > + cb_msg->result = QCOMTEE_MSG_ERROR; > + } > + } else { > + /* If err > 0, then it is user defined error, pass it as is. */ > + cb_msg->result = err; > + } > +} > + > +#endif /* QCOMTEE_MSG_H */ > diff --git a/drivers/tee/qcomtee/qcomtee_private.h b/drivers/tee/qcomtee/qcomtee_private.h > new file mode 100644 > index 000000000000..ab3acad40359 > --- /dev/null > +++ b/drivers/tee/qcomtee/qcomtee_private.h > @@ -0,0 +1,222 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef QCOMTEE_PRIVATE_H > +#define QCOMTEE_PRIVATE_H > + > +#include <linux/firmware/qcom/qcom_tee.h> > +#include <linux/kobject.h> > +#include <linux/tee_core.h> > + > +#include "qcomtee_msg.h" > + > +/* Flags relating to object reference. */ > +#define QCOMTEE_OBJREF_FLAG_TEE BIT(0) > +#define QCOMTEE_OBJREF_FLAG_USER BIT(1) > + > +/** > + * struct qcomtee - Main service struct. > + * @teedev: client device. > + * @pool: shared memory pool. > + * @ctx: driver private context. > + * @wq: workqueue for QTEE async operations. > + */ > +struct qcomtee { > + struct tee_device *teedev; > + struct tee_shm_pool *pool; > + struct tee_context *ctx; > + struct workqueue_struct *wq; > +}; > + > +struct qcomtee_object *qcomtee_idx_erase(u32 idx); > +void qcomtee_qtee_object_free(struct qcomtee_object *object); > +void qcomtee_fetch_async_reqs(struct qcomtee_object_invoke_ctx *oic); > +void qcomtee_release_tee_object(struct qcomtee_object *object); > + > +struct tee_shm_pool *qcomtee_shm_pool_alloc(void); > +void qcomtee_msg_buffers_free(struct qcomtee_object_invoke_ctx *oic); > +int qcomtee_msg_buffers_alloc(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_arg *u); > + > +int qcomtee_object_invoke_ctx_invoke(struct qcomtee_object_invoke_ctx *oic, > + int *result, u64 *res_type); > + > +/** > + * qcomtee_object_do_invoke_internal() - Submit an invocation for an object. > + * @oic: context to use for the current invocation. > + * @object: object being invoked. > + * @op: requested operation on the object. > + * @u: array of arguments for the current invocation. > + * @result: result returned from QTEE. > + * > + * The caller is responsible for keeping track of the refcount for each > + * object, including @object. On return, the caller loses ownership of all > + * input objects of type %QCOMTEE_OBJECT_TYPE_CB. > + * > + * Return: On success, returns 0. On error, returns -EAGAIN if invocation > + * failed and the user may retry the invocation, -ENODEV on fatal failure. > + */ > +int qcomtee_object_do_invoke_internal(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object *object, u32 op, > + struct qcomtee_arg *u, int *result); > + > +/** > + * struct qcomtee_context_data - Clients' or supplicants' context. > + * @qtee_objects_idr: QTEE objects in this context. > + * @qtee_lock: spinlock for @qtee_objects_idr. > + * @reqs_idr: requests currently being processed in this context. > + * @reqs_list: FIFO for requests. > + * @reqs_lock: mutex for @reqs_idr, @reqs_list and request states. > + * @req_c: completion used when the supplicant is waiting for requests. > + * @released: state of this context. > + */ > +struct qcomtee_context_data { > + struct idr qtee_objects_idr; > + /* Synchronize access to @qtee_objects_idr. */ > + spinlock_t qtee_lock; > + > + struct idr reqs_idr; > + struct list_head reqs_list; > + /* Synchronize access to @reqs_idr, @reqs_list and updating requests states. */ > + struct mutex reqs_lock; > + > + struct completion req_c; > + > + int released; Should this rather be a bool, or an enum depending on how many different states it's supposed to have? > +}; > + > +/** > + * qcomtee_context_add_qtee_object() - Add a QTEE object to the context. > + * @param: TEE parameter representing @object. > + * @object: QTEE object. > + * @ctx: context to add the object. > + * > + * It assumes @object is %QCOMTEE_OBJECT_TYPE_TEE and the caller has already > + * issued qcomtee_object_get() for @object. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_context_add_qtee_object(struct tee_param *param, > + struct qcomtee_object *object, > + struct tee_context *ctx); > + > +/* Retrieve the QTEE object added with qcomtee_context_add_qtee_object(). */ > +int qcomtee_context_find_qtee_object(struct qcomtee_object **object, > + struct tee_param *param, > + struct tee_context *ctx); > + > +/** > + * qcomtee_context_del_qtee_object() - Delete a QTEE object from the context. > + * @param: TEE parameter representing @object. > + * @ctx: context for deleting the object. > + * > + * The @param has been initialized by qcomtee_context_add_qtee_object(). > + */ > +void qcomtee_context_del_qtee_object(struct tee_param *param, > + struct tee_context *ctx); > + > +/** > + * qcomtee_objref_to_arg() - Convert OBJREF parameter to QTEE argument. > + * @arg: QTEE argument. > + * @param: TEE parameter. > + * @ctx: context in which the conversion should happen. > + * > + * It assumes @param is an OBJREF. > + * It does not set @arg.type; the caller should initialize it to a correct > + * &enum qcomtee_arg_type value. It gets the object's refcount in @arg; > + * the caller should manage to put it afterward. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_objref_to_arg(struct qcomtee_arg *arg, struct tee_param *param, > + struct tee_context *ctx); > + > +/** > + * qcomtee_objref_from_arg() - Convert QTEE argument to OBJREF param. > + * @param: TEE parameter. > + * @arg: QTEE argument. > + * @ctx: context in which the conversion should happen. > + * > + * It assumes @arg is of %QCOMTEE_ARG_TYPE_IO or %QCOMTEE_ARG_TYPE_OO. > + * It does not set @param.attr; the caller should initialize it to a > + * correct type. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_objref_from_arg(struct tee_param *param, struct qcomtee_arg *arg, > + struct tee_context *ctx); > + > +/* OBJECTS: */ > + > +/* (1) User Object API. */ > + > +/* Is it a user object? */ > +int is_qcomtee_user_object(struct qcomtee_object *object); > + > +/* Set the user object's 'notify on release' flag. */ > +void qcomtee_user_object_set_notify(struct qcomtee_object *object, bool notify); > + > +/* This is called when there are no more users for the ctxdata. */ > +void qcomtee_requests_destroy(struct qcomtee_context_data *ctxdata); > + > +/** > + * qcomtee_user_param_to_object() - OBJREF parameter to &struct qcomtee_object. > + * @object: object returned. > + * @param: TEE parameter. > + * @ctx: context in which the conversion should happen. > + * > + * @param is an OBJREF with %QCOMTEE_OBJREF_FLAG_USER flags. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_user_param_to_object(struct qcomtee_object **object, > + struct tee_param *param, > + struct tee_context *ctx); > + > +/* Reverse what qcomtee_user_param_to_object() does. */ > +int qcomtee_user_param_from_object(struct tee_param *param, > + struct qcomtee_object *object, > + struct tee_context *ctx); > + > +struct qcomtee_user_object_request_data { > + int id; /* ID assigned to the request. */ > + u64 object_id; /* Object ID being invoked by QTEE. */ > + u32 op; /* Requested operation on object. */ > + int np; /* Number of parameters in the request.*/ > +}; > + > +/** > + * qcomtee_user_object_select() - Select a request for a user object. > + * @ctx: context to look for a user object. > + * @params: parameters for @op. > + * @num_params: number of elements in the parameter array. > + * @uaddr: user buffer for output UBUF parameters. > + * @size: size of user buffer @uaddr. > + * @data: information for the selected request. > + * > + * @params is filled along with @data for the selected request. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_user_object_select(struct tee_context *ctx, > + struct tee_param *params, int num_params, > + void __user *uaddr, size_t size, > + struct qcomtee_user_object_request_data *data); > + > +/** > + * qcomtee_user_object_submit() - Submit a response for a user object. > + * @ctx: context to look for a user object. > + * @params: returned parameters. > + * @num_params: number of elements in the parameter array. > + * @req_id: request ID for the response. > + * @errno: result of user object invocation. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_user_object_submit(struct tee_context *ctx, > + struct tee_param *params, int num_params, > + int req_id, int errno); > + > +#endif /* QCOMTEE_PRIVATE_H */ > diff --git a/drivers/tee/qcomtee/release.c b/drivers/tee/qcomtee/release.c > new file mode 100644 > index 000000000000..7d149ef3d26f > --- /dev/null > +++ b/drivers/tee/qcomtee/release.c > @@ -0,0 +1,48 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include "qcomtee_private.h" > + > +static void qcomtee_destroy_user_object(struct work_struct *work) > +{ > + struct qcomtee_object *object; > + struct qcomtee *qcomtee; > + int ret, result; > + > + static struct qcomtee_object_invoke_ctx oic; > + /* RELEASE does not require any argument. */ > + static struct qcomtee_arg args[] = { { .type = QCOMTEE_ARG_TYPE_INV } }; > + > + object = container_of(work, struct qcomtee_object, work); > + qcomtee = tee_get_drvdata(object->info.qcomtee_async_ctx->teedev); > + /* Get the TEE context used for asynchronous operations. */ > + oic.ctx = object->info.qcomtee_async_ctx; > + > + ret = qcomtee_object_do_invoke_internal(&oic, object, > + QCOMTEE_MSG_OBJECT_OP_RELEASE, > + args, &result); > + > + /* Is it safe to retry the release? */ > + if (ret == -EAGAIN) { > + queue_work(qcomtee->wq, &object->work); > + } else { > + if (ret || result) > + pr_err("%s: %s release failed, ret = %d (%x).\n", > + __func__, qcomtee_object_name(object), ret, > + result); > + > + qcomtee_qtee_object_free(object); > + } > +} > + > +/* qcomtee_release_tee_object puts object in release work queue. */ > +void qcomtee_release_tee_object(struct qcomtee_object *object) > +{ > + struct qcomtee *qcomtee = > + tee_get_drvdata(object->info.qcomtee_async_ctx->teedev); > + > + INIT_WORK(&object->work, qcomtee_destroy_user_object); > + queue_work(qcomtee->wq, &object->work); > +} > diff --git a/drivers/tee/qcomtee/shm.c b/drivers/tee/qcomtee/shm.c > new file mode 100644 > index 000000000000..998aabe96434 > --- /dev/null > +++ b/drivers/tee/qcomtee/shm.c > @@ -0,0 +1,149 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/firmware/qcom/qcom_tzmem.h> > +#include <linux/mm.h> > + > +#include "qcomtee_msg.h" > +#include "qcomtee_private.h" > + > +/** > + * define MAX_OUTBOUND_BUFFER_SIZE - Maximum size of outbound buffers. > + * > + * The size of outbound buffer depends on QTEE callback requests. > + * If an invocation requires any size larger than %MAX_OUTBOUND_BUFFER_SIZE, > + * the user should probably use some other form of shared memory with QTEE. > + */ > +#define MAX_OUTBOUND_BUFFER_SIZE SZ_4K > + > +/* Same as %MAX_OUTBOUND_BUFFER_SIZE but for inbound buffer. */ > +#define MAX_INBOUND_BUFFER_SIZE SZ_4M > + > +/** > + * qcomtee_msg_buffers_alloc() - Allocate inbound and outbound buffers. > + * @oic: context to use for the current invocation. > + * @u: array of arguments for the current invocation. > + * > + * It calculates the size of inbound and outbound buffers based on the > + * arguments in @u. It allocates the buffers from the teedev pool. > + * > + * Return: On success, returns 0. On error, returns < 0. > + */ > +int qcomtee_msg_buffers_alloc(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_arg *u) > +{ > + struct tee_context *ctx = oic->ctx; > + struct tee_shm *shm; > + size_t size; > + int i; > + > + /* Start offset in a message for buffer arguments. */ > + size = qcomtee_msg_buffer_args(struct qcomtee_msg_object_invoke, > + qcomtee_args_len(u)); > + if (size > MAX_INBOUND_BUFFER_SIZE) > + return -EINVAL; > + > + /* Add size of IB arguments. */ > + qcomtee_arg_for_each_input_buffer(i, u) { > + size = size_add(size, qcomtee_msg_offset_align(u[i].b.size)); > + if (size > MAX_INBOUND_BUFFER_SIZE) > + return -EINVAL; > + } > + > + /* Add size of OB arguments. */ > + qcomtee_arg_for_each_output_buffer(i, u) { > + size = size_add(size, qcomtee_msg_offset_align(u[i].b.size)); > + if (size > MAX_INBOUND_BUFFER_SIZE) > + return -EINVAL; > + } > + > + shm = tee_shm_alloc_priv_buf(ctx, size); > + if (IS_ERR(shm)) > + return PTR_ERR(shm); > + > + /* Allocate inbound buffer. */ > + oic->in_shm = shm; > + shm = tee_shm_alloc_priv_buf(ctx, MAX_OUTBOUND_BUFFER_SIZE); > + if (IS_ERR(shm)) { > + tee_shm_free(oic->in_shm); > + > + return PTR_ERR(shm); > + } > + /* Allocate outbound buffer. */ > + oic->out_shm = shm; > + > + oic->in_msg.addr = tee_shm_get_va(oic->in_shm, 0); > + oic->in_msg.size = tee_shm_get_size(oic->in_shm); > + oic->out_msg.addr = tee_shm_get_va(oic->out_shm, 0); > + oic->out_msg.size = tee_shm_get_size(oic->out_shm); > + /* QTEE assume unused buffers are zeroed. */ > + memzero_explicit(oic->in_msg.addr, oic->in_msg.size); > + memzero_explicit(oic->out_msg.addr, oic->out_msg.size); > + > + return 0; > +} > + > +void qcomtee_msg_buffers_free(struct qcomtee_object_invoke_ctx *oic) > +{ > + tee_shm_free(oic->in_shm); > + tee_shm_free(oic->out_shm); > +} > + > +/* Dynamic shared memory pool based on tee_dyn_shm_alloc_helper(). */ > + > +static int qcomtee_shm_register(struct tee_context *ctx, struct tee_shm *shm, > + struct page **pages, size_t num_pages, > + unsigned long start) > +{ > + return qcom_tzmem_shm_bridge_create(shm->paddr, shm->size, > + &shm->sec_world_id); > +} > + > +static int qcomtee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm) > +{ > + qcom_tzmem_shm_bridge_delete(shm->sec_world_id); > + > + return 0; > +} > + > +static int pool_op_alloc(struct tee_shm_pool *pool, struct tee_shm *shm, > + size_t size, size_t align) > +{ > + if (!(shm->flags & TEE_SHM_PRIV)) > + return -ENOMEM; > + > + return tee_dyn_shm_alloc_helper(shm, size, align, qcomtee_shm_register); > +} > + > +static void pool_op_free(struct tee_shm_pool *pool, struct tee_shm *shm) > +{ > + tee_dyn_shm_free_helper(shm, qcomtee_shm_unregister); > +} > + > +static void pool_op_destroy_pool(struct tee_shm_pool *pool) > +{ > + kfree(pool); > +} > + > +static const struct tee_shm_pool_ops pool_ops = { > + .alloc = pool_op_alloc, > + .free = pool_op_free, > + .destroy_pool = pool_op_destroy_pool, > +}; > + > +struct tee_shm_pool *qcomtee_shm_pool_alloc(void) > +{ > + struct tee_shm_pool *pool; > + > + pool = kzalloc(sizeof(*pool), GFP_KERNEL); > + if (!pool) > + return ERR_PTR(-ENOMEM); > + > + pool->ops = &pool_ops; > + > + return pool; > +} > diff --git a/drivers/tee/qcomtee/user_obj.c b/drivers/tee/qcomtee/user_obj.c > new file mode 100644 > index 000000000000..95d22e720de7 > --- /dev/null > +++ b/drivers/tee/qcomtee/user_obj.c > @@ -0,0 +1,710 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/slab.h> > +#include "qcomtee_private.h" > + > +/** > + * DOC: User Objects aka Supplicants > + * > + * Any userspace process with access to the TEE device file can behave as a > + * supplicant by creating a user object. Any TEE parameter of type OBJREF with > + * %QCOMTEE_OBJREF_FLAG_USER flag set is considered a user object. > + * > + * A supplicant uses qcomtee_user_object_select() (i.e. TEE_IOC_SUPPL_RECV) to > + * receive a QTEE user object request and qcomtee_user_object_submit() > + * (i.e. TEE_IOC_SUPPL_SEND) to submit a response. QTEE expects to receive the > + * response, including OB and OO in a specific order in the message; parameters > + * submitted with qcomtee_user_object_submit() should maintain this order. > + */ > + > +/** > + * struct qcomtee_user_object - User object. > + * @object: &struct qcomtee_object representing this user object. > + * @ctx: context for which the user object is defined. > + * @object_id: object ID in @ctx. > + * @nor: notify on release. > + * > + * Any object managed in userspace is represented by this struct. > + * If @nor is set, a notification message is sent back to userspace > + * upon release. > + */ > +struct qcomtee_user_object { > + struct qcomtee_object object; > + struct tee_context *ctx; > + u64 object_id; > + bool nor; > +}; > + > +#define to_qcomtee_user_object(o) \ > + container_of((o), struct qcomtee_user_object, object) > + > +static struct qcomtee_object_operations qcomtee_user_object_ops; > + > +/* Is it a user object? */ > +int is_qcomtee_user_object(struct qcomtee_object *object) > +{ > + return object != NULL_QCOMTEE_OBJECT && > + typeof_qcomtee_object(object) == QCOMTEE_OBJECT_TYPE_CB && > + object->ops == &qcomtee_user_object_ops; > +} > + > +/* Set the user object's 'notify on release' flag. */ > +void qcomtee_user_object_set_notify(struct qcomtee_object *object, bool notify) > +{ > + if (is_qcomtee_user_object(object)) > + to_qcomtee_user_object(object)->nor = notify; > +} > + > +/* Supplicant Requests: */ > + > +/** > + * enum qcomtee_req_state - Current state of request. > + * @QCOMTEE_REQ_QUEUED: Request is waiting for supplicant. > + * @QCOMTEE_REQ_PROCESSING: Request has been picked by the supplicant. > + * @QCOMTEE_REQ_PROCESSED: Response has been submitted for the request. > + */ > +enum qcomtee_req_state { > + QCOMTEE_REQ_QUEUED = 1, > + QCOMTEE_REQ_PROCESSING, > + QCOMTEE_REQ_PROCESSED, > +}; > + > +/* User requests sent to supplicants. */ > +struct qcomtee_ureq { > + enum qcomtee_req_state state; > + > + /* User Request: */ > + int req_id; > + u64 object_id; > + u32 op; > + struct qcomtee_arg *args; > + int errno; > + > + struct list_head node; > + struct completion c; /* Completion for whoever wait. */ > +}; > + > +/* > + * Placeholder for a PROCESSING request in qcomtee_context.reqs_idr. > + * > + * If the thread that calls qcomtee_object_invoke() dies and the supplicant > + * is processing the request, we replace the entry in qcomtee_context.reqs_idr > + * with empty_ureq. This ensures that (1) the req_id remains busy and is not > + * reused, and (2) the supplicant fails to submit the response and performs > + * the necessary rollback. > + */ > +static struct qcomtee_ureq empty_ureq = { .state = QCOMTEE_REQ_PROCESSING }; > + > +/* Enqueue a user request for a context and assign a request ID. */ > +static int ureq_enqueue(struct qcomtee_context_data *ctxdata, > + struct qcomtee_ureq *ureq) > +{ > + int ret; > + > + guard(mutex)(&ctxdata->reqs_lock); > + /* Supplicant is dying. */ > + if (ctxdata->released) > + return -ENODEV; > + > + /* Allocate an ID and queue the request. */ > + ret = idr_alloc(&ctxdata->reqs_idr, ureq, 0, 0, GFP_KERNEL); > + if (ret < 0) > + return ret; > + > + ureq->req_id = ret; > + ureq->state = QCOMTEE_REQ_QUEUED; > + list_add_tail(&ureq->node, &ctxdata->reqs_list); > + > + return 0; > +} > + > +/** > + * ureq_dequeue() - Dequeue a user request from a context. > + * @ctxdata: context data for a context to dequeue the request. > + * @req_id: ID of the request to be dequeued. > + * > + * It dequeues a user request and releases its request ID. > + * > + * Context: The caller should hold &qcomtee_context_data->reqs_lock. > + * Return: Returns the user request associated with this ID; otherwise, NULL. > + */ > +static struct qcomtee_ureq *ureq_dequeue(struct qcomtee_context_data *ctxdata, > + int req_id) > +{ > + struct qcomtee_ureq *ureq; > + > + ureq = idr_remove(&ctxdata->reqs_idr, req_id); > + if (ureq == &empty_ureq || !ureq) > + return NULL; > + > + list_del(&ureq->node); > + > + return ureq; > +} > + > +/** > + * ureq_replace() - Replace a user request. > + * @ctxdata: context data for a context to replace the request. > + * @ureq: request to be replaced. > + * @ureq_new: request to replace it with. > + * > + * Context: The caller should hold &qcomtee_context_data->reqs_lock. > + */ > +static void ureq_replace(struct qcomtee_context_data *ctxdata, > + struct qcomtee_ureq *ureq, > + struct qcomtee_ureq *ureq_new) > +{ > + idr_replace(&ctxdata->reqs_idr, ureq_new, ureq->req_id); > + /* ureq does not have a valid ID anymore; remove it from the queue. */ > + list_del(&ureq->node); > +} > + > +/** > + * ureq_select() - Select the next request in a context. > + * @ctxdata: context data for a context to pop a request. > + * @ubuf_size: size of the available buffer for UBUF parameters. > + * @num_params: number of entries for the TEE parameter array. > + * > + * It checks if @num_params is large enough to fit the next request arguments. > + * It checks if @ubuf_size is large enough to fit IB buffer arguments. > + * > + * Context: The caller should hold &qcomtee_context_data->reqs_lock. > + * Return: On success, returns a request; > + * on failure, returns NULL and ERR_PTR. > + */ > +static struct qcomtee_ureq *ureq_select(struct qcomtee_context_data *ctxdata, > + size_t ubuf_size, int num_params) > +{ > + struct qcomtee_ureq *req, *ureq = NULL; > + struct qcomtee_arg *u; > + int i; > + > + /* Find the a queued request. */ > + list_for_each_entry(req, &ctxdata->reqs_list, node) { > + if (req->state == QCOMTEE_REQ_QUEUED) { > + ureq = req; > + break; > + } > + } > + > + if (!ureq) > + return NULL; > + > + u = ureq->args; > + /* (1) Is there enough TEE parameters? */ > + if (num_params < qcomtee_args_len(u)) > + return ERR_PTR(-EINVAL); > + /* (2) Is there enough space to pass input buffers? */ > + qcomtee_arg_for_each_input_buffer(i, u) { > + ubuf_size = size_sub(ubuf_size, u[i].b.size); > + if (ubuf_size == SIZE_MAX) > + return ERR_PTR(-EINVAL); > + > + ubuf_size = round_down(ubuf_size, 8); > + } > + > + return ureq; > +} > + > +/* This is called when there are no more users for the ctxdata. */ > +void qcomtee_requests_destroy(struct qcomtee_context_data *ctxdata) > +{ > + struct qcomtee_ureq *req, *ureq; > + > + guard(mutex)(&ctxdata->reqs_lock); > + /* So ureq_enqueue() refuses new requests from QTEE. */ > + ctxdata->released = 1; Magic number? > + > + list_for_each_entry_safe(ureq, req, &ctxdata->reqs_list, node) { > + ureq_dequeue(ctxdata, ureq->req_id); > + > + /* > + * Terminate requests. > + * Depending on who enqueues the ureq, there may be someone > + * waiting for the response or not: > + */ > + if (ureq->op != QCOMTEE_MSG_OBJECT_OP_RELEASE) { > + /* (1) enqueued by qcomtee_user_object_dispatch(). */ > + ureq->state = QCOMTEE_REQ_PROCESSED; > + ureq->errno = -ENODEV; > + > + complete(&ureq->c); > + } else { > + /* (2) enqueued by qcomtee_user_object_release(). */ > + kfree(ureq); > + } > + } > +} > + > +/* User Object API. */ > + > +/* User object dispatcher. */ > +static int qcomtee_user_object_dispatch(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object *object, u32 op, > + struct qcomtee_arg *args) > +{ > + struct qcomtee_user_object *uo = to_qcomtee_user_object(object); > + struct qcomtee_context_data *ctxdata = uo->ctx->data; > + struct qcomtee_ureq *ureq __free(kfree) = NULL; > + int errno; > + > + ureq = kzalloc(sizeof(*ureq), GFP_KERNEL); > + if (!ureq) > + return -ENOMEM; > + > + init_completion(&ureq->c); > + ureq->object_id = uo->object_id; > + ureq->op = op; > + ureq->args = args; > + > + /* Queue the request. */ > + if (ureq_enqueue(ctxdata, ureq)) > + return -ENODEV; > + > + /* Wakeup supplicant to process it. */ > + complete(&ctxdata->req_c); > + > + /* > + * Wait for the supplicant to process the request. > + * > + * The supplicant is expected to process the request in a timely manner. > + * We wait as KILLABLE in case the supplicant and invoke thread are > + * both running from the same user process; otherwise, the process > + * will be stuck on a fatal signal. > + */ > + if (!wait_for_completion_state(&ureq->c, > + TASK_KILLABLE | TASK_FREEZABLE)) { > + errno = ureq->errno; > + /* On success, notify() frees the request. */ > + if (!errno) > + oic->data = no_free_ptr(ureq); > + } else { > + enum qcomtee_req_state prev_state; > + > + errno = -ENODEV; > + > + scoped_guard(mutex, &ctxdata->reqs_lock) { > + prev_state = ureq->state; > + > + /* Replace with empty_ureq to keep req_id reserved. */ > + if (prev_state == QCOMTEE_REQ_PROCESSING) > + ureq_replace(ctxdata, ureq, &empty_ureq); > + > + /* Remove as supplicant has never seen this request. */ > + else if (prev_state == QCOMTEE_REQ_QUEUED) > + ureq_dequeue(ctxdata, ureq->req_id); > + } > + > + /* Supplicant did some work, we should not discard it. */ > + if (prev_state == QCOMTEE_REQ_PROCESSED) { > + errno = ureq->errno; > + /* On success, notify() frees the request. */ > + if (!errno) > + oic->data = no_free_ptr(ureq); > + } > + } > + > + return errno; > +} > + > +/* This is called after submitting the dispatcher response. */ > +static void qcomtee_user_object_notify(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object *unused_object, > + int err) > +{ > + struct qcomtee_ureq *ureq = oic->data; > + struct qcomtee_arg *u = ureq->args; > + int i; > + > + qcomtee_arg_for_each_output_object(i, u) { > + /* > + * If err, there was a transport issue, and QTEE did not > + * receive the response for the dispatcher. Release the callback > + * object created for QTEE, in addition to the copies of > + * objects kept for the drivers. > + */ > + if (err && > + (typeof_qcomtee_object(u[i].o) == QCOMTEE_OBJECT_TYPE_CB)) > + qcomtee_object_put(u[i].o); > + qcomtee_object_put(u[i].o); > + } > + > + kfree(ureq); > +} > + > +static void qcomtee_user_object_release(struct qcomtee_object *object) > +{ > + struct qcomtee_user_object *uo = to_qcomtee_user_object(object); > + struct qcomtee_context_data *ctxdata = uo->ctx->data; > + struct qcomtee_ureq *ureq; > + > + /* RELEASE does not require any argument. */ > + static struct qcomtee_arg args[] = { { .type = QCOMTEE_ARG_TYPE_INV } }; > + > + if (!uo->nor) > + goto out_no_notify; > + > + ureq = kzalloc(sizeof(*ureq), GFP_KERNEL); > + if (!ureq) > + goto out_no_notify; > + > + /* QUEUE a release request: */ > + ureq->object_id = uo->object_id; > + ureq->op = QCOMTEE_MSG_OBJECT_OP_RELEASE; > + ureq->args = args; > + if (ureq_enqueue(ctxdata, ureq)) { > + kfree(ureq); > + /* Ignore the notification if it cannot be queued. */ > + goto out_no_notify; > + } > + > + complete(&ctxdata->req_c); > + > +out_no_notify: > + teedev_ctx_put(uo->ctx); > + kfree(uo); > +} > + > +static struct qcomtee_object_operations qcomtee_user_object_ops = { > + .release = qcomtee_user_object_release, > + .notify = qcomtee_user_object_notify, > + .dispatch = qcomtee_user_object_dispatch, > +}; > + > +/** > + * qcomtee_user_param_to_object() - OBJREF parameter to &struct qcomtee_object. > + * @object: object returned. > + * @param: TEE parameter. > + * @ctx: context in which the conversion should happen. > + * > + * @param is an OBJREF with %QCOMTEE_OBJREF_FLAG_USER flags. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_user_param_to_object(struct qcomtee_object **object, > + struct tee_param *param, > + struct tee_context *ctx) > +{ > + struct qcomtee_user_object *user_object __free(kfree) = NULL; > + int err; > + > + user_object = kzalloc(sizeof(*user_object), GFP_KERNEL); > + if (!user_object) > + return -ENOMEM; > + > + user_object->ctx = ctx; > + user_object->object_id = param->u.objref.id; > + /* By default, always notify userspace upon release. */ > + user_object->nor = true; > + err = qcomtee_object_user_init(&user_object->object, > + QCOMTEE_OBJECT_TYPE_CB, > + &qcomtee_user_object_ops, "uo-%lu", > + param->u.objref.id); > + if (err) > + return err; > + > + teedev_ctx_get(ctx); > + > + *object = &no_free_ptr(user_object)->object; > + > + return 0; > +} > + > +/* Reverse what qcomtee_user_param_to_object() does. */ > +int qcomtee_user_param_from_object(struct tee_param *param, > + struct qcomtee_object *object, > + struct tee_context *ctx) > +{ > + struct qcomtee_user_object *uo; > + > + uo = to_qcomtee_user_object(object); > + /* Ensure the object is in the same context as the caller. */ > + if (uo->ctx != ctx) > + return -EINVAL; > + > + param->u.objref.id = uo->object_id; > + param->u.objref.flags = QCOMTEE_OBJREF_FLAG_USER; > + > + /* User objects are valid in userspace; do not keep a copy. */ > + qcomtee_object_put(object); > + > + return 0; > +} > + > +/** > + * qcomtee_cb_params_from_args() - Convert QTEE arguments to TEE parameters. > + * @params: TEE parameters. > + * @u: QTEE arguments. > + * @num_params: number of elements in the parameter array. > + * @ubuf_addr: user buffer for arguments of type %QCOMTEE_ARG_TYPE_IB. > + * @ubuf_size: size of the user buffer. > + * @ctx: context in which the conversion should happen. > + * > + * It expects @params to have enough entries for @u. Entries in @params are of > + * %TEE_IOCTL_PARAM_ATTR_TYPE_NONE. > + * > + * Return: On success, returns the number of input parameters; > + * on failure, returns < 0. > + */ > +static int qcomtee_cb_params_from_args(struct tee_param *params, > + struct qcomtee_arg *u, int num_params, > + void __user *ubuf_addr, size_t ubuf_size, > + struct tee_context *ctx) > +{ > + int i, np; > + void __user *uaddr; > + > + qcomtee_arg_for_each(i, u) { > + switch (u[i].type) { > + case QCOMTEE_ARG_TYPE_IB: > + params[i].attr = TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT; > + > + /* Underflow already checked in ureq_select(). */ > + ubuf_size = round_down(ubuf_size - u[i].b.size, 8); > + uaddr = (void *__user)(ubuf_addr + ubuf_size); > + > + params[i].u.ubuf.uaddr = uaddr; > + params[i].u.ubuf.size = u[i].b.size; > + if (copy_to_user(params[i].u.ubuf.uaddr, u[i].b.addr, > + u[i].b.size)) > + goto out_failed; > + > + break; > + case QCOMTEE_ARG_TYPE_OB: > + params[i].attr = TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT; > + /* Let the user knows the maximum size we expect. */ > + params[i].u.ubuf.size = u[i].b.size; > + > + break; > + case QCOMTEE_ARG_TYPE_IO: > + params[i].attr = TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_INPUT; > + if (qcomtee_objref_from_arg(&params[i], &u[i], ctx)) > + goto out_failed; > + > + break; > + case QCOMTEE_ARG_TYPE_OO: > + params[i].attr = > + TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_OUTPUT; > + > + break; > + default: /* Never get here! */ > + goto out_failed; > + } > + } > + > + return i; > + > +out_failed: > + /* Undo qcomtee_objref_from_arg(). */ > + for (np = i; np >= 0; np--) { > + if (params[np].attr == TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_INPUT) > + qcomtee_context_del_qtee_object(&params[np], ctx); > + } > + > + /* Release any IO objects not processed. */ > + for (; u[i].type; i++) { > + if (u[i].type == QCOMTEE_ARG_TYPE_IO) > + qcomtee_object_put(u[i].o); > + } > + > + return -EINVAL; > +} > + > +/** > + * qcomtee_cb_params_to_args() - Convert TEE parameters to QTEE arguments. > + * @u: QTEE arguments. > + * @params: TEE parameters. > + * @num_params: number of elements in the parameter array. > + * @ctx: context in which the conversion should happen. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +static int qcomtee_cb_params_to_args(struct qcomtee_arg *u, > + struct tee_param *params, int num_params, > + struct tee_context *ctx) > +{ > + int i; > + > + qcomtee_arg_for_each(i, u) { > + switch (u[i].type) { > + case QCOMTEE_ARG_TYPE_IB: > + if (params[i].attr != > + TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT) > + goto out_failed; > + > + break; > + case QCOMTEE_ARG_TYPE_OB: > + if (params[i].attr != > + TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT) > + goto out_failed; > + > + /* Client can not send more data than requested. */ > + if (params[i].u.ubuf.size > u[i].b.size) > + goto out_failed; > + > + if (copy_from_user(u[i].b.addr, params[i].u.ubuf.uaddr, > + params[i].u.ubuf.size)) > + goto out_failed; > + > + u[i].b.size = params[i].u.ubuf.size; > + > + break; > + case QCOMTEE_ARG_TYPE_IO: > + if (params[i].attr != > + TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_INPUT) > + goto out_failed; > + > + break; > + case QCOMTEE_ARG_TYPE_OO: > + if (params[i].attr != > + TEE_IOCTL_PARAM_ATTR_TYPE_OBJREF_OUTPUT) > + goto out_failed; > + > + if (qcomtee_objref_to_arg(&u[i], &params[i], ctx)) > + goto out_failed; > + > + break; > + default: /* Never get here! */ > + goto out_failed; > + } > + } > + > + return 0; > + > +out_failed: > + /* Undo qcomtee_objref_to_arg(). */ > + for (i--; i >= 0; i--) { > + if (u[i].type != QCOMTEE_ARG_TYPE_OO) > + continue; > + > + qcomtee_user_object_set_notify(u[i].o, false); > + if (typeof_qcomtee_object(u[i].o) == QCOMTEE_OBJECT_TYPE_CB) > + qcomtee_object_put(u[i].o); > + > + qcomtee_object_put(u[i].o); > + } > + > + return -EINVAL; > +} > + > +/** > + * qcomtee_user_object_select() - Select a request for a user object. > + * @ctx: context to look for a user object. > + * @params: parameters for @op. > + * @num_params: number of elements in the parameter array. > + * @uaddr: user buffer for output UBUF parameters. > + * @size: size of user buffer @uaddr. > + * @data: information for the selected request. > + * > + * @params is filled along with @data for the selected request. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_user_object_select(struct tee_context *ctx, > + struct tee_param *params, int num_params, > + void __user *uaddr, size_t size, > + struct qcomtee_user_object_request_data *data) > +{ > + struct qcomtee_context_data *ctxdata = ctx->data; > + struct qcomtee_ureq *ureq; > + int ret; > + > + while (1) { > + scoped_guard(mutex, &ctxdata->reqs_lock) { > + ureq = ureq_select(ctxdata, size, num_params); > + if (!ureq) > + goto wait_for_request; > + > + if (IS_ERR(ureq)) > + return PTR_ERR(ureq); > + > + /* Processing the request 'QUEUED -> PROCESSING'. */ > + ureq->state = QCOMTEE_REQ_PROCESSING; > + > + /* ''Prepare user request:'' */ > + data->id = ureq->req_id; > + data->object_id = ureq->object_id; > + data->op = ureq->op; > + ret = qcomtee_cb_params_from_args(params, ureq->args, > + num_params, uaddr, > + size, ctx); > + if (ret >= 0) > + goto done_request; > + > + /* Something is wrong with the request. */ > + > + ureq_dequeue(ctxdata, data->id); > + /* Send error to QTEE. */ > + ureq->state = QCOMTEE_REQ_PROCESSED; > + ureq->errno = ret; > + > + complete(&ureq->c); > + } > + > + continue; > +wait_for_request: > + /* Wait for a new QUEUED request. */ > + if (wait_for_completion_interruptible(&ctxdata->req_c)) > + return -ERESTARTSYS; > + } > + > +done_request: > + /* No one is waiting for the response. */ > + if (data->op == QCOMTEE_MSG_OBJECT_OP_RELEASE) { > + scoped_guard(mutex, &ctxdata->reqs_lock) > + ureq_dequeue(ctxdata, data->id); > + kfree(ureq); > + } > + > + data->np = ret; > + > + return 0; > +} > + > +/** > + * qcomtee_user_object_submit() - Submit a response for a user object. > + * @ctx: context to look for a user object. > + * @params: returned parameters. > + * @num_params: number of elements in the parameter array. > + * @req_id: request ID for the response. > + * @errno: result of user object invocation. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_user_object_submit(struct tee_context *ctx, > + struct tee_param *params, int num_params, > + int req_id, int errno) > +{ > + struct qcomtee_context_data *ctxdata = ctx->data; > + struct qcomtee_ureq *ureq; > + > + guard(mutex)(&ctxdata->reqs_lock); > + > + ureq = ureq_dequeue(ctxdata, req_id); > + if (!ureq) > + return -EINVAL; > + > + ureq->state = QCOMTEE_REQ_PROCESSED; > + > + if (!errno) > + ureq->errno = qcomtee_cb_params_to_args(ureq->args, params, > + num_params, ctx); > + else > + ureq->errno = errno; > + > + /* Return errno if qcomtee_cb_params_to_args() failed; otherwise 0. */ > + if (!errno && ureq->errno) > + errno = ureq->errno; > + else > + errno = 0; > + > + /* Send result to QTEE. */ > + complete(&ureq->c); > + > + return errno; > +} > diff --git a/include/linux/firmware/qcom/qcom_tee.h b/include/linux/firmware/qcom/qcom_tee.h > new file mode 100644 > index 000000000000..1d9f9bc320fe > --- /dev/null > +++ b/include/linux/firmware/qcom/qcom_tee.h > @@ -0,0 +1,302 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef __QCOM_TEE_H > +#define __QCOM_TEE_H > + > +#include <linux/completion.h> > +#include <linux/kref.h> > +#include <linux/slab.h> > +#include <linux/workqueue.h> > + > +struct qcomtee_object; > + > +/** > + * DOC: Overview > + * > + * qcomtee_object provides object refcounting, ID allocation for objects hosted > + * in the kernel, and necessary message marshaling for Qualcomm TEE (QTEE). > + * > + * To invoke an object in QTEE, the user calls qcomtee_object_do_invoke() > + * while passing an instance of &struct qcomtee_object and the requested > + * operation + arguments. > + * > + * After boot, QTEE provides a static object %ROOT_QCOMTEE_OBJECT (type of > + * %QCOMTEE_OBJECT_TYPE_ROOT). The root object is invoked to pass the user's > + * credentials and obtain other instances of &struct qcomtee_object (type of > + * %QCOMTEE_OBJECT_TYPE_TEE) that represent services and TAs in QTEE; > + * see &enum qcomtee_object_type. > + * > + * The objects received from QTEE are refcounted. So the owner of these objects > + * can issue qcomtee_object_get() to increase the refcount and pass objects > + * to other clients, or issue qcomtee_object_put() to decrease the refcount > + * and release the resources in QTEE. > + * > + * The kernel can host services accessible to QTEE. A driver should embed > + * an instance of &struct qcomtee_object in the struct it wants to export to > + * QTEE (this is called a callback object). It issues qcomtee_object_user_init() > + * to set the dispatch() operation for the callback object and set its type > + * to %QCOMTEE_OBJECT_TYPE_CB. > + * > + * core.c holds an object table for callback objects. An object ID is assigned > + * to each callback object, which is an index to the object table. QTEE uses > + * these IDs to reference or invoke callback objects. > + * > + * If QTEE invokes a callback object in the kernel, the dispatch() operation is > + * called in the context of the thread that originally called > + * qcomtee_object_do_invoke(). > + */ > + > +/** > + * enum qcomtee_object_type - Object types. > + * @QCOMTEE_OBJECT_TYPE_TEE: object hosted on QTEE. > + * @QCOMTEE_OBJECT_TYPE_CB: object hosted on kernel. > + * @QCOMTEE_OBJECT_TYPE_ROOT: 'primordial' object. > + * @QCOMTEE_OBJECT_TYPE_NULL: NULL object. > + * > + * The primordial object is used for bootstrapping the IPC connection between > + * the kernel and QTEE. It is invoked by the kernel when it wants to get a > + * 'client env'. > + */ > +enum qcomtee_object_type { > + QCOMTEE_OBJECT_TYPE_TEE, > + QCOMTEE_OBJECT_TYPE_CB, > + QCOMTEE_OBJECT_TYPE_ROOT, > + QCOMTEE_OBJECT_TYPE_NULL, > +}; > + > +/** > + * enum qcomtee_arg_type - Type of QTEE argument. > + * @QCOMTEE_ARG_TYPE_INV: invalid type. > + * @QCOMTEE_ARG_TYPE_OB: output buffer (OB). > + * @QCOMTEE_ARG_TYPE_OO: output object (OO). > + * @QCOMTEE_ARG_TYPE_IB: input buffer (IB). > + * @QCOMTEE_ARG_TYPE_IO: input object (IO). > + * > + * Use the invalid type to specify the end of the argument array. > + */ > +enum qcomtee_arg_type { > + QCOMTEE_ARG_TYPE_INV = 0, > + QCOMTEE_ARG_TYPE_OB, > + QCOMTEE_ARG_TYPE_OO, > + QCOMTEE_ARG_TYPE_IB, > + QCOMTEE_ARG_TYPE_IO, > + QCOMTEE_ARG_TYPE_NR, > +}; > + > +/** > + * define QCOMTEE_ARGS_PER_TYPE - Maximum arguments of a specific type. > + * > + * The QTEE transport protocol limits the maximum number of arguments of > + * a specific type (i.e., IB, OB, IO, and OO). > + */ > +#define QCOMTEE_ARGS_PER_TYPE 16 > + > +/* Maximum arguments that can fit in a QTEE message, ignoring the type. */ > +#define QCOMTEE_ARGS_MAX (QCOMTEE_ARGS_PER_TYPE * (QCOMTEE_ARG_TYPE_NR - 1)) > + > +struct qcomtee_buffer { > + union { > + void *addr; > + void __user *uaddr; > + }; > + size_t size; > +}; > + > +/** > + * struct qcomtee_arg - Argument for QTEE object invocation. > + * @type: type of argument as &enum qcomtee_arg_type. > + * @flags: extra flags. > + * @b: address and size if the type of argument is a buffer. > + * @o: object instance if the type of argument is an object. > + * > + * &qcomtee_arg.flags only accepts %QCOMTEE_ARG_FLAGS_UADDR for now, which > + * states that &qcomtee_arg.b contains a userspace address in uaddr. > + */ > +struct qcomtee_arg { > + enum qcomtee_arg_type type; > +/* 'b.uaddr' holds a __user address. */ > +#define QCOMTEE_ARG_FLAGS_UADDR BIT(0) > + unsigned int flags; > + union { > + struct qcomtee_buffer b; > + struct qcomtee_object *o; > + }; > +}; > + > +static inline int qcomtee_args_len(struct qcomtee_arg *args) > +{ > + int i = 0; > + > + while (args[i].type != QCOMTEE_ARG_TYPE_INV) > + i++; > + return i; > +} > + > +/* Context is busy (callback is in progress). */ > +#define QCOMTEE_OIC_FLAG_BUSY BIT(1) > +/* Context needs to notify the current object. */ > +#define QCOMTEE_OIC_FLAG_NOTIFY BIT(2) > +/* Context has shared state with QTEE. */ > +#define QCOMTEE_OIC_FLAG_SHARED BIT(3) > + > +struct qcomtee_object_invoke_ctx { > + /* TEE context for this invocation. */ > + struct tee_context *ctx; > + unsigned long flags; > + int errno; > + > + /* Current object invoked in this callback context. */ > + struct qcomtee_object *object; > + > + /* Dispatcher argument array (+1 for ending QCOMTEE_ARG_TYPE_INV). */ > + struct qcomtee_arg u[QCOMTEE_ARGS_MAX + 1]; > + > + /* Inbound and Outbound buffers shared with QTEE. */ > + struct qcomtee_buffer in_msg; /* Inbound buffer. */ > + struct qcomtee_buffer out_msg; /* Outbound buffer. */ > + struct tee_shm *in_shm; /* TEE shm allocated for inbound buffer. */ > + struct tee_shm *out_shm; /* TEE shm allocated for outbound buffer. */ > + > + /* Extra data attached to this context. */ > + void *data; > +}; > + > +static inline struct qcomtee_object_invoke_ctx * > +qcomtee_object_invoke_ctx_alloc(struct tee_context *ctx) > +{ > + struct qcomtee_object_invoke_ctx *oic; > + > + oic = kzalloc(sizeof(*oic), GFP_KERNEL); > + if (oic) > + oic->ctx = ctx; > + return oic; > +} > + > +/** > + * qcomtee_object_do_invoke() - Submit an invocation for an object. > + * @oic: context to use for the current invocation. > + * @object: object being invoked. > + * @op: requested operation on the object. > + * @u: array of arguments for the current invocation. > + * @result: result returned from QTEE. > + * > + * The caller is responsible for keeping track of the refcount for each object, > + * including @object. On return, the caller loses ownership of all input > + * objects of type %QCOMTEE_OBJECT_TYPE_CB. > + * > + * @object can be of %QCOMTEE_OBJECT_TYPE_ROOT or %QCOMTEE_OBJECT_TYPE_TEE. > + * > + * Return: On success, returns 0. On error, returns -EAGAIN if invocation > + * failed and the user may retry the invocation, -ENODEV on fatal failure. > + */ > +int qcomtee_object_do_invoke(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object *object, u32 op, > + struct qcomtee_arg *u, int *result); > + > +/** > + * struct qcomtee_object_operations - Callback object operations. > + * @release: release the object if QTEE is not using it. > + * @dispatch: dispatch the operation requested by QTEE. > + * @notify: report the status of any pending response submitted by @dispatch. > + */ > +struct qcomtee_object_operations { > + void (*release)(struct qcomtee_object *object); > + int (*dispatch)(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object *object, u32 op, > + struct qcomtee_arg *args); > + void (*notify)(struct qcomtee_object_invoke_ctx *oic, > + struct qcomtee_object *object, int err); > +}; > + > +/** > + * struct qcomtee_object - QTEE or kernel object. > + * @name: object name. > + * @refcount: reference counter. > + * @object_type: object type as &enum qcomtee_object_type. > + * @info: extra information for the object. > + * @ops: callback operations for objects of type %QCOMTEE_OBJECT_TYPE_CB. > + * @work: work for async operations on the object. > + * > + * @work is used for releasing objects of %QCOMTEE_OBJECT_TYPE_TEE type. > + */ > +struct qcomtee_object { > + const char *name; > + struct kref refcount; > + > + enum qcomtee_object_type object_type; > + struct object_info { > + unsigned long qtee_id; > + /* TEE context for QTEE object async requests. */ > + struct tee_context *qcomtee_async_ctx; > + } info; > + > + struct qcomtee_object_operations *ops; > + struct work_struct work; > +}; > + > +/* Static instances of qcomtee_object objects. */ > +#define NULL_QCOMTEE_OBJECT ((struct qcomtee_object *)(0)) > +extern struct qcomtee_object qcomtee_object_root; > +#define ROOT_QCOMTEE_OBJECT (&qcomtee_object_root) > + > +static inline enum qcomtee_object_type > +typeof_qcomtee_object(struct qcomtee_object *object) > +{ > + if (object == NULL_QCOMTEE_OBJECT) > + return QCOMTEE_OBJECT_TYPE_NULL; > + return object->object_type; > +} > + > +static inline const char *qcomtee_object_name(struct qcomtee_object *object) > +{ > + if (object == NULL_QCOMTEE_OBJECT) > + return "null"; > + > + if (!object->name) > + return "no-name"; > + return object->name; > +} > + > +/** > + * qcomtee_object_user_init() - Initialize an object for the user. > + * @object: object to initialize. > + * @ot: type of object as &enum qcomtee_object_type. > + * @ops: instance of callbacks. > + * @fmt: name assigned to the object. > + * > + * Return: On success, returns 0; on failure, returns < 0. > + */ > +int qcomtee_object_user_init(struct qcomtee_object *object, > + enum qcomtee_object_type ot, > + struct qcomtee_object_operations *ops, > + const char *fmt, ...); > + > +/* Object release is RCU protected. */ > +int qcomtee_object_get(struct qcomtee_object *object); > +void qcomtee_object_put(struct qcomtee_object *object); > + > +#define qcomtee_arg_for_each(i, args) \ > + for (i = 0; args[i].type != QCOMTEE_ARG_TYPE_INV; i++) > + > +/* Next argument of type @type after index @i. */ > +int qcomtee_next_arg_type(struct qcomtee_arg *u, int i, > + enum qcomtee_arg_type type); > + > +/* Iterate over argument of given type. */ > +#define qcomtee_arg_for_each_type(i, args, at) \ > + for (i = 0, i = qcomtee_next_arg_type(args, i, at); \ i = qcomtee_next_arg_type(args, 0, at) > + args[i].type != QCOMTEE_ARG_TYPE_INV; \ > + i++, i = qcomtee_next_arg_type(args, i, at)) i = qcomtee_next_arg_type(args, i + 1, at) Cheers, Jens > + > +#define qcomtee_arg_for_each_input_buffer(i, args) \ > + qcomtee_arg_for_each_type(i, args, QCOMTEE_ARG_TYPE_IB) > +#define qcomtee_arg_for_each_output_buffer(i, args) \ > + qcomtee_arg_for_each_type(i, args, QCOMTEE_ARG_TYPE_OB) > +#define qcomtee_arg_for_each_input_object(i, args) \ > + qcomtee_arg_for_each_type(i, args, QCOMTEE_ARG_TYPE_IO) > +#define qcomtee_arg_for_each_output_object(i, args) \ > + qcomtee_arg_for_each_type(i, args, QCOMTEE_ARG_TYPE_OO) > + > +#endif /* __QCOM_TEE_H */ > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h > index 8642ce7e4772..0efb2835d6dc 100644 > --- a/include/uapi/linux/tee.h > +++ b/include/uapi/linux/tee.h > @@ -59,6 +59,7 @@ > #define TEE_IMPL_ID_OPTEE 1 > #define TEE_IMPL_ID_AMDTEE 2 > #define TEE_IMPL_ID_TSTEE 3 > +#define TEE_IMPL_ID_QTEE 4 > > /* > * OP-TEE specific capabilities > > -- > 2.34.1 >

8 months, 1 week

1
0
0 0

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

by Jens Wiklander

Hi Amirreza, On Fri, Mar 28, 2025 at 3:48 AM Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> wrote: > > For drivers that can transfer data to the TEE without using shared > memory from client, it is necessary to receive the user address > directly, bypassing any processing by the TEE subsystem. Introduce > TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT/OUTPUT/INOUT to represent > userspace buffers. > > Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> > --- > drivers/tee/tee_core.c | 33 +++++++++++++++++++++++++++++++++ > include/linux/tee_drv.h | 6 ++++++ > include/uapi/linux/tee.h | 22 ++++++++++++++++------ > 3 files changed, 55 insertions(+), 6 deletions(-) Is this patch needed now that the QCOMTEE driver supports shared memory? I prefer keeping changes to the ABI to a minimum. Cheers, Jens > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c > index 22cc7d624b0c..bc862a11d437 100644 > --- a/drivers/tee/tee_core.c > +++ b/drivers/tee/tee_core.c > @@ -404,6 +404,17 @@ static int params_from_user(struct tee_context *ctx, struct tee_param *params, > params[n].u.value.b = ip.b; > params[n].u.value.c = ip.c; > break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > + params[n].u.ubuf.uaddr = u64_to_user_ptr(ip.a); > + params[n].u.ubuf.size = ip.b; > + > + if (!access_ok(params[n].u.ubuf.uaddr, > + params[n].u.ubuf.size)) > + return -EFAULT; > + > + break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > @@ -472,6 +483,11 @@ static int params_to_user(struct tee_ioctl_param __user *uparams, > put_user(p->u.value.c, &up->c)) > return -EFAULT; > break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > + if (put_user((u64)p->u.ubuf.size, &up->b)) > + return -EFAULT; > + break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > if (put_user((u64)p->u.memref.size, &up->b)) > @@ -672,6 +688,13 @@ static int params_to_supp(struct tee_context *ctx, > ip.b = p->u.value.b; > ip.c = p->u.value.c; > break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > + ip.a = (u64)p->u.ubuf.uaddr; > + ip.b = p->u.ubuf.size; > + ip.c = 0; > + break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > @@ -774,6 +797,16 @@ static int params_from_supp(struct tee_param *params, size_t num_params, > p->u.value.b = ip.b; > p->u.value.c = ip.c; > break; > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT: > + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT: > + p->u.ubuf.uaddr = u64_to_user_ptr(ip.a); > + p->u.ubuf.size = ip.b; > + > + if (!access_ok(params[n].u.ubuf.uaddr, > + params[n].u.ubuf.size)) > + return -EFAULT; > + > + break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > /* > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h > index ce23fd42c5d4..d773f91c6bdd 100644 > --- a/include/linux/tee_drv.h > +++ b/include/linux/tee_drv.h > @@ -82,6 +82,11 @@ struct tee_param_memref { > struct tee_shm *shm; > }; > > +struct tee_param_ubuf { > + void * __user uaddr; > + size_t size; > +}; > + > struct tee_param_value { > u64 a; > u64 b; > @@ -92,6 +97,7 @@ struct tee_param { > u64 attr; > union { > struct tee_param_memref memref; > + struct tee_param_ubuf ubuf; > struct tee_param_value value; > } u; > }; > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h > index d0430bee8292..3e9b1ec5dfde 100644 > --- a/include/uapi/linux/tee.h > +++ b/include/uapi/linux/tee.h > @@ -151,6 +151,13 @@ struct tee_ioctl_buf_data { > #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT 6 > #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT 7 /* input and output */ > > +/* > + * These defines userspace buffer parameters. > + */ > +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT 8 > +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT 9 > +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT 10 /* input and output */ > + > /* > * Mask for the type part of the attribute, leaves room for more types > */ > @@ -186,14 +193,17 @@ struct tee_ioctl_buf_data { > /** > * struct tee_ioctl_param - parameter > * @attr: attributes > - * @a: if a memref, offset into the shared memory object, else a value parameter > - * @b: if a memref, size of the buffer, else a value parameter > + * @a: if a memref, offset into the shared memory object, > + * else if a ubuf, address of the user buffer, > + * else a value parameter > + * @b: if a memref or ubuf, size of the buffer, else a value parameter > * @c: if a memref, shared memory identifier, else a value parameter > * > - * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref or value is used in > - * the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value and > - * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref. TEE_PARAM_ATTR_TYPE_NONE > - * indicates that none of the members are used. > + * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref, ubuf, or value is > + * used in the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value, > + * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref, and TEE_PARAM_ATTR_TYPE_UBUF_* > + * indicates ubuf. TEE_PARAM_ATTR_TYPE_NONE indicates that none of the members > + * are used. > * > * Shared memory is allocated with TEE_IOC_SHM_ALLOC which returns an > * identifier representing the shared memory object. A memref can reference > > -- > 2.34.1 >

8 months, 1 week

1
0
0 0

[PATCH RFC 00/12] dma: Enable dmem cgroup tracking

by Maxime Ripard

Hi, Here's preliminary work to enable dmem tracking for heavy users of DMA allocations on behalf of userspace: v4l2, DRM, and dma-buf heaps. It's not really meant for inclusion at the moment, because I really don't like it that much, and would like to discuss solutions on how to make it nicer. In particular, the dma dmem region accessors don't feel that great to me. It duplicates the logic to select the proper accessor in dma_alloc_attrs(), and it looks fragile and potentially buggy to me. One solution I tried is to do the accounting in dma_alloc_attrs() directly, depending on a flag being set, similar to what __GFP_ACCOUNT is doing. It didn't work because dmem initialises a state pointer when charging an allocation to a region, and expects that state pointer to be passed back when uncharging. Since dma_alloc_attrs() returns a void pointer to the allocated buffer, we need to put that state into a higher-level structure, such as drm_gem_object, or dma_buf. Since we can't share the region selection logic, we need to get the region through some other mean. Another thing I consider was to return the region as part of the allocated buffer (through struct page or folio), but those are lost across the calls and dma_alloc_attrs() will only get a void pointer. So that's not doable without some heavy rework, if it's a good idea at all. So yeah, I went for the dumbest possible solution with the accessors, hoping you could suggest a much smarter idea :) Thanks, Maxime Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Maxime Ripard (12): cma: Register dmem region for each cma region cma: Provide accessor to cma dmem region dma: coherent: Register dmem region for each coherent region dma: coherent: Provide accessor to dmem region dma: contiguous: Provide accessor to dmem region dma: direct: Provide accessor to dmem region dma: Create default dmem region for DMA allocations dma: Provide accessor to dmem region dma-buf: Clear cgroup accounting on release dma-buf: cma: Account for allocations in dmem cgroup drm/gem: Add cgroup memory accounting media: videobuf2: Track buffer allocations through the dmem cgroup drivers/dma-buf/dma-buf.c | 7 ++++ drivers/dma-buf/heaps/cma_heap.c | 18 ++++++++-- drivers/gpu/drm/drm_gem.c | 5 +++ drivers/gpu/drm/drm_gem_dma_helper.c | 6 ++++ .../media/common/videobuf2/videobuf2-dma-contig.c | 19 +++++++++++ include/drm/drm_device.h | 1 + include/drm/drm_gem.h | 2 ++ include/linux/cma.h | 9 +++++ include/linux/dma-buf.h | 5 +++ include/linux/dma-direct.h | 2 ++ include/linux/dma-map-ops.h | 32 ++++++++++++++++++ include/linux/dma-mapping.h | 11 ++++++ kernel/dma/coherent.c | 26 +++++++++++++++ kernel/dma/direct.c | 8 +++++ kernel/dma/mapping.c | 39 ++++++++++++++++++++++ mm/cma.c | 21 +++++++++++- mm/cma.h | 3 ++ 17 files changed, 211 insertions(+), 3 deletions(-) --- base-commit: 55a2aa61ba59c138bd956afe0376ec412a7004cf change-id: 20250307-dmem-cgroups-73febced0989 Best regards, -- Maxime Ripard <mripard(a)kernel.org>

8 months, 1 week

5
29
0 0

Re: [PATCH v3 02/11] tee: add close_context to TEE driver operation

by Jens Wiklander

Hi Amir, On Fri, Mar 28, 2025 at 3:48 AM Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> wrote: > > The tee_context can be used to manage TEE user resources, including > those allocated by the driver for the TEE on behalf of the user. > The release() callback is invoked only when all resources, such as > tee_shm, are released and there are no references to the tee_context. > > When a user closes the device file, the driver should notify the > TEE to release any resources it may hold and drop the context > references. To achieve this, a close_context() callback is > introduced to initiate resource release in the TEE driver when > the device file is closed. > > Relocate teedev_ctx_get, teedev_ctx_put, tee_device_get, and > tee_device_get functions to tee_drv.h to make them accessible > outside the TEE subsystem. > > Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> > --- > drivers/tee/tee_core.c | 39 +++++++++++++++++++++++++++++++++++++++ > drivers/tee/tee_private.h | 6 ------ > include/linux/tee_core.h | 11 +++++++++-- > include/linux/tee_drv.h | 40 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 88 insertions(+), 8 deletions(-) > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c > index 24edce4cdbaa..22cc7d624b0c 100644 > --- a/drivers/tee/tee_core.c > +++ b/drivers/tee/tee_core.c > @@ -72,6 +72,20 @@ struct tee_context *teedev_open(struct tee_device *teedev) > } > EXPORT_SYMBOL_GPL(teedev_open); > > +/** > + * teedev_ctx_get() - Increment the reference count of a context > + * > + * This function increases the refcount of the context, which is tied to > + * resources shared by the same tee_device. During the unregistration process, > + * the context may remain valid even after tee_device_unregister() has returned. > + * > + * Users should ensure that the context's refcount is properly decreased before > + * calling tee_device_put(), typically within the context's release() function. > + * Alternatively, users can call tee_device_get() and teedev_ctx_get() together > + * and release them simultaneously (see shm_alloc_helper()). > + * > + * @ctx: Pointer to the context Please move this @ctx line to before the verbose description of the function. Cheers, Jens > + */ > void teedev_ctx_get(struct tee_context *ctx) > { > if (ctx->releasing) > @@ -79,6 +93,7 @@ void teedev_ctx_get(struct tee_context *ctx) > > kref_get(&ctx->refcount); > } > +EXPORT_SYMBOL_GPL(teedev_ctx_get); > > static void teedev_ctx_release(struct kref *ref) > { > @@ -89,6 +104,10 @@ static void teedev_ctx_release(struct kref *ref) > kfree(ctx); > } > > +/** > + * teedev_ctx_put() - Decrease reference count on a context > + * @ctx: pointer to the context > + */ > void teedev_ctx_put(struct tee_context *ctx) > { > if (ctx->releasing) > @@ -96,11 +115,15 @@ void teedev_ctx_put(struct tee_context *ctx) > > kref_put(&ctx->refcount, teedev_ctx_release); > } > +EXPORT_SYMBOL_GPL(teedev_ctx_put); > > void teedev_close_context(struct tee_context *ctx) > { > struct tee_device *teedev = ctx->teedev; > > + if (teedev->desc->ops->close_context) > + teedev->desc->ops->close_context(ctx); > + > teedev_ctx_put(ctx); > tee_device_put(teedev); > } > @@ -1024,6 +1047,10 @@ int tee_device_register(struct tee_device *teedev) > } > EXPORT_SYMBOL_GPL(tee_device_register); > > +/** > + * tee_device_put() - Decrease the user count for a tee_device > + * @teedev: pointer to the tee_device > + */ > void tee_device_put(struct tee_device *teedev) > { > mutex_lock(&teedev->mutex); > @@ -1037,7 +1064,18 @@ void tee_device_put(struct tee_device *teedev) > } > mutex_unlock(&teedev->mutex); > } > +EXPORT_SYMBOL_GPL(tee_device_put); > > +/** > + * tee_device_get() - Increment the user count for a tee_device > + * @teedev: Pointer to the tee_device > + * > + * If tee_device_unregister() has been called and the final user of @teedev > + * has already released the device, this function will fail to prevent new users > + * from accessing the device during the unregistration process. > + * > + * Returns: true if @teedev remains valid, otherwise false > + */ > bool tee_device_get(struct tee_device *teedev) > { > mutex_lock(&teedev->mutex); > @@ -1049,6 +1087,7 @@ bool tee_device_get(struct tee_device *teedev) > mutex_unlock(&teedev->mutex); > return true; > } > +EXPORT_SYMBOL_GPL(tee_device_get); > > /** > * tee_device_unregister() - Removes a TEE device > diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h > index 9bc50605227c..d3f40a03de36 100644 > --- a/drivers/tee/tee_private.h > +++ b/drivers/tee/tee_private.h > @@ -14,12 +14,6 @@ > > int tee_shm_get_fd(struct tee_shm *shm); > > -bool tee_device_get(struct tee_device *teedev); > -void tee_device_put(struct tee_device *teedev); > - > -void teedev_ctx_get(struct tee_context *ctx); > -void teedev_ctx_put(struct tee_context *ctx); > - > 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); > diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h > index a38494d6b5f4..8a4c9e30b652 100644 > --- a/include/linux/tee_core.h > +++ b/include/linux/tee_core.h > @@ -65,8 +65,9 @@ struct tee_device { > /** > * struct tee_driver_ops - driver operations vtable > * @get_version: returns version of driver > - * @open: called when the device file is opened > - * @release: release this open file > + * @open: called for a context when the device file is opened > + * @close_context: called when the device file is closed > + * @release: called to release the context > * @open_session: open a new session > * @close_session: close a session > * @system_session: declare session as a system session > @@ -76,11 +77,17 @@ struct tee_device { > * @supp_send: called for supplicant to send a response > * @shm_register: register shared memory buffer in TEE > * @shm_unregister: unregister shared memory buffer in TEE > + * > + * The context given to @open might last longer than the device file if it is > + * tied to other resources in the TEE driver. @close_context is called when the > + * client closes the device file, even if there are existing references to the > + * context. The TEE driver can use @close_context to start cleaning up. > */ > struct tee_driver_ops { > void (*get_version)(struct tee_device *teedev, > struct tee_ioctl_version_data *vers); > int (*open)(struct tee_context *ctx); > + void (*close_context)(struct tee_context *ctx); > void (*release)(struct tee_context *ctx); > int (*open_session)(struct tee_context *ctx, > struct tee_ioctl_open_session_arg *arg, > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h > index a54c203000ed..ce23fd42c5d4 100644 > --- a/include/linux/tee_drv.h > +++ b/include/linux/tee_drv.h > @@ -96,6 +96,46 @@ struct tee_param { > } u; > }; > > +/** > + * tee_device_get() - Increment the user count for a tee_device > + * @teedev: Pointer to the tee_device > + * > + * If tee_device_unregister() has been called and the final user of @teedev > + * has already released the device, this function will fail to prevent new users > + * from accessing the device during the unregistration process. > + * > + * Returns: true if @teedev remains valid, otherwise false > + */ > +bool tee_device_get(struct tee_device *teedev); > + > +/** > + * tee_device_put() - Decrease the user count for a tee_device > + * @teedev: pointer to the tee_device > + */ > +void tee_device_put(struct tee_device *teedev); > + > +/** > + * teedev_ctx_get() - Increment the reference count of a context > + * > + * This function increases the refcount of the context, which is tied to > + * resources shared by the same tee_device. During the unregistration process, > + * the context may remain valid even after tee_device_unregister() has returned. > + * > + * Users should ensure that the context's refcount is properly decreased before > + * calling tee_device_put(), typically within the context's release() function. > + * Alternatively, users can call tee_device_get() and teedev_ctx_get() together > + * and release them simultaneously (see shm_alloc_helper()). > + * > + * @ctx: Pointer to the context > + */ > +void teedev_ctx_get(struct tee_context *ctx); > + > +/** > + * teedev_ctx_put() - Decrease reference count on a context > + * @ctx: pointer to the context > + */ > +void teedev_ctx_put(struct tee_context *ctx); > + > /** > * tee_shm_alloc_kernel_buf() - Allocate kernel shared memory for a > * particular TEE client driver > > -- > 2.34.1 >

8 months, 1 week

1
0
0 0

Re: [PATCH v4 2/4] drm/panthor: Add driver IOCTL for setting BO labels

by Liviu Dudau

On Wed, Apr 02, 2025 at 12:54:27PM +0100, Adrián Larumbe wrote: > Allow UM to label a BO for which it possesses a DRM handle. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > --- > drivers/gpu/drm/panthor/panthor_drv.c | 40 +++++++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_gem.h | 2 ++ > include/uapi/drm/panthor_drm.h | 19 +++++++++++++ > 3 files changed, 61 insertions(+) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index 310bb44abe1a..d5277284fe27 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -1330,6 +1330,44 @@ static int panthor_ioctl_vm_get_state(struct drm_device *ddev, void *data, > return 0; > } > > +static int panthor_ioctl_bo_set_label(struct drm_device *ddev, void *data, > + struct drm_file *file) > +{ > + struct drm_panthor_bo_set_label *args = data; > + struct drm_gem_object *obj; > + const char *label; > + int ret = 0; > + > + obj = drm_gem_object_lookup(file, args->handle); > + if (!obj) > + return -ENOENT; > + > + if (args->size && args->label) { > + if (args->size > PANTHOR_BO_LABEL_MAXLEN) { > + ret = -E2BIG; > + goto err_label; > + } > + > + label = strndup_user(u64_to_user_ptr(args->label), args->size); > + if (IS_ERR(label)) { > + ret = PTR_ERR(label); > + goto err_label; > + } > + } else if (args->size && !args->label) { > + ret = -EINVAL; > + goto err_label; > + } else { > + label = NULL; > + } > + > + panthor_gem_bo_set_label(obj, label); > + > +err_label: > + drm_gem_object_put(obj); > + > + return ret; > +} > + > static int > panthor_open(struct drm_device *ddev, struct drm_file *file) > { > @@ -1399,6 +1437,7 @@ static const struct drm_ioctl_desc panthor_drm_driver_ioctls[] = { > PANTHOR_IOCTL(TILER_HEAP_CREATE, tiler_heap_create, DRM_RENDER_ALLOW), > PANTHOR_IOCTL(TILER_HEAP_DESTROY, tiler_heap_destroy, DRM_RENDER_ALLOW), > PANTHOR_IOCTL(GROUP_SUBMIT, group_submit, DRM_RENDER_ALLOW), > + PANTHOR_IOCTL(BO_SET_LABEL, bo_set_label, DRM_RENDER_ALLOW), > }; > > static int panthor_mmap(struct file *filp, struct vm_area_struct *vma) > @@ -1508,6 +1547,7 @@ static void panthor_debugfs_init(struct drm_minor *minor) > * - 1.2 - adds DEV_QUERY_GROUP_PRIORITIES_INFO query > * - adds PANTHOR_GROUP_PRIORITY_REALTIME priority > * - 1.3 - adds DRM_PANTHOR_GROUP_STATE_INNOCENT flag > + * - 1.4 - adds DRM_IOCTL_PANTHOR_BO_SET_LABEL ioctl Hi Adrián, You also need to update the .minor value. With that change, Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> Best regards, Liviu > */ > static const struct drm_driver panthor_drm_driver = { > .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ | > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index 0582826b341a..e18fbc093abd 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -13,6 +13,8 @@ > > struct panthor_vm; > > +#define PANTHOR_BO_LABEL_MAXLEN PAGE_SIZE > + > /** > * struct panthor_gem_object - Driver specific GEM object. > */ > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h > index 97e2c4510e69..26b52f147360 100644 > --- a/include/uapi/drm/panthor_drm.h > +++ b/include/uapi/drm/panthor_drm.h > @@ -127,6 +127,9 @@ enum drm_panthor_ioctl_id { > > /** @DRM_PANTHOR_TILER_HEAP_DESTROY: Destroy a tiler heap. */ > DRM_PANTHOR_TILER_HEAP_DESTROY, > + > + /** @DRM_PANTHOR_BO_SET_LABEL: Label a BO. */ > + DRM_PANTHOR_BO_SET_LABEL, > }; > > /** > @@ -977,6 +980,20 @@ struct drm_panthor_tiler_heap_destroy { > __u32 pad; > }; > > +/** > + * struct drm_panthor_bo_set_label - Arguments passed to DRM_IOCTL_PANTHOR_BO_SET_LABEL > + */ > +struct drm_panthor_bo_set_label { > + /** @handle: Handle of the buffer object to label. */ > + __u32 handle; > + > + /** @size: Length of the label, including the NULL terminator. */ > + __u32 size; > + > + /** @label: User pointer to a NULL-terminated string */ > + __u64 label; > +}; > + > /** > * DRM_IOCTL_PANTHOR() - Build a Panthor IOCTL number > * @__access: Access type. Must be R, W or RW. > @@ -1019,6 +1036,8 @@ enum { > DRM_IOCTL_PANTHOR(WR, TILER_HEAP_CREATE, tiler_heap_create), > DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY = > DRM_IOCTL_PANTHOR(WR, TILER_HEAP_DESTROY, tiler_heap_destroy), > + DRM_IOCTL_PANTHOR_BO_SET_LABEL = > + DRM_IOCTL_PANTHOR(WR, BO_SET_LABEL, bo_set_label), > }; > > #if defined(__cplusplus) > -- > 2.48.1 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯

8 months, 1 week

1
0
0 0

Re: CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP is broken, was Re: [RFC PATCH 0/6] Deep talk about folio vmap

by Christoph Hellwig

On Mon, Apr 07, 2025 at 02:43:20PM +0800, Muchun Song wrote: > By the way, in case you truly struggle to comprehend the fundamental > aspects of HVO, I would like to summarize for you the user-visible > behaviors in comparison to the situation where HVO is disabled. > > HVO Status Tail Page Structures Head Page Structures > Enabled Read-Only (RO) Read-Write (RW) > Disabled Read-Write (RW) Read-Write (RW) > > The sole distinction between the two scenarios lies in whether the > tail page structures are allowed to be written or not. Please refrain > from getting bogged down in the details of the implementation of HVO. This feels extremely fragile to me. I doubt many people know what operations needs read vs write access to tail pages. Or for higher level operations if needs access to tail pages at all.

8 months, 1 week

1
0
0 0

[PATCH v7 00/11] 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) has been identified so far to serve as examples of what can be expected. The use-cases has 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 it's own protected memory pool since different use-cases requires 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 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-v7 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 set can be tested on RockPi in the same way with: xtest --sdp-basic The primitive test are tested on QEMU with the following steps: repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \ -b prototype/sdp-v7 repo sync -j8 cd build make toolchains -j$(nproc) make SPMC_AT_EL=1 all -j$(nproc) make SPMC_AT_EL=1 run-only # login and at the prompt: xtest --sdp-basic The SPMC_AT_EL=1 parameter configures the build with FF-A and an SPMC at S-EL1 inside OP-TEE. The parameter can be changed into SPMC_AT_EL=n to test without FF-A using the original SMC ABI instead. Please remember to do %rm -rf ../trusted-firmware-a/build/qemu for TF-A to be rebuilt properly using the new configuration. https://optee.readthedocs.io/en/latest/building/prerequisites.html list dependencies needed to build the above. The tests are pretty basic, mostly checking that a Trusted Application in the secure world can access and manipulate the memory. There are also some negative tests for out of bounds buffers etc. Thanks, Jens Changes since 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 (10): tee: tee_device_alloc(): copy dma_mask from parent device optee: pass parent device to tee_device_alloc() optee: account for direction while converting parameters optee: sync secure world ABI headers tee: implement protected DMA-heap tee: refactor params_from_user() tee: add tee_shm_alloc_cma_phys_mem() optee: support protected memory allocation optee: FF-A: dynamic protected memory allocation optee: smc abi: dynamic protected memory allocation drivers/tee/Kconfig | 10 + drivers/tee/Makefile | 1 + drivers/tee/optee/Makefile | 1 + drivers/tee/optee/call.c | 10 +- drivers/tee/optee/core.c | 1 + drivers/tee/optee/ffa_abi.c | 195 ++++++++++++- drivers/tee/optee/optee_ffa.h | 27 +- drivers/tee/optee/optee_msg.h | 83 +++++- drivers/tee/optee/optee_private.h | 55 +++- drivers/tee/optee/optee_smc.h | 71 ++++- drivers/tee/optee/protmem.c | 330 +++++++++++++++++++++ drivers/tee/optee/rpc.c | 31 +- drivers/tee/optee/smc_abi.c | 192 ++++++++++-- drivers/tee/tee_core.c | 157 +++++++--- drivers/tee/tee_heap.c | 469 ++++++++++++++++++++++++++++++ drivers/tee/tee_private.h | 16 + drivers/tee/tee_shm.c | 164 ++++++++++- include/linux/tee_core.h | 70 +++++ include/linux/tee_drv.h | 10 + include/uapi/linux/tee.h | 31 ++ 20 files changed, 1792 insertions(+), 132 deletions(-) create mode 100644 drivers/tee/optee/protmem.c create mode 100644 drivers/tee/tee_heap.c base-commit: 38fec10eb60d687e30c8c6b5420d86e8149f7557 -- 2.43.0

8 months, 2 weeks

1
11
0 0

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

by Christian König

Am 03.04.25 um 16:40 schrieb Philipp Stanner: >>>>> >>>> >>>> That looks like a really really awkward approach. The driver >>>> basically uses a the DMA fence infrastructure as middle layer and >>>> callbacks into itself to cleanup it's own structures. >>>> >>> >>> What else are callbacks good for, if not to do something >>> automatically >>> when the fence gets signaled? >>> >> >> Well if you add a callback for a signal you issued yourself then >> that's kind of awkward. >> >> E.g. you call into the DMA fence code, just for the DMA fence code >> to call yourself back again. > Now we're entering CS-Philosophy, because it depends on who "you" and > "yourself" are. In case of the driver, yes, naturally it registers a > callback because at some other place (e.g., in the driver's interrupt > handler) the fence will be signaled and the driver wants the callback > stuff to be done. > > If that's not dma_fences' callbacks' purpose, then I'd be interested in > knowing what their purpose is, because from my POV this discussion > seems to imply that we effectively must never use them for anything. > > How could it ever be different? Who, for example, registers dma_fence > callbacks while not signaling them "himself"? Let me try to improve that explanation. First of all we have components, they can be drivers, frameworks, helpers like the DRM scheduler or generally any code which is more or less stand alone. The definition of components is a bit tricky, but in general people usually get what they mean. E.g. in this case here we have nouveau as single component. Now the DMA fence interface allows sending signals between different components in a standardized way, one component can send a signal to another one and they don't necessarily need to know anything from each other except that both are using the DMA fence framework in the documented manner. When a component is now both the provide and the consumer at the same time you actually need a reason for that. Could be for example that it wants to consume signals from both itself as well as others, but this doesn't apply for this use case here. Considering pool or billiard you can of course do a trick shot and try to hit the 8, but going straight for it just has a better chance to succeed. > > >> >> >>> >>>> >>>> Additional to that we don't guarantee any callback order for the >>>> DMA >>>> fence and so it can be that mix cleaning up the callback with >>>> other >>>> work which is certainly not good when you want to guarantee that >>>> the >>>> cleanup happens under the same lock. >>>> >>> >>> Isn't my perception correct that the primary issue you have with >>> this >>> approach is that dma_fence_put() is called from within the >>> callback? Or >>> do you also take issue with deleting from the list? >>> >> >> Well kind of both. The issue is that the caller of >> dma_fence_signal() or dma_fence_signal_locked() must hold the >> reference until the function returns. >> >> When you do the list cleanup and the drop inside the callback it is >> perfectly possible that the fence pointer becomes stale before you >> return and that's really not a good idea. > In other words, you would prefer if this patch would have a function > with my callback's code in a function, and that function would be > called at every place where the driver signals a fence? > > If that's your opinion, then, IOW, it would mean for us to go almost > back to status quo, with nouveau_fence_signal2.0, but with the > dma_fence_is_signaled() part fixed. Well it could potentially be cleaned up more, but as far as I can see only the two lines I pointed out in the other mail need to move at the right place, yes. I mean it's just two lines. If you open code that or if you make a nouveau_cleanup_list_ref() function (or similar) is perfectly up to you. Regards, Christian. > > > P. > >> >> >>> >>> >>> >>>> >>>> Instead the call to dma_fence_signal_locked() should probably be >>>> removed from nouveau_fence_signal() and into >>>> nouveau_fence_context_kill() and nouveau_fence_update(). >>>> >>>> This way nouveau_fence_is_signaled() can call this function as >>>> well. >>>> >>> >>> Which "this function"? dma_fence_signal_locked() >>> >> >> No the cleanup function for the list entry. Whatever you call that >> then, the name nouveau_fence_signal() is probably not appropriate any >> more. >> >> >>> >>> >>> >>>> >>>> BTW: nouveau_fence_no_signaling() looks completely broken as >>>> well. It >>>> calls nouveau_fence_is_signaled() and then list_del() on the >>>> fence >>>> head. >>>> >>> >>> I can assure you that a great many things in Nouveau look >>> completely >>> broken. >>> >>> The question for us is always the cost-benefit-ratio when fixing >>> bugs. >>> There are fixes that solve the bug with reasonable effort, and >>> there >>> are great reworks towards an ideal state. >>> >> >> I would just simply drop that function. As far as I can see it >> severs no purpose other than doing exactly what the common DMA fence >> code does anyway. >> >> Just one less thing which could fail. >> >> Christian. >> >> >>> >>> >>> P. >>> >>> >>> >>>> >>>> As far as I can see that is completely superfluous and should >>>> probably be dropped. IIRC I once had a patch to clean that up but >>>> it >>>> was dropped for some reason. >>>> >>>> Regards, >>>> Christian. >>>> >>>> >>>> >>>>> >>>>> + dma_fence_put(&fence->base); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> ret = fctx->emit(fence); >>>>> if (!ret) { >>>>> dma_fence_get(&fence->base); >>>>> @@ -246,8 +251,7 @@ nouveau_fence_emit(struct nouveau_fence >>>>> *fence) >>>>> return -ENODEV; >>>>> } >>>>> >>>>> - if (nouveau_fence_update(chan, fctx)) >>>>> - nvif_event_block(&fctx->event); >>>>> + nouveau_fence_update(chan, fctx); >>>>> >>>>> list_add_tail(&fence->head, &fctx->pending); >>>>> spin_unlock_irq(&fctx->lock); >>>>> @@ -270,8 +274,8 @@ nouveau_fence_done(struct nouveau_fence >>>>> *fence) >>>>> >>>>> spin_lock_irqsave(&fctx->lock, flags); >>>>> chan = rcu_dereference_protected(fence- >>>>>> channel, >>>>> lockdep_is_held(&fctx->lock)); >>>>> - if (chan && nouveau_fence_update(chan, fctx)) >>>>> - nvif_event_block(&fctx->event); >>>>> + if (chan) >>>>> + nouveau_fence_update(chan, fctx); >>>>> spin_unlock_irqrestore(&fctx->lock, flags); >>>>> } >>>>> return dma_fence_is_signaled(&fence->base); >>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.h >>>>> b/drivers/gpu/drm/nouveau/nouveau_fence.h >>>>> index 8bc065acfe35..e6b2df7fdc42 100644 >>>>> --- a/drivers/gpu/drm/nouveau/nouveau_fence.h >>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.h >>>>> @@ -10,6 +10,7 @@ struct nouveau_bo; >>>>> >>>>> struct nouveau_fence { >>>>> struct dma_fence base; >>>>> + struct dma_fence_cb cb; >>>>> >>>>> struct list_head head; >>>>> >>>>> >>>> >>> >> >>

8 months, 2 weeks

1
0
0 0

CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP is broken, was Re: [RFC PATCH 0/6] Deep talk about folio vmap

by Christoph Hellwig

After the btrfs compressed bio discussion I think the hugetlb changes that skip the tail pages are fundamentally unsafe in the current kernel. That is because the bio_vec representation assumes tail pages do exist, so as soon as you are doing direct I/O that generates a bvec starting beyond the present head page things will blow up. Other users of bio_vecs might do the same, but the way the block bio_vecs are generated are very suspect to that. So we'll first need to sort that out and a few other things before we can even think of enabling such a feature.

8 months, 2 weeks

1
0
0 0

[PATCH] dma-buf/sw_sync: Decrement refcount on error in sw_sync_ioctl_get_deadline()

by Dan Carpenter

Call dma_fence_put(fence) before returning an error on this error path. Fixes: 70e67aaec2f4 ("dma-buf/sw_sync: Add fence deadline support") Signed-off-by: Dan Carpenter <dan.carpenter(a)linaro.org> --- drivers/dma-buf/sw_sync.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index f5905d67dedb..b7615c5c6cac 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -438,8 +438,10 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a return -EINVAL; pt = dma_fence_to_sync_pt(fence); - if (!pt) + if (!pt) { + dma_fence_put(fence); return -EINVAL; + } spin_lock_irqsave(fence->lock, flags); if (test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { -- 2.47.2

8 months, 2 weeks

2
3
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig