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

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

[Lu Baolu]

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.

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);
 }
 
 /**
-- 
2.34.1

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

[Yi Liu]

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

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

[Baolu Lu]

On 2024/8/20 15:09, Yi Liu wrote:
> 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?

It depends on whether this solves any real or potential issues.

Thanks,
baolu

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

[Yi Liu]

On 2024/8/20 15:37, Baolu Lu wrote:
> On 2024/8/20 15:09, Yi Liu wrote:
>> 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?
> 
> It depends on whether this solves any real or potential issues.

What about the rule for it? A real issue should already been reported or
identified to happen even no one complained yet due to no related tests. :)

-- 
Regards,
Yi Liu

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

[Baolu Lu]

On 8/20/24 11:02 AM, 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.
> 
> 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(-)

Queued for v6.12-rc1.

Thanks,
baolu