Hi Amit. On 9/30/26 11:07 PM, Amit Machhiwal wrote:
kvmppc_hv_find_lock_hpte() requires virtual-mode callers to run with preemption disabled, because it can return with HPTE_V_HVLOCK still held until the caller later unlocks the HPTE. Existing virtual-mode callers in book3s_64_mmu_hv.c already follow that rule, but several paths do not.kvmppc_handle_exit_hv() calls kvmppc_hpte_hv_fault() for hash-mode data-side and instruction-side faults after guest exit with preemption enabled. kvmppc_pseries_do_hcall() executes virtual-mode HPT hcall handlers via kvmppc_pseries_do_hpt_hcall() with preemption enabled; the handlers for H_ENTER, H_REMOVE, H_READ, H_CLEAR_MOD, H_CLEAR_REF, H_PROTECT, and H_BULK_REMOVE all spin on try_lock_hpte() or lock_rmap(). H_ENTER also reaches kvmppc_do_h_enter(), which uses arch_spin_lock() on kvm->mmu_lock. That raw lock choice is intentional because kvmppc_do_h_enter() is also called from real-mode paths, so the correct fix is to establish the proper preemption context at the virtual-mode caller boundary. On the host side, kvm_unmap_rmapp(), kvm_age_rmapp(), kvm_test_clear_dirty_npages(), and resize_hpt_rehash_hpte() also acquire HPTE_V_HVLOCK via try_lock_hpte() in process context with preemption enabled, serving MMU notifier callbacks, dirty-log harvesting, and HPT resize respectively. If any of these threads is preempted while holding HPTE_V_HVLOCK, any other thread on the same CPU spinning on the same bit-lock can never make progress, as the lock owner cannot be rescheduled to release it. This is particularly acute when the spinning thread has preemption disabled: it will never yield, causing a permanent CPU hang. Fix this by adding preempt_disable()/preempt_enable() pairs around the two kvmppc_hpte_hv_fault() call sites in kvmppc_handle_exit_hv(), around the kvmppc_pseries_do_hpt_hcall() invocation in kvmppc_pseries_do_hcall(), and around the try_lock_hpte() hold windows in kvm_unmap_rmapp(), kvm_age_rmapp(), kvm_test_clear_dirty_npages(), and resize_hpt_rehash_hpte(). On failed lock acquisition the guard is released before the cpu_relax() spin so the lock owner can be scheduled. Fixes: 6165d5dd99db ("KVM: PPC: Book3S HV: add virtual mode handlers for HPT hcalls and page faults") Cc: [email protected] # v5.14+ Signed-off-by: Amit Machhiwal <[email protected]> --- Changes in v2: - Extended preempt_disable()/preempt_enable() to also cover four host-side virtual-mode HPTE bit-lock users in book3s_64_mmu_hv.c. - Added warning comment above kvmppc_pseries_do_hpt_hcall(). - Dropped Reviewed-by as the patch was materially extended. arch/powerpc/kvm/book3s_64_mmu_hv.c | 12 ++++++++++++ arch/powerpc/kvm/book3s_hv.c | 10 ++++++++++ 2 files changed, 22 insertions(+) diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c index 2ccb3d138f46..59da958e09cb 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c @@ -823,7 +823,9 @@ static void kvm_unmap_rmapp(struct kvm *kvm, struct kvm_memory_slot *memslot, */ i = *rmapp & KVMPPC_RMAP_INDEX; hptep = (__be64 *) (kvm->arch.hpt.virt + (i << 4)); + preempt_disable(); if (!try_lock_hpte(hptep, HPTE_V_HVLOCK)) { + preempt_enable(); /* unlock rmap before spinning on the HPTE lock */ unlock_rmap(rmapp); while (be64_to_cpu(hptep[0]) & HPTE_V_HVLOCK) @@ -834,6 +836,7 @@ static void kvm_unmap_rmapp(struct kvm *kvm, struct kvm_memory_slot *memslot, kvmppc_unmap_hpte(kvm, i, memslot, rmapp, gfn); unlock_rmap(rmapp); __unlock_hpte(hptep, be64_to_cpu(hptep[0])); + preempt_enable(); } }@@ -909,7 +912,9 @@ static bool kvm_age_rmapp(struct kvm *kvm, struct kvm_memory_slot *memslot,if (!(be64_to_cpu(hptep[1]) & HPTE_R_R)) continue;+ preempt_disable();if (!try_lock_hpte(hptep, HPTE_V_HVLOCK)) { + preempt_enable(); /* unlock rmap before spinning on the HPTE lock */ unlock_rmap(rmapp); while (be64_to_cpu(hptep[0]) & HPTE_V_HVLOCK) @@ -928,6 +933,7 @@ static bool kvm_age_rmapp(struct kvm *kvm, struct kvm_memory_slot *memslot, ret = true; } __unlock_hpte(hptep, be64_to_cpu(hptep[0])); + preempt_enable(); } while ((i = j) != head);unlock_rmap(rmapp);@@ -1043,7 +1049,9 @@ static int kvm_test_clear_dirty_npages(struct kvm *kvm, unsigned long *rmapp) (!hpte_is_writable(hptep1) || vcpus_running(kvm))) continue;+ preempt_disable();if (!try_lock_hpte(hptep, HPTE_V_HVLOCK)) { + preempt_enable(); /* unlock rmap before spinning on the HPTE lock */ unlock_rmap(rmapp); while (hptep[0] & cpu_to_be64(HPTE_V_HVLOCK)) @@ -1054,6 +1062,7 @@ static int kvm_test_clear_dirty_npages(struct kvm *kvm, unsigned long *rmapp) /* Now check and modify the HPTE */ if (!(hptep[0] & cpu_to_be64(HPTE_V_VALID))) { __unlock_hpte(hptep, be64_to_cpu(hptep[0])); + preempt_enable(); continue; }@@ -1077,6 +1086,7 @@ static int kvm_test_clear_dirty_npages(struct kvm *kvm, unsigned long *rmapp)v &= ~HPTE_V_ABSENT; v |= HPTE_V_VALID; __unlock_hpte(hptep, v); + preempt_enable(); } while ((i = j) != head);unlock_rmap(rmapp);@@ -1219,6 +1229,7 @@ static unsigned long resize_hpt_rehash_hpte(struct kvm_resize_hpt *resize, if (!(vpte & HPTE_V_VALID) && !(vpte & HPTE_V_ABSENT)) return 0; /* nothing to do */+ preempt_disable();while (!try_lock_hpte(hptep, HPTE_V_HVLOCK)) cpu_relax();@@ -1346,6 +1357,7 @@ static unsigned long resize_hpt_rehash_hpte(struct kvm_resize_hpt *resize, out:unlock_hpte(hptep, vpte); + preempt_enable();
I don't like this sprinkling of preempt disable/enable. Is there not a way to embedd this in try_lock_hpte/unlock_hpte? Also, Is any of these be called in real mode? Note that currently preempt count is derived from thread info. So it may not be safe in real mode.
return ret; }diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.cindex aa51968e206a..0b7743bb89d9 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -1159,6 +1159,10 @@ static long kvmppc_h_rpt_invalidate(struct kvm_vcpu *vcpu, return H_SUCCESS; }+/*+ * Must be called with preemption disabled. The HPT hcall handlers spin + * on HPTE bit-locks and cannot make any blocking/sleeping calls. + */ static long kvmppc_pseries_do_hpt_hcall(struct kvm_vcpu *vcpu, unsigned long req) { switch (req) { @@ -1212,9 +1216,11 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) case H_CLEAR_REF: case H_PROTECT: case H_BULK_REMOVE: + preempt_disable(); idx = srcu_read_lock(&kvm->srcu); ret = kvmppc_pseries_do_hpt_hcall(vcpu, req); srcu_read_unlock(&kvm->srcu, idx); + preempt_enable(); if (ret == H_TOO_HARD) return RESUME_HOST; break; @@ -1834,8 +1840,10 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu, else vsid = vcpu->arch.fault_gpa;+ preempt_disable();err = kvmppc_hpte_hv_fault(vcpu, vcpu->arch.fault_dar, vsid, vcpu->arch.fault_dsisr, true); + preempt_enable(); if (err == 0) { r = RESUME_GUEST; } else if (err == -1 || err == -2) { @@ -1881,8 +1889,10 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu, else vsid = vcpu->arch.fault_gpa;+ preempt_disable();err = kvmppc_hpte_hv_fault(vcpu, vcpu->arch.fault_dar, vsid, vcpu->arch.fault_dsisr, false); + preempt_enable(); if (err == 0) { r = RESUME_GUEST; } else if (err == -1) {
