Re: [PATCH 5/8] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction

[Sean Christopherson <seanjc@google.com>]

On Wed, Apr 06, 2022, Maciej S. Szmigiero wrote:
> On 6.04.2022 19:10, Sean Christopherson wrote:
> > On Wed, Apr 06, 2022, Maciej S. Szmigiero wrote:
> And what if it's L0 that is trying to inject a NMI into L2?
> In this case is_guest_mode() is true, but the full NMI injection machinery
> should be used.

Gah, you're right, I got misled by a benign bug in nested_vmx_l1_wants_exit() and
was thinking that NMIs always exit.  The "L1 wants" part should be conditioned on
NMI exiting being enabled.  It's benign because KVM always wants "real" NMIs, and
so the path is never encountered.

@@ -5980,7 +6005,7 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu,
        switch ((u16)exit_reason.basic) {
        case EXIT_REASON_EXCEPTION_NMI:
                intr_info = vmx_get_intr_info(vcpu);
-               if (is_nmi(intr_info))
+               if (is_nmi(intr_info) && nested_cpu_has_nmi_exiting(vmcs12))
                        return true;
                else if (is_page_fault(intr_info))
                        return true;


> > > Also, *L2* being the target of such injection definitely should not block
> > > further NMIs for *L1*.
> > 
> > Actually, it should block NMIs for L1.  From L1's perspective, the injection is
> > part of VM-Entry.  That's a single gigantic instruction, thus there is no NMI window
> > until VM-Entry completes from L1's perspetive.  Any exit that occurs on vectoring
> > an injected event and is handled by L0 should not be visible to L1, because from
> > L1's perspective it's all part of VMRUN/VMLAUNCH/VMRESUME.  So blocking new events
> > because an NMI (or any event) needs to be reinjected for L2 is correct.
> 
> I think this kind of NMI blocking will be already handled by having
> the pending new NMI in vcpu->arch.nmi_pending but the one that needs
> re-injecting in vcpu->arch.nmi_injected.
> 
> The pending new NMI in vcpu->arch.nmi_pending won't be handled until
> vcpu->arch.nmi_injected gets cleared (that is, until re-injection is
> successful).

Yep.

> It is incorrect however, to wait for L2 to execute IRET to unblock
> L0 -> L1 NMIs or L1 -> L2 NMIs, in these two cases we (L0) just need the CPU
> to vector that L2 NMI so it no longer shows in EXITINTINFO.

Yep, and the pending NMI should cause KVM to request an "immediate" exit that
occurs after vectoring completes.

> It is also incorrect to block L1 -> L2 NMI injection because either L1
> or L2 is currently under NMI blocking: the first case is obvious,
> the second because it's L1 that is supposed to take care of proper NMI
> blocking for L2 when injecting an NMI there.

Yep, but I don't think there's a bug here.  At least not for nVMX.

> > > * When re-injecting a *hardware* IRQ into L2 GIF is checked (previously
> > > even on the BUG_ON() level), while L1 should be able to inject even when
> > > L2 GIF is off,
> > 
> > Isn't that just a matter of tweaking the assertion to ignore GIF if L2 is
> > active?  Hmm, or deleting the assertion altogether, it's likely doing more harm
> > than good at this point.
> 
> I assume this assertion is meant to catch the case when KVM itself (L0) is
> trying to erroneously inject a hardware interrupt into L1 or L2, so it will
> need to be skipped only for L1 -> L2 event injection.

Yeah, that's what my git archeaology came up with too.

> Whether this assertion benefits outweigh its costs is debatable - don't have
> a strong opinion here (BUG_ON() is for sure too strong, but WARN_ON_ONCE()
> might make sense to catch latent bugs).
> 
> > > With the code in my previous patch set I planned to use
> > > exit_during_event_injection() to detect such case, but if we implement
> > > VMCB12 EVENTINJ parsing we can simply add a flag that the relevant event
> > > comes from L1, so its normal injection side-effects should be skipped.
> > 
> > Do we still need a flag based on the above?  Honest question... I've been staring
> > at all this for the better part of an hour and may have lost track of things.
> 
> If checking just is_guest_mode() is not enough due to reasons I described
> above then we need to somehow determine in the NMI / IRQ injection handler
> whether the event to be injected into L2 comes from L0 or L1.
> For this (assuming we do VMCB12 EVENTINJ parsing) I think we need an extra flag.

Yes :-(  And I believe the extra flag would need to be handled by KVM_{G,S}ET_VCPU_EVENTS.
 
> > > By the way, the relevant VMX code also looks rather suspicious,
> > > especially for the !enable_vnmi case.
> > 
> > I think it's safe to say we can ignore edge cases for !enable_vnmi.  It might even
> > be worth trying to remove that support again (Paolo tried years ago), IIRC the
> > only Intel CPUs that don't support virtual NMIs are some funky Yonah SKUs.
> 
> Ack, we could at least disable nested on !enable_vnmi.
> 
> BTW, I think that besides Yonah cores very early Core 2 CPUs also lacked
> vNMI support, that's why !enable_vnmi support was reinstated.
> But that's hardware even older than !nrips AMD parts.