Re: [Linaro-mm-sig] [PATCH v8 06/11] dma-buf: provide phys_vec to scatter-gather mapping routine

Leon Romanovsky
2025-11-19T13:48:41Z

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
>