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: [email protected] # v5.14+
> > Signed-off-by: Amit Machhiwal <[email protected]>
> > ---
> > 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) {
>
