On Thu, 2025-08-28 at 13:56 +0800, Yan Zhao wrote: > > Reasons that tdh_mem_page_add() could get BUSY: > > 1. If two vCPU's tried to tdh_mem_page_add() the same gpa at the same time > > they > > could contend the SEPT entry lock > > 2. If one vCPU tries to tdh_mem_page_add() while the other zaps (i.e. > > tdh_mem_range_block()). > Hmm, two tdh_mem_page_add()s can't contend as they are protected by both > slot_lock and filemap lock. > > With regard to the contention to tdh_mem_range_block(), please check my > analysis at the above [1]. The analysis missed the tdh_mem_page_add() failure path > > tdh_mem_page_add() could get BUSY though, when a misbehaved userspace invokes > KVM_TDX_INIT_MEM_REGION on one vCPU while initializing another vCPU. > > Please check more details at [2]. > > [2] https://lore.kernel.org/kvm/20250113021050.18828-1-yan.y.zhao@xxxxxxxxx/ Ah, the TDR lock. I actually referred to an older version of your locking analysis that didn't have that one. But this means the premap count could get out of sync for that reason too. > > > > I guess since we don't hold MMU lock while we tdh_mem_page_add(), 2 is a > > possibility. > 2 is possible only for paranoid zaps. > See "case 3. Unexpected zaps" in [1]. Sean's lockdep assert handles half of those cases. Maybe we could also re- consider a KVM_BUG_ON() in the invalid zap paths again if it comes to it. > > > > > What reasonable use case is there for gracefully handling > > > tdh_mem_page_add() > > > failure? > > > > > > If there is a need to handle failure, I gotta imagine it's only for the - > > > EBUSY > > > case. And if it's only for -EBUSY, why can't that be handled by retrying > > > in > > > tdx_vcpu_init_mem_region()? If tdx_vcpu_init_mem_region() guarantees that > > > all > > > pages mapped into the S-EPT are ADDed, then it can assert that there are > > > no > > > pending pages when it completes (even if it "fails"), and similarly > > > tdx_td_finalize() can KVM_BUG_ON/WARN_ON the number of pending pages being > > > non-zero. > > > > Maybe we could take mmu write lock for the retry of tdh_mem_page_add(). Or > > maybe > > even for a single call of it, until someone wants to parallelize the > > operation. > Hmm. I prefer returning -BUSY directly as invoking KVM_TDX_INIT_MEM_REGION > before finishing initializing all vCPUs are uncommon. I was looking guaranteeing its success when Sean posted his suggestion to return to the original pattern. I'm in favor of that direction. If you agree we can call this moot.
