Hi Shrikanth,
Thanks for taking a look at this patch. Please find my responses inline.
On 2026/09/29 11:48 AM, Shrikanth Hegde wrote:
>
>
> On 9/28/26 5:58 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.
> >
<..snip..>
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index aa51968e206a..52f72f30baf0 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -1212,9 +1212,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);
>
> Please add a comment above kvmppc_pseries_do_hpt_hcall, saying preemption
> is disabled and one cannot make any blocking call.
Sure, will do.
>
> > 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 +1836,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 +1885,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) {
>
> Please check if below is possible case.
>
> https://sashiko.dev/#/patchset/20260928113704.48912-4-amachhiw%40linux.ibm.com
Thank you for raising this point and bringing Sashiko AI's review comment to
attention. After conducting a deep-dive audit of the preemption paths and the
virtual-mode locking context, it looks like Sashiko's finding is legitimate, and
extremely subtle.
The Deadlock Mechanism:
=======================
In this patch we added preempt_disable() / preempt_enable() pairs exclusively
around the guest vCPU HPT hcall paths (kvmppc_pseries_do_hpt_hcall) and page
fault paths (kvmppc_hpte_hv_fault) in virtual mode to satisfy the HPTE locking
preemption contract.
However, several host-side virtual-mode paths still acquire and hold the HPTE
bit-lock (HPTE_V_HVLOCK) with preemption enabled. Specifically:
- While MMU notifier paths (unmapping, aging) and the dirty-log path are safe
(since they are called with kvm->mmu_lock held, which disables preemption),
the memslot flushing path (kvmppc_core_flush_memslot_hv() ->
kvm_unmap_rmapp()) runs with only the slots_arch_lock mutex held, leaving
preemption fully enabled.
If a host-side thread executing kvm_unmap_rmapp() successfully acquires the
HPTE_V_HVLOCK on CPU X and is preempted, a guest vCPU thread subsequently
scheduled on CPU X will enter the hcall or fault path, disable preemption (via
the current change), and spin indefinitely on try_lock_hpte(). Because
preemption is disabled on the spinning vCPU, it will never yield CPU X. The
preempted host-side lock owner can never be rescheduled on CPU X to release the
lock.
This converts a potential latency/livelock issue into a permanent hard deadlock.
Proposed Solution:
==================
To make this fix completely watertight and eliminate the deadlock window, no
thread must ever be preempted while holding HPTE_V_HVLOCK in virtual mode. This
means we must disable preemption during the lock-hold window on both the
vCPU-side and the host-side paths.
In the next version, I'll be making following changes:
Host-Side Preemption Disabling: Add preempt_disable() / preempt_enable()
brackets around the HPTE_V_HVLOCK hold windows inside the four host-side
virtual-mode functions in arch/powerpc/kvm/book3s_64_mmu_hv.c:
- kvm_unmap_rmapp()
- kvm_age_rmapp()
- kvm_test_clear_dirty_npages()
- resize_hpt_rehash_hpte()
For example, inside kvm_unmap_rmapp():
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();
}
}
To ensure complete consistency and verify there are no other potential
preemption deadlock sites, I conducted a full audit of all 8 occurrences of
try_lock_hpte() inside arch/powerpc/kvm/book3s_64_mmu_hv.c.
The proposed v2 fixes simply bring the host-side lock users into alignment with
the established, pre-existing design pattern used by every other virtual-mode
caller of try_lock_hpte() in this file:
- Established safe callers (4) — already wrapped in preempt_disable() /
preempt_enable() blocks by design:
- kvmppc_book3s_hv_page_fault()
- read_hpte_for_cma()
- kvmppc_hpte_hv_page_debug()
- Symmetrical alignment in v2 (4):
- kvm_unmap_rmapp()
- kvm_age_rmapp()
- kvm_test_clear_dirty_npages()
- resize_hpt_rehash_hpte()
This ensures 100% preemption-safety symmetry across all virtual-mode
lock-holding windows in KVM.
I hope this sounds good to you.
Thanks,
Amit
