Re: [PATCH 3/7] KVM: TDX: Retry locally in TDX EPT violation handler on RET_PF_RETRY

[Yan Zhao <yan.y.zhao@intel.com>]

On Mon, Jan 27, 2025 at 09:04:59AM -0800, Sean Christopherson wrote:
> On Mon, Jan 27, 2025, Yan Zhao wrote:
> > On Fri, Jan 24, 2025 at 05:23:36PM -0800, Sean Christopherson wrote:
> > My previous consideration is that
> > when there's a pending interrupt that can be recognized, given the current VM
> > Exit reason is EPT violation, the next VM Entry will not deliver the interrupt
> > since the condition to recognize and deliver interrupt is unchanged after the
> > EPT violation VM Exit.
> > So checking pending interrupt brings only false positive, which is unlike
> > checking PID that the vector in the PID could arrive after the EPT violation VM
> > Exit and PID would be cleared after VM Entry even if the interrupts are not
> > deliverable. So checking PID may lead to true positive and less false positive.
> > 
> > But I understand your point now. As checking PID can also be false positive, it
> > would be no harm to introduce another source of false positive.
> 
> Yep.  FWIW, I agree that checking VMXIP is theoretically "worse", in the sense
> that it's much more likely to be a false positive.  Off the top of my head, the
> only time VMXIP will be set with RFLAGS.IF=1 on an EPT Violation is if the EPT
> violation happens in an STI or MOV_SS/POP_SS shadow.
> 
> > So using kvm_vcpu_has_events() looks like a kind of trade-off?
> > kvm_vcpu_has_events() can make TDX's code less special but might lead to the
> > local vCPU more vulnerable to the 0-step mitigation, especially when interrupts
> > are disabled in the guest.
> 
> Ya.  I think it's worth worth using kvm_vcpu_has_events() though.  In practice,
> observing VMXIP=1 with RFLAGS=0 on an EPT Violation means the guest is accessing
> memory for the first time in atomic kernel context.  That alone seems highly
> unlikely.  Add in that triggering retry requires an uncommon race, and the overall
> probability becomes miniscule.
Ok. Will use kvm_vcpu_has_events() instead.

> 
> > > That code needs a comment, because depending on the behavior of that field, it
> > > might not even be correct.
> > > 
> > > > (2) kvm_vcpu_has_events() may lead to unnecessary breaks due to exception
> > > >     pending. However, vt_inject_exception() is NULL for TDs.
> > > 
> > > Wouldn't a pending exception be a KVM bug?
> > Hmm, yes, it should be.
> > Although kvm_vcpu_ioctl_x86_set_mce() can invoke kvm_queue_exception() to queue
> > an exception for TDs, 
> 
> I thought TDX didn't support synthetic #MCs?
Hmm, it's just an example to show kvm_queue_exception() can be invoked for a TD,
as it's not disallowed :)
Another example is kvm_arch_vcpu_ioctl_set_guest_debug() when
vcpu->arch.guest_state_protected is false for a DEBUG TD.

But as you said, they are not possible to be called during vcpu_run().

> > this should not occur while VCPU_RUN is in progress.
> 
> Not "should not", _cannot_, because they're mutually exclusive.
Right, "cannot" is the accurate term.