[Intel-gfx] [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep

[Christian König]

Am 19.11.19 um 22:08 schrieb Daniel Vetter:
> Semnatically it really doesn't matter where we grab the ticket. But
> since the ticket is a fake lockdep lock, it matters for lockdep
> validation purposes.
>
> This means stuff like grabbing a ticket and then doing
> copy_from/to_user isn't allowed anymore. This is a changed compared to
> the current ttm fault handler, which doesn't bother with having a full
> reservation. Since I'm looking into fixing the TODO entry in
> ttm_mem_evict_wait_busy() I think that'll have to change sooner or
> later anyway, better get started. A bit more context on why I'm
> looking into this: For backwards compat with existing i915 gem code I
> think we'll have to do full slowpath locking in the i915 equivalent of
> the eviction code. And with dynamic dma-buf that will leak across
> drivers, so another thing we need to standardize and make sure it's
> done the same way everyway.
>
> Unfortunately this means another full audit of all drivers:
>
> - gem helpers: acquire_init is done right before taking locks, so no
>    problem. Same for acquire_fini and unlocking, which means nothing
>    that's not already covered by the dma_resv_lock rules will be caught
>    with this extension here to the acquire_ctx.
>
> - etnaviv: An absolute massive amount of code is run between the
>    acquire_init and the first lock acquisition in submit_lock_objects.
>    But nothing that would touch user memory and could cause a fault.
>    Furthermore nothing that uses the ticket, so even if I missed
>    something, it would be easy to fix by pushing the acquire_init right
>    before the first use. Similar on the unlock/acquire_fini side.
>
> - i915: Right now (and this will likely change a lot rsn) the acquire
>    ctx and actual locks are right next to each another. No problem.
>
> - msm has a problem: submit_create calls acquire_init, but then
>    submit_lookup_objects() has a bunch of copy_from_user to do the
>    object lookups. That's the only thing before submit_lock_objects
>    call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv
>    does not have this issue since it copies all the userspace structs
>    earlier. submit_cleanup does not have any such issues.
>
>    With the prep patch to pull out the acquire_ctx and reorder it msm
>    is going to be safe too.
>
> - nouveau: acquire_init is right next to ttm_bo_reserve, so all good.
>    Similar on the acquire_fini/ttm_bo_unreserve side.
>
> - ttm execbuf utils: acquire context and locking are even in the same
>    functions here (one function to reserve everything, the other to
>    unreserve), so all good.
>
> - vc4: Another case where acquire context and locking are handled in
>    the same functions (one function to lock everything, the other to
>    unlock).
>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Sumit Semwal <sumit.semwal at linaro.org>
> Cc: linux-media at vger.kernel.org
> Cc: linaro-mm-sig at lists.linaro.org
> Cc: Huang Rui <ray.huang at amd.com>
> Cc: Eric Anholt <eric at anholt.net>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Rob Herring <robh at kernel.org>
> Cc: Lucas Stach <l.stach at pengutronix.de>
> Cc: Russell King <linux+etnaviv at armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner at gmail.com>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Sean Paul <sean at poorly.run>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

Acked-by: Christian König <christian.koenig at amd.com>

> ---
>   drivers/dma-buf/dma-resv.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index d3c760e19991..079e38fde33a 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list)
>   static void __init dma_resv_lockdep(void)
>   {
>   	struct mm_struct *mm = mm_alloc();
> +	struct ww_acquire_ctx ctx;
>   	struct dma_resv obj;
> +	int ret;
>   
>   	if (!mm)
>   		return;
> @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void)
>   	dma_resv_init(&obj);
>   
>   	down_read(&mm->mmap_sem);
> -	ww_mutex_lock(&obj.lock, NULL);
> +	ww_acquire_init(&ctx, &reservation_ww_class);
> +	ret = dma_resv_lock(&obj, &ctx);
> +	if (ret == -EDEADLK)
> +		dma_resv_lock_slow(&obj, &ctx);
>   	fs_reclaim_acquire(GFP_KERNEL);
>   	fs_reclaim_release(GFP_KERNEL);
>   	ww_mutex_unlock(&obj.lock);
> +	ww_acquire_fini(&ctx);
>   	up_read(&mm->mmap_sem);
>   	
>   	mmput(mm);