From: Ackerley Tng The original comment talks about cpus_read_lock() and kvm_usage_count, but doesn't explain why they are related. Elaborate comment on kvm_usage_lock to provide more context. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index 662231e958a07..5564c8b38b9cc 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -248,8 +248,23 @@ time it will be set using the Dirty tracking mechanism described above. :Arch: any :Protects: - kvm_usage_count - hardware virtualization enable/disable -:Comment: Exists to allow taking cpus_read_lock() while kvm_usage_count is - protected, which simplifies the virtualization enabling logic. +:Comment: ``kvm_usage_count`` serves to deduplicate hardware + virtualization enabling and disabling requests from different VMs + being created. + + Hardware virtualization enabling/disabling requires taking + ``cpus_read_lock()``. + + ``kvm_lock`` used to also protect ``kvm_usage_count``, but other + parts of the Linux kernel holding ``cpus_read_lock()`` need to + call into KVM to ensure that VM state remains consistent with the + host's state. For example, when the CPU frequency changes, KVM is + notified. ``kvmclock_cpufreq_notifier()`` takes ``kvm_lock`` to + iterate ``vm_list``. + + To decouple these, use different locks, ``kvm_lock`` for + ``vm_list`` and ``kvm_usage_lock`` for enabling/disabling hardware + virtualization. ``kvm->mn_invalidate_lock`` ^^^^^^^^^^^^^^^^^^^^^^^^^^^ -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng Move the detail about cpu_read_lock() and kvm_lock to where the acquisition order is mentioned. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index 5564c8b38b9cc..1e8cbbe3ba706 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -10,6 +10,11 @@ KVM Lock Overview The acquisition orders for mutexes are as follows: - cpus_read_lock() is taken outside kvm_lock + - Taking cpus_read_lock() outside of kvm_lock is problematic, + despite it being the official ordering, as it is quite easy to + unknowingly trigger cpus_read_lock() while holding kvm_lock. + Use caution when walking vm_list, e.g. avoid complex operations + when possible. - kvm_usage_lock is taken outside cpus_read_lock() @@ -28,13 +33,6 @@ The acquisition orders for mutexes are as follows: are taken on the waiting side when modifying memslots, so MMU notifiers must not take either kvm->slots_lock or kvm->slots_arch_lock. -cpus_read_lock() vs kvm_lock: - -- Taking cpus_read_lock() outside of kvm_lock is problematic, despite that - being the official ordering, as it is quite easy to unknowingly trigger - cpus_read_lock() while holding kvm_lock. Use caution when walking vm_list, - e.g. avoid complex operations when possible. - For SRCU: - ``synchronize_srcu(&kvm->srcu)`` is called inside critical sections -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng Move the detail about ordering between kvm->slots_lock and kvm->irq_lock to where the two locks are first mentioned. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index 1e8cbbe3ba706..67dd2066f6d98 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -21,12 +21,11 @@ The acquisition orders for mutexes are as follows: - kvm->lock is taken outside vcpu->mutex - kvm->lock is taken outside kvm->slots_lock and kvm->irq_lock + - kvm->slots_lock is taken outside kvm->irq_lock, though acquiring + them together is quite rare. - vcpu->mutex is taken outside kvm->slots_lock and kvm->slots_arch_lock -- kvm->slots_lock is taken outside kvm->irq_lock, though acquiring - them together is quite rare. - - kvm->mn_active_invalidate_count ensures that pairs of invalidate_range_start() and invalidate_range_end() callbacks use the same memslots array. kvm->slots_lock and kvm->slots_arch_lock -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng For the :Protects: section of kvm->mmu_lock, a missing space causes the - to render as a literal - instead of a bullet point. Add space to make it render as a bullet point. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index 67dd2066f6d98..e349c2cb94943 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -283,7 +283,7 @@ time it will be set using the Dirty tracking mechanism described above. ^^^^^^^^^^^^^^^^^ :Type: spinlock_t or rwlock_t :Arch: any -:Protects: -shadow page/shadow tlb entry +:Protects: - shadow page/shadow tlb entry :Comment: it is a spinlock since it is used in mmu notifier. ``kvm->srcu`` -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng The Exception section describes some exceptions but not the rule the exception is for. Add a paragraph to clarify that detail. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index e349c2cb94943..5161636cec481 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -61,6 +61,10 @@ sections. 2. Exception ------------ +The general rule in KVM is that any modification to shadow page tables +(and their entries (SPTEs)) must be protected by ``kvm->mmu_lock``, +with the exceptions described below. + Fast page fault: Fast page fault is the fast path which fixes the guest page fault out of -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng Exceptions documented are described but without headings, making it hard to identify where each exception description ended. Use actual headings at a lower level than that of the heading used for Exception to improve readability. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index 5161636cec481..fc4537a7659a9 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -65,7 +65,8 @@ The general rule in KVM is that any modification to shadow page tables (and their entries (SPTEs)) must be protected by ``kvm->mmu_lock``, with the exceptions described below. -Fast page fault: +2.1. Fast page fault +^^^^^^^^^^^^^^^^^^^^ Fast page fault is the fast path which fixes the guest page fault out of the mmu-lock on x86. Currently, the page fault can be fast in one of the @@ -217,7 +218,8 @@ Since the spte is "volatile" if it can be updated out of mmu-lock, we always atomically update the spte and the race caused by fast page fault can be avoided. See the comments in spte_needs_atomic_update() and mmu_spte_update(). -Lockless Access Tracking: +2.2 Lockless Access Tracking +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is used for Intel CPUs that are using EPT but do not support the EPT A/D bits. In this case, PTEs are tagged as A/D disabled (using ignored bits), and -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng The original comment says that synchronize_srcu(&kvm->srcu) is called inside critical sections for kvm->lock, vcpu->mutex and kvm->slots_lock. Drop mention of kvm->lock since this is no longer true. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index fc4537a7659a9..437dbfa0030b9 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -35,8 +35,8 @@ The acquisition orders for mutexes are as follows: For SRCU: - ``synchronize_srcu(&kvm->srcu)`` is called inside critical sections - for kvm->lock, vcpu->mutex and kvm->slots_lock. These locks _cannot_ - be taken inside a kvm->srcu read-side critical section; that is, the + for vcpu->mutex and kvm->slots_lock. These locks _cannot_ be taken + inside a kvm->srcu read-side critical section; that is, the following is broken:: srcu_read_lock(&kvm->srcu); -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng Add example of where vcpu->mutex and kvm->slots_lock are held while calling synchronize_srcu(&kvm->srcu) to concretely show where the synchronization primitives overlap. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index 437dbfa0030b9..f12664443e913 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -35,9 +35,12 @@ The acquisition orders for mutexes are as follows: For SRCU: - ``synchronize_srcu(&kvm->srcu)`` is called inside critical sections - for vcpu->mutex and kvm->slots_lock. These locks _cannot_ be taken - inside a kvm->srcu read-side critical section; that is, the - following is broken:: + for vcpu->mutex and kvm->slots_lock. (For example, when there is a + ``KVM_REQ_APICV_UPDATE`` request, ``vcpu->mutex`` is held in + ``kvm_vcpu_ioctl()``, and then when the memslots get updated, + ``kvm->slots_lock`` is taken.) These locks _cannot_ be taken inside + a kvm->srcu read-side critical section; that is, the following is + broken:: srcu_read_lock(&kvm->srcu); mutex_lock(&kvm->slots_lock); -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng Document how synchronization is used while managing guest faults centrally so code comments can point users at a central place. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 108 +++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index f12664443e913..0663ccfe0633d 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -339,3 +339,111 @@ time it will be set using the Dirty tracking mechanism described above. cpu_hotplug_lock is held, e.g. from cpufreq_boost_trigger_state(), and many operations need to take cpu_hotplug_lock when loading a vendor module, e.g. updating static calls. + +4. Synchronization while managing guest faults +---------------------------------------------- + +This section explains the intersection of these synchronization mechanisms: + +- ``kvm->srcu`` (for memslots) +- ``kvm->mmu_invalidate_*`` (pending invalidations) +- ``kvm->mn_*`` (synchronization for ``kvm->mmu_invalidate_*``) + +4.1 Overview +^^^^^^^^^^^^ + +KVM resolves guest page faults by translating the Guest Frame Number (GFN) into +a Page Frame Number (PFN) via memslots and then populating its shadow page +tables with the resulting mapping. + +While handling the guest page fault, KVM must ensure a consistent view of the +active memslots container, so KVM takes ``srcu_read_lock(&kvm->srcu);``. + +Guest page fault handling can race with some request from host userspace to +invalidate shadow page tables. These requests originate from a few places, such +as + +1. MMU Notifiers: KVM registers callbacks with the kernel’s memory management + subsystem to know when there are changes to mappings in the host userspace + page tables. +2. Memslot Updates: The host userspace VMM, such as QEMU may use the + ``KVM_SET_USER_MEMORY_REGION`` ioctl to add, delete, or move a memslot. KVM + must zap the affected shadow page tables to ensure the guest doesn't access + stale mappings. +3. Memory Attribute Changes: The ``KVM_SET_MEMORY_ATTRIBUTES`` ioctl allows + userspace to change attributes for a range of guest memory (e.g., setting a + range as "private" for Confidential Computing). This also requires + invalidating existing shadow mappings. + +When such a race occurs, KVM optimistically allows the faulting logic to +proceed, but just before committing the fault, KVM will check for a pending +invalidation, and retry the fault process if there is a pending invalidation +affecting the GFN where the fault occurred. + +4.2 Tracking pending invalidations with ``kvm->mmu_invalidate*`` fields +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +A "pending invalidation" is determined using a combination of + +- ``kvm->mmu_invalidate_in_progress`` +- ``kvm->mmu_invalidate_range_start`` and ``kvm->mmu_invalidate_range_end`` +- ``kvm->mmu_invalidate_seq`` + +``is_page_fault_stale()`` shows how the above fields are used to determine if +the page fault is stale and requires a retry. + +To protect the above combination of fields, a lock is used, which is the +``kvm->mmu_lock``. + +4.2.1 Derived information vs pending invalidations +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Generally, the result of any information derived from GFN aka page +attribute/page metadata lookups may race with invalidations. Here are some +examples of lookups: + +- ``host_pfn_mapping_level()`` uses memslot information to find the mapping + level of pages in host userspace page tables. If there's an invalidation, the + pages that were mapped would no longer be mapped and hence the mapping level + result would be stale. + +There are several ways to ensure valid results: + +- Check ``mmu_invalidate_retry_gfn()`` after grabbing the result, before + consuming it. In this case, ``mmu_lock`` doesn't need to be held during the + lookup, but it does need to be held while checking the MMU notifier. KVM's + guest page fault handling uses this option. +- Hold ``mmu_lock`` AND ensure there is no in-progress MMU notifier invalidation + event for the hva. This can be done by explicit checking the MMU notifier or + by ensuring that KVM already has a valid mapping that covers the + hva. ``kvm_mmu_recover_huge_pages()`` uses this option. + +4.3 Further optimization: ignoring invalidations if there is no matching memslot +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Invalidation is only really required when the invalidated memory range overlaps +with some memslot. Without a matching memslot, the invalidation request could +actually just be ignored. Hence, KVM only updates the ``kvm->mmu_invalidate_*`` +fields and takes ``kvm->mmu_lock`` if it finds a matching memslot. + +This creates another problem: if memslots are updated while there is an ongoing +invalidation, then the updates to the fields and the lock would be imbalanced. + +4.4 Synchronization for invalidation lock/fields: ``kvm->mn_*`` +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +To make sure the updates to the invalidation lock/fields are balanced, KVM has a +further layer of synchronization. ``kvm_swap_active_memslots()`` enforces that +changes to memslots are only committed once all pending invalidations are +complete. + +In other words, ``kvm->mn_*`` ensures the following does not happen: + +1. Some memslot existed, causing a pending invalidation request to be recorded + in the ``kvm->mmu_invalidate_*`` fields +2. Memslot got removed, so the invalidation request was never removed from the + ``kvm->mmu_invalidate_*`` fields. + +In addition, ``kvm_swap_active_memslots()`` also enforces that changes to +memslots are complete before doing ``synchronize_srcu(&kvm->srcu)`` to make sure +running readers of the old memslots container are done before freeing it. -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng Clarify the existing comment about synchronize_srcu() and kvm_gmem_get_pfn() to provide further context. Explain which synchronize_srcu() prevents races with how kvm_gmem_get_pfn() is used. Also point reader to documentation for better understanding. Signed-off-by: Ackerley Tng --- virt/kvm/guest_memfd.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c index 69c9d6d546b28..f2218db0af980 100644 --- a/virt/kvm/guest_memfd.c +++ b/virt/kvm/guest_memfd.c @@ -711,8 +711,13 @@ static void __kvm_gmem_unbind(struct kvm_memory_slot *slot, struct gmem_file *f) xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL); /* - * synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn() - * cannot see this memslot. + * This is called when memslots are updated, after the old + * memslot container is no longer in + * use. synchronize_srcu(&kvm->srcu) was called there, so + * kvm_gmem_get_pfn() from KVM's guest fault handling cannot + * see this memslot. See Documentation/virt/kvm/locking.rst + * for more information about kvm->srcu and the memslots + * container. */ WRITE_ONCE(slot->gmem.file, NULL); } -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng After consolidating documentation for host_pfn_mapping_level() in Documentation/virt/kvm/locking.rst, point users of function to docs. Signed-off-by: Ackerley Tng --- arch/loongarch/kvm/mmu.c | 24 ++++-------------------- arch/x86/kvm/mmu/mmu.c | 24 ++++-------------------- 2 files changed, 8 insertions(+), 40 deletions(-) diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c index a7fa458e33605..0313901171a2e 100644 --- a/arch/loongarch/kvm/mmu.c +++ b/arch/loongarch/kvm/mmu.c @@ -641,27 +641,11 @@ static bool fault_supports_huge_mapping(struct kvm_memory_slot *memslot, /* * Lookup the mapping level for @gfn in the current mm. * - * WARNING! Use of host_pfn_mapping_level() requires the caller and the end - * consumer to be tied into KVM's handlers for MMU notifier events! + * WARNING! This derives information from the current state of memslots and + * page mappings and may race with invalidations. * - * There are several ways to safely use this helper: - * - * - Check mmu_invalidate_retry_gfn() after grabbing the mapping level, before - * consuming it. In this case, mmu_lock doesn't need to be held during the - * lookup, but it does need to be held while checking the MMU notifier. - * - * - Hold mmu_lock AND ensure there is no in-progress MMU notifier invalidation - * event for the hva. This can be done by explicit checking the MMU notifier - * or by ensuring that KVM already has a valid mapping that covers the hva. - * - * - Do not use the result to install new mappings, e.g. use the host mapping - * level only to decide whether or not to zap an entry. In this case, it's - * not required to hold mmu_lock (though it's highly likely the caller will - * want to hold mmu_lock anyways, e.g. to modify SPTEs). - * - * Note! The lookup can still race with modifications to host page tables, but - * the above "rules" ensure KVM will not _consume_ the result of the walk if a - * race with the primary MMU occurs. + * See Documentation/virt/kvm/locking.rst to understand how to consuming the + * result of this lookup safely. */ static int host_pfn_mapping_level(struct kvm *kvm, gfn_t gfn, const struct kvm_memory_slot *slot) diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index f8aa7eda661ee..20cdcdd20e78d 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3214,27 +3214,11 @@ static void direct_pte_prefetch(struct kvm_vcpu *vcpu, u64 *sptep) /* * Lookup the mapping level for @gfn in the current mm. * - * WARNING! Use of host_pfn_mapping_level() requires the caller and the end - * consumer to be tied into KVM's handlers for MMU notifier events! + * WARNING! This derives information from the current state of memslots and + * page mappings and may race with invalidations. * - * There are several ways to safely use this helper: - * - * - Check mmu_invalidate_retry_gfn() after grabbing the mapping level, before - * consuming it. In this case, mmu_lock doesn't need to be held during the - * lookup, but it does need to be held while checking the MMU notifier. - * - * - Hold mmu_lock AND ensure there is no in-progress MMU notifier invalidation - * event for the hva. This can be done by explicit checking the MMU notifier - * or by ensuring that KVM already has a valid mapping that covers the hva. - * - * - Do not use the result to install new mappings, e.g. use the host mapping - * level only to decide whether or not to zap an entry. In this case, it's - * not required to hold mmu_lock (though it's highly likely the caller will - * want to hold mmu_lock anyways, e.g. to modify SPTEs). - * - * Note! The lookup can still race with modifications to host page tables, but - * the above "rules" ensure KVM will not _consume_ the result of the walk if a - * race with the primary MMU occurs. + * See Documentation/virt/kvm/locking.rst to understand how to consuming the + * result of this lookup safely. */ static int host_pfn_mapping_level(struct kvm *kvm, gfn_t gfn, const struct kvm_memory_slot *slot) -- 2.54.0.823.g6e5bcc1fc9-goog From: Ackerley Tng Now that the first sentence is already described in more detail in the new section on synchronization while managing guest faults, drop the first sentence. Signed-off-by: Ackerley Tng --- Documentation/virt/kvm/locking.rst | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst index 0663ccfe0633d..f26ea3acd0b70 100644 --- a/Documentation/virt/kvm/locking.rst +++ b/Documentation/virt/kvm/locking.rst @@ -26,11 +26,9 @@ The acquisition orders for mutexes are as follows: - vcpu->mutex is taken outside kvm->slots_lock and kvm->slots_arch_lock -- kvm->mn_active_invalidate_count ensures that pairs of - invalidate_range_start() and invalidate_range_end() callbacks - use the same memslots array. kvm->slots_lock and kvm->slots_arch_lock - are taken on the waiting side when modifying memslots, so MMU notifiers - must not take either kvm->slots_lock or kvm->slots_arch_lock. +- kvm->slots_lock and kvm->slots_arch_lock are taken on the waiting side when + modifying memslots, so MMU notifiers must not take either kvm->slots_lock or + kvm->slots_arch_lock. For SRCU: -- 2.54.0.823.g6e5bcc1fc9-goog