| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4ffda342ee97dff806cdf66766151da910c743b3.camel@linux.intel.com>
Date: Tue, 31 Oct 2023 17:52:05 +0100
From: Thomas Hellström
<thomas.hellstrom@...ux.intel.com>
To: Danilo Krummrich <dakr@...hat.com>, airlied@...il.com,
daniel@...ll.ch, matthew.brost@...el.com, sarah.walker@...tec.com,
donald.robson@...tec.com, boris.brezillon@...labora.com,
christian.koenig@....com, faith@...strand.net
Cc: dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH drm-misc-next v7 5/7] drm/gpuvm: track/lock/validate
external/evicted objects
On Tue, 2023-10-31 at 17:41 +0100, Danilo Krummrich wrote:
> On 10/31/23 12:34, Thomas Hellström wrote:
> > On Mon, 2023-10-23 at 22:16 +0200, Danilo Krummrich wrote:
> > > Currently the DRM GPUVM offers common infrastructure to track GPU
> > > VA
> > > allocations and mappings, generically connect GPU VA mappings to
> > > their
> > > backing buffers and perform more complex mapping operations on
> > > the
> > > GPU VA
> > > space.
> > >
> > > However, there are more design patterns commonly used by drivers,
> > > which
> > > can potentially be generalized in order to make the DRM GPUVM
> > > represent
> > > a basis for GPU-VM implementations. In this context, this patch
> > > aims
> > > at generalizing the following elements.
> > >
> > > 1) Provide a common dma-resv for GEM objects not being used
> > > outside
> > > of
> > > this GPU-VM.
> > >
> > > 2) Provide tracking of external GEM objects (GEM objects which
> > > are
> > > shared with other GPU-VMs).
> > >
> > > 3) Provide functions to efficiently lock all GEM objects dma-resv
> > > the
> > > GPU-VM contains mappings of.
> > >
> > > 4) Provide tracking of evicted GEM objects the GPU-VM contains
> > > mappings
> > > of, such that validation of evicted GEM objects is
> > > accelerated.
> > >
> > > 5) Provide some convinience functions for common patterns.
> > >
> > > Big thanks to Boris Brezillon for his help to figure out locking
> > > for
> > > drivers updating the GPU VA space within the fence signalling
> > > path.
> > >
> > > Suggested-by: Matthew Brost <matthew.brost@...el.com>
> > > Signed-off-by: Danilo Krummrich <dakr@...hat.com>
> >
> > The checkpatch.pl warning still persists:
> > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> > #627: FILE: drivers/gpu/drm/drm_gpuvm.c:1347:
> > + return -ENOTSUPP;
>
> Hm, I thought I changed this one. Seems like it slipped through.
> Gonna
> fix that.
>
> >
> > > ---
> > > drivers/gpu/drm/drm_gpuvm.c | 633
> > > ++++++++++++++++++++++++++++++++++++
> > > include/drm/drm_gpuvm.h | 250 ++++++++++++++
> > > 2 files changed, 883 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gpuvm.c
> > > b/drivers/gpu/drm/drm_gpuvm.c
> > > index 7f4f5919f84c..01cbeb98755a 100644
> > > --- a/drivers/gpu/drm/drm_gpuvm.c
> > > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > > @@ -82,6 +82,21 @@
> > > * &drm_gem_object list of &drm_gpuvm_bos for an existing
> > > instance
> > > of this
> > > * particular combination. If not existent a new instance is
> > > created
> > > and linked
> > > * to the &drm_gem_object.
> > > + *
> > > + * &drm_gpuvm_bo structures, since unique for a given
> > > &drm_gpuvm,
> > > are also used
> > > + * as entry for the &drm_gpuvm's lists of external and evicted
> > > objects. Those
> > > + * lists are maintained in order to accelerate locking of dma-
> > > resv
> > > locks and
> > > + * validation of evicted objects bound in a &drm_gpuvm. For
> > > instance, all
> > > + * &drm_gem_object's &dma_resv of a given &drm_gpuvm can be
> > > locked
> > > by calling
> > > + * drm_gpuvm_exec_lock(). Once locked drivers can call
> > > drm_gpuvm_validate() in
> > > + * order to validate all evicted &drm_gem_objects. It is also
> > > possible to lock
> > > + * additional &drm_gem_objects by providing the corresponding
> > > parameters to
> > > + * drm_gpuvm_exec_lock() as well as open code the &drm_exec loop
> > > while making
> > > + * use of helper functions such as drm_gpuvm_prepare_range() or
> > > + * drm_gpuvm_prepare_objects().
> > > + *
> > > + * Every bound &drm_gem_object is treated as external object
> > > when
> > > its &dma_resv
> > > + * structure is different than the &drm_gpuvm's common &dma_resv
> > > structure.
> > > */
> > >
> > > /**
> > > @@ -429,6 +444,20 @@
> > > * Subsequent calls to drm_gpuvm_bo_obtain() for the same
> > > &drm_gpuvm
> > > and
> > > * &drm_gem_object must be able to observe previous creations
> > > and
> > > destructions
> > > * of &drm_gpuvm_bos in order to keep instances unique.
> > > + *
> > > + * The &drm_gpuvm's lists for keeping track of external and
> > > evicted
> > > objects are
> > > + * protected against concurrent insertion / removal and
> > > iteration
> > > internally.
> > > + *
> > > + * However, drivers still need ensure to protect concurrent
> > > calls to
> > > functions
> > > + * iterating those lists, namely drm_gpuvm_prepare_objects() and
> > > + * drm_gpuvm_validate().
> > > + *
> > > + * Alternatively, drivers can set the &DRM_GPUVM_RESV_PROTECTED
> > > flag
> > > to indicate
> > > + * that the corresponding &dma_resv locks are held in order to
> > > protect the
> > > + * lists. If &DRM_GPUVM_RESV_PROTECTED is set, internal locking
> > > is
> > > disabled and
> > > + * the corresponding lockdep checks are enabled. This is an
> > > optimization for
> > > + * drivers which are capable of taking the corresponding
> > > &dma_resv
> > > locks and
> > > + * hence do not require internal locking.
> > > */
> > >
> > > /**
> > > @@ -641,6 +670,201 @@
> > > * }
> > > */
> > >
> > > +/**
> > > + * get_next_vm_bo_from_list() - get the next vm_bo element
> > > + * @__gpuvm: the &drm_gpuvm
> > > + * @__list_name: the name of the list we're iterating on
> > > + * @__local_list: a pointer to the local list used to store
> > > already
> > > iterated items
> > > + * @__prev_vm_bo: the previous element we got from
> > > get_next_vm_bo_from_list()
> > > + *
> > > + * This helper is here to provide lockless list iteration.
> > > Lockless
> > > as in, the
> > > + * iterator releases the lock immediately after picking the
> > > first
> > > element from
> > > + * the list, so list insertion deletion can happen concurrently.
> > > + *
> > > + * Elements popped from the original list are kept in a local
> > > list,
> > > so removal
> > > + * and is_empty checks can still happen while we're iterating
> > > the
> > > list.
> > > + */
> > > +#define get_next_vm_bo_from_list(__gpuvm, __list_name,
> > > __local_list,
> > > __prev_vm_bo) \
> > > + ({
> > > \
> > > + struct drm_gpuvm_bo *__vm_bo =
> > > NULL; \
> > > +
> > > \
> > > + drm_gpuvm_bo_put(__prev_vm_bo);
> > > \
> > > +
> > > \
> > > + spin_lock(&(__gpuvm)-
> > > > __list_name.lock); \
> > > + if (!(__gpuvm)-
> > > > __list_name.local_list) \
> > > + (__gpuvm)->__list_name.local_list =
> > > __local_list; \
> > > + else
> > > \
> > > + drm_WARN_ON((__gpuvm)-
> > > > drm, \
> > > + (__gpuvm)-
> > > >__list_name.local_list
> > > != __local_list); \
> > > +
> > > \
> > > + while (!list_empty(&(__gpuvm)->__list_name.list))
> > > { \
> > > + __vm_bo = list_first_entry(&(__gpuvm)-
> > > > __list_name.list, \
> > > + struct
> > > drm_gpuvm_bo, \
> > > +
> > > list.entry.__list_name); \
> > > + if (kref_get_unless_zero(&__vm_bo->kref))
> > > { \
> > > + list_move_tail(&(__vm_bo)-
> > > > list.entry.__list_name, \
> > > +
> > > __local_list); \
> > > + break;
> > > \
> > > + } else
> > > { \
> > > + list_del_init(&(__vm_bo)-
> > > > list.entry.__list_name); \
> > > + __vm_bo =
> > > NULL; \
> > > + }
> > > \
> > > + }
> > > \
> > > + spin_unlock(&(__gpuvm)-
> > > > __list_name.lock); \
> > > +
> > > \
> > > + __vm_bo;
> > > \
> > > + })
> > > +
> > > +/**
> > > + * for_each_vm_bo_in_list() - internal vm_bo list iterator
> > > + * @__gpuvm: the &drm_gpuvm
> > > + * @__list_name: the name of the list we're iterating on
> > > + * @__local_list: a pointer to the local list used to store
> > > already
> > > iterated items
> > > + * @__vm_bo: the struct drm_gpuvm_bo to assign in each iteration
> > > step
> > > + *
> > > + * This helper is here to provide lockless list iteration.
> > > Lockless
> > > as in, the
> > > + * iterator releases the lock immediately after picking the
> > > first
> > > element from the
> > > + * list, hence list insertion and deletion can happen
> > > concurrently.
> > > + *
> > > + * It is not allowed to re-assign the vm_bo pointer from inside
> > > this
> > > loop.
> > > + *
> > > + * Typical use:
> > > + *
> > > + * struct drm_gpuvm_bo *vm_bo;
> > > + * LIST_HEAD(my_local_list);
> > > + *
> > > + * ret = 0;
> > > + * for_each_vm_bo_in_list(gpuvm, <list_name>,
> > > &my_local_list,
> > > vm_bo) {
> > > + * ret = do_something_with_vm_bo(..., vm_bo);
> > > + * if (ret)
> > > + * break;
> > > + * }
> > > + * // Drop ref in case we break out of the loop.
> > > + * drm_gpuvm_bo_put(vm_bo);
> > > + * restore_vm_bo_list(gpuvm, <list_name>, &my_local_list);
> > > + *
> > > + *
> > > + * Only used for internal list iterations, not meant to be
> > > exposed
> > > to the outside
> > > + * world.
> > > + */
> > > +#define for_each_vm_bo_in_list(__gpuvm, __list_name,
> > > __local_list,
> > > __vm_bo) \
> > > + for (__vm_bo = get_next_vm_bo_from_list(__gpuvm,
> > > __list_name, \
> > > + __local_list,
> > > NULL); \
> > > +
> > > __vm_bo;
> > > \
> > > + __vm_bo = get_next_vm_bo_from_list(__gpuvm,
> > > __list_name, \
> > > + __local_list,
> > > __vm_bo))
> > > +
> > > +static void
> > > +__restore_vm_bo_list(struct drm_gpuvm *gpuvm, spinlock_t *lock,
> > > + struct list_head *list, struct list_head
> > > **local_list)
> > > +{
> > > + /* Merge back the two lists, moving local list elements
> > > to
> > > the
> > > + * head to preserve previous ordering, in case it
> > > matters.
> > > + */
> > > + spin_lock(lock);
> > > + if (*local_list) {
> > > + list_splice(*local_list, list);
> > > + *local_list = NULL;
> > > + }
> > > + spin_unlock(lock);
> > > +}
> > > +
> > > +/**
> > > + * restore_vm_bo_list() - move vm_bo elements back to their
> > > original
> > > list
> > > + * @__gpuvm: the &drm_gpuvm
> > > + * @__list_name: the name of the list we're iterating on
> > > + *
> > > + * When we're done iterating a vm_bo list, we should call
> > > restore_vm_bo_list()
> > > + * to restore the original state and let new iterations take
> > > place.
> > > + */
> > > +#define restore_vm_bo_list(__gpuvm,
> > > __list_name) \
> > > + __restore_vm_bo_list((__gpuvm), &(__gpuvm)-
> > > > __list_name.lock, \
> > > + &(__gpuvm)-
> > > > __list_name.list, \
> > > + &(__gpuvm)->__list_name.local_list)
> > > +
> > > +static void
> > > +cond_spin_lock(spinlock_t *lock, bool cond)
> > > +{
> > > + if (cond)
> > > + spin_lock(lock);
> > > +}
> > > +
> > > +static void
> > > +cond_spin_unlock(spinlock_t *lock, bool cond)
> > > +{
> > > + if (cond)
> > > + spin_unlock(lock);
> > > +}
> > > +
> > > +static void
> > > +__drm_gpuvm_bo_list_add(struct drm_gpuvm *gpuvm, spinlock_t
> > > *lock,
> > > + struct list_head *entry, struct list_head
> > > *list)
> > > +{
> > > + cond_spin_lock(lock, !!lock);
> > > + if (list_empty(entry))
> > > + list_add_tail(entry, list);
> > > + cond_spin_unlock(lock, !!lock);
> > > +}
> > > +
> > > +/**
> > > + * drm_gpuvm_bo_list_add() - insert a vm_bo into the given list
> > > + * @__vm_bo: the &drm_gpuvm_bo
> > > + * @__list_name: the name of the list to insert into
> > > + * @__lock: whether to lock with the internal spinlock
> > > + *
> > > + * Inserts the given @__vm_bo into the list specified by
> > > @__list_name.
> > > + */
> > > +#define drm_gpuvm_bo_list_add(__vm_bo, __list_name,
> > > __lock) \
> > > + __drm_gpuvm_bo_list_add((__vm_bo)-
> > > > vm, \
> > > + __lock ? &(__vm_bo)->vm-
> > > > __list_name.lock : \
> > > +
> > > NULL, \
> > > + &(__vm_bo)-
> > > > list.entry.__list_name, \
> > > + &(__vm_bo)->vm->__list_name.list)
> > > +
> > > +static void
> > > +__drm_gpuvm_bo_list_del(struct drm_gpuvm *gpuvm, spinlock_t
> > > *lock,
> > > + struct list_head *entry, bool init)
> > > +{
> > > + cond_spin_lock(lock, !!lock);
> > > + if (init) {
> > > + if (!list_empty(entry))
> > > + list_del_init(entry);
> > > + } else {
> > > + list_del(entry);
> > > + }
> > > + cond_spin_unlock(lock, !!lock);
> > > +}
> > > +
> > > +/**
> > > + * drm_gpuvm_bo_list_del_init() - remove a vm_bo from the given
> > > list
> > > + * @__vm_bo: the &drm_gpuvm_bo
> > > + * @__list_name: the name of the list to insert into
> > > + * @__lock: whether to lock with the internal spinlock
> > > + *
> > > + * Removes the given @__vm_bo from the list specified by
> > > @__list_name.
> > > + */
> > > +#define drm_gpuvm_bo_list_del_init(__vm_bo, __list_name,
> > > __lock) \
> > > + __drm_gpuvm_bo_list_del((__vm_bo)-
> > > > vm, \
> > > + __lock ? &(__vm_bo)->vm-
> > > > __list_name.lock : \
> > > +
> > > NULL, \
> > > + &(__vm_bo)-
> > > > list.entry.__list_name, \
> > > + true)
> > > +
> > > +/**
> > > + * drm_gpuvm_bo_list_del() - remove a vm_bo from the given list
> > > + * @__vm_bo: the &drm_gpuvm_bo
> > > + * @__list_name: the name of the list to insert into
> > > + * @__lock: whether to lock with the internal spinlock
> > > + *
> > > + * Removes the given @__vm_bo from the list specified by
> > > @__list_name.
> > > + */
> > > +#define drm_gpuvm_bo_list_del(__vm_bo, __list_name,
> > > __lock) \
> > > + __drm_gpuvm_bo_list_del((__vm_bo)-
> > > > vm, \
> > > + __lock ? &(__vm_bo)->vm-
> > > > __list_name.lock : \
> > > +
> > > NULL, \
> > > + &(__vm_bo)-
> > > > list.entry.__list_name, \
> > > + false)
> > > +
> > > #define to_drm_gpuva(__node) container_of((__node), struct
> > > drm_gpuva, rb.node)
> > >
> > > #define GPUVA_START(node) ((node)->va.addr)
> > > @@ -763,6 +987,12 @@ drm_gpuvm_init(struct drm_gpuvm *gpuvm,
> > > const
> > > char *name,
> > > gpuvm->rb.tree = RB_ROOT_CACHED;
> > > INIT_LIST_HEAD(&gpuvm->rb.list);
> > >
> > > + INIT_LIST_HEAD(&gpuvm->extobj.list);
> > > + spin_lock_init(&gpuvm->extobj.lock);
> > > +
> > > + INIT_LIST_HEAD(&gpuvm->evict.list);
> > > + spin_lock_init(&gpuvm->evict.lock);
> > > +
> > > gpuvm->name = name ? name : "unknown";
> > > gpuvm->flags = flags;
> > > gpuvm->ops = ops;
> > > @@ -805,10 +1035,352 @@ drm_gpuvm_destroy(struct drm_gpuvm
> > > *gpuvm)
> > > drm_WARN(gpuvm->drm, !RB_EMPTY_ROOT(&gpuvm-
> > > >rb.tree.rb_root),
> > > "GPUVA tree is not empty, potentially leaking
> > > memory.\n");
> > >
> > > + drm_WARN(gpuvm->drm, !list_empty(&gpuvm->extobj.list),
> > > + "Extobj list should be empty.\n");
> > > + drm_WARN(gpuvm->drm, !list_empty(&gpuvm->evict.list),
> > > + "Evict list should be empty.\n");
> > > +
> > > drm_gem_object_put(gpuvm->r_obj);
> > > }
> > > EXPORT_SYMBOL_GPL(drm_gpuvm_destroy);
> > >
> > > +static int
> > > +__drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
> > > + struct drm_exec *exec,
> > > + unsigned int num_fences)
> > > +{
> > > + struct drm_gpuvm_bo *vm_bo;
> > > + LIST_HEAD(extobjs);
> > > + int ret = 0;
> > > +
> > > + for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) {
> > > + ret = drm_exec_prepare_obj(exec, vm_bo->obj,
> > > num_fences);
> > > + if (ret)
> > > + break;
> > > + }
> > > + /* Drop ref in case we break out of the loop. */
> > > + drm_gpuvm_bo_put(vm_bo);
> > > + restore_vm_bo_list(gpuvm, extobj);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int
> > > +drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm,
> > > + struct drm_exec *exec,
> > > + unsigned int num_fences)
> > > +{
> > > + struct drm_gpuvm_bo *vm_bo;
> > > + int ret = 0;
> > > +
> > > + drm_gpuvm_resv_assert_held(gpuvm);
> > > + list_for_each_entry(vm_bo, &gpuvm->extobj.list,
> > > list.entry.extobj) {
> > > + ret = drm_exec_prepare_obj(exec, vm_bo->obj,
> > > num_fences);
> > > + if (ret)
> > > + break;
> > > +
> > > + if (vm_bo->evicted)
> > > + drm_gpuvm_bo_list_add(vm_bo, evict,
> > > false);
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * drm_gpuvm_prepare_objects() - prepare all assoiciated BOs
> > > + * @gpuvm: the &drm_gpuvm
> > > + * @exec: the &drm_exec locking context
> > > + * @num_fences: the amount of &dma_fences to reserve
> > > + *
> > > + * Calls drm_exec_prepare_obj() for all &drm_gem_objects the
> > > given
> > > + * &drm_gpuvm contains mappings of.
> > > + *
> > > + * Using this function directly, it is the drivers
> > > responsibility to
> > > call
> > > + * drm_exec_init() and drm_exec_fini() accordingly.
> > > + *
> > > + * Note: This function is safe against concurrent insertion and
> > > removal of
> > > + * external objects, however it is not safe against concurrent
> > > usage
> > > itself.
> > > + *
> > > + * Drivers need to make sure to protect this case with either an
> > > outer VM lock
> > > + * or by calling drm_gpuvm_prepare_vm() before this function
> > > within
> > > the
> > > + * drm_exec_until_all_locked() loop, such that the GPUVM's dma-
> > > resv
> > > lock ensures
> > > + * mutual exclusion.
> > > + *
> > > + * Returns: 0 on success, negative error code on failure.
> >
> > s/Returns:/Return:/g
> >
> > Otherwise LGTM.
>
> Sounds like you want to offer your RB? :)
Yes, with the above return value fix,
Reviewed-by: Thomas Hellström <thomas.hellstrom@...ux.intel.com>
Thanks,
Thomas
>
> >
> > /Thomas
> >
> >
>
Powered by blists - more mailing lists