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

[Baolu Lu]

On 3/28/24 3:47 PM, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxx>
> > ---
> >   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