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

[Baolu Lu <baolu.lu@linux.intel.com>]

On 7/17/25 03:57, Jason Gunthorpe wrote:
> 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
>    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
> 
> Remove all the unused iommu_domain page table code. The debufs paths have
> their own independent page table walker that is left alone for now.
> 
> This corrects a race with the non-coherent walker that the ARM
> implementations have fixed:
> 
>       CPU 0                               CPU 1
>    pfn_to_dma_pte()                    pfn_to_dma_pte()
>     pte = &parent[offset];
>     if (!dma_pte_present(pte)) {
>       try_cmpxchg64(&pte->val)
> 					pte = &parent[offset];
> 					.. dma_pte_present(pte) ..
> 				        [...]
> 					// iommu_map() completes
> 					// Device does DMA
>       domain_flush_cache(pte)
> 
> The CPU 1 mapping operation shares a page table level with the CPU 0
> mapping operation. CPU 0 installed a new page table level but has not
> flushed it yet. CPU1 returns from iommu_map() and the device does DMA. The
> non coherent walker fails to see the new table level installed by CPU 0
> and fails the DMA with non-present.
> 
> The iommupt PT_FEAT_DMA_INCOHERENT implementation uses the ARM design of
> storing a flag when CPU 0 completes the flush. If the flag is not set CPU
> 1 will also flush to ensure the HW can fully walk to the PTE being
> installed.
> 
> Cc: Tina Zhang <tina.zhang@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/intel/Kconfig  |   4 +
>   drivers/iommu/intel/iommu.c  | 903 ++++++-----------------------------
>   drivers/iommu/intel/iommu.h  |  99 +---
>   drivers/iommu/intel/nested.c |   5 -
>   drivers/iommu/intel/pasid.c  |  29 +-
>   5 files changed, 182 insertions(+), 858 deletions(-)
> 

[...]

> @@ -3338,7 +2805,9 @@ static struct iommu_domain *
>   intel_iommu_domain_alloc_first_stage(struct device *dev,
>   				     struct intel_iommu *iommu, u32 flags)
>   {
> +	struct pt_iommu_x86_64_cfg cfg = {};
>   	struct dmar_domain *dmar_domain;
> +	int ret;
>   
>   	if (flags & ~IOMMU_HWPT_ALLOC_PASID)
>   		return ERR_PTR(-EOPNOTSUPP);
> @@ -3347,19 +2816,72 @@ intel_iommu_domain_alloc_first_stage(struct device *dev,
>   	if (!sm_supported(iommu) || !ecap_flts(iommu->ecap))
>   		return ERR_PTR(-EOPNOTSUPP);
>   
> -	dmar_domain = paging_domain_alloc(dev, true);
> +	dmar_domain = paging_domain_alloc();
>   	if (IS_ERR(dmar_domain))
>   		return ERR_CAST(dmar_domain);
>   
> +	if (cap_fl5lp_support(iommu->cap))
> +		cfg.common.hw_max_vasz_lg2 = 57;
> +	else
> +		cfg.common.hw_max_vasz_lg2 = 48;
> +	cfg.common.hw_max_oasz_lg2 = 52;
> +	cfg.common.features = BIT(PT_FEAT_SIGN_EXTEND) |
> +			      BIT(PT_FEAT_FLUSH_RANGE);
> +	/* First stage always uses scalable mode */
> +	if (WARN_ON(!ecap_smpwc(iommu->ecap)))
> +		return ERR_PTR(-EINVAL);

I don't follow here. Why WARN_ON and return failure when hardware
doesn't support a feature?

> +	if (ecap_smpwc(iommu->ecap))
> +		cfg.common.features |= BIT(PT_FEAT_DMA_INCOHERENT);

My understanding is that if hardware possibly walks the page table
incoherently, we need to set up the PT_FEAT_DMA_INCOHERENT feature;
otherwise, there is no need.

If that's correct, perhaps what we need here is:

	if (!ecap_smpwc(iommu->ecap))
		cfg.common.features |= BIT(PT_FEAT_DMA_INCOHERENT);

The Intel iommu driver always enforces page walk coherence in the PASID
table entry if the capability is supported.

> +	dmar_domain->iommu.iommu_device = dev;
> +	dmar_domain->iommu.nid = dev_to_node(dev);
>   	dmar_domain->domain.ops = &intel_fs_paging_domain_ops;
> +
> +	ret = pt_iommu_x86_64_init(&dmar_domain->fspt, &cfg, GFP_KERNEL);
> +	if (ret) {
> +		kfree(dmar_domain);
> +		return ERR_PTR(ret);
> +	}
> +
> +	if (!cap_fl1gp_support(iommu->cap))
> +		dmar_domain->domain.pgsize_bitmap &= ~(u64)SZ_1G;
> +	if (!intel_iommu_superpage)
> +		dmar_domain->domain.pgsize_bitmap = SZ_4K;
> +
>   	return &dmar_domain->domain;
>   }

[...]

>   static struct iommu_domain *
>   intel_iommu_domain_alloc_second_stage(struct device *dev,
>   				      struct intel_iommu *iommu, u32 flags)
>   {
> +	struct pt_iommu_vtdss_cfg cfg = {};
>   	struct dmar_domain *dmar_domain;
> +	unsigned int sslps;
> +	int ret;
>   
>   	if (flags &
>   	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
> @@ -3376,15 +2898,48 @@ intel_iommu_domain_alloc_second_stage(struct device *dev,
>   	if (sm_supported(iommu) && !ecap_slts(iommu->ecap))
>   		return ERR_PTR(-EOPNOTSUPP);
>   
> -	dmar_domain = paging_domain_alloc(dev, false);
> +	dmar_domain = paging_domain_alloc();
>   	if (IS_ERR(dmar_domain))
>   		return ERR_CAST(dmar_domain);
>   
> +	cfg.common.hw_max_vasz_lg2 = compute_vasz_lg2_ss(iommu);
> +	cfg.common.hw_max_oasz_lg2 = 52;
> +	cfg.common.features = BIT(PT_FEAT_FLUSH_RANGE);
> +
> +	/*
> +	 * Read-only mapping is disallowed on the domain which serves as the
> +	 * parent in a nested configuration, due to HW errata
> +	 * (ERRATA_772415_SPR17)
> +	 */
> +	if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT)
> +		cfg.common.features |= BIT(PT_FEAT_VTDSS_FORCE_WRITEABLE);
> +
> +	if (WARN_ON(!iommu_paging_structure_coherency(iommu)))
> +		return ERR_PTR(-EINVAL);
> +	if (!iommu_paging_structure_coherency(iommu))
> +		cfg.common.features |= BIT(PT_FEAT_DMA_INCOHERENT);

Similarly here.

> +	dmar_domain->iommu.iommu_device = dev;
> +	dmar_domain->iommu.nid = dev_to_node(dev);

Thanks,
baolu