Re: [PATCH 04/14] iommu/pages: Add APIs to preserve/unpreserve/restore iommu pages

[Samiullah Khawaja]

On Fri, Mar 20, 2026 at 09:28:28AM +0000, Pranjal Shrivastava wrote:
> On Tue, Mar 17, 2026 at 01:59:03PM -0700, Vipin Sharma wrote:
> > On Tue, Feb 03, 2026 at 10:09:38PM +0000, Samiullah Khawaja wrote:
> > > +
> > > +void iommu_unpreserve_pages(struct iommu_pages_list *list, int count)
> > > +{
> > > +	struct ioptdesc *iopt;
> > > +
> > > +	if (!count)
> > > +		return;
> > > +
> > > +	/* If less than zero then unpreserve all pages. */
> > > +	if (count < 0)
> > > +		count = 0;
> > > +
> > > +	list_for_each_entry(iopt, &list->pages, iopt_freelist_elm) {
> > > +		kho_unpreserve_folio(ioptdesc_folio(iopt));
> > > +		if (count > 0 && --count ==  0)
> > > +			break;
> > > +	}
> > > +}
> >
> > I see you are trying to have common function for error handling in
> > iommu_preserve_pages() down and unpreserving in the next patch with
> > overloaded meaning of count.
> >
> >  < 0 means unpreserve all of the pages
> >  = 0 means do nothing
> >  > 0 unpreserve these many pages.
> >
> > It seems non-intuitive to me and code also end up having multiple ifs. I
> > will recommend to just have this function go through all of the pages
> > without passing count argument. In iommu_preserve_pages() function down,
> > handle the error case with a "goto err" statement. If this is the way
> > things happen in iommu codebase feel free to ignore this suggestion.
>
> +1 to this. The current version of overloaded count is counter-intuitive
> as it expects a back-handed API contract. Since we're using lists here,
> I'd recommend using the list_for_each_entry_continue_reverse() helper:

Agreed. I will use this approach in next revision.

Thanks,
Sami

> +void iommu_unpreserve_pages(struct iommu_pages_list *list)
> +{
> +	struct ioptdesc *iopt;
> +
> +	list_for_each_entry(iopt, &list->pages, iopt_freelist_elm)
> +		kho_unpreserve_folio(ioptdesc_folio(iopt));
> +}
> +EXPORT_SYMBOL_GPL(iommu_unpreserve_pages);
> +
> +int iommu_preserve_pages(struct iommu_pages_list *list)
> +{
> +	struct ioptdesc *iopt;
> +	int ret;
> +
> +	list_for_each_entry(iopt, &list->pages, iopt_freelist_elm) {
> +		ret = kho_preserve_folio(ioptdesc_folio(iopt));
> +		if (ret)
> +			goto err_rollback;
> +	}
> +	return 0;
> +
> +err_rollback:
> +	/* Rollback only the successfully preserved pages */
> +	list_for_each_entry_continue_reverse(iopt, &list->pages, iopt_freelist_elm)
> +		kho_unpreserve_folio(ioptdesc_folio(iopt));
> +	return ret;
> +}
>
> [ ---- snip >8---- ]
>
> Thanks,
> Praan