Re: [PATCH v3 4/7] iommu/vt-d: Prepare for global static identity domain

From: Baolu Lu
Date: Wed Aug 07 2024 - 09:44:57 EST


On 2024/8/7 20:17, Jason Gunthorpe wrote:
On Wed, Aug 07, 2024 at 02:41:39PM +0800, Baolu Lu wrote:
On 2024/8/7 1:12, Jason Gunthorpe wrote:
On Tue, Aug 06, 2024 at 10:39:38AM +0800, Lu Baolu wrote:
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index c019fb3b3e78..f37c8c3cba3c 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1270,6 +1270,9 @@ void domain_update_iotlb(struct dmar_domain *domain)
bool has_iotlb_device = false;
unsigned long flags;
+ if (!domain)
+ return;
+
This seems really strange, maybe wrong..

The only callers that could take advantage are
iommu_enable_pci_caps()/iommu_disable_pci_caps()
Yes.

When the PCI ATS status changes, the domain attached to the device
should have its domain->has_iotlb_device flag updated.

The global static identity domain is a dummy domain without a
corresponding dmar_domain structure. Consequently, the device's
info->domain will be NULL. This is why a check is necessary.
I get it, but you can't have ATS turned on at all if you can push the
invalidations. So it seems like something is missing to enforce that
with the identity domains.

So I looked at this and, uh, who even reads domain->has_iotlb_device ?
The has_iotlb_device flag indicates whether a domain is attached to
devices with ATS enabled. If a domain lacks this flag, no device TBLs
need to be invalidated during unmap operations. This optimization avoids
unnecessary looping through all attached devices.
Not any more, that was removed in commit 06792d067989 ("iommu/vt-d:
Cleanup use of iommu_flush_iotlb_psi()")

Yeah! How stupid I was! It's actually a dead code after the
implementation of cache tags.


This compiles, so you should do this instead:

Yes. I will cleanup this in a separated patch.

Thanks,
baolu