Re: [PATCH] KVM: x86: Try to enable irr_pending state with disabled APICv

[Sean Christopherson <seanjc@google.com>]

On Thu, Oct 31, 2024, zhuangel570 wrote:
> On Thu, Oct 31, 2024 at 2:31 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Oct 23, 2024, Yong He wrote:
> > > From: Yong He <alexyonghe@tencent.com>
> > >
> > > Try to enable irr_pending when set APIC state, if there is
> > > pending interrupt in IRR with disabled APICv.
> > >
> > > In save/restore VM scenery with disabled APICv. Qemu/CloudHypervisor
> > > always send signals to stop running vcpu threads, then save
> > > entire VM state, including APIC state. There may be a pending
> > > timer interrupt in the saved APIC IRR that is injected before
> > > vcpu_run return. But when restoring the VM, since APICv is
> > > disabled, irr_pending is disabled by default, so this may cause
> > > the timer interrupt in the IRR to be suspended for a long time,
> > > until the next interrupt comes.
> > >
> > > Signed-off-by: Yong He <alexyonghe@tencent.com>
> > > ---
> > >  arch/x86/kvm/lapic.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 2098dc689088..7373f649958b 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -3099,6 +3099,10 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
> > >                                               apic_find_highest_irr(apic));
> > >               kvm_x86_call(hwapic_isr_update)(apic_find_highest_isr(apic));
> > >       }
> > > +
> > > +     /* Search the IRR and enable irr_pending state with disabled APICv*/
> > > +     if (!enable_apicv && apic_search_irr(apic) != -1)
> >
> > This can/should be an "else" from the above "if (apic->apicv_active)".  I also
> > think KVM can safely clear irr_pending in this case, which is also why irr_pending
> > isn't handling in kvm_apic_update_apicv().  When APICv is disabled (inhibited) at
> > runtime, an IRQ may be in-flight, i.e. apic_search_irr() can get a false negative.
> 
> Thank you for your review and suggestions.
> 
> >
> > But when stuffing APIC state, I don't see how that can happen.  So this?
> 
> Here is our case.

Sorry for not being clear.  I wasn't saying that the bug you encountered can't
happen.  I 100% agree it's a real bug.  What I was saying can't happen is an
in-flight virtual IRQ from another vCPU or device racing with setting APIC state,
and the VM (or VMM) having any expectation that everything would work correctly.

And so, I think it's save for KVM to explicitly set irr_pending, i.e. to potentially
_clear_ irr_pending.

If you are able to verify the psuedo-patch I posted fixes the bug you encountered,
that is how I would like to fix the bug.