On Wed, Nov 19, 2025 at 02:42:18PM +0100, Christian König wrote:
> On 11/19/25 14:25, Jason Gunthorpe wrote:
> > On Wed, Nov 19, 2025 at 02:16:57PM +0100, Christian König wrote:
> >>> +/**
> >>> + * dma_buf_map - Returns the scatterlist table of the attachment from 
> >>> arrays
> >>> + * of physical vectors. This funciton is intended for MMIO memory only.
> >>> + * @attach:      [in]    attachment whose scatterlist is to be returned
> >>> + * @provider:    [in]    p2pdma provider
> >>> + * @phys_vec:    [in]    array of physical vectors
> >>> + * @nr_ranges:   [in]    number of entries in phys_vec array
> >>> + * @size:        [in]    total size of phys_vec
> >>> + * @dir: [in]    direction of DMA transfer
> >>> + *
> >>> + * Returns sg_table containing the scatterlist to be returned; returns 
> >>> ERR_PTR
> >>> + * on error. May return -EINTR if it is interrupted by a signal.
> >>> + *
> >>> + * On success, the DMA addresses and lengths in the returned scatterlist 
> >>> are
> >>> + * PAGE_SIZE aligned.
> >>> + *
> >>> + * A mapping must be unmapped by using dma_buf_unmap().
> >>> + */
> >>> +struct sg_table *dma_buf_map(struct dma_buf_attachment *attach,
> >>
> >> That is clearly not a good name for this function. We already have 
> >> overloaded the term *mapping* with something completely different.
> >>
> >>> +                      struct p2pdma_provider *provider,
> >>> +                      struct dma_buf_phys_vec *phys_vec,
> >>> +                      size_t nr_ranges, size_t size,
> >>> +                      enum dma_data_direction dir)
> >>> +{
> >>> + unsigned int nents, mapped_len = 0;
> >>> + struct dma_buf_dma *dma;
> >>> + struct scatterlist *sgl;
> >>> + dma_addr_t addr;
> >>> + size_t i;
> >>> + int ret;
> >>> +
> >>> + dma_resv_assert_held(attach->dmabuf->resv);
> >>> +
> >>> + if (WARN_ON(!attach || !attach->dmabuf || !provider))
> >>> +         /* This function is supposed to work on MMIO memory only */
> >>> +         return ERR_PTR(-EINVAL);
> >>> +
> >>> + dma = kzalloc(sizeof(*dma), GFP_KERNEL);
> >>> + if (!dma)
> >>> +         return ERR_PTR(-ENOMEM);
> >>> +
> >>> + switch (pci_p2pdma_map_type(provider, attach->dev)) {
> >>> + case PCI_P2PDMA_MAP_BUS_ADDR:
> >>> +         /*
> >>> +          * There is no need in IOVA at all for this flow.
> >>> +          */
> >>> +         break;
> >>> + case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE:
> >>> +         dma->state = kzalloc(sizeof(*dma->state), GFP_KERNEL);
> >>> +         if (!dma->state) {
> >>> +                 ret = -ENOMEM;
> >>> +                 goto err_free_dma;
> >>> +         }
> >>> +
> >>> +         dma_iova_try_alloc(attach->dev, dma->state, 0, size);
> >>
> >> Oh, that is a clear no-go for the core DMA-buf code.
> >>
> >> It's intentionally up to the exporter how to create the DMA
> >> addresses the importer can work with.
> > 
> > I can't fully understand this remark?
> 
> The exporter should be able to decide if it actually wants to use P2P when 
> the transfer has to go through the host bridge (e.g. when IOMMU/bridge 
> routing bits are enabled).
> 
> Thinking more about it exporters can now probably call 
> pci_p2pdma_map_type(provider, attach->dev) before calling this function so 
> that is probably ok.
> 
> >> We could add something like a dma_buf_sg_helper.c or similar and put it in 
> >> there.
> > 
> > Yes, the intention is this function is an "exporter helper" that an
> > exporter can call if it wants to help generate the scatterlist.
> > 
> > So your "no-go" is just about what file it is in, not anything about
> > how it works?
> 
> Yes, exactly that. Just move it into a separate file somewhere and it's 
> probably good to go as far as I can see.
> 
> But only take that as Acked-by, I would need at least a day (or week) of free 
> time to wrap my head around all the technical details again. And that is 
> something I won't have before January or even later.

If it helps, we can meet at LPC. Jason and/or I will be happy to assist.

Thanks

> 
> Regards,
> Christian.
> 
> > 
> > Thanks,
> > Jason
> 

Reply via email to