[PATCH v2 0/3] KVM: PPC: Fixes for Book3S HV HPT locking and paired-single decoding

[PATCH v2 0/3] KVM: PPC: Fixes for Book3S HV HPT locking and paired-single decoding

[Amit Machhiwal]

This series addresses bug fixes across KVM PPC Book3S HV
locking/synchronization and paired-single instruction decoding.

Patches 1 & 2 fix synchronization and preemption issues introduced in
commit 6165d5dd99db ("KVM: PPC: Book3S HV: add virtual mode handlers for
HPT hcalls and page faults"):

- Patch 1 adds SRCU read lock protection when walking memslots during
  virtual-mode HPT hcalls to prevent use-after-free races with concurrent
  memslot updates.

- Patch 2 adds preempt_disable() around all virtual-mode HPTE bit-lock
  holders — both the guest vCPU paths (kvmppc_hpte_hv_fault() and
  kvmppc_pseries_do_hpt_hcall()) and the host-side paths
  (kvm_unmap_rmapp(), kvm_age_rmapp(), kvm_test_clear_dirty_npages(),
  resize_hpt_rehash_hpte()) — to prevent CPU stalls and deadlocks on
  preemption.

Patch 3 fixes paired-single D-form instruction emulation:

- Patch 3 fixes get_d_signext() to correctly extract the full 12-bit D
  displacement field and perform proper two's-complement sign extension.

Testing:
========
All test kernel builds were compiled with CONFIG_DEBUG_ATOMIC_SLEEP=y.
The following scenarios were verified:

1. Power9 PowerNV (L0) in Radix mode:
   - Booted L0 host with kernel containing all 3 patches.
   - Booted KVM guests in both Radix and Hash modes.
   - Ran kernel build workload inside guests — no errors observed.
   - Booted L1 guest with the same kernel — booted and ran cleanly.

2. Power9 PowerNV (L0) in Hash mode:
   - Booted L0 host with kernel containing all 3 patches.
   - Booted KVM guest in Hash mode.
   - Ran kernel build workload inside guest — no errors observed.
   - Booted L1 nested guest with the same kernel — booted and ran cleanly.

3. Power10 LPAR (L1):
   - Booted Power10 LPAR with the patched kernel.
   - Booted KVM guest and ran workloads — no errors observed.

Changes in v2:
==============
- v1: https://lore.kernel.org/all/20260928122837.8782-1-amachhiw@linux.ibm.com/
- Patch 2: Extended preempt_disable()/preempt_enable() coverage to also wrap
  the HPTE bit-lock hold windows in four host-side virtual-mode functions:
  kvm_unmap_rmapp(), kvm_age_rmapp(), kvm_test_clear_dirty_npages(), and
  resize_hpt_rehash_hpte(). These were identified as vulnerable by Sashiko AI
  review and confirmed correct by audit.
- Dropped Reviewed-by from Ritesh as the patch was materially extended.
- Patch 2: Added warning comment above kvmppc_pseries_do_hpt_hcall()
  documenting the preemption requirement.

Amit Machhiwal (3):
  KVM: PPC: Book3S HV: Add SRCU protection for virtual-mode HPT hcalls
  KVM: PPC: Book3S HV: Add preempt_disable() around virtual-mode HPTE
    bit-lock users
  KVM: PPC: Fix get_d_signext() 12-bit displacement for paired-single
    D-form

 arch/powerpc/kvm/book3s_64_mmu_hv.c      | 12 ++++
 arch/powerpc/kvm/book3s_hv.c             | 80 +++++++++++++-----------
 arch/powerpc/kvm/book3s_paired_singles.c |  7 +--
 3 files changed, 58 insertions(+), 41 deletions(-)


base-commit: 551c722f40809618230001baccf219193e22fc5a
-- 
2.54.0 (Apple Git-157)

[PATCH v2 1/3] KVM: PPC: Book3S HV: Add SRCU protection for virtual-mode HPT hcalls

[Amit Machhiwal]

The virtual-mode HPT hcall handlers (H_ENTER, H_REMOVE, H_BULK_REMOVE,
H_CLEAR_REF, H_CLEAR_MOD) call into book3s_hv_rm_mmu.c, which accesses
memslots via kvm_memslots_raw().  kvm_memslots_raw() uses
rcu_dereference_raw_check() to bypass SRCU lockdep annotation checking.
This is safe in real mode because the entire guest entry/exit is wrapped
in srcu_read_lock/unlock inside kvmppc_run_core() and
kvmhv_run_single_vcpu().

However, since commit 6165d5dd99db ("KVM: PPC: Book3S HV: add virtual
mode handlers for HPT hcalls and page faults"), these same handlers are
also executed in virtual mode via kvmppc_pseries_do_hcall(), which runs
after SRCU has already been released on guest exit.

A concurrent KVM_SET_USER_MEMORY_REGION deletion or move can therefore
call synchronize_srcu_expedited() — which does not wait for this thread —
and then kfree(slot) and vfree(slot->arch.rmap) while the hcall handler
still holds a raw pointer to the memslot.  lock_rmap() then writes to
freed vmalloc memory, leading to use-after-free and memory corruption.
Because kvm_memslots_raw() suppresses lockdep checks, this race is
entirely silent.

Fix this by factoring out the 7 HPT hcall handlers (H_REMOVE, H_ENTER,
H_READ, H_CLEAR_MOD, H_CLEAR_REF, H_PROTECT, H_BULK_REMOVE) into a
helper function, kvmppc_pseries_do_hpt_hcall(), and wrapping its call
site in srcu_read_lock(&kvm->srcu) / srcu_read_unlock(&kvm->srcu, idx).
Targeting only the HPT hcalls avoids wrapping non-HPT hcalls that sleep
(such as H_CONFER, H_REGISTER_VPA, H_PAGE_INIT) or handlers that already
acquire SRCU internally (such as H_RTAS).

Fixes: 6165d5dd99db ("KVM: PPC: Book3S HV: add virtual mode handlers for HPT hcalls and page faults")
Cc: stable@vger.kernel.org # v5.14+
Suggested-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Amit Machhiwal <amachhiw@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_hv.c | 70 ++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index dbac3573b2c8..aa51968e206a 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1159,6 +1159,38 @@ static long kvmppc_h_rpt_invalidate(struct kvm_vcpu *vcpu,
 	return H_SUCCESS;
 }
 
+static long kvmppc_pseries_do_hpt_hcall(struct kvm_vcpu *vcpu, unsigned long req)
+{
+	switch (req) {
+	case H_REMOVE:
+		return kvmppc_h_remove(vcpu, kvmppc_get_gpr(vcpu, 4),
+				       kvmppc_get_gpr(vcpu, 5),
+				       kvmppc_get_gpr(vcpu, 6));
+	case H_ENTER:
+		return kvmppc_h_enter(vcpu, kvmppc_get_gpr(vcpu, 4),
+				      kvmppc_get_gpr(vcpu, 5),
+				      kvmppc_get_gpr(vcpu, 6),
+				      kvmppc_get_gpr(vcpu, 7));
+	case H_READ:
+		return kvmppc_h_read(vcpu, kvmppc_get_gpr(vcpu, 4),
+				     kvmppc_get_gpr(vcpu, 5));
+	case H_CLEAR_MOD:
+		return kvmppc_h_clear_mod(vcpu, kvmppc_get_gpr(vcpu, 4),
+					  kvmppc_get_gpr(vcpu, 5));
+	case H_CLEAR_REF:
+		return kvmppc_h_clear_ref(vcpu, kvmppc_get_gpr(vcpu, 4),
+					  kvmppc_get_gpr(vcpu, 5));
+	case H_PROTECT:
+		return kvmppc_h_protect(vcpu, kvmppc_get_gpr(vcpu, 4),
+					kvmppc_get_gpr(vcpu, 5),
+					kvmppc_get_gpr(vcpu, 6));
+	case H_BULK_REMOVE:
+		return kvmppc_h_bulk_remove(vcpu);
+	}
+
+	return H_FUNCTION;
+}
+
 int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = vcpu->kvm;
@@ -1174,47 +1206,15 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
 
 	switch (req) {
 	case H_REMOVE:
-		ret = kvmppc_h_remove(vcpu, kvmppc_get_gpr(vcpu, 4),
-					kvmppc_get_gpr(vcpu, 5),
-					kvmppc_get_gpr(vcpu, 6));
-		if (ret == H_TOO_HARD)
-			return RESUME_HOST;
-		break;
 	case H_ENTER:
-		ret = kvmppc_h_enter(vcpu, kvmppc_get_gpr(vcpu, 4),
-					kvmppc_get_gpr(vcpu, 5),
-					kvmppc_get_gpr(vcpu, 6),
-					kvmppc_get_gpr(vcpu, 7));
-		if (ret == H_TOO_HARD)
-			return RESUME_HOST;
-		break;
 	case H_READ:
-		ret = kvmppc_h_read(vcpu, kvmppc_get_gpr(vcpu, 4),
-					kvmppc_get_gpr(vcpu, 5));
-		if (ret == H_TOO_HARD)
-			return RESUME_HOST;
-		break;
 	case H_CLEAR_MOD:
-		ret = kvmppc_h_clear_mod(vcpu, kvmppc_get_gpr(vcpu, 4),
-					kvmppc_get_gpr(vcpu, 5));
-		if (ret == H_TOO_HARD)
-			return RESUME_HOST;
-		break;
 	case H_CLEAR_REF:
-		ret = kvmppc_h_clear_ref(vcpu, kvmppc_get_gpr(vcpu, 4),
-					kvmppc_get_gpr(vcpu, 5));
-		if (ret == H_TOO_HARD)
-			return RESUME_HOST;
-		break;
 	case H_PROTECT:
-		ret = kvmppc_h_protect(vcpu, kvmppc_get_gpr(vcpu, 4),
-					kvmppc_get_gpr(vcpu, 5),
-					kvmppc_get_gpr(vcpu, 6));
-		if (ret == H_TOO_HARD)
-			return RESUME_HOST;
-		break;
 	case H_BULK_REMOVE:
-		ret = kvmppc_h_bulk_remove(vcpu);
+		idx = srcu_read_lock(&kvm->srcu);
+		ret = kvmppc_pseries_do_hpt_hcall(vcpu, req);
+		srcu_read_unlock(&kvm->srcu, idx);
 		if (ret == H_TOO_HARD)
 			return RESUME_HOST;
 		break;
-- 
2.54.0 (Apple Git-157)

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

[Amit Machhiwal]

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();
 	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) {
-- 
2.54.0 (Apple Git-157)

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

[sashiko-bot]

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

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

[Amit Machhiwal]

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

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

[Shrikanth Hegde]

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

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

[Amit Machhiwal]

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

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

[Shrikanth Hegde]

On 10/5/26 11:57 PM, Amit Machhiwal wrote:
> Hi Shrikanth,
> 
> Thanks for the review.  Please find my responses inline below.
> 

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

Fair enough.

Reviewed-by: Shrikanth Hegde <sshegde@linux.ibm.com>

[PATCH v2 3/3] KVM: PPC: Fix get_d_signext() 12-bit displacement for paired-single D-form

[Amit Machhiwal]

get_d_signext() has two compounding bugs since its introduction in 2010:

1. The extraction mask 0x8FF silently drops bits 8-10 of the 12-bit D
   field (PPC ISA bits 21-23), corrupting any displacement that has any
   of those bits set.

2. The sign-magnitude idiom "-(d & 0x7ff)" is wrong for two's-complement:
   for D=0xFFC (encoding of -4) it returns -252 instead of -4.

Together these errors produce a wrong effective address for psq_l, psq_lu,
psq_st, and psq_stu whenever the displacement is negative or is a positive
value >= 0x100 with bits 8-10 set — essentially any real-world paired-
single stack-relative access.

The D field occupies the bottom 12 bits of the instruction word (confirmed
by the adjacent W and I extractions via inst_get_field(inst,16,16) and
inst_get_field(inst,17,19)). Replace the open-coded logic with the standard
sign_extend32(inst & 0xfff, 11), which correctly performs two's-complement
sign extension from 12 bits to 32 bits.

Fixes: 831317b605e7 ("KVM: PPC: Implement Paired Single emulation")
Cc: stable@vger.kernel.org
Signed-off-by: Amit Machhiwal <amachhiw@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_paired_singles.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_paired_singles.c b/arch/powerpc/kvm/book3s_paired_singles.c
index bc39c76c9d9f..532f96293de0 100644
--- a/arch/powerpc/kvm/book3s_paired_singles.c
+++ b/arch/powerpc/kvm/book3s_paired_singles.c
@@ -479,12 +479,7 @@ static bool kvmppc_inst_is_paired_single(struct kvm_vcpu *vcpu, u32 inst)
 
 static int get_d_signext(u32 inst)
 {
-	int d = inst & 0x8ff;
-
-	if (d & 0x800)
-		return -(d & 0x7ff);
-
-	return (d & 0x7ff);
+	return sign_extend32(inst & 0xfff, 11);
 }
 
 static int kvmppc_ps_three_in(struct kvm_vcpu *vcpu, bool rc,
-- 
2.54.0 (Apple Git-157)

Re: [PATCH v2 3/3] KVM: PPC: Fix get_d_signext() 12-bit displacement for paired-single D-form

[Shrikanth Hegde]

On 9/30/26 11:07 PM, Amit Machhiwal wrote:
> get_d_signext() has two compounding bugs since its introduction in 2010:
> 
> 1. The extraction mask 0x8FF silently drops bits 8-10 of the 12-bit D
>     field (PPC ISA bits 21-23), corrupting any displacement that has any
>     of those bits set.
> 
> 2. The sign-magnitude idiom "-(d & 0x7ff)" is wrong for two's-complement:
>     for D=0xFFC (encoding of -4) it returns -252 instead of -4.
> 
> Together these errors produce a wrong effective address for psq_l, psq_lu,
> psq_st, and psq_stu whenever the displacement is negative or is a positive
> value >= 0x100 with bits 8-10 set — essentially any real-world paired-
> single stack-relative access.
> 
> The D field occupies the bottom 12 bits of the instruction word (confirmed
> by the adjacent W and I extractions via inst_get_field(inst,16,16) and
> inst_get_field(inst,17,19)). Replace the open-coded logic with the standard
> sign_extend32(inst & 0xfff, 11), which correctly performs two's-complement
> sign extension from 12 bits to 32 bits.
> 
> Fixes: 831317b605e7 ("KVM: PPC: Implement Paired Single emulation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Amit Machhiwal <amachhiw@linux.ibm.com>
> ---
>   arch/powerpc/kvm/book3s_paired_singles.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_paired_singles.c b/arch/powerpc/kvm/book3s_paired_singles.c
> index bc39c76c9d9f..532f96293de0 100644
> --- a/arch/powerpc/kvm/book3s_paired_singles.c
> +++ b/arch/powerpc/kvm/book3s_paired_singles.c
> @@ -479,12 +479,7 @@ static bool kvmppc_inst_is_paired_single(struct kvm_vcpu *vcpu, u32 inst)
>   
>   static int get_d_signext(u32 inst)
>   {
> -	int d = inst & 0x8ff;
> -
> -	if (d & 0x800)
> -		return -(d & 0x7ff);
> -
> -	return (d & 0x7ff);
> +	return sign_extend32(inst & 0xfff, 11);
>   }
>   
>   static int kvmppc_ps_three_in(struct kvm_vcpu *vcpu, bool rc,

If the intent was just to get the signed value based on bit11 (0 based index)
then patch looks right.

Reviewed-by: Shrikanth Hegde <sshegde@linux.ibm.com>