Re: [PATCH 1/2] KVM: x86: Synthesize at most one PMI per VM-exit

[Sean Christopherson <seanjc@google.com>]

On Fri, Sep 22, 2023, Jim Mattson wrote:
> On Fri, Sep 22, 2023 at 11:46 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Sep 01, 2023, Jim Mattson wrote:
> > > When the irq_work callback, kvm_pmi_trigger_fn(), is invoked during a
> > > VM-exit that also invokes __kvm_perf_overflow() as a result of
> > > instruction emulation, kvm_pmu_deliver_pmi() will be called twice
> > > before the next VM-entry.
> > >
> > > That shouldn't be a problem. The local APIC is supposed to
> > > automatically set the mask flag in LVTPC when it handles a PMI, so the
> > > second PMI should be inhibited. However, KVM's local APIC emulation
> > > fails to set the mask flag in LVTPC when it handles a PMI, so two PMIs
> > > are delivered via the local APIC. In the common case, where LVTPC is
> > > configured to deliver an NMI, the first NMI is vectored through the
> > > guest IDT, and the second one is held pending. When the NMI handler
> > > returns, the second NMI is vectored through the IDT. For Linux guests,
> > > this results in the "dazed and confused" spurious NMI message.
> > >
> > > Though the obvious fix is to set the mask flag in LVTPC when handling
> > > a PMI, KVM's logic around synthesizing a PMI is unnecessarily
> > > convoluted.
> >
> > To address Like's question about whether not this is necessary, I think we should
> > rephrase this to explicitly state this is a bug irrespective of the whole LVTPC
> > masking thing.
> >
> > And I think it makes sense to swap the order of the two patches.  The LVTPC masking
> > fix is a clearcut architectural violation.  This is a bit more of a grey area,
> > though still blatantly buggy.
> 
> The reason I ordered the patches as I did is that when this patch
> comes first, it actually fixes the problem that was introduced in
> commit 9cd803d496e7 ("KVM: x86: Update vPMCs when retiring
> instructions"). If this patch comes second, it's less clear that it
> fixes a bug, since the other patch renders this one essentially moot.

Yeah, but as Like pointed out, the way the changelog is worded just raises the
question of why this change is necessary.

I think we should tag them both for stable.  They're both bug fixes, regardless
of the ordering.