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

[Amit Machhiwal]

Hi Ritesh,

Thanks for reviewing this patch.  Please find my response below.

On 2026/10/07 11:48 AM, Ritesh Harjani wrote:
> Amit Machhiwal <amachhiw@xxxxxxxxxxxxx> writes:
> 
> > 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.
> >
> 
> I was going over all the callers of lock_rmap() and try_lock_hpte() on,
> and I see that we might have missed kvm_htab_write() path...
> 
> ... after spending sometime looks like we need this diff for
> kvm_htab_write() path as well, since it calls kvmppc_do_h_remove() which
> calls try_lock_hpte() and lock_rmap(), although the race window is much
> narrower and maybe very hard to hit.
> 
> But still, could you kindly look into this and if needed please take it
> forward too. Please note that this is not tested, so hoping that you
> could take care of that too.

Thanks for catching this — you're right, that was a genuine miss.  I will fix it
in v4 by introducing kvmppc_virtmode_do_h_remove() as you suggested.

One small divergence from your proposed diff: I retained the full flags and avpn
parameters rather than baking in 0, 0.  The flags field carries the PAPR-defined
H_AVPN and H_ANDCOND conditional guards, and a future virtual-mode caller may
legitimately need them.  Baking the constants in would silently prevent that,
while passing them through costs nothing.  This mirrors exactly how
kvmppc_virtmode_do_h_enter() is structured.

I'm in the middle of making the changes and testing the same.  The v4 will be
out soon.

Thanks,
Amit