Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

["Christian König" <christian.koenig@amd.com>]

[SNIP]
>>> When the BO is imported into the same GPU, you get a reference to 
>>> the same BO, so the imported BO has the same mmap_offset as the 
>>> original BO.
>>>
>>> When the BO is imported into a different GPU, it is a new BO with a 
>>> new mmap_offset.
>>
>> That won't work.
>>
>>> I don't think this is incorrect.
>>
>> No, this is completely incorrect. It mixes up the reverse tracking of 
>> mappings and might crash the system.
>
> I don't understand that. The imported BO is a different BO with a 
> different mmap offset in a different device file. I don't see how that 
> messes with the tracking of mappings.

The tracking keeps note which piece of information is accessible through 
which address space object and offset. I you suddenly have two address 
spaces and offsets pointing to the same piece of information that won't 
work any more.

>
>> This is the reason why we can't mmap() imported BOs.
>
> I don't see anything preventing that. For userptr BOs, there is this 
> code in amdgpu_gem_object_mmap:
>
>         if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>                 return -EPERM;
>
> I don't see anything like this preventing mmapping of imported dmabuf 
> BOs. What am I missing?
>

At some point I really need to make a big presentation about all this 
stuff, we had the same discussion multiple times now :)

It's the same reason why you can't mmap() VRAM through the kfd node: 
Each file can have only one address space object associated with it.

See dma_buf_mmap() and vma_set_file() how this is worked around in DMA-buf.

>>
>>> mmapping the memory with that new offset should still work. The 
>>> imported BO is created with ttm_bo_type_sg, and AFAICT ttm_bo_vm.c 
>>> supports mapping of SG BOs.
>>
>> Actually it shouldn't. This can go boom really easily.
>
> OK. I don't think we're doing this, but after Xiaogang raised the 
> question I went looking through the code whether it's theoretically 
> possible. I didn't find anything in the code that says that mmapping 
> imported dmabufs would be prohibited or even dangerous. On the 
> contrary, I found that ttm_bo_vm explicitly supports mmapping SG BOs.
>
>
>>
>> When you have imported a BO the only correct way of to mmap() it is 
>> to do so on the original exporter.
>
> That seems sensible, and this is what we do today. That said, if 
> mmapping an imported BO is dangerous, I'm missing a mechanism to 
> protect against this. It could be as simple as setting 
> AMDGPU_GEM_CREATE_NO_CPU_ACCESS in amdgpu_dma_buf_create_obj.

At least for the GEM mmap() handler this is double checked very early by 
looking at obj->import_attach and then either rejecting it or 
redirecting the request to the DMA-buf file instead.

We probably need something similar when stuff is mapped through the KFD 
node. But I think we don't do that any more for "normal" BOs anyway, 
don't we?

Regards,
Christian.

>
> Regards,
>   Felix