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

Samiullah Khawaja
2026-03-20T18:27:45Z

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