Re: [PATCH v3 02/12] iommu/vt-d: Add cache tag invalidation helpers

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

On 4/16/24 4:06 PM, Lu Baolu wrote:
> Add several helpers to invalidate the caches after mappings in the
> affected domain are changed.
> 
> - cache_tag_flush_range() invalidates a range of caches after mappings
>    within this range are changed. It uses the page-selective cache
>    invalidation methods.
> 
> - cache_tag_flush_all() invalidates all caches tagged by a domain ID.
>    It uses the domain-selective cache invalidation methods.
> 
> - cache_tag_flush_range_np() invalidates a range of caches when new
>    mappings are created in the domain and the corresponding page table
>    entries change from non-present to present.
> 
> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> ---
>   drivers/iommu/intel/iommu.h |  14 +++
>   drivers/iommu/intel/cache.c | 195 ++++++++++++++++++++++++++++++++++++
>   drivers/iommu/intel/iommu.c |  12 ---
>   3 files changed, 209 insertions(+), 12 deletions(-)

[...]

> +
> +/*
> + * Invalidates a range of IOVA from @start (inclusive) to @end (inclusive)
> + * when the memory mappings in the target domain have been modified.
> + */
> +void cache_tag_flush_range(struct dmar_domain *domain, unsigned long start,
> +			   unsigned long end, int ih)
> +{
> +	unsigned long pages, mask, addr;
> +	struct cache_tag *tag;
> +	unsigned long flags;
> +
> +	addr = calculate_psi_aligned_address(start, end, &pages, &mask);
> +
> +	spin_lock_irqsave(&domain->cache_lock, flags);
> +	list_for_each_entry(tag, &domain->cache_tags, node) {
> +		struct intel_iommu *iommu = tag->iommu;
> +		struct device_domain_info *info;
> +		u16 sid;
> +
> +		switch (tag->type) {
> +		case CACHE_TAG_IOTLB:
> +		case CACHE_TAG_NESTING_IOTLB:
> +			if (domain->use_first_level) {
> +				qi_flush_piotlb(iommu, tag->domain_id,
> +						tag->pasid, addr, pages, ih);
> +			} else {
> +				/*
> +				 * Fallback to domain selective flush if no
> +				 * PSI support or the size is too big.
> +				 */
> +				if (!cap_pgsel_inv(iommu->cap) ||
> +				    mask > cap_max_amask_val(iommu->cap))
> +					iommu->flush.flush_iotlb(iommu, tag->domain_id,
> +								 0, 0, DMA_TLB_DSI_FLUSH);
> +				else
> +					iommu->flush.flush_iotlb(iommu, tag->domain_id,
> +								 addr | ih, mask,
> +								 DMA_TLB_PSI_FLUSH);
> +			}
> +			break;
> +		case CACHE_TAG_NESTING_DEVTLB:
> +			/*
> +			 * Address translation cache in device side caches the
> +			 * result of nested translation. There is no easy way
> +			 * to identify the exact set of nested translations
> +			 * affected by a change in S2. So just flush the entire
> +			 * device cache.
> +			 */
> +			addr = 0;
> +			mask = MAX_AGAW_PFN_WIDTH;
> +			fallthrough;

I realized that the logic above is not right. Setting both @addr and
@mask to 0 doesn't means flush all caches on the device. I will change
it like below:

diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
index e8418cdd8331..18debb82272a 100644
--- a/drivers/iommu/intel/cache.c
+++ b/drivers/iommu/intel/cache.c
@@ -302,9 +302,14 @@ void cache_tag_flush_range(struct dmar_domain 
*domain, unsigned long start,
                          * affected by a change in S2. So just flush 
the entire
                          * device cache.
                          */
-                       addr = 0;
-                       mask = MAX_AGAW_PFN_WIDTH;
-                       fallthrough;
+                       info = dev_iommu_priv_get(tag->dev);
+                       sid = PCI_DEVID(info->bus, info->devfn);
+
+                       qi_flush_dev_iotlb(iommu, sid, info->pfsid, 
info->ats_qdep,
+                                          0, MAX_AGAW_PFN_WIDTH);
+                       quirk_extra_dev_tlb_flush(info, 0, 
MAX_AGAW_PFN_WIDTH,
+                                                 IOMMU_NO_PASID, 
info->ats_qdep);
+                       break;
                 case CACHE_TAG_DEVTLB:
                         info = dev_iommu_priv_get(tag->dev);
                         sid = PCI_DEVID(info->bus, info->devfn);

> +		case CACHE_TAG_DEVTLB:
> +			info = dev_iommu_priv_get(tag->dev);
> +			sid = PCI_DEVID(info->bus, info->devfn);
> +
> +			if (tag->pasid == IOMMU_NO_PASID)
> +				qi_flush_dev_iotlb(iommu, sid, info->pfsid,
> +						   info->ats_qdep, addr, mask);
> +			else
> +				qi_flush_dev_iotlb_pasid(iommu, sid, info->pfsid,
> +							 tag->pasid, info->ats_qdep,
> +							 addr, mask);
> +
> +			quirk_extra_dev_tlb_flush(info, addr, mask, tag->pasid, info->ats_qdep);
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&domain->cache_lock, flags);

Best regards,
baolu