Re: [PATCH 1/9] iommu/pages: Add support for a incoherent IOMMU page walker

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Aug 15, 2025 at 11:28:06AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, July 17, 2025 3:58 AM
> > 
> > @@ -82,6 +92,9 @@ static void __iommu_free_desc(struct ioptdesc *iopt)
> >  	struct folio *folio = ioptdesc_folio(iopt);
> >  	const unsigned long pgcnt = 1UL << folio_order(folio);
> > 
> > +	if (!IS_ENABLED(CONFIG_X86))
> > +		WARN_ON_ONCE(iopt->incoherent);
> > +
> 
> a comment is welcomed here.

How about this instead:

-       if (!IS_ENABLED(CONFIG_X86))
+       if (IOMMU_PAGES_USE_DMA_API)
                WARN_ON_ONCE(iopt->incoherent);

And all the other CONFIG_X86 removed

> > +/**
> > + * iommu_pages_start_incoherent - Setup the page for cache incoherent
> > operation
> > + * @virt: The page to setup
> > + * @dma_dev: The iommu device
> > + *
> > + * For incoherent memory we use the DMA API to manage the cache
> > flushing. This
> > + * is a lot of complexity compared to just calling
> > arch_sync_dma_for_device(),
> > + * but it is what the existing ARM iommu drivers have been doing. This
> > keeps
> > + * track of the dma map inside the ioptdesc so that error paths are simple
> > for
> > + * the caller.
> > + */
> 
> also talk about the x86 specific case.

/**
 * iommu_pages_start_incoherent - Setup the page for cache incoherent operation
 * @virt: The page to setup
 * @dma_dev: The iommu device
 *
 * For incoherent memory this will use the DMA API to manage the cache flushing
 * on some arches. This is a lot of complexity compared to just calling
 * arch_sync_dma_for_device(), but it is what the existing ARM iommu drivers
 * have been doing. The DMA API requires keeping track of the DMA map and
 * freeing it when required. This keeps track of the dma map inside the ioptdesc
 * so that error paths are simple for the caller.
 */

> > +int iommu_pages_start_incoherent_list(struct iommu_pages_list *list,
> > +				      struct device *dma_dev)
> > +{
> > +	struct ioptdesc *cur;
> > +	int ret;
> > +
> > +	list_for_each_entry(cur, &list->pages, iopt_freelist_elm) {
> > +		if (cur->incoherent)
> > +			continue;
> 
> Is it a valid case to have the caller pass in a page which is already
> made incoherent?

I think no, I made it into a warn_on and will find out :)

> If yes the current definition of start/stop is not strictly no par, as
> there is no record whether item->incoherent is set in this start
> call or left over from previous call in a failure path. Then the caller
> just passes in the same list to stop to cleanup which reverts all
> items with incoherent in the list i.e. more that what a failed start
> may change.

But this would still be desired because the point of stop is
basically to prepare to free the list. It is structured like this
because we have to pass in the struct device * to call the DMA API..

Thanks,
Jason