Re: [PATCH v5 8/9] iommu/arm-smmu-v3: Change how the tlbi describes the invalidation

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Sep 07, 2026 at 02:42:57PM +0000, Mostafa Saleh wrote:
> On Tue, Sep 01, 2026 at 02:49:57PM -0300, Jason Gunthorpe wrote:
> > The RIL logic has long had a FIXME that there is not enough
> > information to properly compute the RIL. There is also subtly not
> > enough information to properly compute the single stride either.
> > 
> > Change tlbi to use the information format that iommupt is going to
> > use for ARM. This prepares the invalidation code to support iommupt
> > and fixes two small limitations with the current code.
> > 
> > iommupt is designed to accumulate all invalidation into a single
> > gather, then the iommu driver should issue a small number of commands
> > to execute the gather to control invalidation latency. This is in
> > contrast to io-pgtable-arm.c which generates many gather flushes and
> > direct walk cache flushes as it progresses.
> > 
> > To accommodate this the gather will accumulate "damage" in bitmaps,
> > one for leaf changes and one for table changes. This is enough
> > information for SMMUv3 to compute the proper stride for single
> > invalidation and to generate ideal hints for range invalidation.
> 
> I am not sure I understand that, in what situation the leaf_bitmap
> would be used instead of a single page size?=
> Would iommupt combine different page sizes in a single invalidation?

Yes

> And then I see in this patch it has:
> if (!is_power_of_2(leaf_bitmap))
> 	return 0;

Right, ARM doesn't support mixed leaves in a RIL so we can't use TTL
if iommupt has constructed something like that.

> Would that actually be better for performance than using 2 sets of
> RILs, one for each page size?
> As I'd imagine the HW will spend more effort on the TTL=0 case
> otherwise it wouldn't require it.

I have no idea, it is a hint. Since SW has no knowledge I think it
should just issue as few commands as possible. There is no way to know
what will work better on any particular HW.

> > @@ -140,17 +140,34 @@ static void arm_smmu_mm_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn,
> >  {
> >  	struct arm_smmu_domain *smmu_domain =
> >  		container_of(mn, struct arm_smmu_domain, mmu_notifier);
> > +	u8 tgsz_lg2 = smmu_domain->tgsz_lg2;
> >  	struct arm_smmu_tlbi tlbi = {
> >  		.tgsz_lg2 = smmu_domain->tgsz_lg2,
> > -		.iova = start,
> > +		.start = start,
> > +		.last = end - 1,
> >  		/*
> > -		 * The mm_types defines vm_end as the first byte after the end
> > -		 * address, different from IOMMU subsystem using the last
> > -		 * address of an address range.
> > +		 * No information comes from the mm, assume the worst case that
> > +		 * it changed every table level. The way this is hooked into the
> > +		 * mm is tricky, the range won't be expanded to include an
> > +		 * entire table level if one was removed like the iommu gather
> > +		 * does. Thus even if this is a 4k invalidation it may be
> > +		 * including any table level too.
> >  		 */
> > -		.size = end - start,
> > -		.iopte_size = PAGE_SIZE,
> > +		.table_levels_bitmap = 0xfe,
> 
> What does that mean, won't arm have a max of 4 levels?

It is really ~1, the extra leading 1s don't matter. Just can't have
the leaf bit set.

> > +	/*
> > +	 * If the size is small then we can infer the invalidation is PTE only
> > +	 * and set the PTE level only. Otherwise it could be some other
> > +	 * combination so just set them all. This allows RIL to use TTL=3 in
> > +	 * cases of PTE only changes. The mm must not try to partially
> > +	 * invalidate pmd/etc.
> > +	 */
> 
> How does that work with splitting blocks?  I imagine that might be
> ossible with userspace.

If mm splits anything then the invalidation will not be PAGE_SIZE
big. A split requires invalidating the original larger size.

> > +static u8 arm_smmu_tlbi_calc_stride(struct arm_smmu_tlbi *tlbi)
> > +{
> > +	u8 combined = tlbi->table_levels_bitmap | tlbi->leaf_levels_bitmap;
> > +	u8 tg_szlg2 = tlbi->tgsz_lg2;
> > +
> > +	if (WARN_ON(!combined))
> > +		return U8_MAX;
> 
> When can that happen?

It can't, thats why it is a WARN_ON :)

> > @@ -4152,21 +4237,28 @@ static void arm_smmu_flush_iotlb_all(struct iommu_domain *domain)
> >  		arm_smmu_tlb_inv_context(smmu_domain);
> >  }
> >  
> > +/*
> > + * Called by io-pgtable-arm.c for each run of same pgsize leaf only
> 
> I believe that it is called from dma-iommu.c, io-pgtable-arm.c will
> call the tlb_add_page which builds the gather though.

Sort of, for the purposes of this comment the important flush is
initiated by io-pgtable-arm.c under tlb_add_page() when it calls
arm_smmu_tlb_inv_page_nosync(), which calls
iommu_iotlb_gather_add_page(), which calls iommu_iotlb_sync()

That's done in a way that guarentees the same-pgsize property:

	if ((gather->pgsize && gather->pgsize != size) ||

Yes it is also called from dma-iommu.c, but only for the "trailing"
gather and that doesn't do anything to change what is in the gather..

I'll add a few more words here

> > + * invalidation. If it has to change to a different leaf level then it flushes
> > + * the gather and starts a fresh one. Thus this always targets only a single
> > + * leaf level.
> > + */
> >  static void arm_smmu_iotlb_sync(struct iommu_domain *domain,
> >  				struct iommu_iotlb_gather *gather)
> >  {
> >  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > +	unsigned int tg = smmu_domain->tgsz_lg2;
> >  	struct arm_smmu_tlbi tlbi = {
> >  		.tgsz_lg2 = smmu_domain->tgsz_lg2,
> > -		.iova = gather->start,
> > -		.size = gather->end - gather->start + 1,
> > -		.iopte_size = gather->pgsize,
> > -		.leaf_only = true,
> > +		.start = gather->start,
> > +		.last = gather->end,
> >  	};
> >  
> > -	if (!gather->pgsize)
> > +	if (WARN_ON(gather->pgsize < BIT(tg)))
> >  		return;
> >  
> > +	tlbi.leaf_levels_bitmap = BIT((ilog2(gather->pgsize) - tg) / (tg - 3));
> 
> Having some page table macros would be helpful (and in other places in
> this patch)

At least this one gets deleted in the next series, so I left it like
this deliberately. Was there something else you saw that had
duplication?

Thanks,
Jason