linux-kernel - Re: [PATCH 04/12] iommu/vt-d: Use cache_tag_flush_all() in flush_iotlb

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]

Date: Sun, 7 Apr 2024 13:56:31 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: "Tian, Kevin" <kevin.tian@...el.com>, Joerg Roedel <joro@...tes.org>,
 Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
 Jason Gunthorpe <jgg@...pe.ca>
Cc: baolu.lu@...ux.intel.com, "Zhang, Tina" <tina.zhang@...el.com>,
 "Liu, Yi L" <yi.l.liu@...el.com>,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 04/12] iommu/vt-d: Use cache_tag_flush_all() in
 flush_iotlb_all

On 3/28/24 3:47 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@...ux.intel.com>
>> Sent: Monday, March 25, 2024 10:17 AM
>>
>> The flush_iotlb_all callback is called by the iommu core to flush
>> all caches for the affected domain. Use cache_tag_flush_all() in
>> this callback.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 21 +--------------------
>>   1 file changed, 1 insertion(+), 20 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 93e4422c9b10..4ce98f23917c 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1560,26 +1560,7 @@ static void parent_domain_flush(struct
>> dmar_domain *domain,
>>
>>   static void intel_flush_iotlb_all(struct iommu_domain *domain)
>>   {
>> -	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>> -	struct iommu_domain_info *info;
>> -	unsigned long idx;
>> -
>> -	xa_for_each(&dmar_domain->iommu_array, idx, info) {
>> -		struct intel_iommu *iommu = info->iommu;
>> -		u16 did = domain_id_iommu(dmar_domain, iommu);
>> -
>> -		if (dmar_domain->use_first_level)
>> -			domain_flush_pasid_iotlb(iommu, dmar_domain, 0,
>> -1, 0);
>> -		else
>> -			iommu->flush.flush_iotlb(iommu, did, 0, 0,
>> -						 DMA_TLB_DSI_FLUSH);
>> -
>> -		if (!cap_caching_mode(iommu->cap))
>> -			iommu_flush_dev_iotlb(dmar_domain, 0,
>> MAX_AGAW_PFN_WIDTH);
>> -	}
>> -
>> -	if (dmar_domain->nested_parent)
>> -		parent_domain_flush(dmar_domain, 0, -1, 0);
>> +	cache_tag_flush_all(to_dmar_domain(domain));
>>   }
>>
> 
> this replacement causes a functional change. Now devtlb is always
> invalidated while old code doesn't do so for caching mode.

You are right.

As my understanding of the VT-d spec, caching mode has nothing to do
with the device TLB, hence we should remove all checks on caching mode
before a device TLB invalidation.

I understand that people are leveraging the cache mode to avoid the
unnecessary VMEXIT from the guest OS since the host iommu unmap() path
ensures that device TLB is also invalidated.

Our direction should be to use the batched queue invalidation requests.
As we are consolidating code to a common place, that becomes much
easier. :-)

> Probably you may want to first clean up all inconsistent devtlb
> invalidation policies for caching mode before going to this series...

Sure thing!

Best regards,
baolu