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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9047e540-a993-473c-9d24-8359cec67b5b@intel.com>
Date: Tue, 20 Aug 2024 15:09:03 +0800
From: Yi Liu <yi.l.liu@...el.com>
To: Lu Baolu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>, "Will
 Deacon" <will@...nel.org>, Robin Murphy <robin.murphy@....com>, "Jason
 Gunthorpe" <jgg@...pe.ca>, Kevin Tian <kevin.tian@...el.com>
CC: <iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] iommu/vt-d: Unconditionally flush device TLB for
 pasid table updates

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@...el.com>
> Signed-off-by: Lu Baolu <baolu.lu@...ux.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