On Thu, 2023-01-19 at 15:37 +0000, Sean Christopherson wrote: > On Thu, Jan 19, 2023, Huang, Kai wrote: > > 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()? > > That's effectively what happens by unwinding up the stak with an error code. > Eventually the page fault handler will get the error and retry the guest. > > > Basically we treat TDX_OPERAND_BUSY as seamcall_needbreak(), similar to > > rwlock_needbreak(). I haven't thought about details though. > > I am strongly opposed to that approach. I do not want to pollute KVM's MMU code > with a bunch of retry logic and error handling just because the TDX module is > ultra paranoid and hostile to hypervisors. Right. But IIUC there's legal cases that SEPT SEAMCALL can return BUSY due to multiple threads trying to read/modify SEPT simultaneously in case of TDP MMU. For instance, parallel page faults on different vcpus on private pages. I believe this is the main reason to retry. We previously used spinlock around the SEAMCALLs to avoid, but looks that is not preferred. > > The problematic scenario of faulting indefinitely on a single instruction should > never happen under normal circumstances, and so KVM should treat such scenarios > as attacks/breakage and pass the buck to userspace. Totally agree zero-step attack can be treated KVM bug.
