Re: [PATCH v2 2/3] KVM: PPC: Book3S HV: Add preempt_disable() around virtual-mode HPTE bit-lock users

[Amit Machhiwal <amachhiw@linux.ibm.com>]

On 2026/09/30 05:52 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] kvm: ppc: deadlock in lock_rmap() due to preemption guard in HPT hcalls
> - [Medium] kvm: ppc: resize_hpt_rehash_hpte() spins with preemption disabled
> --
> 
> commit 68aaf6c48f58de2d582ba27ebef746d63f655779
> Author: Amit Machhiwal <amachhiw@linux.ibm.com>
> 
> KVM: PPC: Book3S HV: Add preempt_disable() around virtual-mode HPTE bit-lock users
> 
> This patch adds preemption disable guards around virtual-mode HPTE bit-lock
> users in KVM PPC. This prevents situations where a lock-holding thread
> gets preempted, starving other threads spinning on the same lock.
> 
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index 2ccb3d138f46c..59da958e09cb4 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> 
> [ ... ]
> 
> > @@ -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();
> >  
> 
> [Severity: Medium]
> The commit message states: "On failed lock acquisition the guard is released
> before the cpu_relax() spin so the lock owner can be scheduled."
> 
> Does this unconditional spin with preemption disabled violate that
> description?

Acknowledged.  The commit message description of the preempt_disable() guard is
imprecise for resize_hpt_rehash_hpte().  I'll look into clarifying it.

> 
> [ ... ]
> 
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index aa51968e206a7..0b7743bb89d9a 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> 
> [ ... ]
> 
> > @@ -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;
> 
> [Severity: High]
> Does this preemption guard create a permanent deadlock with lock_rmap()?
> 
> For example, a host thread executing kvm_test_clear_dirty_npages() or
> kvm_unmap_rmapp() acquires lock_rmap() with preemption enabled:
> 
> arch/powerpc/kvm/book3s_64_mmu_hv.c:kvm_test_clear_dirty_npages() {
>     ...
>     retry:
>         lock_rmap(rmapp);
>     ...
> }
> 
> If the host thread is preempted while holding the bit-lock, and a VCPU
> thread for the same guest is scheduled on the same CPU, it can execute an
> H_ENTER hypercall.
> 
> When the VCPU thread reaches kvmppc_do_h_enter(), it attempts to acquire
> the same rmap lock:
> 
> arch/powerpc/kvm/book3s_hv_rm_mmu.c:kvmppc_do_h_enter() {
>     ...
>     if (realmode)
>         rmap = real_vmalloc_addr(rmap);
>     lock_rmap(rmap);
>     ...
> }
> 
> Since preemption is now disabled across the entire hcall by this patch,
> the VCPU thread could spin indefinitely on contention. It would never yield
> the CPU, meaning the preempted host thread could never resume to release
> the lock.

This is correct.  The v2 fix placed preempt_disable() just before try_lock_hpte(),
but lock_rmap() is acquired earlier in kvm_unmap_rmapp() and kvm_age_rmapp()
with preemption still enabled — leaving a residual window where a host thread
can be preempted while holding lock_rmap() alone.  I'll look into fixing this.

Thanks,
Amit