[Intel-gfx] [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify

[Koenig, Christian]

Am 05.11.19 um 14:50 schrieb Daniel Vetter:
> On Tue, Nov 5, 2019 at 2:39 PM Christian König
> <ckoenig.leichtzumerken at gmail.com> wrote:
>> Am 05.11.19 um 11:52 schrieb Daniel Vetter:
>>> On Tue, Oct 29, 2019 at 11:40:49AM +0100, Christian König wrote:
>>>> Implement the importer side of unpinned DMA-buf handling.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 28 ++++++++++++++++++++-
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  6 +++++
>>>>    2 files changed, 33 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>> index 3629cfe53aad..af39553c51ad 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>> @@ -456,7 +456,33 @@ amdgpu_dma_buf_create_obj(struct drm_device *dev, struct dma_buf *dma_buf)
>>>>       return ERR_PTR(ret);
>>>>    }
>>>>
>>>> +/**
>>>> + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation
>>>> + *
>>>> + * @attach: the DMA-buf attachment
>>>> + *
>>>> + * Invalidate the DMA-buf attachment, making sure that the we re-create the
>>>> + * mapping before the next use.
>>>> + */
>>>> +static void
>>>> +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
>>>> +{
>>>> +    struct ttm_operation_ctx ctx = { false, false };
>>>> +    struct drm_gem_object *obj = attach->importer_priv;
>>>> +    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>>> +    struct ttm_placement placement = {};
>>>> +    int r;
>>>> +
>>>> +    if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM)
>>>> +            return;
>>>> +
>>>> +    r = ttm_bo_validate(&bo->tbo, &placement, &ctx);
>>>> +    if (r)
>>>> +            DRM_ERROR("Failed to invalidate DMA-buf import (%d))\n", r);
>>> Where do you update pagetables?
>>>
>>> The only thing I've found is in the amdgpu CS code, which is way to late
>>> for this stuff here. Plus TTM doesn't handle virtual memory at all (aside
>>> from the gart tt), so clearly you need to call into amdgpu code somewhere
>>> for this. But I didn't find it, neither in your ->move_notify nor the
>>> ->move callback in ttm_bo_driver.
>>>
>>> How does this work?
>> Page tables are not updated until the next command submission, e.g. in
>> amdgpu_cs.c
>>
>> This is save since all previous command submissions are added to the
>> dma_resv object as fences and the dma_buf can't be moved before those
>> are signaled.
> Hm, I thought you still allow explicit buffer lists for each cs in
> amdgpu? Code looks at least like that, not everything goes through the
> context working set stuff.
>
> How do you prevent the security leak if userspace simply lies about
> not using a given buffer in a batch, and then abusing that to read
> that virtual address range anyway and peek at whatever is now going to
> be there when an eviction happened?

Oh, yeah that is a really good point. And no that isn't handled 
correctly at all.

I wanted to rework that for quite some time now, but always got into 
issues with TTM.

Thanks for the notice, so I need to put my TTM rework before of this. 
Crap, that adds a whole bunch of TODOs to my list.

Regards,
Christian.

> -Daniel
>
>> Christian.
>>
>>> -Daniel
>>>
>>>> +}
>>>> +
>>>>    static const struct dma_buf_attach_ops amdgpu_dma_buf_attach_ops = {
>>>> +    .move_notify = amdgpu_dma_buf_move_notify
>>>>    };
>>>>
>>>>    /**
>>>> @@ -492,7 +518,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
>>>>               return obj;
>>>>
>>>>       attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
>>>> -                                    &amdgpu_dma_buf_attach_ops, NULL);
>>>> +                                    &amdgpu_dma_buf_attach_ops, obj);
>>>>       if (IS_ERR(attach)) {
>>>>               drm_gem_object_put(obj);
>>>>               return ERR_CAST(attach);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> index ac776d2620eb..cfa46341c9a7 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> @@ -861,6 +861,9 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>>>>               return 0;
>>>>       }
>>>>
>>>> +    if (bo->tbo.base.import_attach)
>>>> +            dma_buf_pin(bo->tbo.base.import_attach);
>>>> +
>>>>       bo->flags |= AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS;
>>>>       /* force to pin into visible video ram */
>>>>       if (!(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
>>>> @@ -944,6 +947,9 @@ int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>>>>
>>>>       amdgpu_bo_subtract_pin_size(bo);
>>>>
>>>> +    if (bo->tbo.base.import_attach)
>>>> +            dma_buf_unpin(bo->tbo.base.import_attach);
>>>> +
>>>>       for (i = 0; i < bo->placement.num_placement; i++) {
>>>>               bo->placements[i].lpfn = 0;
>>>>               bo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
>>>> --
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>