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. So, put this patch second, and replace the above paragraphs with something like? Calling kvm_pmu_deliver_pmi() twice is unlikely to be problematic now that KVM sets the LVTPC mask bit when delivering a PMI. But using IRQ work to trigger the PMI is still broken, albeit very theoretically. E.g. if the self-IPI to trigger IRQ work is be delayed long enough for the vCPU to be migrated to a different pCPU, then it's possible for kvm_pmi_trigger_fn() to race with the kvm_pmu_deliver_pmi() from KVM_REQ_PMI and still generate two PMIs. KVM could set the mask bit using an atomic operation, but that'd just be piling on unnecessary code to workaround what is effectively a hack. The *only* reason KVM uses IRQ work is to ensure the PMI is treated as a wake event, e.g. if the vCPU just executed HLT. > Remove the irq_work callback for synthesizing a PMI, and all of the > logic for invoking it. Instead, to prevent a vcpu from leaving C0 with > a PMI pending, add a check for KVM_REQ_PMI to kvm_vcpu_has_events().
