Re: [RFC PATCH 01/12] dma-buf: Introduce dma_buf_get_pfn_unlocked() kAPI

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

[-- Attachment #1: Type: text/plain, Size: 6387 bytes --]

Am 08.01.25 um 20:22 schrieb Xu Yilun:
> On Wed, Jan 08, 2025 at 07:44:54PM +0100, Simona Vetter wrote:
>> On Wed, Jan 08, 2025 at 12:22:27PM -0400, Jason Gunthorpe wrote:
>>> On Wed, Jan 08, 2025 at 04:25:54PM +0100, Christian König wrote:
>>>> Am 08.01.25 um 15:58 schrieb Jason Gunthorpe:
>>>>> I have imagined a staged approach were DMABUF gets a new API that
>>>>> works with the new DMA API to do importer mapping with "P2P source
>>>>> information" and a gradual conversion.
>>>> To make it clear as maintainer of that subsystem I would reject such a step
>>>> with all I have.
>>> This is unexpected, so you want to just leave dmabuf broken? Do you
>>> have any plan to fix it, to fix the misuse of the DMA API, and all
>>> the problems I listed below? This is a big deal, it is causing real
>>> problems today.
>>>
>>> If it going to be like this I think we will stop trying to use dmabuf
>>> and do something simpler for vfio/kvm/iommufd :(
>> As the gal who help edit the og dma-buf spec 13 years ago, I think adding
>> pfn isn't a terrible idea. By design, dma-buf is the "everything is
>> optional" interface. And in the beginning, even consistent locking was
>> optional, but we've managed to fix that by now :-/

Well you were also the person who mangled the struct page pointers in 
the scatterlist because people were abusing this and getting a bloody 
nose :)

>> Where I do agree with Christian is that stuffing pfn support into the
>> dma_buf_attachment interfaces feels a bit much wrong.
> So it could a dmabuf interface like mmap/vmap()? I was also wondering
> about that. But finally I start to use dma_buf_attachment interface
> because of leveraging existing buffer pin and move_notify.

Exactly that's the point, sharing pfn doesn't work with the pin and 
move_notify interfaces because of the MMU notifier approach Sima mentioned.

>>>> We have already gone down that road and it didn't worked at all and
>>>> was a really big pain to pull people back from it.
>>> Nobody has really seriously tried to improve the DMA API before, so I
>>> don't think this is true at all.
>> Aside, I really hope this finally happens!

Sorry my fault. I was not talking about the DMA API, but rather that 
people tried to look behind the curtain of DMA-buf backing stores.

In other words all the fun we had with scatterlists and that people try 
to modify the struct pages inside of them.

Improving the DMA API is something I really really hope for as well.

>>>>> 3) Importing devices need to know if they are working with PCI P2P
>>>>> addresses during mapping because they need to do things like turn on
>>>>> ATS on their DMA. As for multi-path we have the same hacks inside mlx5
>>>>> today that assume DMABUFs are always P2P because we cannot determine
>>>>> if things are P2P or not after being DMA mapped.
>>>> Why would you need ATS on PCI P2P and not for system memory accesses?
>>> ATS has a significant performance cost. It is mandatory for PCI P2P,
>>> but ideally should be avoided for CPU memory.
>> Huh, I didn't know that. And yeah kinda means we've butchered the pci p2p
>> stuff a bit I guess ...

Hui? Why should ATS be mandatory for PCI P2P?

We have tons of production systems using PCI P2P without ATS. And it's 
the first time I hear that.

>>>>> 5) iommufd and kvm are both using CPU addresses without DMA. No
>>>>> exporter mapping is possible
>>>> We have customers using both KVM and XEN with DMA-buf, so I can clearly
>>>> confirm that this isn't true.
>>> Today they are mmaping the dma-buf into a VMA and then using KVM's
>>> follow_pfn() flow to extract the CPU pfn from the PTE. Any mmapable
>>> dma-buf must have a CPU PFN.
>>>
>>> Here Xu implements basically the same path, except without the VMA
>>> indirection, and it suddenly not OK? Illogical.
>> So the big difference is that for follow_pfn() you need mmu_notifier since
>> the mmap might move around, whereas with pfn smashed into
>> dma_buf_attachment you need dma_resv_lock rules, and the move_notify
>> callback if you go dynamic.
>>
>> So I guess my first question is, which locking rules do you want here for
>> pfn importers?
> follow_pfn() is unwanted for private MMIO, so dma_resv_lock.

As Sima explained you either have follow_pfn() and mmu_notifier or you 
have DMA addresses and dma_resv lock / dma_fence.

Just giving out PFNs without some lifetime associated with them is one 
of the major problems we faced before and really not something you can do.

>> If mmu notifiers is fine, then I think the current approach of follow_pfn
>> should be ok. But if you instead dma_resv_lock rules (or the cpu mmap
>> somehow is an issue itself), then I think the clean design is create a new
> cpu mmap() is an issue, this series is aimed to eliminate userspace
> mapping for private MMIO resources.

Why?

>> separate access mechanism just for that. It would be the 5th or so (kernel
>> vmap, userspace mmap, dma_buf_attach and driver private stuff like
>> virtio_dma_buf.c where you access your buffer with a uuid), so really not
>> a big deal.
> OK, will think more about that.

Please note that we have follow_pfn() + mmu_notifier working for KVM/XEN 
with MMIO mappings and P2P. And that required exactly zero DMA-buf 
changes :)

I don't fully understand your use case, but I think it's quite likely 
that we already have that working.

Regards,
Christian.

>
> Thanks,
> Yilun
>
>> And for non-contrived exporters we might be able to implement the other
>> access methods in terms of the pfn method generically, so this wouldn't
>> even be a terrible maintenance burden going forward. And meanwhile all the
>> contrived exporters just keep working as-is.
>>
>> The other part is that cpu mmap is optional, and there's plenty of strange
>> exporters who don't implement. But you can dma map the attachment into
>> plenty devices. This tends to mostly be a thing on SoC devices with some
>> very funky memory. But I guess you don't care about these use-case, so
>> should be ok.
>>
>> I couldn't come up with a good name for these pfn users, maybe
>> dma_buf_pfn_attachment? This does _not_ have a struct device, but maybe
>> some of these new p2p source specifiers (or a list of those which are
>> allowed, no idea how this would need to fit into the new dma api).
>>
>> Cheers, Sima
>> -- 
>> Simona Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

[-- Attachment #2: Type: text/html, Size: 10134 bytes --]