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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

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?

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