Re: [PATCH v2 07/15] iommupt: Add map_pages op

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, May 05, 2025 at 11:18:37AM -0300, Jason Gunthorpe wrote:
> +/*
> + * Check that the items in a contiguous block are all empty. This will
> + * recursively check any tables in the block to validate they are empty and
> + * accumulate them on the free list. Makes no change on failure. On success
> + * caller must fill the items.
> + */
> +static int clear_contig(const struct pt_state *start_pts,
> +			struct iommu_iotlb_gather *iotlb_gather,
> +			unsigned int step, unsigned int pgsize_lg2)
> +{
> +	struct pt_iommu *iommu_table =
> +		iommu_from_common(start_pts->range->common);
> +	struct pt_range range = *start_pts->range;
> +	struct pt_state pts =
> +		pt_init(&range, start_pts->level, start_pts->table);
> +	struct pt_iommu_collect_args collect = {
> +		.free_list = IOMMU_PAGES_LIST_INIT(collect.free_list),
> +	};
> +	pt_vaddr_t start_va = range.va;
> +	int ret;
> +
> +	pts.index = start_pts->index;
> +	pts.end_index = start_pts->index + step;
> +	for (; _pt_iter_load(&pts); pt_next_entry(&pts)) {
> +		if (pts.type == PT_ENTRY_TABLE) {
> +			ret = pt_walk_descend_all(&pts, __collect_tables,
> +						  &collect);
> +			if (ret)
> +				return ret;
> +
> +			iommu_pages_list_add(&collect.free_list,
> +					     pt_table_ptr(&pts));
> +		} else if (pts.type != PT_ENTRY_EMPTY) {
> +			return -EADDRINUSE;
> +		}
> +	}
> +
> +	if (!iommu_pages_list_empty(&collect.free_list)) {
> +		gather_range(iotlb_gather, iommu_table, start_va,
> +			     range.va - start_va);
> +		iommu_pages_list_splice(&collect.free_list,
> +					&iotlb_gather->freelist);
> +	}

For the PT_FEAT_FLUSH_RANGE_NO_GAPS mode this doesn't work, it could
cause an iommu_iotlb_sync() and free to be done before the PTEs have
been updated. The original idea was that the PTEs would all be set
directly to the new OA PTEs by the caller, which worked OK only for
PT_FEAT_FLUSH_RANGE.

It looks like fixing it by clearing the table levels as soon as we
know they are empty is the best thing:

+                       /*
+                        * The table item must be cleared before we can update
+                        * the gather
+                        */
+                       pt_clear_entry(&pts, ilog2(1));
+
                        iommu_pages_list_add(&collect.free_list,
                                             pt_table_ptr(&pts));
+                       gather_range_pages(
+                               iotlb_gather, iommu_table, range.va,
+                               log2_to_int(pt_table_item_lg2sz(&pts)),
+                               &collect.free_list);

Jason