Re: [PATCH 1/1] iommu/vt-d: Unconditionally flush device TLB for pasid table updates

[Yi Liu <yi.l.liu@intel.com>]

Hi Baolu,

On 2024/8/20 11:02, Lu Baolu wrote:
> The caching mode of an IOMMU is irrelevant to the behavior of the device
> TLB. Previously, commit <304b3bde24b5> ("iommu/vt-d: Remove caching mode
> check before device TLB flush") removed this redundant check in the
> domain unmap path.
> 
> Checking the caching mode before flushing the device TLB after a pasid
> table entry is updated is unnecessary and can lead to inconsistent
> behavior.
> 
> Extends this consistency by removing the caching mode check in the pasid
> table update path.

I'm wondering if a fix tag is needed here. Before this patch, the guest
kernel does not issue device TLB invalidation. This may be a problem for
the emulated devices that support ATS capability. The cache in device
side would be stale. Although some vIOMMU like QEMU virtual VT-d would
notify the emulated devices to flush their cache when handling the unmap
event. [1]. But this is not required by VT-d spec. So it's possible that
other vIOMMU may not do it. So this patch appears to fix an issue, so a fix
tag may be necessary. How about your thought?

[1] 
https://lore.kernel.org/qemu-devel/20240805062727.2307552-14-zhenzhong.duan@intel.com/

> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/iommu/intel/pasid.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 5792c817cefa..dc00eac6be31 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -264,9 +264,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
>   	else
>   		iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
>   
> -	/* Device IOTLB doesn't need to be flushed in caching mode. */
> -	if (!cap_caching_mode(iommu->cap))
> -		devtlb_invalidation_with_pasid(iommu, dev, pasid);
> +	devtlb_invalidation_with_pasid(iommu, dev, pasid);
>   }
>   
>   /*
> @@ -493,9 +491,7 @@ int intel_pasid_setup_dirty_tracking(struct intel_iommu *iommu,
>   
>   	iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
>   
> -	/* Device IOTLB doesn't need to be flushed in caching mode. */
> -	if (!cap_caching_mode(iommu->cap))
> -		devtlb_invalidation_with_pasid(iommu, dev, pasid);
> +	devtlb_invalidation_with_pasid(iommu, dev, pasid);
>   
>   	return 0;
>   }
> @@ -572,9 +568,7 @@ void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
>   	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
>   	qi_flush_piotlb(iommu, did, pasid, 0, -1, 0);
>   
> -	/* Device IOTLB doesn't need to be flushed in caching mode. */
> -	if (!cap_caching_mode(iommu->cap))
> -		devtlb_invalidation_with_pasid(iommu, dev, pasid);
> +	devtlb_invalidation_with_pasid(iommu, dev, pasid);
>   }
>   
>   /**

-- 
Regards,
Yi Liu