iommu/vt-d: Fix incorrect cache invalidation for mm notification
Subsystems (3)
Lu Baolu
maintainerPATCH 1/1 iommu/vt-d: Fix incorrect cache invalidation for mm notification
Commit 6bbd42e2df8f ("mmu_notifiers: call invalidate_range() when invalidating TLBs") moved the secondary TLB invalidations into the TLB invalidation functions to ensure that all secondary TLB invalidations happen at the same time as the CPU invalidation and added a flush-all type of secondary TLB invalidation for the batched mode, where a range of [0, -1UL) is used to indicates that the range extends to the end of the address space.
However, using an end address of -1UL caused an overflow in the Intel IOMMU driver, where the end address was rounded up to the next page. As a result, both the IOTLB and device ATC were not invalidated correctly.
Add a flush all helper function and call it when the invalidation range is from 0 to -1UL, ensuring that the entire caches are invalidated correctly.
--- drivers/iommu/intel/svm.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
drivers/iommu/intel/svm.c +26 -0
@@ -216,6 +216,27 @@ static void intel_flush_svm_range(struct intel_svm *svm, unsigned long address,rcu_read_unlock (); } +static void intel_flush_svm_all (struct intel_svm *svm) +{ +struct device_domain_info *info; +struct intel_svm_dev *sdev; + +rcu_read_lock (); +list_for_each_entry_rcu (sdev, &svm->devs, list) { + info =dev_iommu_priv_get (sdev->dev); + +qi_flush_piotlb (sdev->iommu, sdev->did, svm->pasid,0 ,-1UL ,1 ); +if (info->ats_enabled) { +qi_flush_dev_iotlb_pasid (sdev->iommu, sdev->sid, info->pfsid, + svm->pasid, sdev->qdep, +0 ,64 -VTD_PAGE_SHIFT ); +quirk_extra_dev_tlb_flush (info,0 ,64 -VTD_PAGE_SHIFT , + svm->pasid, sdev->qdep); + } + } +rcu_read_unlock (); +} +/* Pages have been freed at this point */ static void intel_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn, struct mm_struct *mm,
@@ -223,6 +244,11 @@ static void intel_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn, { struct intel_svm *svm = container_of(mn, struct intel_svm, notifier); +if (start ==0 && end ==-1UL ) { +intel_flush_svm_all (svm); +return ; + } +intel_flush_svm_range (svm, start, (end - start +PAGE_SIZE -1 ) >>VTD_PAGE_SHIFT ,0 ); }
Signature · 1 line
2.34.1
Quoted 23 lines · Lu Baolu
Commit
6bbd42e2df8f("mmu_notifiers: callinvalidate_range()when invalidating TLBs") moved the secondary TLB invalidations into the TLB invalidation functions to ensure that all secondary TLB invalidations happen at the same time as the CPU invalidation and added a flush-all type of secondary TLB invalidation for the batched mode, where a range of [0, -1UL) is used to indicates that the range extends to the end of the address space.However, using an end address of -1UL caused an overflow in the Intel IOMMU driver, where the end address was rounded up to the next page. As a result, both the IOTLB and device ATC were not invalidated correctly.
Add a flush all helper function and call it when the invalidation range is from 0 to -1UL, ensuring that the entire caches are invalidated correctly.
Fixes 6bbd42e2df8f ("mmu_notifiers: call invalidate_range() when invalidating TLBs")Cc Huang Ying <[email protected]>Cc Alistair Popple <[email protected]>Tested-by Luo Yuzhang <[email protected]> # QATTested-by Tony Zhu <[email protected]> # DSASigned-off-by Lu Baolu <[email protected]>
Quoted 3 lines
--- drivers/iommu/intel/svm.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
This should go to -rc
Jason
Quoted 11 lines · Lu Baolu
Commit
6bbd42e2df8f("mmu_notifiers: callinvalidate_range()when invalidating TLBs") moved the secondary TLB invalidations into the TLB invalidation functions to ensure that all secondary TLB invalidations happen at the same time as the CPU invalidation and added a flush-all type of secondary TLB invalidation for the batched mode, where a range of [0, -1UL) is used to indicates that the range extends to the end of the address space.However, using an end address of -1UL caused an overflow in the Intel IOMMU driver, where the end address was rounded up to the next page. As a result, both the IOTLB and device ATC were not invalidated correctly.
Thanks for catching. This fix looks good so:
However examining the fixes patch again I note that we are calling mmu_notifier_invalidate_range(mm, 0, -1UL) from arch_tlbbatch_add_pending() in arch/x86/include/asm/tlbflush.h.
That seems suboptimal because we would be doing an invalidate all for every page unmap, and as of db6c1f6f236d ("mm/tlbbatch: introduce arch_flush_tlb_batched_pending()") arch_flush_tlb_batched_pending() calls flush_tlb_mm() anyway. So I think we can probably drop the explicit notifier call from arch_flush_tlb_batched_pending().
Will put togeather a patch for that.
- Alistair
Quoted 59 lines
Add a flush all helper function and call it when the invalidation range is from 0 to -1UL, ensuring that the entire caches are invalidated correctly.
Fixes 6bbd42e2df8f ("mmu_notifiers: call invalidate_range() when invalidating TLBs")Cc Huang Ying <[email protected]>Cc Alistair Popple <[email protected]>Tested-by Luo Yuzhang <[email protected]> # QATTested-by Tony Zhu <[email protected]> # DSASigned-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel/svm.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)drivers/iommu/intel/svm.c +26
-0@@ -216,6 +216,27 @@ static void intel_flush_svm_range(struct intel_svm *svm, unsigned long address,rcu_read_unlock (); } +static void intel_flush_svm_all (struct intel_svm *svm) +{ +struct device_domain_info *info; +struct intel_svm_dev *sdev; + +rcu_read_lock (); +list_for_each_entry_rcu (sdev, &svm->devs, list) { + info =dev_iommu_priv_get (sdev->dev); + +qi_flush_piotlb (sdev->iommu, sdev->did, svm->pasid,0 ,-1UL ,1 ); +if (info->ats_enabled) { +qi_flush_dev_iotlb_pasid (sdev->iommu, sdev->sid, info->pfsid, + svm->pasid, sdev->qdep, +0 ,64 -VTD_PAGE_SHIFT ); +quirk_extra_dev_tlb_flush (info,0 ,64 -VTD_PAGE_SHIFT , + svm->pasid, sdev->qdep); + } + } +rcu_read_unlock (); +} +/* Pages have been freed at this point */ static void intel_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn, struct mm_struct *mm,@@ -223,6 +244,11 @@ static void intel_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn, { struct intel_svm *svm = container_of(mn, struct intel_svm, notifier); +if (start ==0 && end ==-1UL ) { +intel_flush_svm_all (svm); +return ; + } +intel_flush_svm_range (svm, start, (end - start +PAGE_SIZE -1 ) >>VTD_PAGE_SHIFT ,0 ); }
Quoted 16 lines · Alistair Popple
Quoted 11 lines · Lu Baolu
Commit
6bbd42e2df8f("mmu_notifiers: callinvalidate_range()when invalidating TLBs") moved the secondary TLB invalidations into the TLB invalidation functions to ensure that all secondary TLB invalidations happen at the same time as the CPU invalidation and added a flush-all type of secondary TLB invalidation for the batched mode, where a range of [0, -1UL) is used to indicates that the range extends to the end of the address space.However, using an end address of -1UL caused an overflow in the Intel IOMMU driver, where the end address was rounded up to the next page. As a result, both the IOTLB and device ATC were not invalidated correctly.
Thanks for catching. This fix looks good so:
Quoted 8 lines
Reviewed-by Alistair Popple <[email protected]>However examining the fixes patch again I note that we are calling
mmu_notifier_invalidate_range(mm, 0, -1UL)fromarch_tlbbatch_add_pending()inarch/x86/include/asm/tlbflush.h.That seems suboptimal because we would be doing an invalidate all for every page unmap,
Yes. This can be performance regression for IOMMU TLB flushing. For CPU, it's "flush smaller ranges with more IPI" vs. "flush whole range with less IPI", and in general the later wins because the high overhead of IPI. But, IIUC, for IOMMU TLB, it becomes "flush smaller ranges" vs. "flush whole range". That is generally bad. It may be better to restore the original behavior. Can we just pass the size of TLB flushing in set_tlb_ubc_flush_pending()->arch_tlbbatch_add_pending(), and flush the IOMMU TLB for the range?
Quoted 4 lines
and as of
db6c1f6f236d("mm/tlbbatch: introducearch_flush_tlb_batched_pending()")arch_flush_tlb_batched_pending()callsflush_tlb_mm()anyway. So I think we can probably drop the explicit notifier call fromarch_flush_tlb_batched_pending().
arch_flush_tlb_batched_pending() is used when we need to change page table (e.g., munmap()) in parallel with TLB flushing batching (e.g., try_to_unmap()). The actual TLB flushing part for set_tlb_ubc_flush_pending()->arch_tlbbatch_add_pending() is try_to_unmap_flush()->arch_tlbbatch_flush().
Quoted 8 lines
Will put togeather a patch for that.
- Alistair
Quoted 4 lines
Add a flush all helper function and call it when the invalidation range is from 0 to -1UL, ensuring that the entire caches are invalidated correctly.
[snip]
--
Best Regards,
Huang, Ying
Quoted 24 lines · Ying
Quoted 22 lines · Alistair Popple
Quoted 11 lines · Lu Baolu
Commit
6bbd42e2df8f("mmu_notifiers: callinvalidate_range()when invalidating TLBs") moved the secondary TLB invalidations into the TLB invalidation functions to ensure that all secondary TLB invalidations happen at the same time as the CPU invalidation and added a flush-all type of secondary TLB invalidation for the batched mode, where a range of [0, -1UL) is used to indicates that the range extends to the end of the address space.However, using an end address of -1UL caused an overflow in the Intel IOMMU driver, where the end address was rounded up to the next page. As a result, both the IOTLB and device ATC were not invalidated correctly.
Thanks for catching. This fix looks good so:
Reviewed-by Alistair Popple <[email protected]>However examining the fixes patch again I note that we are calling
mmu_notifier_invalidate_range(mm, 0, -1UL)fromarch_tlbbatch_add_pending()inarch/x86/include/asm/tlbflush.h.
Quoted 8 lines
Quoted 2 lines
That seems suboptimal because we would be doing an invalidate all for every page unmap,
Yes. This can be performance regression for IOMMU TLB flushing. For CPU, it's "flush smaller ranges with more IPI" vs. "flush whole range with less IPI", and in general the later wins because the high overhead of IPI. But, IIUC, for IOMMU TLB, it becomes "flush smaller ranges" vs. "flush whole range". That is generally bad.
The "flush smaller ranges" vs. "flush whole range" is equally valid for some architectures, or at least some implementations of SMMU on ARM because flushing the whole range is a single IOMMU command vs. multiple for flushing a range. See for example
https://lore.kernel.org/linux-arm-kernel/[email protected]/
which switches to a full invalidate depending on the range. I've no idea if that's true more generally though, although a similar situation existed on POWER9.
Quoted 4 lines
It may be better to
restore the original behavior. Can we just pass the size of TLB flushing in set_tlb_ubc_flush_pending()->arch_tlbbatch_add_pending(), and flush the IOMMU TLB for the range?
Ideally we'd push the notifier call down the stack, closer to where the actual HW tlb invalidate gets called. I think I was just getting lost through all the indirection in the lower level x86_64 TLB flushing and batching code though. Will take another look.
Quoted 10 lines
Quoted 4 lines
and as of
db6c1f6f236d("mm/tlbbatch: introducearch_flush_tlb_batched_pending()")arch_flush_tlb_batched_pending()callsflush_tlb_mm()anyway. So I think we can probably drop the explicit notifier call fromarch_flush_tlb_batched_pending().
arch_flush_tlb_batched_pending()is used when we need to change page table (e.g.,munmap()) in parallel with TLB flushing batching (e.g.,try_to_unmap()). The actual TLB flushing part for set_tlb_ubc_flush_pending()->arch_tlbbatch_add_pending() is try_to_unmap_flush()->arch_tlbbatch_flush().
Thanks for the pointer. I must have got arch_tlbbatch_flush() and arch_flush_tlb_batched_pending() crossed at some point.
- Alistair
Quoted 10 lines
Quoted 8 lines
Will put togeather a patch for that.
- Alistair
Quoted 4 lines
Add a flush all helper function and call it when the invalidation range is from 0 to -1UL, ensuring that the entire caches are invalidated correctly.
[snip]
Quoted 41 lines
From: Lu Baolu <[email protected]>
Sent: Friday, November 17, 2023 5:10 PMCommit
6bbd42e2df8f("mmu_notifiers: callinvalidate_range()when invalidating TLBs") moved the secondary TLB invalidations into the TLB invalidation functions to ensure that all secondary TLB invalidations happen at the same time as the CPU invalidation and added a flush-all type of secondary TLB invalidation for the batched mode, where a range of [0, -1UL) is used to indicates that the range extends to the end of the address space.However, using an end address of -1UL caused an overflow in the Intel IOMMU driver, where the end address was rounded up to the next page. As a result, both the IOTLB and device ATC were not invalidated correctly.
Add a flush all helper function and call it when the invalidation range is from 0 to -1UL, ensuring that the entire caches are invalidated correctly.
Fixes 6bbd42e2df8f ("mmu_notifiers: call invalidate_range() wheninvalidating TLBs")
Cc Huang Ying <[email protected]>Cc Alistair Popple <[email protected]>Tested-by Luo Yuzhang <[email protected]> # QATTested-by Tony Zhu <[email protected]> # DSASigned-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel/svm.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)drivers/iommu/intel/svm.c +1
-0@@ -216,6 +216,27 @@ static void intel_flush_svm_range(struct intel_svm *svm, unsigned long address,rcu_read_unlock (); } +static void intel_flush_svm_all (struct intel_svm *svm)
drivers/iommu/intel/svm.c static void intel_flush_svm_range(struct intel_svm · line 230
Quoted 10 lines
drivers/iommu/intel/svm.c +9
-0+{ +struct device_domain_info *info; +struct intel_svm_dev *sdev; + +rcu_read_lock (); +list_for_each_entry_rcu (sdev, &svm->devs, list) { + info =dev_iommu_priv_get (sdev->dev); + +qi_flush_piotlb (sdev->iommu, sdev->did, svm->pasid,0 ,-1UL ,1 );
Why setting 'ih' to skip invalidating page structure caches?
Quoted 32 lines
drivers/iommu/intel/svm.c +5
-0+if (info->ats_enabled) { +qi_flush_dev_iotlb_pasid (sdev->iommu, sdev->sid, info->pfsid, + svm->pasid, sdev->qdep, +0 ,64 -VTD_PAGE_SHIFT ); + quirk_extra_dev_tlb_flush(info, 0, 64 -
VTD_PAGE_SHIFT,drivers/iommu/intel/svm.c +11
-0+ svm->pasid, sdev->qdep); + } + } +rcu_read_unlock (); +} +/* Pages have been freed at this point */ static void intel_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn, struct mm_struct *mm,@@ -223,6 +244,11 @@ static void intel_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn, { struct intel_svm *svm = container_of(mn, struct intel_svm, notifier); +if (start ==0 && end ==-1UL ) { +intel_flush_svm_all (svm); +return ; + } + intel_flush_svm_range(svm, start, (end - start + PAGE_SIZE - 1) >> VTD_PAGE_SHIFT,0);
}
--
2.34.1
Baolu Lu
maintainerQuoted 40 lines · Kevin
Quoted 40 lines
From: Lu Baolu<[email protected]>
Sent: Friday, November 17, 2023 5:10 PMCommit
6bbd42e2df8f("mmu_notifiers: callinvalidate_range()when invalidating TLBs") moved the secondary TLB invalidations into the TLB invalidation functions to ensure that all secondary TLB invalidations happen at the same time as the CPU invalidation and added a flush-all type of secondary TLB invalidation for the batched mode, where a range of [0, -1UL) is used to indicates that the range extends to the end of the address space.However, using an end address of -1UL caused an overflow in the Intel IOMMU driver, where the end address was rounded up to the next page. As a result, both the IOTLB and device ATC were not invalidated correctly.
Add a flush all helper function and call it when the invalidation range is from 0 to -1UL, ensuring that the entire caches are invalidated correctly.
Fixes 6bbd42e2df8f ("mmu_notifiers: call invalidate_range() wheninvalidating TLBs")
Cc Huang Ying<[email protected]>Cc Alistair Popple<[email protected]>Tested-by Luo Yuzhang<[email protected]> # QATTested-by Tony Zhu<[email protected]> # DSASigned-off-by Lu Baolu<[email protected]>--- drivers/iommu/intel/svm.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)drivers/iommu/intel/svm.c +0
-0@@ -216,6 +216,27 @@ static void intel_flush_svm_range(struct intel_svm *svm, unsigned long address,rcu_read_unlock (); }
Quoted 12 lines
drivers/iommu/intel/svm.c static void intel_flush_svm_range(struct intel_svm *svm, unsigned long address,
Quoted 11 lines
drivers/iommu/intel/svm.c +10
-0+static void intel_flush_svm_all (struct intel_svm *svm) +{ +struct device_domain_info *info; +struct intel_svm_dev *sdev; + +rcu_read_lock (); +list_for_each_entry_rcu (sdev, &svm->devs, list) { + info =dev_iommu_priv_get (sdev->dev); + +qi_flush_piotlb (sdev->iommu, sdev->did, svm->pasid,0 ,-1UL ,1 );Why setting 'ih' to skip invalidating page structure caches?
It should be set to '0'. Good catch! Thank you!
Best regards,
baolu