Re: [PATCH] iommu: Always fill in gather when unmapping

Robin Murphy
2026-04-02T18:11:32Z

On 2026-04-01 6:36 pm, Jason Gunthorpe wrote:
> On Wed, Apr 01, 2026 at 05:33:28PM +0100, Robin Murphy wrote:
>>> io-pgtable might have intended to allow the driver to choose between
>>> gather or immediate flush because it passed gather to
>>> ops->tlb_add_page(), however no driver does anything with it.
>>
>> Apart from arm-smmu-v3...
>
> Bah, I did my research on the wrong tree and missed this.
>
>>> mtk uses io-pgtable-arm-v7s but added the range to the gather in the
>>> unmap callback. Move this into the io-pgtable-arm unmap itself. That
>>> will fix all the armv7 using drivers (arm-smmu, qcom_iommu,
>>> ipmmu-vmsa).
>>
>> io-pgtable-arm-v7s != io-pgtable-arm. You're *breaking* MTK (and failing
>> to fix the other v7s user, which is MSM).
>
> I was very confused what you were talking about, but I see now that
> the hunk adding iommu_iotlb_gather_add_range() to v7 got lost somehow!
>
> @@ -596,6 +596,9 @@ static size_t __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
>
> __arm_v7s_set_pte(ptep, 0, num_entries, &iop->cfg);
>
> + if (!iommu_iotlb_gather_queued(gather))
> + iommu_iotlb_gather_add_range(gather, iova, size);
> +
> for (i = 0; i < num_entries; i++) {
> if (ARM_V7S_PTE_IS_TABLE(pte[i], lvl)) {
> /* Also flush any partial walks */
>
>>> arm-smmu uses both ARM_V7S and ARM LPAE formats. The LPAE formats
>>> already have the gather population because SMMUv3 requires it, so it
>>> becomes consistent.
>>
>> Huh? arm-smmu-v3 invokes iommu_iotlb_gather_add_page() itself, because
>> arm-smmu-v3 uses gathers
>
> Yeah, I missed this whole bit, it needs some changes.
>
>> Invoking add range before add_page will end up defeating the
>> iommu_iotlb_gather_is_disjoint() check and making SMMUv3
>> overinvalidate between disjoint ranges.
>
> Right, that flow needs fixing.
>
>> I guess now I remember why we weren't validating gathers in core code
>> before :(
>
> My point is not filling the gather is a micro-optimization that
> benefits a few drivers. I think it is so small compared to an IOTLB
> flush that it isn't worth worrying about.

It's hardly a "micro-optimisation" for drivers to just not touch an
optional mechanism which offers no benefit to them, especially when in
many cases said mechanism is newer than the code that isn't using it
anyway. The only required semantic of .iotlb_sync is to ensure that any
previous .unmap_pages calls are complete and their associated
translations invalidated. The entire concept of gathering and deferred
invalidation is irrelevant to many IOMMU designs where it would only
stand to make overall invalidation performance worse.

I'm starting to wish I'd been able to page all this context back in
before reviewing the first patch, as I too only really had Intel and
SMMUv3 in mind at the time... :(

> So, I'd like to make everything the same and populate the gather
> correctly in all flows. I'll fix the SMMUv3 thing and lets look again,
> this patch is not so scary to make me think we shouldn't do that.
>
>> @@ -2714,6 +2714,10 @@ static size_t __iommu_unmap(struct iommu_domain *domain,
>> pr_debug("unmapped: iova 0x%lx size 0x%zx\n",
>> iova, unmapped_page);
>> + /* If the driver itself isn't using the gather, mark it used */
>> + if (iotlb_gather->end <= iotlb_gather->start)
>> + iommu_iotlb_gather_add_range(&iotlb_gather, iova, unmapped_page);
>
> The gathers can be joined across unmaps and now we are inviting subtly
> ill-formed gathers as only the first unmap will get included.

Ill-formed? It's a perfectly valid range for the purposes of any
subsequent generic check - which couldn't realistically be anything
beyond empty vs. non-empty anyway - and it's only being set at all in
the case where we know the driver doesn't care, because if the driver
*was* going to look at gather->start or gather->end in its .iotlb_sync
then it must have already set them to meaningful values in the prior
successful .unmap_pages call. I think we can safely consider it invalid
for a driver to suddenly decide to start using a gather mid-way through
an unmap (or indeed to use start/end in any intentionally non-obvious
manner either).

> We do have error cases where the gather is legitimately empty, and
> this would squash that, it probably needs to check unmapped_page for 0
> too, at least.

Maybe try looking at the rest of the code around these lines...

Thanks,
Robin.