Re: [PATCH 2/2] KVM: x86: Mask LVTPC when handling a PMI

[Mingwei Zhang]

On Mon, Sep 25, 2023 at 10:52 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Mon, Sep 25, 2023, Mingwei Zhang wrote:
> > From: Jim Mattson <jmattson@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
>
> > Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
> > Tested-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>