iommu/vt-d: Support enforce_cache_coherency only for empty domains
Subsystems (2)
Lu Baolu
maintainerPATCH 1/1 iommu/vt-d: Support enforce_cache_coherency only for empty domains
The enforce_cache_coherency callback ensures DMA cache coherency for devices attached to the domain.
Intel IOMMU supports enforced DMA cache coherency when the Snoop Control bit in the IOMMU's extended capability register is set. Supporting it differs between legacy and scalable modes.
In legacy mode, it's supported page-level by setting the SNP field in second-stage page-table entries. In scalable mode, it's supported in PASID-table granularity by setting the PGSNP field in PASID-table entries.
In legacy mode, mappings before attaching to a device have SNP fields cleared, while mappings after the callback have them set. This means partial DMAs are cache coherent while others are not.
One possible fix is replaying mappings and flipping SNP bits when attaching a domain to a device. But this seems to be over-engineered, given that all real use cases just attach an empty domain to a device.
To meet practical needs while reducing mode differences, only support enforce_cache_coherency on a domain without mappings if SNP field is used.
--- drivers/iommu/intel/iommu.h | 3 +++ drivers/iommu/intel/iommu.c | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-)
drivers/iommu/intel/iommu.h +3 -0
@@ -602,6 +602,9 @@ struct dmar_domain {*/ u8 dirty_tracking:1; /* Dirty tracking is enabled */ u8 nested_parent:1; /* Has other domains nested on it */ + u8 has_mappings:1; /* Has mappings configured through +* iommu_map() interface. +*/ spinlock_t lock;/* Protect device tracking lists */ struct list_head devices;/* all devices' list */
drivers/iommu/intel/iommu.c +4 -1
@@ -2207,6 +2207,8 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn, attr |=DMA_FL_PTE_DIRTY ; } + domain->has_mappings = true; + pteval = ((phys_addr_t )phys_pfn <<VTD_PAGE_SHIFT ) | attr;while (nr_pages >0 ) {
@@ -4360,7 +4362,8 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)return true;spin_lock_irqsave (&dmar_domain->lock, flags); -if (!domain_support_force_snooping (dmar_domain)) { +if (!domain_support_force_snooping (dmar_domain) || + (!dmar_domain->use_first_level && dmar_domain->has_mappings)) {spin_unlock_irqrestore (&dmar_domain->lock, flags);return false; }
Signature · 1 line
2.34.1
Lu Baolu
maintainerPATCH 1/1 iommu/vt-d: Omit devTLB invalidation requests when TES=0
The latest VT-d spec indicates that when remapping hardware is disabled (TES=0 in Global Status Register), upstream ATS Invalidation Completion requests are treated as UR (Unsupported Request).
Consequently, the spec recommends in section 4.3 Handling of Device-TLB Invalidations that software refrain from submitting any Device-TLB invalidation requests when address remapping hardware is disabled.
Verify address remapping hardware is enabled prior to submitting Device- TLB invalidation requests.
--- drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/iommu/intel/dmar.c +18 -0
@@ -1522,6 +1522,15 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, {struct qi_desc desc; +/* +* VT-d spec, section 4.3: +* +* Software is recommended to not submit any Device-TLB invalidation +* requests while address remapping hardware is disabled. +*/ +if (!(iommu->gcmd &DMA_GCMD_TE )) +return ; +if (mask) { addr |= (1ULL << (VTD_PAGE_SHIFT + mask -1 )) -1 ; desc.qw1 =QI_DEV_IOTLB_ADDR (addr) |QI_DEV_IOTLB_SIZE ;
@@ -1587,6 +1596,15 @@ void qi_flush_dev_iotlb_pasid(struct intel_iommu *iommu, u16 sid, u16 pfsid,unsigned long mask =1UL << (VTD_PAGE_SHIFT + size_order -1 );struct qi_desc desc = {.qw1 =0 , .qw2 =0 , .qw3 =0 }; +/* +* VT-d spec, section 4.3: +* +* Software is recommended to not submit any Device-TLB invalidation +* requests while address remapping hardware is disabled. +*/ +if (!(iommu->gcmd &DMA_GCMD_TE )) +return ; + desc.qw0 =QI_DEV_EIOTLB_PASID (pasid) |QI_DEV_EIOTLB_SID (sid) |QI_DEV_EIOTLB_QDEP (qdep) |QI_DEIOTLB_TYPE |QI_DEV_IOTLB_PFSID (pfsid);
Signature · 1 line
2.34.1
RE: [PATCH 1/1] iommu/vt-d: Omit devTLB invalidation requests when TES=0
Quoted 35 lines
From: Lu Baolu <[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMThe latest VT-d spec indicates that when remapping hardware is disabled (TES=0 in Global Status Register), upstream ATS Invalidation Completion requests are treated as UR (Unsupported Request).
Consequently, the spec recommends in section 4.3 Handling of Device-TLB Invalidations that software refrain from submitting any Device-TLB invalidation requests when address remapping hardware is disabled.
Verify address remapping hardware is enabled prior to submitting Device- TLB invalidation requests.
Fixes 792fb43ce2c9 ("iommu/vt-d: Enable Intel IOMMU scalable mode bydefault")
Signed-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/iommu/intel/dmar.c +4
-0@@ -1522,6 +1522,15 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, {struct qi_desc desc; +/* +* VT-d spec, section 4.3: +* +* Software is recommended to not submit any Device-TLB invalidation
drivers/iommu/intel/dmar.c void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, · line 1533
Quoted 5 lines
drivers/iommu/intel/dmar.c +5
-0+* requests while address remapping hardware is disabled. +*/ +if (!(iommu->gcmd &DMA_GCMD_TE )) +return ; +
Is it a valid case to see such request when the iommu is disabled? If not then let's add a WARN.
Baolu Lu
maintainerRe: [PATCH 1/1] iommu/vt-d: Omit devTLB invalidation requests when TES=0
Quoted 30 lines · Kevin
Quoted 30 lines
From: Lu Baolu<[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMThe latest VT-d spec indicates that when remapping hardware is disabled (TES=0 in Global Status Register), upstream ATS Invalidation Completion requests are treated as UR (Unsupported Request).
Consequently, the spec recommends in section 4.3 Handling of Device-TLB Invalidations that software refrain from submitting any Device-TLB invalidation requests when address remapping hardware is disabled.
Verify address remapping hardware is enabled prior to submitting Device- TLB invalidation requests.
Fixes 792fb43ce2c9 ("iommu/vt-d: Enable Intel IOMMU scalable mode bydefault")
Signed-off-by Lu Baolu<[email protected]>--- drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/iommu/intel/dmar.c +0
-0@@ -1522,6 +1522,15 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, {struct qi_desc desc;
Quoted 12 lines
drivers/iommu/intel/dmar.c void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, · line 1533
Quoted 10 lines
drivers/iommu/intel/dmar.c +4
-0+/* +* VT-d spec, section 4.3: +* +* Software is recommended to not submit any Device-TLB invalidation
drivers/iommu/intel/dmar.c +5
-0+* requests while address remapping hardware is disabled. +*/ +if (!(iommu->gcmd &DMA_GCMD_TE )) +return ; +Is it a valid case to see such request when the iommu is disabled? If not then let's add a WARN.
There might be valid cases. The VT-d translation is turned on after all devices get probed.
Best regards,
baolu
RE: [PATCH 1/1] iommu/vt-d: Omit devTLB invalidation requests when TES=0
Quoted 47 lines
From: Baolu Lu <[email protected]>
Sent: Tuesday, November 14, 2023 11:14 AMQuoted 30 lines · Kevin
Quoted 30 lines
From: Lu Baolu<[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMThe latest VT-d spec indicates that when remapping hardware is disabled (TES=0 in Global Status Register), upstream ATS Invalidation Completion requests are treated as UR (Unsupported Request).
Consequently, the spec recommends in section 4.3 Handling of Device-TLB Invalidations that software refrain from submitting any Device-TLB invalidation requests when address remapping hardware is disabled.
Verify address remapping hardware is enabled prior to submitting Device- TLB invalidation requests.
Fixes 792fb43ce2c9 ("iommu/vt-d: Enable Intel IOMMU scalable mode bydefault")
Signed-off-by Lu Baolu<[email protected]>--- drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/iommu/intel/dmar.c +0
-0@@ -1522,6 +1522,15 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, {struct qi_desc desc;Quoted 12 lines
drivers/iommu/intel/dmar.c void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, · line 1533
Quoted 10 lines
drivers/iommu/intel/dmar.c +4
-0+/* +* VT-d spec, section 4.3: +* +* Software is recommended to not submit any Device-TLB invalidation
drivers/iommu/intel/dmar.c +5
-0+* requests while address remapping hardware is disabled. +*/ +if (!(iommu->gcmd &DMA_GCMD_TE )) +return ; +Is it a valid case to see such request when the iommu is disabled? If not then let's add a WARN.
Quoted 3 lines
There might be valid cases. The VT-d translation is turned on after all devices get probed.
but I didn't get why there will be actual mapping changes before vtd translation is enabled...
Baolu Lu
maintainerRe: [PATCH 1/1] iommu/vt-d: Omit devTLB invalidation requests when TES=0
Quoted 47 lines · Kevin
Quoted 47 lines
From: Baolu Lu <[email protected]>
Sent: Tuesday, November 14, 2023 11:14 AMQuoted 42 lines · Kevin
Quoted 35 lines
From: Lu Baolu<[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMThe latest VT-d spec indicates that when remapping hardware is disabled (TES=0 in Global Status Register), upstream ATS Invalidation Completion requests are treated as UR (Unsupported Request).
Consequently, the spec recommends in section 4.3 Handling of Device-TLB Invalidations that software refrain from submitting any Device-TLB invalidation requests when address remapping hardware is disabled.
Verify address remapping hardware is enabled prior to submitting Device- TLB invalidation requests.
Fixes 792fb43ce2c9 ("iommu/vt-d: Enable Intel IOMMU scalable mode bydefault")
Signed-off-by Lu Baolu<[email protected]>--- drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/iommu/intel/dmar.c +4
-0@@ -1522,6 +1522,15 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, {struct qi_desc desc; +/* +* VT-d spec, section 4.3: +* +* Software is recommended to not submit any Device-TLB invalidation
drivers/iommu/intel/dmar.c void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, · line 1533
Quoted 5 lines
drivers/iommu/intel/dmar.c +5
-0+* requests while address remapping hardware is disabled. +*/ +if (!(iommu->gcmd &DMA_GCMD_TE )) +return ; +Is it a valid case to see such request when the iommu is disabled? If not then let's add a WARN.
Quoted 6 lines
Quoted 3 lines
There might be valid cases. The VT-d translation is turned on after all devices get probed.
but I didn't get why there will be actual mapping changes before vtd translation is enabled...
For an example, in iommu_create_device_direct_mappings(), iommu_flush_iotlb_all() is called after direct mappings are created.
Best regards,
baolu
Tian, Kevin
Reviewed-byRE: [PATCH 1/1] iommu/vt-d: Omit devTLB invalidation requests when TES=0
Quoted 59 lines
From: Baolu Lu <[email protected]>
Sent: Tuesday, November 14, 2023 12:55 PMQuoted 8 lines · Kevin
Quoted 8 lines
From: Baolu Lu <[email protected]>
Sent: Tuesday, November 14, 2023 11:14 AMQuoted 4 lines · Kevin
Quoted 4 lines
From: Lu Baolu<[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMThe latest VT-d spec indicates that when remapping hardware is
disabled
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
(TES=0 in Global Status Register), upstream ATS Invalidation Completion requests are treated as UR (Unsupported Request).
Consequently, the spec recommends in section 4.3 Handling of Device-
TLB
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Invalidations that software refrain from submitting any Device-TLB invalidation requests when address remapping hardware is disabled.
Verify address remapping hardware is enabled prior to submitting
Device-
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
TLB invalidation requests.
Fixes 792fb43ce2c9 ("iommu/vt-d: Enable Intel IOMMU scalable modeby
Quoted 32 lines
Quoted 31 lines
Quoted 15 lines
Quoted 15 lines
default")
Signed-off-by Lu Baolu<[email protected]>--- drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/iommu/intel/dmar.c +0
-0@@ -1522,6 +1522,15 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, {struct qi_desc desc;Quoted 12 lines
drivers/iommu/intel/dmar.c void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid, · line 1533
Quoted 10 lines
drivers/iommu/intel/dmar.c +4
-0+/* +* VT-d spec, section 4.3: +* +* Software is recommended to not submit any Device-TLB invalidation
drivers/iommu/intel/dmar.c +5
-0+* requests while address remapping hardware is disabled. +*/ +if (!(iommu->gcmd &DMA_GCMD_TE )) +return ; +Is it a valid case to see such request when the iommu is disabled? If not then let's add a WARN.
There might be valid cases. The VT-d translation is turned on after all devices get probed.
Quoted 6 lines
Quoted 2 lines
but I didn't get why there will be actual mapping changes before vtd translation is enabled...
For an example, in
iommu_create_device_direct_mappings(),iommu_flush_iotlb_all()is called after direct mappings are created.
Okay.
Re: [PATCH 1/1] iommu/vt-d: Omit devTLB invalidation requests when TES=0
In reply to Lu BaoluQuoted 8 lines · Lu Baolu
The latest VT-d spec indicates that when remapping hardware is disabled (TES=0 in Global Status Register), upstream ATS Invalidation Completion requests are treated as UR (Unsupported Request).
Consequently, the spec recommends in section 4.3 Handling of Device-TLB Invalidations that software refrain from submitting any Device-TLB invalidation requests when address remapping hardware is disabled.
Quoted 8 lines
Verify address remapping hardware is enabled prior to submitting Device- TLB invalidation requests.
Fixes 792fb43ce2c9 ("iommu/vt-d: Enable Intel IOMMU scalable mode by default")Signed-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
How did you get to the point where flush_dev_iotlb could even be called if the iommu has somehow been globally disabled?
Shouldn't the attach of the domain compeltely fail if the HW is disabled?
If the domain is not attached to anything why would flushing happen?
Jason
Baolu Lu
maintainerRe: [PATCH 1/1] iommu/vt-d: Omit devTLB invalidation requests when TES=0
Quoted 17 lines · Jason Gunthorpe
Quoted 8 lines · Lu Baolu
The latest VT-d spec indicates that when remapping hardware is disabled (TES=0 in Global Status Register), upstream ATS Invalidation Completion requests are treated as UR (Unsupported Request).
Consequently, the spec recommends in section 4.3 Handling of Device-TLB Invalidations that software refrain from submitting any Device-TLB invalidation requests when address remapping hardware is disabled.
Quoted 8 lines
Verify address remapping hardware is enabled prior to submitting Device- TLB invalidation requests.
Fixes 792fb43ce2c9 ("iommu/vt-d: Enable Intel IOMMU scalable mode by default")Signed-off-by Lu Baolu<[email protected]>--- drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Quoted 7 lines
How did you get to the point where
flush_dev_iotlbcould even be called if the iommu has somehow been globally disabled?Shouldn't the attach of the domain compeltely fail if the HW is disabled?
If the domain is not attached to anything why would flushing happen?
The VT-d hardware can be in a state where the hardware is on but DMA translation is deactivated. In this state, the device probe process during boot proceeds as follows:
1) Initialize the IOMMU contexts: This sets up the data structures that the IOMMU uses to manage address translation for DMA operations.
2) Register the IOMMU devices: This registers the IOMMU devices to the core. The core then probes devices on buses like PCI.
3) Enable DMA translation: This step activates DMA translation.
With regard to step 2), the call to iommu_flush_iotlb_all() in iommu_create_device_direct_mappings() can potentially cause device TBL invalidation when the VT-d DMA translation is deactivated.
Best regards,
baolu
Re: [PATCH 1/1] iommu/vt-d: Omit devTLB invalidation requests when TES=0
Quoted 33 lines · Baolu Lu
Quoted 17 lines · Jason Gunthorpe
Quoted 8 lines · Lu Baolu
The latest VT-d spec indicates that when remapping hardware is disabled (TES=0 in Global Status Register), upstream ATS Invalidation Completion requests are treated as UR (Unsupported Request).
Consequently, the spec recommends in section 4.3 Handling of Device-TLB Invalidations that software refrain from submitting any Device-TLB invalidation requests when address remapping hardware is disabled.
Quoted 8 lines
Verify address remapping hardware is enabled prior to submitting Device- TLB invalidation requests.
Fixes 792fb43ce2c9 ("iommu/vt-d: Enable Intel IOMMU scalable mode by default")Signed-off-by Lu Baolu<[email protected]>--- drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)Quoted 7 lines
How did you get to the point where
flush_dev_iotlbcould even be called if the iommu has somehow been globally disabled?Shouldn't the attach of the domain compeltely fail if the HW is disabled?
If the domain is not attached to anything why would flushing happen?
The VT-d hardware can be in a state where the hardware is on but DMA translation is deactivated. In this state, the device probe process during boot proceeds as follows:
1) Initialize the IOMMU contexts: This sets up the data structures that the IOMMU uses to manage address translation for DMA operations.
Quoted 8 lines
2) Register the IOMMU devices: This registers the IOMMU devices to the core. The core then probes devices on buses like PCI.
3) Enable DMA translation: This step activates DMA translation.
With regard to step 2), the call to
iommu_flush_iotlb_all()iniommu_create_device_direct_mappings()can potentially cause device TBL invalidation when the VT-d DMA translation is deactivated.
You are trying to create an atomic change at boot from non-translating to DMA translating for HW that doesn't support the identity mode?
This should probably get a comment in this patch..
Jason
Lu Baolu
maintainerPATCH 1/1 iommu/vt-d: Disable PCI ATS in legacy passthrough mode
In reply to Lu BaoluWhen IOMMU hardware operates in legacy mode, the TT field of the context entry determines the translation type, with three supported types (Section 9.3 Context Entry):
- DMA translation without device TLB support
- DMA translation with device TLB support
- Passthrough mode with translated and translation requests blocked
Device TLB support is absent when hardware is configured in passthrough mode.
Disable the PCI ATS feature when IOMMU is configured for passthrough translation type in legacy (non-scalable) mode.
--- drivers/iommu/intel/iommu.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/iommu/intel/iommu.c +4 -0
@@ -1413,6 +1413,10 @@ static void iommu_enable_pci_caps(struct device_domain_info *info)if (!dev_is_pci (info->dev))return ; +if (!sm_supported (info->iommu) && info->domain && +domain_type_is_si (info->domain)) +return ; + pdev =to_pci_dev (info->dev);/* The PCIe spec, in its wisdom, declares that the behaviour of
Signature · 1 line
2.34.1
RE: [PATCH 1/1] iommu/vt-d: Disable PCI ATS in legacy passthrough mode
Quoted 11 lines
From: Lu Baolu <[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMWhen IOMMU hardware operates in legacy mode, the TT field of the context entry determines the translation type, with three supported types (Section 9.3 Context Entry):
- DMA translation without device TLB support
- DMA translation with device TLB support
- Passthrough mode with translated and translation requests blocked
Quoted 8 lines
Device TLB support is absent when hardware is configured in passthrough mode.
Disable the PCI ATS feature when IOMMU is configured for passthrough translation type in legacy (non-scalable) mode.
Fixes 0faa19a1515f ("iommu/vt-d: Decouple PASID & PRI enabling from SVA")Signed-off-by Lu Baolu <[email protected]>
Baolu Lu
maintainerRe: [PATCH 1/1] iommu/vt-d: Disable PCI ATS in legacy passthrough mode
In reply to Lu BaoluQuoted 25 lines · Lu Baolu
When IOMMU hardware operates in legacy mode, the TT field of the context entry determines the translation type, with three supported types (Section 9.3 Context Entry):
- DMA translation without device TLB support
- DMA translation with device TLB support
- Passthrough mode with translated and translation requests blockedDevice TLB support is absent when hardware is configured in passthrough mode.
Disable the PCI ATS feature when IOMMU is configured for passthrough translation type in legacy (non-scalable) mode.
Fixes 0faa19a1515f ("iommu/vt-d: Decouple PASID & PRI enabling from SVA")Signed-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel/iommu.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/iommu/intel/iommu.c +0
-0
drivers/iommu/intel/iommu.c static void iommu_enable_pci_caps(struct device_domain_info *info) · line 1422
Quoted 10 lines
drivers/iommu/intel/iommu.c +4
-0@@ -1413,6 +1413,10 @@ static void iommu_enable_pci_caps(struct device_domain_info *info)if (!dev_is_pci (info->dev))return ; +if (!sm_supported (info->iommu) && info->domain && +domain_type_is_si (info->domain)) +return ; + pdev =to_pci_dev (info->dev);/* The PCIe spec, in its wisdom, declares that the behaviour of
Perhaps we could move the check into the caller and make this helper transparent to the iommu mode and domain type?
drivers/iommu/intel/iommu.c +2 -1
@@ -2492,7 +2492,8 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,return ret; } -iommu_enable_pci_caps (info); +if (sm_supported (info->iommu) || !domain_type_is_si (info->domain)) +iommu_enable_pci_caps (info);return 0 ; }
Best regards,
baolu
RE: [PATCH 1/1] iommu/vt-d: Disable PCI ATS in legacy passthrough mode
Quoted 51 lines
From: Baolu Lu <[email protected]>
Sent: Thursday, November 16, 2023 3:35 PMQuoted 1 line · Lu Baolu
When IOMMU hardware operates in legacy mode, the TT field of the
context
Quoted 7 lines
entry determines the translation type, with three supported types (Section 9.3 Context Entry):
- DMA translation without device TLB support
- DMA translation with device TLB support
- Passthrough mode with translated and translation requests blockedQuoted 7 lines
Device TLB support is absent when hardware is configured in passthrough mode.
Disable the PCI ATS feature when IOMMU is configured for passthrough translation type in legacy (non-scalable) mode.
Fixes 0faa19a1515f ("iommu/vt-d: Decouple PASID & PRI enabling fromSVA")
drivers/iommu/intel/iommu.c static void iommu_enable_pci_caps(struct
Quoted 10 lines
Signed-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel/iommu.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/iommu/intel/iommu.c +0
-0@@ -1413,6 +1413,10 @@ static void iommu_enable_pci_caps(struct
device_domain_info*info)drivers/iommu/intel/iommu.c static void iommu_enable_pci_caps(struct · line 1422
Quoted 10 lines
drivers/iommu/intel/iommu.c +4
-0if (!dev_is_pci (info->dev))return ; +if (!sm_supported (info->iommu) && info->domain && +domain_type_is_si (info->domain)) +return ; + pdev =to_pci_dev (info->dev);/* The PCIe spec, in its wisdom, declares that the behaviour of Perhaps we could move the check into the caller and make this helper transparent to the iommu mode and domain type?
drivers/iommu/intel/iommu.c +0
-0
drivers/iommu/intel/iommu.c static int dmar_domain_attach_device(struct · line 2498
Quoted 8 lines
drivers/iommu/intel/iommu.c +2
-1@@ -2492,7 +2492,8 @@ static int dmar_domain_attach_device(structdmar_domain *domain, return ret; } -iommu_enable_pci_caps (info); +if (sm_supported (info->iommu) || !domain_type_is_si (info->domain)) +iommu_enable_pci_caps (info);
IMHO both old and this new version are confusing regarding to that the commit msg talks only about ATS but the actual code disable all pci caps. It's correct, being that only ATS is relevant in legacy mode, but the readability is not good.
what about introducing a helper e.g. device_domain_ats_supported(info) which includes above checks plus info->ats_supported and then use it to replace info->ats_supported in iommu_enable_pci_caps()?
Baolu Lu
maintainerRe: [PATCH 1/1] iommu/vt-d: Disable PCI ATS in legacy passthrough mode
Quoted 55 lines · Kevin
Quoted 55 lines
From: Baolu Lu <[email protected]>
Sent: Thursday, November 16, 2023 3:35 PMQuoted 1 line · Lu Baolu
When IOMMU hardware operates in legacy mode, the TT field of the
context
Quoted 7 lines
entry determines the translation type, with three supported types (Section 9.3 Context Entry):
- DMA translation without device TLB support
- DMA translation with device TLB support
- Passthrough mode with translated and translation requests blockedQuoted 7 lines
Device TLB support is absent when hardware is configured in passthrough mode.
Disable the PCI ATS feature when IOMMU is configured for passthrough translation type in legacy (non-scalable) mode.
Fixes 0faa19a1515f ("iommu/vt-d: Decouple PASID & PRI enabling fromSVA")
drivers/iommu/intel/iommu.c static void iommu_enable_pci_caps(struct
Quoted 10 lines
Signed-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel/iommu.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/iommu/intel/iommu.c +0
-0@@ -1413,6 +1413,10 @@ static void iommu_enable_pci_caps(struct
device_domain_info*info)drivers/iommu/intel/iommu.c static void iommu_enable_pci_caps(struct · line 1422
Quoted 10 lines
drivers/iommu/intel/iommu.c +4
-0if (!dev_is_pci (info->dev))return ; +if (!sm_supported (info->iommu) && info->domain && +domain_type_is_si (info->domain)) +return ; + pdev =to_pci_dev (info->dev);/* The PCIe spec, in its wisdom, declares that the behaviour of Perhaps we could move the check into the caller and make this helper transparent to the iommu mode and domain type?
drivers/iommu/intel/iommu.c +0
-0@@ -2492,7 +2492,8 @@ static int dmar_domain_attach_device(structdmar_domain *domain, return ret; }
Quoted 9 lines
drivers/iommu/intel/iommu.c static int dmar_domain_attach_device(struct dmar_domain *domain, · line 2497
Quoted 4 lines
drivers/iommu/intel/iommu.c +2
-1-iommu_enable_pci_caps (info); +if (sm_supported (info->iommu) || !domain_type_is_si (info->domain)) +iommu_enable_pci_caps (info);IMHO both old and this new version are confusing regarding to that the commit msg talks only about ATS but the actual code disable all pci caps. It's correct, being that only ATS is relevant in legacy mode, but the readability is not good.
The function name is a bit misleading, but its actual purpose is to enable the ATS feature. PASID is enabled within this function due to a specification requirement that prevents modifications to PASID registers after ATS activation.
Quoted 5 lines
what about introducing a helper e.g.
device_domain_ats_supported(info)which includes above checks plusinfo->ats_supportedand then use it to replaceinfo->ats_supportediniommu_enable_pci_caps()?
I have a patch series under test which moves the ATS control out to the device drivers or users (through sysfs node). With that done, we could simply remove all these and give the control to device drivers or users.
How about considering above after I post that series? And for now, we can make the fix patch quick and simple.
Best regards,
baolu
RE: [PATCH 1/1] iommu/vt-d: Disable PCI ATS in legacy passthrough mode
Quoted 84 lines
From: Baolu Lu <[email protected]>
Sent: Friday, November 17, 2023 9:10 AMQuoted 7 lines · Kevin
Quoted 7 lines
From: Baolu Lu <[email protected]>
Sent: Thursday, November 16, 2023 3:35 PMQuoted 1 line · Lu Baolu
When IOMMU hardware operates in legacy mode, the TT field of the
context
Quoted 1 line
entry determines the translation type, with three supported types
(Section
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
9.3 Context Entry):
- DMA translation without device TLB support
- DMA translation with device TLB support
- Passthrough mode with translated and translation requests blockedDevice TLB support is absent when hardware is configured in
passthrough
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
mode.
Disable the PCI ATS feature when IOMMU is configured for passthrough translation type in legacy (non-scalable) mode.
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Fixes 0faa19a1515f ("iommu/vt-d: Decouple PASID & PRI enabling fromSVA")
Review
Quoted 6 lines
Signed-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel/iommu.c | 4 ++++ 1 file changed, 4 insertions(+)(quoted diff) +0
-0
b/drivers/iommu/intel/iommu.cQuoted 31 lines
Quoted 24 lines
drivers/iommu/intel/iommu.c static void iommu_enable_pci_caps(struct
Quoted 4 lines
index
11670cd812a3..c3ec09118ab1100644drivers/iommu/intel/iommu.c +0
-0@@ -1413,6 +1413,10 @@ static void iommu_enable_pci_caps(struct
device_domain_info*info)drivers/iommu/intel/iommu.c static void iommu_enable_pci_caps(struct · line 1422
Quoted 10 lines
drivers/iommu/intel/iommu.c +4
-0if (!dev_is_pci (info->dev))return ; +if (!sm_supported (info->iommu) && info->domain && +domain_type_is_si (info->domain)) +return ; + pdev =to_pci_dev (info->dev);/* The PCIe spec, in its wisdom, declares that the behaviour of Perhaps we could move the check into the caller and make this helper transparent to the iommu mode and domain type?
drivers/iommu/intel/iommu.c +0
-0drivers/iommu/intel/iommu.c static int dmar_domain_attach_device(struct · line 2496
Quoted 6 lines
drivers/iommu/intel/iommu.c +1
-1@@ -2492,7 +2492,8 @@ static int dmar_domain_attach_device(structdmar_domain *domain, return ret; } -iommu_enable_pci_caps (info); + if (sm_supported(info->iommu) || !domain_type_is_si(info-domain))
Quoted 7 lines
drivers/iommu/intel/iommu.c static int dmar_domain_attach_device(struct dmar_domain *domain, · line 2497
Quoted 2 lines
drivers/iommu/intel/iommu.c +1
-0+iommu_enable_pci_caps (info);IMHO both old and this new version are confusing regarding to that the commit msg talks only about ATS but the actual code disable all pci caps. It's correct, being that only ATS is relevant in legacy mode, but the readability is not good.
The function name is a bit misleading, but its actual purpose is to enable the ATS feature. PASID is enabled within this function due to a specification requirement that prevents modifications to PASID registers after ATS activation.
Quoted 5 lines
what about introducing a helper e.g.
device_domain_ats_supported(info)which includes above checks plusinfo->ats_supportedand then use it to replaceinfo->ats_supportediniommu_enable_pci_caps()?
Quoted 7 lines
I have a patch series under test which moves the ATS control out to the device drivers or users (through sysfs node). With that done, we could simply remove all these and give the control to device drivers or users.
How about considering above after I post that series? And for now, we can make the fix patch quick and simple.
Then please add a comment for the rationale.
Re: [PATCH 1/1] iommu/vt-d: Disable PCI ATS in legacy passthrough mode
In reply to Lu BaoluQuoted 8 lines · Lu Baolu
When IOMMU hardware operates in legacy mode, the TT field of the context entry determines the translation type, with three supported types (Section 9.3 Context Entry):
- DMA translation without device TLB support
- DMA translation with device TLB support
- Passthrough mode with translated and translation requests blocked
Quoted 5 lines
Device TLB support is absent when hardware is configured in passthrough mode.
Disable the PCI ATS feature when IOMMU is configured for passthrough translation type in legacy (non-scalable) mode.
Oh.. That is the same horrible outcome that ARM has :(
The issue is what to do if the RID translation is in identity but a PASID is attached that should be using ATS - eg do you completely loose SVA support if the RID is set to the optimized identity mode?
I vote no. We should make the drivers aware that they should not use ATS on their RIDs instead :(
Jason
Baolu Lu
maintainerRe: [PATCH 1/1] iommu/vt-d: Disable PCI ATS in legacy passthrough mode
Quoted 12 lines · Jason Gunthorpe
Quoted 11 lines · Lu Baolu
When IOMMU hardware operates in legacy mode, the TT field of the context entry determines the translation type, with three supported types (Section 9.3 Context Entry):
- DMA translation without device TLB support
- DMA translation with device TLB support
- Passthrough mode with translated and translation requests blockedDevice TLB support is absent when hardware is configured in passthrough mode.
Quoted 7 lines
Quoted 2 lines
Disable the PCI ATS feature when IOMMU is configured for passthrough translation type in legacy (non-scalable) mode.
Oh.. That is the same horrible outcome that ARM has 🙁
The issue is what to do if the RID translation is in identity but a PASID is attached that should be using ATS - eg do you completely loose SVA support if the RID is set to the optimized identity mode?
This fix only affects the non-scalable mode that doesn't support PASID
features.
Quoted 2 lines
I vote no. We should make the drivers aware that they should not use ATS on their RIDs instead 🙁
Intel VT-d hardware does not supported ATS in non-scalable mode when translation is set to passthrough mode. Historically, the Intel IOMMU driver has never enabled the ATS feature in this configuration.
Commit 0faa19a1515f changed this behavior by accident, potentially leading to incorrect hardware configuration. This patch fixes the issue by reverting to the previous behavior. Otherwise, it leads to hardware configuration errors and potential DMA malfunction, as all translated DMA requests would be blocked by the IOMMU.
Going forward, I agree that device drivers should have the ability to access and potentially control the ATS status. I have a upcoming series that will enable device drivers or user-space to manage the ATS as we have discussed in other threads before.
Best regards,
baolu
Re: [PATCH 1/1] iommu/vt-d: Disable PCI ATS in legacy passthrough mode
Quoted 15 lines · Baolu Lu
Quoted 14 lines · Jason Gunthorpe
Quoted 13 lines · Lu Baolu
When IOMMU hardware operates in legacy mode, the TT field of the context entry determines the translation type, with three supported types (Section 9.3 Context Entry):
- DMA translation without device TLB support
- DMA translation with device TLB support
- Passthrough mode with translated and translation requests blockedDevice TLB support is absent when hardware is configured in passthrough mode.
Disable the PCI ATS feature when IOMMU is configured for passthrough translation type in legacy (non-scalable) mode.
Quoted 8 lines
Quoted 5 lines
Oh.. That is the same horrible outcome that ARM has 🙁
The issue is what to do if the RID translation is in identity but a PASID is attached that should be using ATS - eg do you completely loose SVA support if the RID is set to the optimized identity mode?
This fix only affects the non-scalable mode that doesn't support PASID
features.
Ah, OK so it is OK. I'm glad the new mode supports ATS against passthrough.
Thanks,
Jason
Lu Baolu
maintainerPATCH 1/1 iommu/vt-d: Make context clearing consistent with context mapping
In reply to Lu BaoluIn the iommu probe_device path, domain_context_mapping() allows setting up the context entry for a non-PCI device. However, in the iommu release_device path, domain_context_clear() only clears context entries for PCI devices.
Make domain_context_clear() behave consistently with domain_context_mapping() by clearing context entries for both PCI and non-PCI devices.
--- drivers/iommu/intel/iommu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/iommu/intel/iommu.c +2 -2
@@ -3931,8 +3931,8 @@ static int domain_context_clear_one_cb(struct pci_dev *pdev, u16 alias, void *op*/ static void domain_context_clear (struct device_domain_info *info) { -if (!info->iommu || !info->dev || !dev_is_pci (info->dev)) -return ; +if (!dev_is_pci (info->dev)) +domain_context_clear_one (info, info->bus, info->devfn);pci_for_each_dma_alias (to_pci_dev (info->dev), &domain_context_clear_one_cb, info);
Signature · 1 line
2.34.1
RE: [PATCH 1/1] iommu/vt-d: Make context clearing consistent with context mapping
Quoted 8 lines
From: Lu Baolu <[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMIn the iommu
probe_devicepath,domain_context_mapping()allows setting up the context entry for a non-PCI device. However, in the iommurelease_devicepath,domain_context_clear()only clears context entries for PCI devices.
Quoted 6 lines
Make
domain_context_clear()behave consistently withdomain_context_mapping()by clearing context entries for both PCI and non-PCI devices.Fixes 579305f75d34 ("iommu/vt-d: Update to use PCI DMA aliases")Signed-off-by Lu Baolu <[email protected]>
The code before the fix tag also has the same problem. If we really want backport then let's find out the very first commit which exhibits this problem.
But I wonder the actual impact w/o such fix. If there is no hot-remove possible for those non-PCI devices the context entry will be leaved enabled until the machine is off. Then this fix is nice-to-have then probably no need to backport?
Baolu Lu
maintainerRe: [PATCH 1/1] iommu/vt-d: Make context clearing consistent with context mapping
Quoted 12 lines · Kevin
Quoted 12 lines
From: Lu Baolu <[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMIn the iommu
probe_devicepath,domain_context_mapping()allows setting up the context entry for a non-PCI device. However, in the iommurelease_devicepath,domain_context_clear()only clears context entries for PCI devices.Make
domain_context_clear()behave consistently withdomain_context_mapping()by clearing context entries for both PCI and non-PCI devices.
Quoted 6 lines
Quoted 2 lines
Fixes 579305f75d34 ("iommu/vt-d: Update to use PCI DMA aliases")Signed-off-by Lu Baolu <[email protected]>The code before the fix tag also has the same problem. If we really want backport then let's find out the very first commit which exhibits this problem.
Commit 579305f75d34 allows non-PCI devices.
+ if (!dev_is_pci(dev))
+ return domain_context_mapping_one(domain, iommu, bus, devfn,
translation); + + data.domain = domain; + data.iommu = iommu; + data.translation = translation;
+
+ return pci_for_each_dma_alias(to_pci_dev(dev),
+ &domain_context_mapping_cb, &data);
But it forgot to update the domain_context_clear() helper. So this is actually a fix for that commit.
Quoted 5 lines
But I wonder the actual impact w/o such fix. If there is no hot-remove possible for those non-PCI devices the context entry will be leaved enabled until the machine is off. Then this fix is nice-to-have then probably no need to backport?
It doesn't cause real issues as far as I can see. So there's no need to back port it to stable kernels. That's the reason I didn't add cc-stable tag. But we still need a fix tag as it's actually a fix.
Best regards,
baolu
Tian, Kevin
Reviewed-byRE: [PATCH 1/1] iommu/vt-d: Make context clearing consistent with context mapping
Quoted 37 lines
From: Baolu Lu <[email protected]>
Sent: Tuesday, November 14, 2023 11:22 AMQuoted 4 lines · Kevin
Quoted 4 lines
From: Lu Baolu <[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMIn the iommu
probe_devicepath,domain_context_mapping()allowssetting
Quoted 8 lines
Quoted 8 lines
up the context entry for a non-PCI device. However, in the iommu
release_devicepath,domain_context_clear()only clears context entries for PCI devices.Make
domain_context_clear()behave consistently withdomain_context_mapping()by clearing context entries for both PCI and non-PCI devices.Quoted 6 lines
Quoted 2 lines
Fixes 579305f75d34 ("iommu/vt-d: Update to use PCI DMA aliases")Signed-off-by Lu Baolu <[email protected]>The code before the fix tag also has the same problem. If we really want backport then let's find out the very first commit which exhibits this problem.
Commit
579305f75d34allows non-PCI devices.(quoted diff) +9
-0+ if (!dev_is_pci(dev)) + return domain_context_mapping_one(domain, iommu, bus, devfn, translation); + + data.domain = domain; + data.iommu = iommu; + data.translation = translation; + + return pci_for_each_dma_alias(to_pci_dev(dev), + &domain_context_mapping_cb, &data);
Quoted 2 lines
But it forgot to update the
domain_context_clear()helper. So this is actually a fix for that commit.
thanks. I didn't note that side effect from commit msg.
Quoted 11 lines
Quoted 6 lines
But I wonder the actual impact w/o such fix. If there is no hot-remove possible for those non-PCI devices the context entry will be leaved enabled until the machine is off. Then this fix is nice-to-have then probably no need to backport?
It doesn't cause real issues as far as I can see. So there's no need to back port it to stable kernels. That's the reason I didn't add cc-stable tag. But we still need a fix tag as it's actually a fix.
make sense.
Quoted 23 lines
From: Lu Baolu <[email protected]>
Sent: Tuesday, November 14, 2023 9:11 AMThe
enforce_cache_coherencycallback ensures DMA cache coherency for devices attached to the domain.Intel IOMMU supports enforced DMA cache coherency when the Snoop Control bit in the IOMMU's extended capability register is set. Supporting it differs between legacy and scalable modes.
In legacy mode, it's supported page-level by setting the SNP field in second-stage page-table entries. In scalable mode, it's supported in PASID-table granularity by setting the PGSNP field in PASID-table entries.
In legacy mode, mappings before attaching to a device have SNP fields cleared, while mappings after the callback have them set. This means partial DMAs are cache coherent while others are not.
One possible fix is replaying mappings and flipping SNP bits when attaching a domain to a device. But this seems to be over-engineered, given that all real use cases just attach an empty domain to a device.
Quoted 7 lines
To meet practical needs while reducing mode differences, only support
enforce_cache_coherencyon a domain without mappings if SNP field is used.Fixes fc0051cb9590 ("iommu/vt-d: Check domain force_snooping againstattached devices")
Signed-off-by Lu Baolu <[email protected]>
drivers/iommu/intel/iommu.h struct dmar_domain { · line 607
Quoted 11 lines · Lu Baolu
drivers/iommu/intel/iommu.h +3
-0@@ -602,6 +602,9 @@ struct dmar_domain {*/ u8 dirty_tracking:1; /* Dirty tracking is enabled */ u8 nested_parent:1; /* Has other domains nested on it */ + u8 has_mappings:1; /* Has mappings configured through +* iommu_map() interface. +*/
Is it racey?
The other option is to make iommfd do this and forbid it from switching the enforce_cache_coherency if the IOAS has any maps attached. We can get the correct locking at that point.
AMD has the same issue if it ever wants to implement its per-PTE bit
Jason
Baolu Lu
maintainerQuoted 14 lines · Jason Gunthorpe
drivers/iommu/intel/iommu.h struct dmar_domain { · line 607
Quoted 11 lines · Lu Baolu
drivers/iommu/intel/iommu.h +3
-0@@ -602,6 +602,9 @@ struct dmar_domain {*/ u8 dirty_tracking:1; /* Dirty tracking is enabled */ u8 nested_parent:1; /* Has other domains nested on it */ + u8 has_mappings:1; /* Has mappings configured through +* iommu_map() interface. +*/ Is it racey?
Quoted 5 lines
The other option is to make iommfd do this and forbid it from switching the
enforce_cache_coherencyif the IOAS has any maps attached. We can get the correct locking at that point.AMD has the same issue if it ever wants to implement its per-PTE bit
Yes. It's better to do this in iommufd. With that done, we can then remove this code.
Best regards,
baolu