lkml.org 
[lkml]   [2023]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH 2/2] KVM: x86: Mask LVTPC when handling a PMI
    From
    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
    >

    \
     
     \ /
      Last update: 2023-09-25 19:52    [W:3.052 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site