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

[Samiullah Khawaja <skhawaja@google.com>]

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