Re: [RFC PATCH v2 1/2] dma-buf: Add support for private interconnects

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

On 26.09.25 16:41, Jason Gunthorpe wrote:
> On Fri, Sep 26, 2025 at 03:51:21PM +0200, Thomas Hellström wrote:
> 
>> Well both exporter and exporter has specific information WRT this. The
>> ultimate decision is done in the exporter attach() callback, just like
>> pcie_p2p. And the exporter acknowledges that by setting the
>> dma_buf_attachment::interconnect_attach field. In analogy with the
>> dma_buf_attachment::peer2peer member.
> 
> Having a single option seems too limited to me..

Yeah, agree.

> I think it would be nice if the importer could supply a list of
> 'interconnects' it can accept, eg:
> 
>  - VRAM offset within this specific VRAM memory
>  - dma_addr_t for this struct device
>  - "IOVA" for this initiator on a private interconnect
>  - PCI bar slice
>  - phys_addr_t (used between vfio, kvm, iommufd)

I would rather say that the exporter should provide the list of what interconnects the buffer might be accessible through.

Having the p2p flag set by the importer was basically just the easiest approach to implement the flow, that is not necessarily a design pattern how to approach a general solution.

Regards,
Christian.

> 
> The exporter has a function to run down the list and return the first
> compatible. Maybe something like
> 
>     struct dma_buf_interconnect_negotiation {
>     	   struct dma_buf_interconnect *interconnect,
> 	   void *interconnect_args,
>     };
> 
>     struct dma_buf_interconnect_negotiation importer_offer[2] = { // On stack
>         [0] = {.interconnect = myself->xe_vram},
>         [1] = {.interconnect = &dmabuf_generic_dma_addr_t, .interconnects_args = dev},
>     };
>     idx = dma_buf_negotiate(dmabuf, importer_offer, ARRAY_SIZE(importer_offer));
>     if (idx < 0)
>         return -EOPNOTSUPP;
> 
> Then you'd 'interconnect attach' with that compatible item and get
> back an attach. Using container_of to get the specific ops which then
> has a function to get the address list.
> 
>    attach = dma_buf_attach_interconnect(dmabuf, importer_offer[idx],  &dma_buf_attach_ops);
> 
>    if (idx == 0) {
>           xe_vram_ops = container_of(attach->ops, ..);
>           struct device_private_address *addrs = xe_vram_ops->map(attach);
> 	  [..]
> 	  xe_vram_ops->unmap(attach);
>    }
>    dma_buf_detach_interconnect(attach);
> 
> I can imagine some scheme where if the exporter does not support
> interconnect then the core code will automatically look for
> dmabuf_generic_dma_addr_t, select it, and supply some ops that call
> existing dma_buf_dynamic_attach()/dma_buf_map_attachment()
> transparently.
> 
>> So the above function mimics the dma_buf_attach_ops::allow_peer2peer
>> bool, except it's not a single interconnect so we'd either use a set of
>> bools, one for each potential interconnect, or a function like this.
>> A function has the benefit that it can also provide any additional
>> attach ops the interconnect might need.
> 
> allow_peer2peer seems to indicate if sg_page() can be used on the
> sgt? It doesn't have any meaning for an importer only using
> dma_addr_t?
> 
> In the above language it would be an interconnect exchanging 'struct
> page *'.. I'm a little confused by this I thought touching the struct
> page was forbidden?
> 
>> Is this to not overload the map_attachment() and unmap_attachment()
>> functions that otherwise could be used? Is it because they return an
>> sg_table?
> 
> It would be good to avoid going through APIs that use sg_table in the
> design..
> 
> Jason