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

[Dmitry Osipenko]

16.01.2023 18:11, Christian König пишет:
> 
>>>>>
>>>>>> 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.
>>
>> Can you point me at where this check is? I see a check for
>> obj->import_attach in drm_gem_dumb_map_offset. But I can't see how
>> this function is called in amdgpu. I don't think it is used at all.
> 
> Uff, good question! @Thomas and @Dmitry: I clearly remember that one of
> you guys was involved in the DRM/GEM mmap cleanup and DMA-buf with
> workarounds for the KFD and DMA-buf.
> 
> What was the final solution to this? I can't find it of hand any more.

I was looking at it. The AMDGPU indeed allows to map imported GEMs, but
then touching the mapped area by CPU results in a bus fault. You,
Christian, suggested that this an AMDGPU bug that should be fixed by
prohibiting the mapping in the first place and I was going to fix it,
but then the plan changed from prohibiting the mapping into fixing it.

The first proposal was to make DRM core to handle the dma-buf mappings
for all drivers universally [1]. Then we decided that will be better to
prohibit mapping of imported GEMs [2]. In the end, Rob Clark argued that
better to implement the [1], otherwise current userspace (Android) will
be broken if mapping will be prohibited.

The last question was about the cache syncing of imported dma-bufs, how
to ensure that drivers will do the cache maintenance/syncing properly.
Rob suggested that it should be a problem for drivers and not for DRM core.

I was going to re-send the [1], but other things were getting priority.
It's good that you reminded me about it :) I may re-send it sometime
soon if there are no new objections.

[1] https://patchwork.freedesktop.org/patch/487481/

[2]
https://lore.kernel.org/all/20220701090240.1896131-1-dmitry.osipenko@collabora.com/