On Tue, 2023-01-17 at 21:01 +0000, Sean Christopherson wrote: > On Tue, Jan 17, 2023, Sean Christopherson wrote: > > On Tue, Jan 17, 2023, Zhi Wang wrote: > > > 2) As TDX module doesn't provide contention-and-wait, I guess the following > > > approach might have been discussed when designing this "retry". > > > > > > KERNEL TDX MODULE > > > > > > SEAMCALL A -> PATH A: Taking locks > > > > > > SEAMCALL B -> PATH B: Contention on a lock > > > > > > <- Return "operand busy" > > > > > > SEAMCALL B -| > > > | <- Wait on a kernel waitqueue > > > SEAMCALL B <-| > > > > > > SEAMCALL A <- PATH A: Return > > > > > > SEAMCALL A -| > > > | <- Wake up the waitqueue > > > SEMACALL A <-| > > > > > > SEAMCALL B -> PATH B: Taking the locks > > > ... > > > > > > Why not this scheme wasn't chosen? > > > > AFAIK, I don't think a waitqueue approach as ever been discussed publicly. Intel > > may have considered the idea internally, but I don't recall anything being proposed > > publically (though it's entirely possible I just missed the discussion). > > > > Anways, I don't think a waitqueue would be a good fit, at least not for S-EPT > > management, which AFAICT is the only scenario where KVM does the arbitrary "retry > > X times and hope things work". If the contention occurs due to the TDX Module > > taking an S-EPT lock in VM-Enter, then KVM won't get a chance to do the "Wake up > > the waitqueue" action until the next VM-Exit, which IIUC is well after the TDX > > Module drops the S-EPT lock. In other words, immediately retrying and then punting > > the problem further up the stack in KVM does seem to be the least awful "solution" > > if there's contention. > > Oh, the other important piece I forgot to mention is that dropping mmu_lock deep > in KVM's MMU in order to wait isn't always an option. Most flows would play nice > with dropping mmu_lock and sleeping, but some paths, e.g. from the mmu_notifier, > (conditionally) disallow sleeping. Could we do something similar to tdp_mmu_iter_cond_resched() but not simple busy retrying "X times", at least at those paths that can release mmu_lock()? Basically we treat TDX_OPERAND_BUSY as seamcall_needbreak(), similar to rwlock_needbreak(). I haven't thought about details though.
