[PATCH 2/2] Documentation: explain issues with taking locks inside kvm_lock

Paolo Bonzini
2025-01-24T19:11:21Z

kvm_lock should be used sparingly, and it is easy to protect
vm_list walks with kvm_get_kvm and kvm_put_kvm. Make it
a hard rule to drop kvm_lock before taking another mutex,
and document it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Documentation/virt/kvm/locking.rst | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/Documentation/virt/kvm/locking.rst b/Documentation/virt/kvm/locking.rst
index c56d5f26c750..f94aad9b95ab 100644
--- a/Documentation/virt/kvm/locking.rst
+++ b/Documentation/virt/kvm/locking.rst
@@ -26,13 +26,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
@@ -59,6 +52,23 @@ On x86:
Everything else is a leaf: no other lock is taken inside the critical
sections.

+In particular no other mutex should be taken inside kvm_lock, and the
+amount of code that can be run inside kvm_lock should be limited; this
+is because ``cpus_read_lock()`` might be triggered unknowingly and cause
+a circular dependency. For example, if you take ``kvm->slots_lock``
+inside ``kvm_lock``, the following can happen on x86:
+
+- ``kvm->srcu`` is synchronized with ``kvm->slots_lock`` taken
+- you wait for ``kvm->slots_lock`` with ``kvm_lock`` taken
+- ``__kvmclock_cpufreq_notifier()`` waits for ``kvm_lock`` and
+ is called within ``cpus_read_lock()``.
+- ``KVM_RUN`` can trigger static key updates, which call ``cpus_read_lock()``,
+ with ``kvm->srcu`` taken
+- therefore ``synchronize_srcu(&kvm->srcu)`` never completes.
+
+This rule applies to all architectures.
+
+
2. Exception
------------

@@ -238,6 +248,9 @@ time it will be set using the Dirty tracking mechanism described above.
:Type: mutex
:Arch: any
:Protects: - vm_list
+ - kvm_createvm_count
+ - kvm_active_vms
+:Comment: Do not take any mutex inside.

``kvm_usage_lock``
^^^^^^^^^^^^^^^^^^
--
2.43.5