| Message ID | 20230925173448.3518223-3-mizhang@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] KVM: x86: Synthesize at most one PMI per VM-exit | expand |
On Mon, Sep 25, 2023, Mingwei Zhang wrote: > From: Jim Mattson <jmattson@google.com> > > Per the SDM, "When the local APIC handles a performance-monitoring > counters interrupt, it automatically sets the mask flag in the LVT > performance counter register." > > Add this behavior to KVM's local APIC emulation, to reduce the > incidence of "dazed and confused" spurious NMI warnings in Linux > guests (at least, those that use a PMI handler with "late_ack"). > > Fixes: 23930f9521c9 ("KVM: x86: Enable NMI Watchdog via in-kernel PIT source") This Fixes is wrong. Prior to commit f5132b01386b ("KVM: Expose a version 2 architectural PMU to a guests"), KVM didn't ever deliver interrupts via the LVTPC entry. E.g. prior to that commit, the only reference to APIC_LVTPC is in kvm_lapic_reg_write: arch/x86/kvm $ git grep APIC_LVTPC f5132b01386b^ f5132b01386b^:lapic.c: case APIC_LVTPC: Commit 23930f9521c9 definitely set the PMU support up to fail, but the bug would never have existed if kvm_deliver_pmi() had been written as: void kvm_deliver_pmi(struct kvm_vcpu *vcpu) { struct kvm_lapic *apic = vcpu->arch.apic; if (apic && kvm_apic_local_deliver(apic, APIC_LVTPC)) kvm_lapic_set_reg(apic, APIC_LVTPC, kvm_lapic_get_reg(apic, LVTPC) | APIC_LVT_MASKED); } And this needs an explicit Cc: to stable because KVM opts out of AUTOSEL. So Fixes: f5132b01386b ("KVM: Expose a version 2 architectural PMU to a guests") Cc: stable@vger.kernel.org > Signed-off-by: Jim Mattson <jmattson@google.com> > Tested-by: Mingwei Zhang <mizhang@google.com> When posting patches on behalf of others, you need to provide your SoB. > --- > arch/x86/kvm/lapic.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 113ca9661ab2..1f3d56a1f45f 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -2729,13 +2729,17 @@ int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type) > { > u32 reg = kvm_lapic_get_reg(apic, lvt_type); > int vector, mode, trig_mode; > + int r; > > if (kvm_apic_hw_enabled(apic) && !(reg & APIC_LVT_MASKED)) { > vector = reg & APIC_VECTOR_MASK; > mode = reg & APIC_MODE_MASK; > trig_mode = reg & APIC_LVT_LEVEL_TRIGGER; > - return __apic_accept_irq(apic, mode, vector, 1, trig_mode, > - NULL); > + > + r = __apic_accept_irq(apic, mode, vector, 1, trig_mode, NULL); > + if (r && lvt_type == APIC_LVTPC) > + kvm_lapic_set_reg(apic, lvt_type, reg | APIC_LVT_MASKED); Belated feedback, I think I'd prefer to write this as kvm_lapic_set_reg(apic, APIC_LVTPC, reg | APIC_LVT_MASKED); so that this code will show up when searching for APIC_LVTPC. > + return r; > } > return 0; > } > -- > 2.42.0.515.g380fc7ccd1-goog >
On Mon, Sep 25, 2023 at 10:52 AM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Sep 25, 2023, Mingwei Zhang wrote: > > From: Jim Mattson <jmattson@google.com> > > > > Per the SDM, "When the local APIC handles a performance-monitoring > > counters interrupt, it automatically sets the mask flag in the LVT > > performance counter register." > > > > Add this behavior to KVM's local APIC emulation, to reduce the > > incidence of "dazed and confused" spurious NMI warnings in Linux > > guests (at least, those that use a PMI handler with "late_ack"). > > > > Fixes: 23930f9521c9 ("KVM: x86: Enable NMI Watchdog via in-kernel PIT source") > > This Fixes is wrong. Prior to commit f5132b01386b ("KVM: Expose a version 2 > architectural PMU to a guests"), KVM didn't ever deliver interrupts via the LVTPC > entry. E.g. prior to that commit, the only reference to APIC_LVTPC is in > kvm_lapic_reg_write: > > arch/x86/kvm $ git grep APIC_LVTPC f5132b01386b^ > f5132b01386b^:lapic.c: case APIC_LVTPC: > > Commit 23930f9521c9 definitely set the PMU support up to fail, but the bug would > never have existed if kvm_deliver_pmi() had been written as: > > void kvm_deliver_pmi(struct kvm_vcpu *vcpu) > { > struct kvm_lapic *apic = vcpu->arch.apic; > > if (apic && kvm_apic_local_deliver(apic, APIC_LVTPC)) > kvm_lapic_set_reg(apic, APIC_LVTPC, > kvm_lapic_get_reg(apic, LVTPC) | APIC_LVT_MASKED); > } > > And this needs an explicit Cc: to stable because KVM opts out of AUTOSEL. > > So > > Fixes: f5132b01386b ("KVM: Expose a version 2 architectural PMU to a guests") > Cc: stable@vger.kernel.org > > > Signed-off-by: Jim Mattson <jmattson@google.com> > > Tested-by: Mingwei Zhang <mizhang@google.com> > > When posting patches on behalf of others, you need to provide your SoB. > > > --- > > arch/x86/kvm/lapic.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 113ca9661ab2..1f3d56a1f45f 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -2729,13 +2729,17 @@ int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type) > > { > > u32 reg = kvm_lapic_get_reg(apic, lvt_type); > > int vector, mode, trig_mode; > > + int r; > > > > if (kvm_apic_hw_enabled(apic) && !(reg & APIC_LVT_MASKED)) { > > vector = reg & APIC_VECTOR_MASK; > > mode = reg & APIC_MODE_MASK; > > trig_mode = reg & APIC_LVT_LEVEL_TRIGGER; > > - return __apic_accept_irq(apic, mode, vector, 1, trig_mode, > > - NULL); > > + > > + r = __apic_accept_irq(apic, mode, vector, 1, trig_mode, NULL); > > + if (r && lvt_type == APIC_LVTPC) > > + kvm_lapic_set_reg(apic, lvt_type, reg | APIC_LVT_MASKED); > > Belated feedback, I think I'd prefer to write this as > > kvm_lapic_set_reg(apic, APIC_LVTPC, reg | APIC_LVT_MASKED); > > so that this code will show up when searching for APIC_LVTPC. > > > + return r; > > } > > return 0; > > } > > -- > > 2.42.0.515.g380fc7ccd1-goog > > Signed-off-by: Mingwei Zhang <mizhang@google.com>
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 113ca9661ab2..1f3d56a1f45f 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -2729,13 +2729,17 @@ int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type) { u32 reg = kvm_lapic_get_reg(apic, lvt_type); int vector, mode, trig_mode; + int r; if (kvm_apic_hw_enabled(apic) && !(reg & APIC_LVT_MASKED)) { vector = reg & APIC_VECTOR_MASK; mode = reg & APIC_MODE_MASK; trig_mode = reg & APIC_LVT_LEVEL_TRIGGER; - return __apic_accept_irq(apic, mode, vector, 1, trig_mode, - NULL); + + r = __apic_accept_irq(apic, mode, vector, 1, trig_mode, NULL); + if (r && lvt_type == APIC_LVTPC) + kvm_lapic_set_reg(apic, lvt_type, reg | APIC_LVT_MASKED); + return r; } return 0; }