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

Pranjal Shrivastava
2026-03-20T09:28:38Z

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:

+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