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

[sashiko-bot@kernel.org]

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?

[ ... ]

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930173750.56759-1-amachhiw@linux.ibm.com?part=2