With the static identity domain in place, the domain field of per-device
iommu driver data can be either a pointer to a DMA translation domain, or
NULL, indicating that the static identity domain is attached. Refactor
the code to prepare for this change.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/intel/iommu.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
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;
+
spin_lock_irqsave(&domain->lock, flags);
list_for_each_entry(info, &domain->devices, link) {
if (info->ats_enabled) {
@@ -3706,11 +3709,9 @@ int prepare_domain_attach_device(struct iommu_domain *domain,
static int intel_iommu_attach_device(struct iommu_domain *domain,
struct device *dev)
{
- struct device_domain_info *info = dev_iommu_priv_get(dev);
int ret;
- if (info->domain)
- device_block_translation(dev);
+ device_block_translation(dev);
ret = prepare_domain_attach_device(domain, dev);
if (ret)
@@ -4321,6 +4322,11 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
struct intel_iommu *iommu = info->iommu;
unsigned long flags;
+ if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+ intel_pasid_tear_down_entry(iommu, dev, pasid, false);
+ return;
+ }
+
spin_lock_irqsave(&dmar_domain->lock, flags);
list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
if (curr->dev == dev && curr->pasid == pasid) {
--
2.34.1On 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() But if they are mucking with ATS then the ATC flushes should not be done wrong! So I looked at this and, uh, who even reads domain->has_iotlb_device ? So I'd just delete domain->has_iotlb_device and domain_update_iotlb() as well. Jason
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 might need to make this check explicit with an additional change.
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f37c8c3cba3c..d59e9ac223ba 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1270,9 +1270,6 @@ void domain_update_iotlb(struct dmar_domain *domain)
bool has_iotlb_device = false;
unsigned long flags;
- if (!domain)
- return;
-
spin_lock_irqsave(&domain->lock, flags);
list_for_each_entry(info, &domain->devices, link) {
if (info->ats_enabled) {
@@ -1330,7 +1327,8 @@ static void iommu_enable_pci_caps(struct
device_domain_info *info)
if (info->ats_supported && pci_ats_page_aligned(pdev) &&
!pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
info->ats_enabled = 1;
- domain_update_iotlb(info->domain);
+ if (info->domain)
+ domain_update_iotlb(info->domain);
}
}
@@ -1346,7 +1344,8 @@ static void iommu_disable_pci_caps(struct
device_domain_info *info)
if (info->ats_enabled) {
pci_disable_ats(pdev);
info->ats_enabled = 0;
- domain_update_iotlb(info->domain);
+ if (info->domain)
+ domain_update_iotlb(info->domain);
}
if (info->pasid_enabled) {
>
> But if they are mucking with ATS then the ATC flushes should not be
> done wrong!
>
> 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.
> So I'd just delete domain->has_iotlb_device and domain_update_iotlb()
> as well.
Thanks,
baolu
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()")
This compiles, so you should do this instead:
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index ad81db026ab236..6549488d1f6bb1 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -492,7 +492,6 @@ void domain_update_iommu_cap(struct dmar_domain *domain)
domain->domain.geometry.aperture_end = __DOMAIN_MAX_ADDR(domain->gaw);
domain->domain.pgsize_bitmap |= domain_super_pgsize_bitmap(domain);
- domain_update_iotlb(domain);
}
struct context_entry *iommu_context_addr(struct intel_iommu *iommu, u8 bus,
@@ -1270,32 +1269,6 @@ domain_lookup_dev_info(struct dmar_domain *domain,
return NULL;
}
-void domain_update_iotlb(struct dmar_domain *domain)
-{
- struct dev_pasid_info *dev_pasid;
- struct device_domain_info *info;
- bool has_iotlb_device = false;
- unsigned long flags;
-
- spin_lock_irqsave(&domain->lock, flags);
- list_for_each_entry(info, &domain->devices, link) {
- if (info->ats_enabled) {
- has_iotlb_device = true;
- break;
- }
- }
-
- list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain) {
- info = dev_iommu_priv_get(dev_pasid->dev);
- if (info->ats_enabled) {
- has_iotlb_device = true;
- break;
- }
- }
- domain->has_iotlb_device = has_iotlb_device;
- spin_unlock_irqrestore(&domain->lock, flags);
-}
-
/*
* The extra devTLB flush quirk impacts those QAT devices with PCI device
* IDs ranging from 0x4940 to 0x4943. It is exempted from risky_device()
@@ -1332,10 +1305,8 @@ static void iommu_enable_pci_caps(struct device_domain_info *info)
info->pasid_enabled = 1;
if (info->ats_supported && pci_ats_page_aligned(pdev) &&
- !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
+ !pci_enable_ats(pdev, VTD_PAGE_SHIFT))
info->ats_enabled = 1;
- domain_update_iotlb(info->domain);
- }
}
static void iommu_disable_pci_caps(struct device_domain_info *info)
@@ -1350,7 +1321,6 @@ static void iommu_disable_pci_caps(struct device_domain_info *info)
if (info->ats_enabled) {
pci_disable_ats(pdev);
info->ats_enabled = 0;
- domain_update_iotlb(info->domain);
}
if (info->pasid_enabled) {
@@ -1524,7 +1494,6 @@ static struct dmar_domain *alloc_domain(unsigned int type)
domain->nid = NUMA_NO_NODE;
if (first_level_by_default(type))
domain->use_first_level = true;
- domain->has_iotlb_device = false;
INIT_LIST_HEAD(&domain->devices);
INIT_LIST_HEAD(&domain->dev_pasids);
INIT_LIST_HEAD(&domain->cache_tags);
@@ -3622,7 +3591,6 @@ static struct dmar_domain *paging_domain_alloc(struct device *dev, bool first_st
xa_init(&domain->iommu_array);
domain->nid = dev_to_node(dev);
- domain->has_iotlb_device = info->ats_enabled;
domain->use_first_level = first_stage;
/* calculate the address width */
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index b67c14da12408b..e80aed3bc06e61 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -588,7 +588,6 @@ struct dmar_domain {
int nid; /* node id */
struct xarray iommu_array; /* Attached IOMMU array */
- u8 has_iotlb_device: 1;
u8 iommu_coherency: 1; /* indicate coherency of iommu access */
u8 force_snooping : 1; /* Create IOPTEs with snoop control */
u8 set_pte_snp:1;
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
© 2016 - 2025 Red Hat, Inc.