Hi Ritesh, Thanks for reviewing this patch. Please find my response below.
On 2026/10/07 11:48 AM, Ritesh Harjani wrote: > Amit Machhiwal <[email protected]> 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
