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>]

Hi Shrikanth,

Thanks for the review.  Please find my responses inline below.

On 2026/10/05 07:58 PM, Shrikanth Hegde wrote:
> 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: stable@vger.kernel.org # v5.14+
> > Signed-off-by: Amit Machhiwal <amachhiw@linux.ibm.com>
> > ---
> > 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?

I understand the scattering of preempt_disable()/preempt_enable() looks ugly.
But there are two blockers for that approach:

1. The real-mode hcall dispatch table (hcall_real_table in
   book3s_hv_rmhandlers.S) calls kvmppc_h_enter(), kvmppc_h_remove(),
   kvmppc_h_bulk_remove() and others in book3s_hv_rm_mmu.c, which all call
   try_lock_hpte() from real mode.  Adding preempt_disable() inside
   try_lock_hpte() would affect those real-mode callers, which brings us to your
   second point.

2. The lock/unlock pair crosses function boundaries in several callers. For
   example, kvmppc_hv_find_lock_hpte() acquires HPTE_V_HVLOCK via
   try_lock_hpte() internally but returns with the lock still held — the comment
   above it documents this.  The caller is responsible for the matching
   unlock_hpte().  So even if preempt_disable() were placed inside
   try_lock_hpte(), preempt_enable() would still need to be scattered across
   every caller after their unlock_hpte() — the pairing cannot be made automatic
   inside the primitives alone.

   This caller-side pattern is already established in the tree:
   kvmppc_virtmode_do_h_enter() (book3s_64_mmu_hv.c:298) and
   kvmppc_mmu_book3s_64_hv_xlate() (book3s_64_mmu_hv.c:367) both wrap
   try_lock_hpte() call sites with preempt_disable()/preempt_enable() at the
   caller level, not inside the primitives.

> 
> 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.

The four functions modified in book3s_64_mmu_hv.c — kvm_unmap_rmapp(),
kvm_age_rmapp(), kvm_test_clear_dirty_npages(), and resize_hpt_rehash_hpte() —
are all static functions in book3s_64_mmu_hv.c, and not reachable from the
real-mode hcall table.  They run in process context as MMU notifier callbacks,
dirty-log harvesting, and HPT resize ioctl respectively. The call sites in
book3s_hv.c are likewise post-guest-exit virtual mode.  The preempt_disable()
placements in this patch are safe.

Thanks,
Amit

> 
> >   	return ret;
> >   }
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index 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) {
>