On Wed, Feb 08, 2023, Santosh Shukla wrote: > On 2/1/2023 5:36 AM, Sean Christopherson wrote: > > On Tue, Jan 31, 2023, Sean Christopherson wrote: > >> On Tue, Nov 29, 2022, Maxim Levitsky wrote: > >>> @@ -10015,13 +10022,34 @@ static void process_nmi(struct kvm_vcpu *vcpu) > >>> * Otherwise, allow two (and we'll inject the first one immediately). > >>> */ > >>> if (static_call(kvm_x86_get_nmi_mask)(vcpu) || vcpu->arch.nmi_injected) > >>> - limit = 1; > >>> + limit--; > >>> + > >>> + /* Also if there is already a NMI hardware queued to be injected, > >>> + * decrease the limit again > >>> + */ > >>> + if (static_call(kvm_x86_get_hw_nmi_pending)(vcpu)) > >>> + limit--; > >> > >> I don't think this is correct. If a vNMI is pending and NMIs are blocked, then > >> limit will end up '0' and KVM will fail to pend the additional NMI in software. > > > > Scratch that, dropping the second NMI in this case is correct. The "running" part > > of the existing "x86 is limited to one NMI running, and one NMI pending after it" > > confused me. The "running" thing is really just a variant on NMIs being blocked. > > > > I'd also like to avoid the double decrement logic. Accounting the virtual NMI is > > a very different thing than dealing with concurrent NMIs, I'd prefer to reflect > > that in the code. > > > > Any objection to folding in the below to end up with: > > > > unsigned limit; > > > > /* > > * x86 is limited to one NMI pending, but because KVM can't react to > > * incoming NMIs as quickly as bare metal, e.g. if the vCPU is > > * scheduled out, KVM needs to play nice with two queued NMIs showing > > * up at the same time. To handle this scenario, allow two NMIs to be > > * (temporarily) pending so long as NMIs are not blocked and KVM is not > > * waiting for a previous NMI injection to complete (which effectively > > * blocks NMIs). KVM will immediately inject one of the two NMIs, and > > * will request an NMI window to handle the second NMI. > > */ > > if (static_call(kvm_x86_get_nmi_mask)(vcpu) || vcpu->arch.nmi_injected) > > limit = 1; > > else > > limit = 2; > > > > /* > > * Adjust the limit to account for pending virtual NMIs, which aren't > > * tracked in in vcpu->arch.nmi_pending. > > */ > > if (static_call(kvm_x86_is_vnmi_pending)(vcpu)) > > limit--; > > > > vcpu->arch.nmi_pending += atomic_xchg(&vcpu->arch.nmi_queued, 0); > > vcpu->arch.nmi_pending = min(vcpu->arch.nmi_pending, limit); > > > > I believe, you missed the function below hunk - > > if (vcpu->arch.nmi_pending && > static_call(kvm_x86_set_vnmi_pending(vcpu))) > vcpu->arch.nmi_pending--; > > Or am I missing something.. please suggest. You're not missing anything, I'm pretty sure I just lost tracking of things.
