Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

15 participants
3025 discussions

Re: [PATCH v4 0/4] Implement dmabuf direct I/O via copy_file_range

by Christoph Hellwig

This is a really weird interface. No one has yet to explain why dmabuf is so special that we can't support direct I/O to it when we can support it to otherwise exotic mappings like PCI P2P ones.

3 months, 1 week

Re: [PATCH v4 2/9] dma-fence: Use a flag for 64-bit seqnos

by Christian König

On 6/3/25 14:48, Tvrtko Ursulin wrote: > > On 03/06/2025 13:40, Christian König wrote: >> On 6/3/25 13:30, Tvrtko Ursulin wrote: >>> >>> On 02/06/2025 19:00, Christian König wrote: >>>> On 6/2/25 17:25, Tvrtko Ursulin wrote: >>>>> >>>>> On 02/06/2025 15:42, Christian König wrote: >>>>>> On 6/2/25 15:05, Tvrtko Ursulin wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On 15/05/2025 14:15, Christian König wrote: >>>>>>>> Hey drm-misc maintainers, >>>>>>>> >>>>>>>> can you guys please backmerge drm-next into drm-misc-next? >>>>>>>> >>>>>>>> I want to push this patch here but it depends on changes which are partially in drm-next and partially in drm-misc-next. >>>>>>> >>>>>>> Looks like the backmerge is still pending? >>>>>> >>>>>> Yes, @Maarten, @Maxime and @Thomas ping on this. >>>>>> >>>>>>> In the meantime, Christian, any chance you will have some bandwith to think about the tail end of the series? Specifically patch 6 and how that is used onward. >>>>>> >>>>>> Well the RCU grace period is quite a nifty hack. I wanted to go over it again after merging the first patches from this series. >>>>>> >>>>>> In general looks like a good idea to me, I just don't like that we explicitely need to expose dma_fence_access_begin() and dma_fence_access_end(). >>>>>> >>>>>> Especially we can't do that while calling fence->ops->release. >>>>> >>>>> Hm why not? You think something will take offence of the rcu_read_lock()? >>>> >>>> Yes, especially it is perfectly legitimate to call synchronize_rcu() or lock semaphores/mutexes from that callback. >>>> >>>> Either keep the RCU critical section only for the trace or even better come up with some different approach, e.g. copying the string under the RCU lock or something like that. >>> >>> Hmm but the kerneldoc explicity says callback can be called from irq context: >>> >>> /** >>> * @release: >>> * >>> * Called on destruction of fence to release additional resources. >>> * Can be called from irq context. This callback is optional. If it is >>> * NULL, then dma_fence_free() is instead called as the default >>> * implementation. >>> */ >>> void (*release)(struct dma_fence *fence); >> >> Ah, right. I mixed that up with the dma-buf object. >> >> Yeah in that case that is probably harmless. We delegate the final free to a work item if necessary anyway. >> >> But I would still like to avoid having the RCU cover the release as well. Or why is there any reason why we would explicitely want to do this? > > I can't remember there was a particular reason. Obviously the driver/timeline name vfunc access I needed a dma_fence_access_begin/end() block so maybe I was just sloppy and put the end at the end of the function instead of at the end of the block which can dereference them. Yeah that's the next topic I would rather like to improve. We are kind of hiding that the returned strings are using RCU protection. In other words it would be nicer if we could add an __rcu tag to the get_driver_name/get_timeline_name callbacks and let the automated tools complain if somebody isn't doing the proper RCU handling. The problem is that as far as I know that is not supported by the automated tools (would be cool if somebody could double check that). +We would need to convert the get_timeline/get_timeline_name function to something like func(struct dma_fence *fence, const char __rcu **out) to make that work. Regards, Christian. > > I will pull it earlier for the next respin, assuming no gotchas get discovered in the process. > > Regards, > > Tvrtko > >> >> Regards, >> Christian. >> >>> >>> >>> Regards, >>> >>> Tvrtko >>> >>>> >>>> Regards, >>>> Christian. >>>> >>>>> >>>>> Regards, >>>>> >>>>> Tvrtko >>>>> >>>>>>>> On 5/15/25 11:49, Tvrtko Ursulin wrote: >>>>>>>>> With the goal of reducing the need for drivers to touch (and dereference) >>>>>>>>> fence->ops, we move the 64-bit seqnos flag from struct dma_fence_ops to >>>>>>>>> the fence->flags. >>>>>>>>> >>>>>>>>> Drivers which were setting this flag are changed to use new >>>>>>>>> dma_fence_init64() instead of dma_fence_init(). >>>>>>>>> >>>>>>>>> v2: >>>>>>>>> * Streamlined init and added kerneldoc. >>>>>>>>> * Rebase for amdgpu userq which landed since. >>>>>>>>> >>>>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> >>>>>>>>> Reviewed-by: Christian König <christian.koenig(a)amd.com> # v1 >>>>>>>>> --- >>>>>>>>> drivers/dma-buf/dma-fence-chain.c | 5 +- >>>>>>>>> drivers/dma-buf/dma-fence.c | 69 ++++++++++++++----- >>>>>>>>> .../drm/amd/amdgpu/amdgpu_eviction_fence.c | 7 +- >>>>>>>>> .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 5 +- >>>>>>>>> .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 5 +- >>>>>>>>> include/linux/dma-fence.h | 14 ++-- >>>>>>>>> 6 files changed, 64 insertions(+), 41 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c >>>>>>>>> index 90424f23fd73..a8a90acf4f34 100644 >>>>>>>>> --- a/drivers/dma-buf/dma-fence-chain.c >>>>>>>>> +++ b/drivers/dma-buf/dma-fence-chain.c >>>>>>>>> @@ -218,7 +218,6 @@ static void dma_fence_chain_set_deadline(struct dma_fence *fence, >>>>>>>>> } >>>>>>>>> const struct dma_fence_ops dma_fence_chain_ops = { >>>>>>>>> - .use_64bit_seqno = true, >>>>>>>>> .get_driver_name = dma_fence_chain_get_driver_name, >>>>>>>>> .get_timeline_name = dma_fence_chain_get_timeline_name, >>>>>>>>> .enable_signaling = dma_fence_chain_enable_signaling, >>>>>>>>> @@ -262,8 +261,8 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, >>>>>>>>> seqno = max(prev->seqno, seqno); >>>>>>>>> } >>>>>>>>> - dma_fence_init(&chain->base, &dma_fence_chain_ops, >>>>>>>>> - &chain->lock, context, seqno); >>>>>>>>> + dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, >>>>>>>>> + context, seqno); >>>>>>>>> /* >>>>>>>>> * Chaining dma_fence_chain container together is only allowed through >>>>>>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c >>>>>>>>> index f0cdd3e99d36..705b59787731 100644 >>>>>>>>> --- a/drivers/dma-buf/dma-fence.c >>>>>>>>> +++ b/drivers/dma-buf/dma-fence.c >>>>>>>>> @@ -989,24 +989,9 @@ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) >>>>>>>>> } >>>>>>>>> EXPORT_SYMBOL(dma_fence_describe); >>>>>>>>> -/** >>>>>>>>> - * dma_fence_init - Initialize a custom fence. >>>>>>>>> - * @fence: the fence to initialize >>>>>>>>> - * @ops: the dma_fence_ops for operations on this fence >>>>>>>>> - * @lock: the irqsafe spinlock to use for locking this fence >>>>>>>>> - * @context: the execution context this fence is run on >>>>>>>>> - * @seqno: a linear increasing sequence number for this context >>>>>>>>> - * >>>>>>>>> - * Initializes an allocated fence, the caller doesn't have to keep its >>>>>>>>> - * refcount after committing with this fence, but it will need to hold a >>>>>>>>> - * refcount again if &dma_fence_ops.enable_signaling gets called. >>>>>>>>> - * >>>>>>>>> - * context and seqno are used for easy comparison between fences, allowing >>>>>>>>> - * to check which fence is later by simply using dma_fence_later(). >>>>>>>>> - */ >>>>>>>>> -void >>>>>>>>> -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>>>> - spinlock_t *lock, u64 context, u64 seqno) >>>>>>>>> +static void >>>>>>>>> +__dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>>>> + spinlock_t *lock, u64 context, u64 seqno, unsigned long flags) >>>>>>>>> { >>>>>>>>> BUG_ON(!lock); >>>>>>>>> BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name); >>>>>>>>> @@ -1017,9 +1002,55 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>>>> fence->lock = lock; >>>>>>>>> fence->context = context; >>>>>>>>> fence->seqno = seqno; >>>>>>>>> - fence->flags = 0UL; >>>>>>>>> + fence->flags = flags; >>>>>>>>> fence->error = 0; >>>>>>>>> trace_dma_fence_init(fence); >>>>>>>>> } >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> + * dma_fence_init - Initialize a custom fence. >>>>>>>>> + * @fence: the fence to initialize >>>>>>>>> + * @ops: the dma_fence_ops for operations on this fence >>>>>>>>> + * @lock: the irqsafe spinlock to use for locking this fence >>>>>>>>> + * @context: the execution context this fence is run on >>>>>>>>> + * @seqno: a linear increasing sequence number for this context >>>>>>>>> + * >>>>>>>>> + * Initializes an allocated fence, the caller doesn't have to keep its >>>>>>>>> + * refcount after committing with this fence, but it will need to hold a >>>>>>>>> + * refcount again if &dma_fence_ops.enable_signaling gets called. >>>>>>>>> + * >>>>>>>>> + * context and seqno are used for easy comparison between fences, allowing >>>>>>>>> + * to check which fence is later by simply using dma_fence_later(). >>>>>>>>> + */ >>>>>>>>> +void >>>>>>>>> +dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>>>> + spinlock_t *lock, u64 context, u64 seqno) >>>>>>>>> +{ >>>>>>>>> + __dma_fence_init(fence, ops, lock, context, seqno, 0UL); >>>>>>>>> +} >>>>>>>>> EXPORT_SYMBOL(dma_fence_init); >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> + * dma_fence_init64 - Initialize a custom fence with 64-bit seqno support. >>>>>>>>> + * @fence: the fence to initialize >>>>>>>>> + * @ops: the dma_fence_ops for operations on this fence >>>>>>>>> + * @lock: the irqsafe spinlock to use for locking this fence >>>>>>>>> + * @context: the execution context this fence is run on >>>>>>>>> + * @seqno: a linear increasing sequence number for this context >>>>>>>>> + * >>>>>>>>> + * Initializes an allocated fence, the caller doesn't have to keep its >>>>>>>>> + * refcount after committing with this fence, but it will need to hold a >>>>>>>>> + * refcount again if &dma_fence_ops.enable_signaling gets called. >>>>>>>>> + * >>>>>>>>> + * Context and seqno are used for easy comparison between fences, allowing >>>>>>>>> + * to check which fence is later by simply using dma_fence_later(). >>>>>>>>> + */ >>>>>>>>> +void >>>>>>>>> +dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>>>> + spinlock_t *lock, u64 context, u64 seqno) >>>>>>>>> +{ >>>>>>>>> + __dma_fence_init(fence, ops, lock, context, seqno, >>>>>>>>> + BIT(DMA_FENCE_FLAG_SEQNO64_BIT)); >>>>>>>>> +} >>>>>>>>> +EXPORT_SYMBOL(dma_fence_init64); >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c >>>>>>>>> index 1a7469543db5..79713421bffe 100644 >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c >>>>>>>>> @@ -134,7 +134,6 @@ static bool amdgpu_eviction_fence_enable_signaling(struct dma_fence *f) >>>>>>>>> } >>>>>>>>> static const struct dma_fence_ops amdgpu_eviction_fence_ops = { >>>>>>>>> - .use_64bit_seqno = true, >>>>>>>>> .get_driver_name = amdgpu_eviction_fence_get_driver_name, >>>>>>>>> .get_timeline_name = amdgpu_eviction_fence_get_timeline_name, >>>>>>>>> .enable_signaling = amdgpu_eviction_fence_enable_signaling, >>>>>>>>> @@ -160,9 +159,9 @@ amdgpu_eviction_fence_create(struct amdgpu_eviction_fence_mgr *evf_mgr) >>>>>>>>> ev_fence->evf_mgr = evf_mgr; >>>>>>>>> get_task_comm(ev_fence->timeline_name, current); >>>>>>>>> spin_lock_init(&ev_fence->lock); >>>>>>>>> - dma_fence_init(&ev_fence->base, &amdgpu_eviction_fence_ops, >>>>>>>>> - &ev_fence->lock, evf_mgr->ev_fence_ctx, >>>>>>>>> - atomic_inc_return(&evf_mgr->ev_fence_seq)); >>>>>>>>> + dma_fence_init64(&ev_fence->base, &amdgpu_eviction_fence_ops, >>>>>>>>> + &ev_fence->lock, evf_mgr->ev_fence_ctx, >>>>>>>>> + atomic_inc_return(&evf_mgr->ev_fence_seq)); >>>>>>>>> return ev_fence; >>>>>>>>> } >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >>>>>>>>> index 029cb24c28b3..5e92d00a591f 100644 >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >>>>>>>>> @@ -239,8 +239,8 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq, >>>>>>>>> fence = &userq_fence->base; >>>>>>>>> userq_fence->fence_drv = fence_drv; >>>>>>>>> - dma_fence_init(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, >>>>>>>>> - fence_drv->context, seq); >>>>>>>>> + dma_fence_init64(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, >>>>>>>>> + fence_drv->context, seq); >>>>>>>>> amdgpu_userq_fence_driver_get(fence_drv); >>>>>>>>> dma_fence_get(fence); >>>>>>>>> @@ -334,7 +334,6 @@ static void amdgpu_userq_fence_release(struct dma_fence *f) >>>>>>>>> } >>>>>>>>> static const struct dma_fence_ops amdgpu_userq_fence_ops = { >>>>>>>>> - .use_64bit_seqno = true, >>>>>>>>> .get_driver_name = amdgpu_userq_fence_get_driver_name, >>>>>>>>> .get_timeline_name = amdgpu_userq_fence_get_timeline_name, >>>>>>>>> .signaled = amdgpu_userq_fence_signaled, >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c >>>>>>>>> index 51cddfa3f1e8..5d26797356a3 100644 >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c >>>>>>>>> @@ -71,7 +71,6 @@ static void amdgpu_tlb_fence_work(struct work_struct *work) >>>>>>>>> } >>>>>>>>> static const struct dma_fence_ops amdgpu_tlb_fence_ops = { >>>>>>>>> - .use_64bit_seqno = true, >>>>>>>>> .get_driver_name = amdgpu_tlb_fence_get_driver_name, >>>>>>>>> .get_timeline_name = amdgpu_tlb_fence_get_timeline_name >>>>>>>>> }; >>>>>>>>> @@ -101,8 +100,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm >>>>>>>>> INIT_WORK(&f->work, amdgpu_tlb_fence_work); >>>>>>>>> spin_lock_init(&f->lock); >>>>>>>>> - dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock, >>>>>>>>> - vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); >>>>>>>>> + dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock, >>>>>>>>> + vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); >>>>>>>>> /* TODO: We probably need a separate wq here */ >>>>>>>>> dma_fence_get(&f->base); >>>>>>>>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h >>>>>>>>> index 48b5202c531d..a34a0dcdc446 100644 >>>>>>>>> --- a/include/linux/dma-fence.h >>>>>>>>> +++ b/include/linux/dma-fence.h >>>>>>>>> @@ -97,6 +97,7 @@ struct dma_fence { >>>>>>>>> }; >>>>>>>>> enum dma_fence_flag_bits { >>>>>>>>> + DMA_FENCE_FLAG_SEQNO64_BIT, >>>>>>>>> DMA_FENCE_FLAG_SIGNALED_BIT, >>>>>>>>> DMA_FENCE_FLAG_TIMESTAMP_BIT, >>>>>>>>> DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >>>>>>>>> @@ -124,14 +125,6 @@ struct dma_fence_cb { >>>>>>>>> * >>>>>>>>> */ >>>>>>>>> struct dma_fence_ops { >>>>>>>>> - /** >>>>>>>>> - * @use_64bit_seqno: >>>>>>>>> - * >>>>>>>>> - * True if this dma_fence implementation uses 64bit seqno, false >>>>>>>>> - * otherwise. >>>>>>>>> - */ >>>>>>>>> - bool use_64bit_seqno; >>>>>>>>> - >>>>>>>>> /** >>>>>>>>> * @get_driver_name: >>>>>>>>> * >>>>>>>>> @@ -262,6 +255,9 @@ struct dma_fence_ops { >>>>>>>>> void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>>>> spinlock_t *lock, u64 context, u64 seqno); >>>>>>>>> +void dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>>>> + spinlock_t *lock, u64 context, u64 seqno); >>>>>>>>> + >>>>>>>>> void dma_fence_release(struct kref *kref); >>>>>>>>> void dma_fence_free(struct dma_fence *fence); >>>>>>>>> void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq); >>>>>>>>> @@ -454,7 +450,7 @@ static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2) >>>>>>>>> * 32bit sequence numbers. Use a 64bit compare when the driver says to >>>>>>>>> * do so. >>>>>>>>> */ >>>>>>>>> - if (fence->ops->use_64bit_seqno) >>>>>>>>> + if (test_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags)) >>>>>>>>> return f1 > f2; >>>>>>>>> return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0; >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >

3 months, 1 week

Re: [PATCH v4 2/4] dmabuf: Implement copy_file_range callback for dmabuf direct I/O prep

by Christoph Hellwig

On Tue, Jun 03, 2025 at 05:52:43PM +0800, wangtao wrote: > +static ssize_t dma_buf_rw_file(struct dma_buf *dmabuf, loff_t my_pos, > + struct file *file, loff_t pos, size_t count, bool is_write) > +{ > + if (!dmabuf->ops->rw_file) > + return -EINVAL; > + > + if (my_pos >= dmabuf->size) > + count = 0; > + else > + count = min_t(size_t, count, dmabuf->size - my_pos); > + if (!count) > + return 0; > + > + return dmabuf->ops->rw_file(dmabuf, my_pos, file, pos, count, is_write); So despite claiming in the cover letter that dmabufs can't support direct I/O you are just reimplementing it badly here using a side interface.

3 months, 1 week

Re: [PATCH v4 2/9] dma-fence: Use a flag for 64-bit seqnos

by Christian König

On 6/3/25 13:30, Tvrtko Ursulin wrote: > > On 02/06/2025 19:00, Christian König wrote: >> On 6/2/25 17:25, Tvrtko Ursulin wrote: >>> >>> On 02/06/2025 15:42, Christian König wrote: >>>> On 6/2/25 15:05, Tvrtko Ursulin wrote: >>>>> >>>>> Hi, >>>>> >>>>> On 15/05/2025 14:15, Christian König wrote: >>>>>> Hey drm-misc maintainers, >>>>>> >>>>>> can you guys please backmerge drm-next into drm-misc-next? >>>>>> >>>>>> I want to push this patch here but it depends on changes which are partially in drm-next and partially in drm-misc-next. >>>>> >>>>> Looks like the backmerge is still pending? >>>> >>>> Yes, @Maarten, @Maxime and @Thomas ping on this. >>>> >>>>> In the meantime, Christian, any chance you will have some bandwith to think about the tail end of the series? Specifically patch 6 and how that is used onward. >>>> >>>> Well the RCU grace period is quite a nifty hack. I wanted to go over it again after merging the first patches from this series. >>>> >>>> In general looks like a good idea to me, I just don't like that we explicitely need to expose dma_fence_access_begin() and dma_fence_access_end(). >>>> >>>> Especially we can't do that while calling fence->ops->release. >>> >>> Hm why not? You think something will take offence of the rcu_read_lock()? >> >> Yes, especially it is perfectly legitimate to call synchronize_rcu() or lock semaphores/mutexes from that callback. >> >> Either keep the RCU critical section only for the trace or even better come up with some different approach, e.g. copying the string under the RCU lock or something like that. > > Hmm but the kerneldoc explicity says callback can be called from irq context: > > /** > * @release: > * > * Called on destruction of fence to release additional resources. > * Can be called from irq context. This callback is optional. If it is > * NULL, then dma_fence_free() is instead called as the default > * implementation. > */ > void (*release)(struct dma_fence *fence); Ah, right. I mixed that up with the dma-buf object. Yeah in that case that is probably harmless. We delegate the final free to a work item if necessary anyway. But I would still like to avoid having the RCU cover the release as well. Or why is there any reason why we would explicitely want to do this? Regards, Christian. > > > Regards, > > Tvrtko > >> >> Regards, >> Christian. >> >>> >>> Regards, >>> >>> Tvrtko >>> >>>>>> On 5/15/25 11:49, Tvrtko Ursulin wrote: >>>>>>> With the goal of reducing the need for drivers to touch (and dereference) >>>>>>> fence->ops, we move the 64-bit seqnos flag from struct dma_fence_ops to >>>>>>> the fence->flags. >>>>>>> >>>>>>> Drivers which were setting this flag are changed to use new >>>>>>> dma_fence_init64() instead of dma_fence_init(). >>>>>>> >>>>>>> v2: >>>>>>> * Streamlined init and added kerneldoc. >>>>>>> * Rebase for amdgpu userq which landed since. >>>>>>> >>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> >>>>>>> Reviewed-by: Christian König <christian.koenig(a)amd.com> # v1 >>>>>>> --- >>>>>>> drivers/dma-buf/dma-fence-chain.c | 5 +- >>>>>>> drivers/dma-buf/dma-fence.c | 69 ++++++++++++++----- >>>>>>> .../drm/amd/amdgpu/amdgpu_eviction_fence.c | 7 +- >>>>>>> .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 5 +- >>>>>>> .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 5 +- >>>>>>> include/linux/dma-fence.h | 14 ++-- >>>>>>> 6 files changed, 64 insertions(+), 41 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c >>>>>>> index 90424f23fd73..a8a90acf4f34 100644 >>>>>>> --- a/drivers/dma-buf/dma-fence-chain.c >>>>>>> +++ b/drivers/dma-buf/dma-fence-chain.c >>>>>>> @@ -218,7 +218,6 @@ static void dma_fence_chain_set_deadline(struct dma_fence *fence, >>>>>>> } >>>>>>> const struct dma_fence_ops dma_fence_chain_ops = { >>>>>>> - .use_64bit_seqno = true, >>>>>>> .get_driver_name = dma_fence_chain_get_driver_name, >>>>>>> .get_timeline_name = dma_fence_chain_get_timeline_name, >>>>>>> .enable_signaling = dma_fence_chain_enable_signaling, >>>>>>> @@ -262,8 +261,8 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, >>>>>>> seqno = max(prev->seqno, seqno); >>>>>>> } >>>>>>> - dma_fence_init(&chain->base, &dma_fence_chain_ops, >>>>>>> - &chain->lock, context, seqno); >>>>>>> + dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, >>>>>>> + context, seqno); >>>>>>> /* >>>>>>> * Chaining dma_fence_chain container together is only allowed through >>>>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c >>>>>>> index f0cdd3e99d36..705b59787731 100644 >>>>>>> --- a/drivers/dma-buf/dma-fence.c >>>>>>> +++ b/drivers/dma-buf/dma-fence.c >>>>>>> @@ -989,24 +989,9 @@ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) >>>>>>> } >>>>>>> EXPORT_SYMBOL(dma_fence_describe); >>>>>>> -/** >>>>>>> - * dma_fence_init - Initialize a custom fence. >>>>>>> - * @fence: the fence to initialize >>>>>>> - * @ops: the dma_fence_ops for operations on this fence >>>>>>> - * @lock: the irqsafe spinlock to use for locking this fence >>>>>>> - * @context: the execution context this fence is run on >>>>>>> - * @seqno: a linear increasing sequence number for this context >>>>>>> - * >>>>>>> - * Initializes an allocated fence, the caller doesn't have to keep its >>>>>>> - * refcount after committing with this fence, but it will need to hold a >>>>>>> - * refcount again if &dma_fence_ops.enable_signaling gets called. >>>>>>> - * >>>>>>> - * context and seqno are used for easy comparison between fences, allowing >>>>>>> - * to check which fence is later by simply using dma_fence_later(). >>>>>>> - */ >>>>>>> -void >>>>>>> -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>> - spinlock_t *lock, u64 context, u64 seqno) >>>>>>> +static void >>>>>>> +__dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>> + spinlock_t *lock, u64 context, u64 seqno, unsigned long flags) >>>>>>> { >>>>>>> BUG_ON(!lock); >>>>>>> BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name); >>>>>>> @@ -1017,9 +1002,55 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>> fence->lock = lock; >>>>>>> fence->context = context; >>>>>>> fence->seqno = seqno; >>>>>>> - fence->flags = 0UL; >>>>>>> + fence->flags = flags; >>>>>>> fence->error = 0; >>>>>>> trace_dma_fence_init(fence); >>>>>>> } >>>>>>> + >>>>>>> +/** >>>>>>> + * dma_fence_init - Initialize a custom fence. >>>>>>> + * @fence: the fence to initialize >>>>>>> + * @ops: the dma_fence_ops for operations on this fence >>>>>>> + * @lock: the irqsafe spinlock to use for locking this fence >>>>>>> + * @context: the execution context this fence is run on >>>>>>> + * @seqno: a linear increasing sequence number for this context >>>>>>> + * >>>>>>> + * Initializes an allocated fence, the caller doesn't have to keep its >>>>>>> + * refcount after committing with this fence, but it will need to hold a >>>>>>> + * refcount again if &dma_fence_ops.enable_signaling gets called. >>>>>>> + * >>>>>>> + * context and seqno are used for easy comparison between fences, allowing >>>>>>> + * to check which fence is later by simply using dma_fence_later(). >>>>>>> + */ >>>>>>> +void >>>>>>> +dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>> + spinlock_t *lock, u64 context, u64 seqno) >>>>>>> +{ >>>>>>> + __dma_fence_init(fence, ops, lock, context, seqno, 0UL); >>>>>>> +} >>>>>>> EXPORT_SYMBOL(dma_fence_init); >>>>>>> + >>>>>>> +/** >>>>>>> + * dma_fence_init64 - Initialize a custom fence with 64-bit seqno support. >>>>>>> + * @fence: the fence to initialize >>>>>>> + * @ops: the dma_fence_ops for operations on this fence >>>>>>> + * @lock: the irqsafe spinlock to use for locking this fence >>>>>>> + * @context: the execution context this fence is run on >>>>>>> + * @seqno: a linear increasing sequence number for this context >>>>>>> + * >>>>>>> + * Initializes an allocated fence, the caller doesn't have to keep its >>>>>>> + * refcount after committing with this fence, but it will need to hold a >>>>>>> + * refcount again if &dma_fence_ops.enable_signaling gets called. >>>>>>> + * >>>>>>> + * Context and seqno are used for easy comparison between fences, allowing >>>>>>> + * to check which fence is later by simply using dma_fence_later(). >>>>>>> + */ >>>>>>> +void >>>>>>> +dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>> + spinlock_t *lock, u64 context, u64 seqno) >>>>>>> +{ >>>>>>> + __dma_fence_init(fence, ops, lock, context, seqno, >>>>>>> + BIT(DMA_FENCE_FLAG_SEQNO64_BIT)); >>>>>>> +} >>>>>>> +EXPORT_SYMBOL(dma_fence_init64); >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c >>>>>>> index 1a7469543db5..79713421bffe 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c >>>>>>> @@ -134,7 +134,6 @@ static bool amdgpu_eviction_fence_enable_signaling(struct dma_fence *f) >>>>>>> } >>>>>>> static const struct dma_fence_ops amdgpu_eviction_fence_ops = { >>>>>>> - .use_64bit_seqno = true, >>>>>>> .get_driver_name = amdgpu_eviction_fence_get_driver_name, >>>>>>> .get_timeline_name = amdgpu_eviction_fence_get_timeline_name, >>>>>>> .enable_signaling = amdgpu_eviction_fence_enable_signaling, >>>>>>> @@ -160,9 +159,9 @@ amdgpu_eviction_fence_create(struct amdgpu_eviction_fence_mgr *evf_mgr) >>>>>>> ev_fence->evf_mgr = evf_mgr; >>>>>>> get_task_comm(ev_fence->timeline_name, current); >>>>>>> spin_lock_init(&ev_fence->lock); >>>>>>> - dma_fence_init(&ev_fence->base, &amdgpu_eviction_fence_ops, >>>>>>> - &ev_fence->lock, evf_mgr->ev_fence_ctx, >>>>>>> - atomic_inc_return(&evf_mgr->ev_fence_seq)); >>>>>>> + dma_fence_init64(&ev_fence->base, &amdgpu_eviction_fence_ops, >>>>>>> + &ev_fence->lock, evf_mgr->ev_fence_ctx, >>>>>>> + atomic_inc_return(&evf_mgr->ev_fence_seq)); >>>>>>> return ev_fence; >>>>>>> } >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >>>>>>> index 029cb24c28b3..5e92d00a591f 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >>>>>>> @@ -239,8 +239,8 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq, >>>>>>> fence = &userq_fence->base; >>>>>>> userq_fence->fence_drv = fence_drv; >>>>>>> - dma_fence_init(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, >>>>>>> - fence_drv->context, seq); >>>>>>> + dma_fence_init64(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, >>>>>>> + fence_drv->context, seq); >>>>>>> amdgpu_userq_fence_driver_get(fence_drv); >>>>>>> dma_fence_get(fence); >>>>>>> @@ -334,7 +334,6 @@ static void amdgpu_userq_fence_release(struct dma_fence *f) >>>>>>> } >>>>>>> static const struct dma_fence_ops amdgpu_userq_fence_ops = { >>>>>>> - .use_64bit_seqno = true, >>>>>>> .get_driver_name = amdgpu_userq_fence_get_driver_name, >>>>>>> .get_timeline_name = amdgpu_userq_fence_get_timeline_name, >>>>>>> .signaled = amdgpu_userq_fence_signaled, >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c >>>>>>> index 51cddfa3f1e8..5d26797356a3 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c >>>>>>> @@ -71,7 +71,6 @@ static void amdgpu_tlb_fence_work(struct work_struct *work) >>>>>>> } >>>>>>> static const struct dma_fence_ops amdgpu_tlb_fence_ops = { >>>>>>> - .use_64bit_seqno = true, >>>>>>> .get_driver_name = amdgpu_tlb_fence_get_driver_name, >>>>>>> .get_timeline_name = amdgpu_tlb_fence_get_timeline_name >>>>>>> }; >>>>>>> @@ -101,8 +100,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm >>>>>>> INIT_WORK(&f->work, amdgpu_tlb_fence_work); >>>>>>> spin_lock_init(&f->lock); >>>>>>> - dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock, >>>>>>> - vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); >>>>>>> + dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock, >>>>>>> + vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); >>>>>>> /* TODO: We probably need a separate wq here */ >>>>>>> dma_fence_get(&f->base); >>>>>>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h >>>>>>> index 48b5202c531d..a34a0dcdc446 100644 >>>>>>> --- a/include/linux/dma-fence.h >>>>>>> +++ b/include/linux/dma-fence.h >>>>>>> @@ -97,6 +97,7 @@ struct dma_fence { >>>>>>> }; >>>>>>> enum dma_fence_flag_bits { >>>>>>> + DMA_FENCE_FLAG_SEQNO64_BIT, >>>>>>> DMA_FENCE_FLAG_SIGNALED_BIT, >>>>>>> DMA_FENCE_FLAG_TIMESTAMP_BIT, >>>>>>> DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >>>>>>> @@ -124,14 +125,6 @@ struct dma_fence_cb { >>>>>>> * >>>>>>> */ >>>>>>> struct dma_fence_ops { >>>>>>> - /** >>>>>>> - * @use_64bit_seqno: >>>>>>> - * >>>>>>> - * True if this dma_fence implementation uses 64bit seqno, false >>>>>>> - * otherwise. >>>>>>> - */ >>>>>>> - bool use_64bit_seqno; >>>>>>> - >>>>>>> /** >>>>>>> * @get_driver_name: >>>>>>> * >>>>>>> @@ -262,6 +255,9 @@ struct dma_fence_ops { >>>>>>> void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>> spinlock_t *lock, u64 context, u64 seqno); >>>>>>> +void dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>>>> + spinlock_t *lock, u64 context, u64 seqno); >>>>>>> + >>>>>>> void dma_fence_release(struct kref *kref); >>>>>>> void dma_fence_free(struct dma_fence *fence); >>>>>>> void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq); >>>>>>> @@ -454,7 +450,7 @@ static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2) >>>>>>> * 32bit sequence numbers. Use a 64bit compare when the driver says to >>>>>>> * do so. >>>>>>> */ >>>>>>> - if (fence->ops->use_64bit_seqno) >>>>>>> + if (test_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags)) >>>>>>> return f1 > f2; >>>>>>> return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0; >>>>>> >>>>> >>>> >>> >> >

3 months, 1 week

Re: [RFC PATCH 17/30] iommufd/device: Add TSM Bind/Unbind for TIO support

by Jason Gunthorpe

On Tue, Jun 03, 2025 at 02:20:51PM +0800, Xu Yilun wrote: > > Wouldn’t it be simpler to skip the reference count increment altogether > > and just call tsm_unbind in the virtual device’s destroy callback? > > (iommufd_vdevice_destroy()) > > The vdevice refcount is the main concern, there is also an IOMMU_DESTROY > ioctl. User could just free the vdevice instance if no refcount, while VFIO > is still in bound state. That seems not the correct free order. Freeing the vdevice should automatically unbind it.. Jason

3 months, 1 week

Re: [PATCH v4 2/4] dmabuf: Implement copy_file_range callback for dmabuf direct I/O prep

by Christian König

On 6/3/25 11:52, wangtao wrote: > First determine if dmabuf reads from or writes to the file. > Then call exporter's rw_file callback function. > > Signed-off-by: wangtao <tao.wangtao(a)honor.com> > --- > drivers/dma-buf/dma-buf.c | 32 ++++++++++++++++++++++++++++++++ > include/linux/dma-buf.h | 16 ++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 5baa83b85515..fc9bf54c921a 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -523,7 +523,38 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) > spin_unlock(&dmabuf->name_lock); > } > > +static ssize_t dma_buf_rw_file(struct dma_buf *dmabuf, loff_t my_pos, > + struct file *file, loff_t pos, size_t count, bool is_write) > +{ > + if (!dmabuf->ops->rw_file) > + return -EINVAL; > + > + if (my_pos >= dmabuf->size) > + count = 0; > + else > + count = min_t(size_t, count, dmabuf->size - my_pos); > + if (!count) > + return 0; > + > + return dmabuf->ops->rw_file(dmabuf, my_pos, file, pos, count, is_write); > +} > + > +static ssize_t dma_buf_copy_file_range(struct file *file_in, loff_t pos_in, > + struct file *file_out, loff_t pos_out, > + size_t count, unsigned int flags) > +{ > + if (is_dma_buf_file(file_in) && file_out->f_op->write_iter) > + return dma_buf_rw_file(file_in->private_data, pos_in, > + file_out, pos_out, count, true); > + else if (is_dma_buf_file(file_out) && file_in->f_op->read_iter) > + return dma_buf_rw_file(file_out->private_data, pos_out, > + file_in, pos_in, count, false); > + else > + return -EINVAL; > +} > + > static const struct file_operations dma_buf_fops = { > + .fop_flags = FOP_MEMORY_FILE, > .release = dma_buf_file_release, > .mmap = dma_buf_mmap_internal, > .llseek = dma_buf_llseek, > @@ -531,6 +562,7 @@ static const struct file_operations dma_buf_fops = { > .unlocked_ioctl = dma_buf_ioctl, > .compat_ioctl = compat_ptr_ioctl, > .show_fdinfo = dma_buf_show_fdinfo, > + .copy_file_range = dma_buf_copy_file_range, > }; > > /* > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index 36216d28d8bd..d3636e985399 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -22,6 +22,7 @@ > #include <linux/fs.h> > #include <linux/dma-fence.h> > #include <linux/wait.h> > +#include <uapi/linux/dma-buf.h> > > struct device; > struct dma_buf; > @@ -285,6 +286,21 @@ struct dma_buf_ops { > > int (*vmap)(struct dma_buf *dmabuf, struct iosys_map *map); > void (*vunmap)(struct dma_buf *dmabuf, struct iosys_map *map); > + > + /** > + * @rw_file: > + * > + * If an Exporter needs to support Direct I/O file operations, it can > + * implement this optional callback. The exporter must verify that no > + * other objects hold the sg_table, ensure exclusive access to the > + * dmabuf's sg_table, and only then proceed with the I/O operation. Explain why and not what. E.g. something like "Allows direct I/O between this DMA-buf and the file". Completely drop mentioning the sg_table, that is irrelevant. Exclusive access depends on how the exporter implements the whole thing. Regards, Christian. > + * > + * Returns: > + * > + * 0 on success or a negative error code on failure. > + */ > + ssize_t (*rw_file)(struct dma_buf *dmabuf, loff_t my_pos, > + struct file *file, loff_t pos, size_t count, bool is_write); > }; > > /**

3 months, 1 week

Re: [PATCH v4 2/9] dma-fence: Use a flag for 64-bit seqnos

by Christian König

On 6/2/25 17:25, Tvrtko Ursulin wrote: > > On 02/06/2025 15:42, Christian König wrote: >> On 6/2/25 15:05, Tvrtko Ursulin wrote: >>> >>> Hi, >>> >>> On 15/05/2025 14:15, Christian König wrote: >>>> Hey drm-misc maintainers, >>>> >>>> can you guys please backmerge drm-next into drm-misc-next? >>>> >>>> I want to push this patch here but it depends on changes which are partially in drm-next and partially in drm-misc-next. >>> >>> Looks like the backmerge is still pending? >> >> Yes, @Maarten, @Maxime and @Thomas ping on this. >> >>> In the meantime, Christian, any chance you will have some bandwith to think about the tail end of the series? Specifically patch 6 and how that is used onward. >> >> Well the RCU grace period is quite a nifty hack. I wanted to go over it again after merging the first patches from this series. >> >> In general looks like a good idea to me, I just don't like that we explicitely need to expose dma_fence_access_begin() and dma_fence_access_end(). >> >> Especially we can't do that while calling fence->ops->release. > > Hm why not? You think something will take offence of the rcu_read_lock()? Yes, especially it is perfectly legitimate to call synchronize_rcu() or lock semaphores/mutexes from that callback. Either keep the RCU critical section only for the trace or even better come up with some different approach, e.g. copying the string under the RCU lock or something like that. Regards, Christian. > > Regards, > > Tvrtko > >>>> On 5/15/25 11:49, Tvrtko Ursulin wrote: >>>>> With the goal of reducing the need for drivers to touch (and dereference) >>>>> fence->ops, we move the 64-bit seqnos flag from struct dma_fence_ops to >>>>> the fence->flags. >>>>> >>>>> Drivers which were setting this flag are changed to use new >>>>> dma_fence_init64() instead of dma_fence_init(). >>>>> >>>>> v2: >>>>> * Streamlined init and added kerneldoc. >>>>> * Rebase for amdgpu userq which landed since. >>>>> >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> >>>>> Reviewed-by: Christian König <christian.koenig(a)amd.com> # v1 >>>>> --- >>>>> drivers/dma-buf/dma-fence-chain.c | 5 +- >>>>> drivers/dma-buf/dma-fence.c | 69 ++++++++++++++----- >>>>> .../drm/amd/amdgpu/amdgpu_eviction_fence.c | 7 +- >>>>> .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 5 +- >>>>> .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 5 +- >>>>> include/linux/dma-fence.h | 14 ++-- >>>>> 6 files changed, 64 insertions(+), 41 deletions(-) >>>>> >>>>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c >>>>> index 90424f23fd73..a8a90acf4f34 100644 >>>>> --- a/drivers/dma-buf/dma-fence-chain.c >>>>> +++ b/drivers/dma-buf/dma-fence-chain.c >>>>> @@ -218,7 +218,6 @@ static void dma_fence_chain_set_deadline(struct dma_fence *fence, >>>>> } >>>>> const struct dma_fence_ops dma_fence_chain_ops = { >>>>> - .use_64bit_seqno = true, >>>>> .get_driver_name = dma_fence_chain_get_driver_name, >>>>> .get_timeline_name = dma_fence_chain_get_timeline_name, >>>>> .enable_signaling = dma_fence_chain_enable_signaling, >>>>> @@ -262,8 +261,8 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, >>>>> seqno = max(prev->seqno, seqno); >>>>> } >>>>> - dma_fence_init(&chain->base, &dma_fence_chain_ops, >>>>> - &chain->lock, context, seqno); >>>>> + dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, >>>>> + context, seqno); >>>>> /* >>>>> * Chaining dma_fence_chain container together is only allowed through >>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c >>>>> index f0cdd3e99d36..705b59787731 100644 >>>>> --- a/drivers/dma-buf/dma-fence.c >>>>> +++ b/drivers/dma-buf/dma-fence.c >>>>> @@ -989,24 +989,9 @@ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) >>>>> } >>>>> EXPORT_SYMBOL(dma_fence_describe); >>>>> -/** >>>>> - * dma_fence_init - Initialize a custom fence. >>>>> - * @fence: the fence to initialize >>>>> - * @ops: the dma_fence_ops for operations on this fence >>>>> - * @lock: the irqsafe spinlock to use for locking this fence >>>>> - * @context: the execution context this fence is run on >>>>> - * @seqno: a linear increasing sequence number for this context >>>>> - * >>>>> - * Initializes an allocated fence, the caller doesn't have to keep its >>>>> - * refcount after committing with this fence, but it will need to hold a >>>>> - * refcount again if &dma_fence_ops.enable_signaling gets called. >>>>> - * >>>>> - * context and seqno are used for easy comparison between fences, allowing >>>>> - * to check which fence is later by simply using dma_fence_later(). >>>>> - */ >>>>> -void >>>>> -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>> - spinlock_t *lock, u64 context, u64 seqno) >>>>> +static void >>>>> +__dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>> + spinlock_t *lock, u64 context, u64 seqno, unsigned long flags) >>>>> { >>>>> BUG_ON(!lock); >>>>> BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name); >>>>> @@ -1017,9 +1002,55 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>> fence->lock = lock; >>>>> fence->context = context; >>>>> fence->seqno = seqno; >>>>> - fence->flags = 0UL; >>>>> + fence->flags = flags; >>>>> fence->error = 0; >>>>> trace_dma_fence_init(fence); >>>>> } >>>>> + >>>>> +/** >>>>> + * dma_fence_init - Initialize a custom fence. >>>>> + * @fence: the fence to initialize >>>>> + * @ops: the dma_fence_ops for operations on this fence >>>>> + * @lock: the irqsafe spinlock to use for locking this fence >>>>> + * @context: the execution context this fence is run on >>>>> + * @seqno: a linear increasing sequence number for this context >>>>> + * >>>>> + * Initializes an allocated fence, the caller doesn't have to keep its >>>>> + * refcount after committing with this fence, but it will need to hold a >>>>> + * refcount again if &dma_fence_ops.enable_signaling gets called. >>>>> + * >>>>> + * context and seqno are used for easy comparison between fences, allowing >>>>> + * to check which fence is later by simply using dma_fence_later(). >>>>> + */ >>>>> +void >>>>> +dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>> + spinlock_t *lock, u64 context, u64 seqno) >>>>> +{ >>>>> + __dma_fence_init(fence, ops, lock, context, seqno, 0UL); >>>>> +} >>>>> EXPORT_SYMBOL(dma_fence_init); >>>>> + >>>>> +/** >>>>> + * dma_fence_init64 - Initialize a custom fence with 64-bit seqno support. >>>>> + * @fence: the fence to initialize >>>>> + * @ops: the dma_fence_ops for operations on this fence >>>>> + * @lock: the irqsafe spinlock to use for locking this fence >>>>> + * @context: the execution context this fence is run on >>>>> + * @seqno: a linear increasing sequence number for this context >>>>> + * >>>>> + * Initializes an allocated fence, the caller doesn't have to keep its >>>>> + * refcount after committing with this fence, but it will need to hold a >>>>> + * refcount again if &dma_fence_ops.enable_signaling gets called. >>>>> + * >>>>> + * Context and seqno are used for easy comparison between fences, allowing >>>>> + * to check which fence is later by simply using dma_fence_later(). >>>>> + */ >>>>> +void >>>>> +dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>> + spinlock_t *lock, u64 context, u64 seqno) >>>>> +{ >>>>> + __dma_fence_init(fence, ops, lock, context, seqno, >>>>> + BIT(DMA_FENCE_FLAG_SEQNO64_BIT)); >>>>> +} >>>>> +EXPORT_SYMBOL(dma_fence_init64); >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c >>>>> index 1a7469543db5..79713421bffe 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c >>>>> @@ -134,7 +134,6 @@ static bool amdgpu_eviction_fence_enable_signaling(struct dma_fence *f) >>>>> } >>>>> static const struct dma_fence_ops amdgpu_eviction_fence_ops = { >>>>> - .use_64bit_seqno = true, >>>>> .get_driver_name = amdgpu_eviction_fence_get_driver_name, >>>>> .get_timeline_name = amdgpu_eviction_fence_get_timeline_name, >>>>> .enable_signaling = amdgpu_eviction_fence_enable_signaling, >>>>> @@ -160,9 +159,9 @@ amdgpu_eviction_fence_create(struct amdgpu_eviction_fence_mgr *evf_mgr) >>>>> ev_fence->evf_mgr = evf_mgr; >>>>> get_task_comm(ev_fence->timeline_name, current); >>>>> spin_lock_init(&ev_fence->lock); >>>>> - dma_fence_init(&ev_fence->base, &amdgpu_eviction_fence_ops, >>>>> - &ev_fence->lock, evf_mgr->ev_fence_ctx, >>>>> - atomic_inc_return(&evf_mgr->ev_fence_seq)); >>>>> + dma_fence_init64(&ev_fence->base, &amdgpu_eviction_fence_ops, >>>>> + &ev_fence->lock, evf_mgr->ev_fence_ctx, >>>>> + atomic_inc_return(&evf_mgr->ev_fence_seq)); >>>>> return ev_fence; >>>>> } >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >>>>> index 029cb24c28b3..5e92d00a591f 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >>>>> @@ -239,8 +239,8 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq, >>>>> fence = &userq_fence->base; >>>>> userq_fence->fence_drv = fence_drv; >>>>> - dma_fence_init(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, >>>>> - fence_drv->context, seq); >>>>> + dma_fence_init64(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, >>>>> + fence_drv->context, seq); >>>>> amdgpu_userq_fence_driver_get(fence_drv); >>>>> dma_fence_get(fence); >>>>> @@ -334,7 +334,6 @@ static void amdgpu_userq_fence_release(struct dma_fence *f) >>>>> } >>>>> static const struct dma_fence_ops amdgpu_userq_fence_ops = { >>>>> - .use_64bit_seqno = true, >>>>> .get_driver_name = amdgpu_userq_fence_get_driver_name, >>>>> .get_timeline_name = amdgpu_userq_fence_get_timeline_name, >>>>> .signaled = amdgpu_userq_fence_signaled, >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c >>>>> index 51cddfa3f1e8..5d26797356a3 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c >>>>> @@ -71,7 +71,6 @@ static void amdgpu_tlb_fence_work(struct work_struct *work) >>>>> } >>>>> static const struct dma_fence_ops amdgpu_tlb_fence_ops = { >>>>> - .use_64bit_seqno = true, >>>>> .get_driver_name = amdgpu_tlb_fence_get_driver_name, >>>>> .get_timeline_name = amdgpu_tlb_fence_get_timeline_name >>>>> }; >>>>> @@ -101,8 +100,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm >>>>> INIT_WORK(&f->work, amdgpu_tlb_fence_work); >>>>> spin_lock_init(&f->lock); >>>>> - dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock, >>>>> - vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); >>>>> + dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock, >>>>> + vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); >>>>> /* TODO: We probably need a separate wq here */ >>>>> dma_fence_get(&f->base); >>>>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h >>>>> index 48b5202c531d..a34a0dcdc446 100644 >>>>> --- a/include/linux/dma-fence.h >>>>> +++ b/include/linux/dma-fence.h >>>>> @@ -97,6 +97,7 @@ struct dma_fence { >>>>> }; >>>>> enum dma_fence_flag_bits { >>>>> + DMA_FENCE_FLAG_SEQNO64_BIT, >>>>> DMA_FENCE_FLAG_SIGNALED_BIT, >>>>> DMA_FENCE_FLAG_TIMESTAMP_BIT, >>>>> DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >>>>> @@ -124,14 +125,6 @@ struct dma_fence_cb { >>>>> * >>>>> */ >>>>> struct dma_fence_ops { >>>>> - /** >>>>> - * @use_64bit_seqno: >>>>> - * >>>>> - * True if this dma_fence implementation uses 64bit seqno, false >>>>> - * otherwise. >>>>> - */ >>>>> - bool use_64bit_seqno; >>>>> - >>>>> /** >>>>> * @get_driver_name: >>>>> * >>>>> @@ -262,6 +255,9 @@ struct dma_fence_ops { >>>>> void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>> spinlock_t *lock, u64 context, u64 seqno); >>>>> +void dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, >>>>> + spinlock_t *lock, u64 context, u64 seqno); >>>>> + >>>>> void dma_fence_release(struct kref *kref); >>>>> void dma_fence_free(struct dma_fence *fence); >>>>> void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq); >>>>> @@ -454,7 +450,7 @@ static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2) >>>>> * 32bit sequence numbers. Use a 64bit compare when the driver says to >>>>> * do so. >>>>> */ >>>>> - if (fence->ops->use_64bit_seqno) >>>>> + if (test_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags)) >>>>> return f1 > f2; >>>>> return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0; >>>> >>> >> >

3 months, 1 week

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

by Jens Wiklander

Hi Amir, On Fri, May 30, 2025 at 4:13 AM Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com> wrote: > > Hi Jens, > > On 5/21/2025 1:16 AM, Jens Wiklander wrote: > > Implement DMA heap for protected DMA-buf allocation in the TEE > > subsystem. > > > > Restricted memory refers to memory buffers behind a hardware enforced > > firewall. It is not accessible to the kernel during normal circumstances > > but rather only accessible to certain hardware IPs or CPUs executing in > > higher or differently privileged mode than the kernel itself. This > > interface allows to allocate and manage such protected memory buffers > > via interaction with a TEE implementation. > > > > The protected memory is allocated for a specific use-case, like Secure > > Video Playback, Trusted UI, or Secure Video Recording where certain > > hardware devices can access the memory. > > > > The DMA-heaps are enabled explicitly by the TEE backend driver. The TEE > > backend drivers needs to implement protected memory pool to manage the > > protected memory. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/Makefile | 1 + > > drivers/tee/tee_heap.c | 487 ++++++++++++++++++++++++++++++++++++++ > > drivers/tee/tee_private.h | 6 + > > include/linux/tee_core.h | 65 +++++ > > 4 files changed, 559 insertions(+) > > create mode 100644 drivers/tee/tee_heap.c > > > > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile > > index 5488cba30bd2..949a6a79fb06 100644 > > --- a/drivers/tee/Makefile > > +++ b/drivers/tee/Makefile > > @@ -1,6 +1,7 @@ > > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_TEE) += tee.o > > tee-objs += tee_core.o > > +tee-objs += tee_heap.o > > tee-objs += tee_shm.o > > tee-objs += tee_shm_pool.o > > obj-$(CONFIG_OPTEE) += optee/ > > diff --git a/drivers/tee/tee_heap.c b/drivers/tee/tee_heap.c > > new file mode 100644 > > index 000000000000..a332805f9f26 > > --- /dev/null > > +++ b/drivers/tee/tee_heap.c > > @@ -0,0 +1,487 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2025, Linaro Limited > > + */ > > + > > +#include <linux/dma-buf.h> > > +#include <linux/dma-heap.h> > > +#include <linux/genalloc.h> > > +#include <linux/module.h> > > +#include <linux/scatterlist.h> > > +#include <linux/slab.h> > > +#include <linux/tee_core.h> > > +#include <linux/xarray.h> > > + > > +#include "tee_private.h" > > + > > +struct tee_dma_heap { > > + struct dma_heap *heap; > > + enum tee_dma_heap_id id; > > + struct tee_protmem_pool *pool; > > + struct tee_device *teedev; > > + /* Protects pool and teedev above */ > > + struct mutex mu; > > +}; > > + > > +struct tee_heap_buffer { > > + struct tee_protmem_pool *pool; > > + struct tee_device *teedev; > > + size_t size; > > + size_t offs; > > + struct sg_table table; > > +}; > > + > > +struct tee_heap_attachment { > > + struct sg_table table; > > + struct device *dev; > > +}; > > + > > +struct tee_protmem_static_pool { > > + struct tee_protmem_pool pool; > > + struct gen_pool *gen_pool; > > + phys_addr_t pa_base; > > + void *base; > > +}; > > + > > Isn't using an xarray excessive for just three entries, given static, limited IDs? The interface is nice and scales well. Do you think it adds too much overhead? What should we use instead? > > > +#if IS_ENABLED(CONFIG_DMABUF_HEAPS) > > +static DEFINE_XARRAY_ALLOC(tee_dma_heap); > > + > > Why are we even considering sgl in the first place, given that tee_shm_register_fd() > does not accept any dma_buf with more than one entry? Wouldn't it make sense to > ensure sgl has a single entry after calling alloc()? I didn't want to close that door completely, even if we can currently only have a single entry. > > > +static int copy_sg_table(struct sg_table *dst, struct sg_table *src) > > +{ > > + struct scatterlist *dst_sg; > > + struct scatterlist *src_sg; > > + int ret; > > + int i; > > + > > + ret = sg_alloc_table(dst, src->orig_nents, GFP_KERNEL); > > + if (ret) > > + return ret; > > + > > + dst_sg = dst->sgl; > > + for_each_sgtable_sg(src, src_sg, i) { > > + sg_set_page(dst_sg, sg_page(src_sg), src_sg->length, > > + src_sg->offset); > > + dst_sg = sg_next(dst_sg); > > + } > > + > > + return 0; > > +} > > + > > +static int tee_heap_attach(struct dma_buf *dmabuf, > > + struct dma_buf_attachment *attachment) > > +{ > > + struct tee_heap_buffer *buf = dmabuf->priv; > > + struct tee_heap_attachment *a; > > + int ret; > > + > > + a = kzalloc(sizeof(*a), GFP_KERNEL); > > + if (!a) > > + return -ENOMEM; > > + > > + ret = copy_sg_table(&a->table, &buf->table); > > + if (ret) { > > + kfree(a); > > + return ret; > > + } > > + > > + a->dev = attachment->dev; > > + attachment->priv = a; > > + > > + return 0; > > +} > > + > > +static void tee_heap_detach(struct dma_buf *dmabuf, > > + struct dma_buf_attachment *attachment) > > +{ > > + struct tee_heap_attachment *a = attachment->priv; > > + > > + sg_free_table(&a->table); > > + kfree(a); > > +} > > + > > +static struct sg_table * > > +tee_heap_map_dma_buf(struct dma_buf_attachment *attachment, > > + enum dma_data_direction direction) > > +{ > > + struct tee_heap_attachment *a = attachment->priv; > > + int ret; > > + > > + ret = dma_map_sgtable(attachment->dev, &a->table, direction, > > + DMA_ATTR_SKIP_CPU_SYNC); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return &a->table; > > +} > > + > > +static void tee_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, > > + struct sg_table *table, > > + enum dma_data_direction direction) > > +{ > > + struct tee_heap_attachment *a = attachment->priv; > > + > > + WARN_ON(&a->table != table); > > + > > + dma_unmap_sgtable(attachment->dev, table, direction, > > + DMA_ATTR_SKIP_CPU_SYNC); > > +} > > + > > +static void tee_heap_buf_free(struct dma_buf *dmabuf) > > +{ > > + struct tee_heap_buffer *buf = dmabuf->priv; > > + struct tee_device *teedev = buf->teedev; > > + > > + buf->pool->ops->free(buf->pool, &buf->table); > > + tee_device_put(teedev); > > +} > > + > > +static const struct dma_buf_ops tee_heap_buf_ops = { > > + .attach = tee_heap_attach, > > + .detach = tee_heap_detach, > > + .map_dma_buf = tee_heap_map_dma_buf, > > + .unmap_dma_buf = tee_heap_unmap_dma_buf, > > + .release = tee_heap_buf_free, > > +}; > > + > > +static struct dma_buf *tee_dma_heap_alloc(struct dma_heap *heap, > > + unsigned long len, u32 fd_flags, > > + u64 heap_flags) > > +{ > > + struct tee_dma_heap *h = dma_heap_get_drvdata(heap); > > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > > + struct tee_device *teedev = NULL; > > + struct tee_heap_buffer *buf; > > + struct tee_protmem_pool *pool; > > + struct dma_buf *dmabuf; > > + int rc; > > + > > + mutex_lock(&h->mu); > > + if (tee_device_get(h->teedev)) { > > + teedev = h->teedev; > > + pool = h->pool; > > + } > > + mutex_unlock(&h->mu); > > + > > + if (!teedev) > > + return ERR_PTR(-EINVAL); > > + > > + buf = kzalloc(sizeof(*buf), GFP_KERNEL); > > + if (!buf) { > > + dmabuf = ERR_PTR(-ENOMEM); > > + goto err; > > + } > > + buf->size = len; > > + buf->pool = pool; > > + buf->teedev = teedev; > > + > > + rc = pool->ops->alloc(pool, &buf->table, len, &buf->offs); > > + if (rc) { > > + dmabuf = ERR_PTR(rc); > > + goto err_kfree; > > + } > > + > > + exp_info.ops = &tee_heap_buf_ops; > > + exp_info.size = len; > > + exp_info.priv = buf; > > + exp_info.flags = fd_flags; > > + dmabuf = dma_buf_export(&exp_info); > > + if (IS_ERR(dmabuf)) > > + goto err_protmem_free; > > + > > + return dmabuf; > > + > > +err_protmem_free: > > + pool->ops->free(pool, &buf->table); > > +err_kfree: > > + kfree(buf); > > +err: > > + tee_device_put(h->teedev); > > + return dmabuf; > > +} > > + > > +static const struct dma_heap_ops tee_dma_heap_ops = { > > + .allocate = tee_dma_heap_alloc, > > +}; > > + > > +static const char *heap_id_2_name(enum tee_dma_heap_id id) > > +{ > > + switch (id) { > > + case TEE_DMA_HEAP_SECURE_VIDEO_PLAY: > > + return "protected,secure-video"; > > + case TEE_DMA_HEAP_TRUSTED_UI: > > + return "protected,trusted-ui"; > > + case TEE_DMA_HEAP_SECURE_VIDEO_RECORD: > > + return "protected,secure-video-record"; > > + default: > > + return NULL; > > + } > > +} > > + > > +static int alloc_dma_heap(struct tee_device *teedev, enum tee_dma_heap_id id, > > + struct tee_protmem_pool *pool) > > +{ > > + struct dma_heap_export_info exp_info = { > > + .ops = &tee_dma_heap_ops, > > + .name = heap_id_2_name(id), > > + }; > > + struct tee_dma_heap *h; > > + int rc; > > + > > + if (!exp_info.name) > > + return -EINVAL; > > + > > + if (xa_reserve(&tee_dma_heap, id, GFP_KERNEL)) { > > + if (!xa_load(&tee_dma_heap, id)) > > + return -EEXIST; > > + return -ENOMEM; > > + } > > + > > + h = kzalloc(sizeof(*h), GFP_KERNEL); > > + if (!h) > > + return -ENOMEM; > > + h->id = id; > > + h->teedev = teedev; > > + h->pool = pool; > > + mutex_init(&h->mu); > > + > > + exp_info.priv = h; > > + h->heap = dma_heap_add(&exp_info); > > + if (IS_ERR(h->heap)) { > > + rc = PTR_ERR(h->heap); > > + kfree(h); > > + > > + return rc; > > + } > > + > > + /* "can't fail" due to the call to xa_reserve() above */ > > + return WARN(xa_store(&tee_dma_heap, id, h, GFP_KERNEL), > > + "xa_store() failed"); > > +} > > + > > +int tee_device_register_dma_heap(struct tee_device *teedev, > > + enum tee_dma_heap_id id, > > + struct tee_protmem_pool *pool) > > +{ > > + struct tee_dma_heap *h; > > + int rc; > > + > > + h = xa_load(&tee_dma_heap, id); > > + if (h) { > > + mutex_lock(&h->mu); > > + if (h->teedev) { > > + rc = -EBUSY; > > + } else { > > + h->teedev = teedev; > > + h->pool = pool; > > + rc = 0; > > + } > > + mutex_unlock(&h->mu); > > + } else { > > + rc = alloc_dma_heap(teedev, id, pool); > > + } > > + > > + if (rc) > > + dev_err(&teedev->dev, "can't register DMA heap id %d (%s)\n", > > + id, heap_id_2_name(id)); > > + > > + return rc; > > +} > > +EXPORT_SYMBOL_GPL(tee_device_register_dma_heap); > > + > > +void tee_device_unregister_all_dma_heaps(struct tee_device *teedev) > > +{ > > + struct tee_protmem_pool *pool; > > + struct tee_dma_heap *h; > > + u_long i; > > + > > + xa_for_each(&tee_dma_heap, i, h) { > > + if (h) { > > + pool = NULL; > > + mutex_lock(&h->mu); > > + if (h->teedev == teedev) { > > + pool = h->pool; > > + h->teedev = NULL; > > + h->pool = NULL; > > + } > > + mutex_unlock(&h->mu); > > + if (pool) > > + pool->ops->destroy_pool(pool); > > + } > > + } > > +} > > +EXPORT_SYMBOL_GPL(tee_device_unregister_all_dma_heaps); > > + > > (Please first see comment below for update_shm()) > > Other than calling update_shm(), nothing significant happens in this function. > If we remove update_shm(), these operations can be moved to the register function. Yes, but at the expense of making that function more complicated. > > However, I also argue that since we have a well-designed generic function like > tee_ioctl_shm_register_fd(), why should we restrict ourselves to checking the type of > dma_buf? Any dma_buf should be acceptable to register as tee_shm (thougn it should fail > if it is used in protected operation). Otherwise, TEE_IOC_SHM_REGISTER_FD is > not an accurate name it should be TEE_IOC_SHM_REGISTER_PROTECTED_FD (or something similar). tee_shm_register_fd() can handle both DMA-bufs from a registered TEE heap and DMA-bufs allocated by other means. The backend driver may have further restrictions. The OP-TEE FF-A driver can only accept DMA-bufs allocated from a registered TEE heap since it needs the shared memory handle of the underlying pool to pass the buffer as an argument to the secure world. > > > +int tee_heap_update_from_dma_buf(struct tee_device *teedev, > > + struct dma_buf *dmabuf, size_t *offset, > > + struct tee_shm *shm, > > + struct tee_shm **parent_shm) > > +{ > > + struct tee_heap_buffer *buf; > > + int rc; > > + > > + /* The DMA-buf must be from our heap */ > > + if (dmabuf->ops != &tee_heap_buf_ops) > > + return -EINVAL; > > + > > + buf = dmabuf->priv; > > + /* The buffer must be from the same teedev */ > > + if (buf->teedev != teedev) > > + return -EINVAL; > > + > > + shm->size = buf->size; > > + > > + rc = buf->pool->ops->update_shm(buf->pool, &buf->table, buf->offs, shm, > > + parent_shm); > > + if (!rc && *parent_shm) > > + *offset = buf->offs; > > + > > + return rc; > > +} > > +#else > > +int tee_device_register_dma_heap(struct tee_device *teedev __always_unused, > > + enum tee_dma_heap_id id __always_unused, > > + struct tee_protmem_pool *pool __always_unused) > > +{ > > + return -EINVAL; > > +} > > +EXPORT_SYMBOL_GPL(tee_device_register_dma_heap); > > + > > +void > > +tee_device_unregister_all_dma_heaps(struct tee_device *teedev __always_unused) > > +{ > > +} > > +EXPORT_SYMBOL_GPL(tee_device_unregister_all_dma_heaps); > > + > > +int tee_heap_update_from_dma_buf(struct tee_device *teedev __always_unused, > > + struct dma_buf *dmabuf __always_unused, > > + size_t *offset __always_unused, > > + struct tee_shm *shm __always_unused, > > + struct tee_shm **parent_shm __always_unused) > > +{ > > + return -EINVAL; > > +} > > +#endif > > + > > +static struct tee_protmem_static_pool * > > +to_protmem_static_pool(struct tee_protmem_pool *pool) > > +{ > > + return container_of(pool, struct tee_protmem_static_pool, pool); > > +} > > + > > +static int protmem_pool_op_static_alloc(struct tee_protmem_pool *pool, > > + struct sg_table *sgt, size_t size, > > + size_t *offs) > > +{ > > + struct tee_protmem_static_pool *stp = to_protmem_static_pool(pool); > > + phys_addr_t pa; > > + int ret; > > + > > + pa = gen_pool_alloc(stp->gen_pool, size); > > + if (!pa) > > + return -ENOMEM; > > + > > + ret = sg_alloc_table(sgt, 1, GFP_KERNEL); > > + if (ret) { > > + gen_pool_free(stp->gen_pool, pa, size); > > + return ret; > > + } > > + > > + sg_set_page(sgt->sgl, phys_to_page(pa), size, 0); > > + *offs = pa - stp->pa_base; > > + > > + return 0; > > +} > > + > > +static void protmem_pool_op_static_free(struct tee_protmem_pool *pool, > > + struct sg_table *sgt) > > +{ > > + struct tee_protmem_static_pool *stp = to_protmem_static_pool(pool); > > + struct scatterlist *sg; > > + int i; > > + > > Should be a loop? > > > + for_each_sgtable_sg(sgt, sg, i) > > + gen_pool_free(stp->gen_pool, sg_phys(sg), sg->length); > > + sg_free_table(sgt); > > +} > > + > > +static int protmem_pool_op_static_update_shm(struct tee_protmem_pool *pool, > > + struct sg_table *sgt, size_t offs, > > + struct tee_shm *shm, > > + struct tee_shm **parent_shm) > > +{ > > + struct tee_protmem_static_pool *stp = to_protmem_static_pool(pool); > > + > > + shm->paddr = stp->pa_base + offs; > > + *parent_shm = NULL; > > + > > + return 0; > > +} > > + > > +static void protmem_pool_op_static_destroy_pool(struct tee_protmem_pool *pool) > > +{ > > + struct tee_protmem_static_pool *stp = to_protmem_static_pool(pool); > > + > > + gen_pool_destroy(stp->gen_pool); > > + memunmap(stp->base); > > + kfree(stp); > > +} > > + > > +static struct tee_protmem_pool_ops protmem_pool_ops_static = { > > + .alloc = protmem_pool_op_static_alloc, > > + .free = protmem_pool_op_static_free, > > + .update_shm = protmem_pool_op_static_update_shm, > > + .destroy_pool = protmem_pool_op_static_destroy_pool, > > +}; > > + > > +struct tee_protmem_pool *tee_protmem_static_pool_alloc(phys_addr_t paddr, > > + size_t size) > > +{ > > + const size_t page_mask = PAGE_SIZE - 1; > > + struct tee_protmem_static_pool *stp; > > + int rc; > > + > > + /* Check it's page aligned */ > > + if ((paddr | size) & page_mask) > > + return ERR_PTR(-EINVAL); > > + > > + stp = kzalloc(sizeof(*stp), GFP_KERNEL); > > + if (!stp) > > + return ERR_PTR(-ENOMEM); > > + > > I understand your reasoning for this, but considering that paddr comes from > the backend, isn’t it the backend’s responsibility to ensure that pfn_valid() > passes? For example, it might want to call devm_memremap_pages() itself. > So, should we simply ensure that the pages are valid , and fail on return, > rather than attempting to fix them? You have a point. I'll update accordingly. > > > + /* > > + * Map the memory as uncached to make sure the kernel can work with > > + * __pfn_to_page() and friends since that's needed when passing the > > + * protected DMA-buf to a device. The memory should otherwise not > > + * be touched by the kernel since it's likely to cause an external > > + * abort due to the protection status. > > + */ > > + stp->base = memremap(paddr, size, MEMREMAP_WC); > > + if (!stp->base) { > > + rc = -EINVAL; > > + goto err_free; > > + } > > + > > + stp->gen_pool = gen_pool_create(PAGE_SHIFT, -1); > > + if (!stp->gen_pool) { > > + rc = -ENOMEM; > > + goto err_unmap; > > + } > > + > > + rc = gen_pool_add(stp->gen_pool, paddr, size, -1); > > + if (rc) > > + goto err_free_pool; > > + > > + stp->pool.ops = &protmem_pool_ops_static; > > + stp->pa_base = paddr; > > + return &stp->pool; > > + > > +err_free_pool: > > + gen_pool_destroy(stp->gen_pool); > > +err_unmap: > > + memunmap(stp->base); > > +err_free: > > + kfree(stp); > > + > > + return ERR_PTR(rc); > > +} > > +EXPORT_SYMBOL_GPL(tee_protmem_static_pool_alloc); > > diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h > > index 9bc50605227c..6c6ff5d5eed2 100644 > > --- a/drivers/tee/tee_private.h > > +++ b/drivers/tee/tee_private.h > > @@ -8,6 +8,7 @@ > > #include <linux/cdev.h> > > #include <linux/completion.h> > > #include <linux/device.h> > > +#include <linux/dma-buf.h> > > #include <linux/kref.h> > > #include <linux/mutex.h> > > #include <linux/types.h> > > @@ -24,4 +25,9 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size); > > struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx, > > unsigned long addr, size_t length); > > > > +int tee_heap_update_from_dma_buf(struct tee_device *teedev, > > + struct dma_buf *dmabuf, size_t *offset, > > + struct tee_shm *shm, > > + struct tee_shm **parent_shm); > > + > > #endif /*TEE_PRIVATE_H*/ > > diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h > > index a38494d6b5f4..b8b99c97e00c 100644 > > --- a/include/linux/tee_core.h > > +++ b/include/linux/tee_core.h > > @@ -8,9 +8,11 @@ > > > > #include <linux/cdev.h> > > #include <linux/device.h> > > +#include <linux/dma-buf.h> > > #include <linux/idr.h> > > #include <linux/kref.h> > > #include <linux/list.h> > > +#include <linux/scatterlist.h> > > #include <linux/tee.h> > > #include <linux/tee_drv.h> > > #include <linux/types.h> > > @@ -30,6 +32,12 @@ > > #define TEE_DEVICE_FLAG_REGISTERED 0x1 > > #define TEE_MAX_DEV_NAME_LEN 32 > > > > +enum tee_dma_heap_id { > > + TEE_DMA_HEAP_SECURE_VIDEO_PLAY = 1, > > + TEE_DMA_HEAP_TRUSTED_UI, > > + TEE_DMA_HEAP_SECURE_VIDEO_RECORD, > > +}; > > + > > /** > > * struct tee_device - TEE Device representation > > * @name: name of device > > @@ -116,6 +124,36 @@ struct tee_desc { > > u32 flags; > > }; > > > > +/** > > + * struct tee_protmem_pool - protected memory pool > > + * @ops: operations > > + * > > + * This is an abstract interface where this struct is expected to be > > + * embedded in another struct specific to the implementation. > > + */ > > +struct tee_protmem_pool { > > + const struct tee_protmem_pool_ops *ops; > > +}; > > + > > +/** > > + * struct tee_protmem_pool_ops - protected memory pool operations > > + * @alloc: called when allocating protected memory > > + * @free: called when freeing protected memory > > + * @update_shm: called when registering a dma-buf to update the @shm > > + * with physical address of the buffer or to return the > > + * @parent_shm of the memory pool > > + * @destroy_pool: called when destroying the pool > > + */ > > +struct tee_protmem_pool_ops { > > + int (*alloc)(struct tee_protmem_pool *pool, struct sg_table *sgt, > > + size_t size, size_t *offs); > > + void (*free)(struct tee_protmem_pool *pool, struct sg_table *sgt); > > Why do we need update_shm()? Currently, it seems to do nothing beyond setting > parent_shm, simply indicating that this shm is part of a larger shm. > > What if the backend wants to handle the buffer using something other than > tee_shm - for example, if it doesn’t want to use tee_shm_alloc_dma_mem()? > Would removing update_shm() and replacing it with shm_release(), while also > getting rid of tee_heap_update_from_dma_buf(), be a more streamlined approach?" > > This way, tee_shm for dma_buf would be treated like any other tee_shm - using > refcounting based on tee_shm rather than its parent - and we would simply call > shm_release() upon release. > > With this change, we can even accept any dm_buf making the TEE_IOC_SHM_REGISTER_FD generic. The parent_shm is a must for the dynamic allocation of protected memory for the OP-TEE backend driver. Cheers, Jens > > Best Regards, > Amir > > > + int (*update_shm)(struct tee_protmem_pool *pool, struct sg_table *sgt, > > + size_t offs, struct tee_shm *shm, > > + struct tee_shm **parent_shm); > > + void (*destroy_pool)(struct tee_protmem_pool *pool); > > +}; > > + > > /** > > * tee_device_alloc() - Allocate a new struct tee_device instance > > * @teedesc: Descriptor for this driver > > @@ -154,6 +192,11 @@ int tee_device_register(struct tee_device *teedev); > > */ > > void tee_device_unregister(struct tee_device *teedev); > > > > +int tee_device_register_dma_heap(struct tee_device *teedev, > > + enum tee_dma_heap_id id, > > + struct tee_protmem_pool *pool); > > +void tee_device_unregister_all_dma_heaps(struct tee_device *teedev); > > + > > /** > > * tee_device_set_dev_groups() - Set device attribute groups > > * @teedev: Device to register > > @@ -229,6 +272,28 @@ static inline void tee_shm_pool_free(struct tee_shm_pool *pool) > > pool->ops->destroy_pool(pool); > > } > > > > +/** > > + * tee_protmem_static_pool_alloc() - Create a protected memory manager > > + * @paddr: Physical address of start of pool > > + * @size: Size in bytes of the pool > > + * > > + * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure. > > + */ > > +struct tee_protmem_pool *tee_protmem_static_pool_alloc(phys_addr_t paddr, > > + size_t size); > > + > > +/** > > + * tee_protmem_pool_free() - Free a protected memory pool > > + * @pool: The protected memory pool to free > > + * > > + * There must be no remaining protected memory allocated from this pool > > + * when this function is called. > > + */ > > +static inline void tee_protmem_pool_free(struct tee_protmem_pool *pool) > > +{ > > + pool->ops->destroy_pool(pool); > > +} > > + > > /** > > * tee_get_drvdata() - Return driver_data pointer > > * @returns the driver_data pointer supplied to tee_register(). >

3 months, 1 week

[PATCH 6.6 391/444] drm/gem: Internally test import_attach for imported objects

by Greg Kroah-Hartman

6.6-stable review patch. If anyone has any objections, please let me know. ------------------ From: Thomas Zimmermann <tzimmermann(a)suse.de> commit 8260731ccad0451207b45844bb66eb161a209218 upstream. Test struct drm_gem_object.import_attach to detect imported objects. During object clenanup, the dma_buf field might be NULL. Testing it in an object's free callback then incorrectly does a cleanup as for native objects. Happens for calls to drm_mode_destroy_dumb_ioctl() that clears the dma_buf field in drm_gem_object_exported_dma_buf_free(). v3: - only test for import_attach (Boris) v2: - use import_attach.dmabuf instead of dma_buf (Christian) Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with helper") Reported-by: Andy Yan <andyshrk(a)163.com> Closes: https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.Coremail.andysh… Tested-by: Andy Yan <andyshrk(a)163.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: David Airlie <airlied(a)gmail.com> Cc: Simona Vetter <simona(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Reviewed-by: Simona Vetter <simona.vetter(a)ffwll.ch> Link: https://lore.kernel.org/r/20250416065820.26076-1-tzimmermann@suse.de Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- include/drm/drm_gem.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -567,8 +567,7 @@ static inline bool drm_gem_object_is_sha */ static inline bool drm_gem_is_imported(const struct drm_gem_object *obj) { - /* The dma-buf's priv field points to the original GEM object. */ - return obj->dma_buf && (obj->dma_buf->priv != obj); + return !!obj->import_attach; } #ifdef CONFIG_LOCKDEP

3 months, 1 week

Re: [RFC PATCH 00/30] Host side (KVM/VFIO/IOMMUFD) support for TDISP using TSM

by Jason Gunthorpe

On Thu, May 29, 2025 at 01:34:43PM +0800, Xu Yilun wrote: > This series has 3 sections: I really think this is too big to try to progress, even in RFC form. > Patch 1 - 11 deal with the private MMIO mapping in KVM MMU via DMABUF. > Leverage Jason & Vivek's latest VFIO dmabuf series [3], see Patch 2 - 4. > The concern for get_pfn() kAPI [4] is not addressed so are marked as > HACK, will investigate later. I would probably split this out entirely into its own topic. It doesn't seem directly related to TSM as KVM can use DMABUF for good reasons independently . > Patch 12 - 22 is about TSM Bind/Unbind/Guest request management in VFIO > & IOMMUFD. Picks some of Shameer's patch in [5], see Patch 12 & 14. This is some reasonable topic on its own after Dan's series > Patch 23 - 30 is a solution to meet the TDX specific sequence > enforcement on various device Unbind cases, including converting device > back to shared, hot unplug, TD destroy. Start with a tdx_tsm driver > prototype and finally implement the Unbind enforcement inside the > driver. To be honest it is still awkward to me, but I need help. Then you have a series or two to implement TDX using the infrastructure. Jason

3 months, 1 week

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig