On 7/18/23 13:26, Alistair Popple wrote:
> The main change is to move secondary TLB invalidation mmu notifier
> callbacks into the architecture specific TLB flushing functions. This
> makes secondary TLB invalidation mostly match CPU invalidation while
> still allowing efficient range based invalidations based on the
> existing TLB batching code.
>
> ==========
> Background
> ==========
>
> The arm64 architecture specifies TLB permission bits may be cached and
> therefore the TLB must be invalidated during permission upgrades. For
> the CPU this currently occurs in the architecture specific
> ptep_set_access_flags() routine.
>
> Secondary TLBs such as implemented by the SMMU IOMMU match the CPU
> architecture specification and may also cache permission bits and
> require the same TLB invalidations. This may be achieved in one of two
> ways.
>
> Some SMMU implementations implement broadcast TLB maintenance
> (BTM). This snoops CPU TLB invalidates and will invalidate any
> secondary TLB at the same time as the CPU. However implementations are
> not required to implement BTM.
So, the implementations with BTM do not even need a MMU notifier callback
for secondary TLB invalidation purpose ? Perhaps mmu_notifier_register()
could also be skipped for such cases i.e with ARM_SMMU_FEAT_BTM enabled ?
BTW, dont see ARM_SMMU_FEAT_BTM being added as a feature any where during
the probe i.e arm_smmu_device_hw_probe().
>
> Implementations without BTM rely on mmu notifier callbacks to send
> explicit TLB invalidation commands to invalidate SMMU TLB. Therefore
> either generic kernel code or architecture specific code needs to call
> the mmu notifier on permission upgrade.
>
> Currently that doesn't happen so devices will fault indefinitely when
> writing to a PTE that was previously read-only as nothing invalidates
> the SMMU TLB.
Why does not the current SMMU MMU notifier intercept all invalidation from
generic MM code and do the required secondary TLB invalidation ? Is there
a timing issue involved here ? Secondary TLB invalidation does happen but
after the damage has been done ? Could you please point us to a real world
bug report taking such indefinite faults as mentioned above ?
>
> ========
> Solution
> ========
>
> To fix this the series first renames the .invalidate_range() callback
> to .arch_invalidate_secondary_tlbs() as suggested by Jason and Sean to
> make it clear this callback is only used for secondary TLBs. That was
> made possible thanks to Sean's series [1] to remove KVM's incorrect
> usage.
>
> Based on feedback from Jason [2] the proposed solution to the bug is
> to move the calls to mmu_notifier_arch_invalidate_secondary_tlbs()
> closer to the architecture specific TLB invalidation code. This
> ensures the secondary TLB won't miss invalidations, including the
> existing invalidation in the ARM64 code to deal with permission
> upgrade.
ptep_set_access_flags() is the only problematic place where this issue
is being reported ? If yes, why dont fix that instead of moving these
into platform specific callbacks ? OR there are other problematic areas
I might be missing.
>
> Currently only ARM64, PowerPC and x86 have IOMMU with secondary TLBs
> requiring SW invalidation so the notifier is only called for those
> architectures. It is also not called for invalidation of kernel
> mappings as no secondary IOMMU implementations can access those and
> hence it is not required.
>
> [1] - https://lore.kernel.org/all/20230602011518.787006-1-sea...@google.com/
> [2] - https://lore.kernel.org/linux-mm/zjmr5bw8l+bbz...@ziepe.ca/
>
> Alistair Popple (4):
> mm_notifiers: Rename invalidate_range notifier
> arm64/smmu: Use TLBI ASID when invalidating entire range
> mmu_notifiers: Call arch_invalidate_secondary_tlbs() when invalidating TLBs
> mmu_notifiers: Don't invalidate secondary TLBs as part of
> mmu_notifier_invalidate_range_end()
>
> arch/arm64/include/asm/tlbflush.h | 5 +-
> arch/powerpc/include/asm/book3s/64/tlbflush.h | 1 +-
> arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 1 +-
> arch/powerpc/mm/book3s64/radix_tlb.c | 6 +-
> arch/x86/mm/tlb.c | 3 +-
> drivers/iommu/amd/iommu_v2.c | 10 +-
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 29 +++--
> drivers/iommu/intel/svm.c | 8 +-
> drivers/misc/ocxl/link.c | 8 +-
> include/asm-generic/tlb.h | 1 +-
> include/linux/mmu_notifier.h | 104 ++++-------------
> kernel/events/uprobes.c | 2 +-
> mm/huge_memory.c | 29 +----
> mm/hugetlb.c | 8 +-
> mm/memory.c | 8 +-
> mm/migrate_device.c | 9 +-
> mm/mmu_notifier.c | 47 +++-----
> mm/rmap.c | 40 +-------
> 18 files changed, 109 insertions(+), 210 deletions(-)
>
> base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
