Re: [PATCH 8/9] iommu/vt-d: Use the generic iommu page table

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Aug 22, 2025 at 09:35:27AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, July 17, 2025 3:58 AM
> > 
> > Replace the VT-D iommu_domain implementation of the VTD second stage
> > and
> > first stage page tables with the iommupt VTDSS and x86_64
> > pagetables. x86_64 is shared with the AMD driver.
> > 
> > There are a couple notable things in VT-D:
> > - Like AMD the second stage format is not sign extended, unlike AMD it
> >   cannot decode a full 64 bits. The first stage format is a normal sign
> >   extended x86 page table
> > - The HW caps can indicate how many levels, how many address bits and
> > what
> >   leaf page sizes are supported in HW. As before the highest number of
> >   levels that can translate the entire supported address width is used.
> >   The supported page sizes are adjusted directly from the dedicated
> >   first/second stage cap bits.
> > - VTD requires flushing 'write buffers'. This logic is left unchanged,
> >   the write buffer flushes on any gather flush or through iotlb_sync_map.
> > - Like ARM, VTD has an optional non-coherent page table walker that
> 
> 'optional' sounds like a software choice. but here is actually a hardware
> limitation on certain platforms.

I wrote it like this because from a spec perspective there is a PWSNP
field in the PASID table entry that SW can choose to use.

Yes, some HW requires it to be set to 0..

> >   requires cache flushing. This is supported through
> > PT_FEAT_DMA_INCOHERENT
> >   the same as ARM, however x86 can't use the DMA API for flush, it must
> >   call the arch function clflush_cache_range()
> > - The PT_FEAT_DYNAMIC_TOP can probably be supported on VTD someday
> > for the
> >   second stage when it uses 128 bit atomic stores for the HW context
> >   structures.
> > - PT_FEAT_VTDSS_FORCE_WRITEABLE is used to work around
> > ERRATA_772415_SPR17
> > - A kernel command line parameter "sp_off" disables all page sizes except 4k
> 
> No related code in this patch. Is it from the common logic in the AMD
> series?

It is this:

static struct iommu_domain *
intel_iommu_domain_alloc_first_stage(struct device *dev,
                                     struct intel_iommu *iommu, u32 flags)
{
        if (!intel_iommu_superpage)
                dmar_domain->domain.pgsize_bitmap = SZ_4K;


intel_iommu_superpage is driven by sp_off:

                } else if (!strncmp(str, "sp_off", 6)) {
                        pr_info("Disable supported super page\n");
                        intel_iommu_superpage = 0;

> > @@ -3442,6 +2990,16 @@ static int
> > paging_domain_compatible_first_stage(struct dmar_domain *dmar_domain,
> >  	if (!sm_supported(iommu) || !ecap_flts(iommu->ecap))
> >  		return -EINVAL;
> > 
> > +	if (!ecap_smpwc(iommu->ecap) &&
> > +	    !(dmar_domain->fspt.x86_64_pt.common.features &
> > +	      BIT(PT_FEAT_DMA_INCOHERENT)))
> > +		return -EINVAL;
> 
> this change leads to a slightly different behavior from the old one.
> 
> the old logic is that iommu/domain must have the same attribute:
> 
> -	if (dmar_domain->iommu_coherency !=
> -			iommu_paging_structure_coherency(iommu))
> -		return -EINVAL;
> 
> but with this change a coherent walker can be attached to a
> incoherent domain.

Yes, and it should set PWSNP in the PASID entry too, not wire it like
this:

        pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));

So like:

       pasid_set_page_snoop(pte, !(s2_domain->sspt.vtdss_pt.common.features &
                                   BIT(PT_FEAT_DMA_INCOHERENT)));

And so on..

> Conceptually it's correct, as the only side effect
> is unnecessary cost of cache flush. But it does lead to user-visible
> impact i.e. a previously failed attach may succeed now.

Yes and yes
 
> In reality it might be fine as I'm not aware of a platform with IOMMUs
> having mixed coherent/incoherent walkers. But since this series is 
> more for refactoring, better leave the uAPI semantics change to future.

But sure it can be split..

So like this:

	if (!!ecap_smpwc(iommu->ecap) !=
	    !(dmar_domain->fspt.x86_64_pt.common.features &
	      BIT(PT_FEAT_DMA_INCOHERENT)))
		return -EINVAL;

	if (iommu_paging_structure_coherency(iommu) !=
	    !(dmar_domain->sspt.vtdss_pt.common.features &
	      BIT(PT_FEAT_DMA_INCOHERENT)))
		return -EINVAL;

And I added a patch to put it back with the above set_page_snoop
related changes.

Thanks,
Jason