[PATCH] KVM: x86: Fix lapic timer interrupt lost after loading a snapshot.

[PATCH] KVM: x86: Fix lapic timer interrupt lost after loading a snapshot.

[Haitao Shan]

This issue exists in kernel version 6.3-rc1 or above. The issue is
introduced by the commit 8e6ed96cdd50 ("KVM: x86: fire timer when it is
migrated and expired, and in oneshot mode"). The issue occurs on Intel
platform which APIC virtualization and posted interrupt processing.

The issue is first discovered when running the Android Emulator which
is based on QEMU 2.12. I can reproduce the issue with QEMU 8.0.4 in
Debian 12.

With the above commit, the timer gets fired immediately inside the
KVM_SET_LAPIC call when loading the snapshot. On such Intel platforms,
this eventually leads to setting the corresponding PIR bit. However,
the whole PIR bits get cleared later in the same KVM_SET_LAPIC call.
Missing lapic timer interrupt freeze the guest kernel.

Signed-off-by: Haitao Shan <hshan@google.com>
---
 arch/x86/kvm/lapic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index a983a16163b1..6f73406b875a 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2977,14 +2977,14 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
 	apic_update_lvtt(apic);
 	apic_manage_nmi_watchdog(apic, kvm_lapic_get_reg(apic, APIC_LVT0));
 	update_divide_count(apic);
-	__start_apic_timer(apic, APIC_TMCCT);
-	kvm_lapic_set_reg(apic, APIC_TMCCT, 0);
 	kvm_apic_update_apicv(vcpu);
 	if (apic->apicv_active) {
 		static_call_cond(kvm_x86_apicv_post_state_restore)(vcpu);
 		static_call_cond(kvm_x86_hwapic_irr_update)(vcpu, apic_find_highest_irr(apic));
 		static_call_cond(kvm_x86_hwapic_isr_update)(apic_find_highest_isr(apic));
 	}
+	__start_apic_timer(apic, APIC_TMCCT);
+	kvm_lapic_set_reg(apic, APIC_TMCCT, 0);
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	if (ioapic_in_kernel(vcpu->kvm))
 		kvm_rtc_eoi_tracking_restore_one(vcpu);
-- 
2.42.0.283.g2d96d420d3-goog

Re: [PATCH] KVM: x86: Fix lapic timer interrupt lost after loading a snapshot.

[Brett Creeley]

On 9/12/2023 9:15 AM, Haitao Shan wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> This issue exists in kernel version 6.3-rc1 or above. The issue is
> introduced by the commit 8e6ed96cdd50 ("KVM: x86: fire timer when it is
> migrated and expired, and in oneshot mode"). The issue occurs on Intel
> platform which APIC virtualization and posted interrupt processing.
> 
> The issue is first discovered when running the Android Emulator which
> is based on QEMU 2.12. I can reproduce the issue with QEMU 8.0.4 in
> Debian 12.
> 
> With the above commit, the timer gets fired immediately inside the
> KVM_SET_LAPIC call when loading the snapshot. On such Intel platforms,
> this eventually leads to setting the corresponding PIR bit. However,
> the whole PIR bits get cleared later in the same KVM_SET_LAPIC call.
> Missing lapic timer interrupt freeze the guest kernel.
> 

Should there be a "Fixes" tag here with the problematic commit?

Thanks,

Brett

> Signed-off-by: Haitao Shan <hshan@google.com>
> ---
>   arch/x86/kvm/lapic.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index a983a16163b1..6f73406b875a 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2977,14 +2977,14 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
>          apic_update_lvtt(apic);
>          apic_manage_nmi_watchdog(apic, kvm_lapic_get_reg(apic, APIC_LVT0));
>          update_divide_count(apic);
> -       __start_apic_timer(apic, APIC_TMCCT);
> -       kvm_lapic_set_reg(apic, APIC_TMCCT, 0);
>          kvm_apic_update_apicv(vcpu);
>          if (apic->apicv_active) {
>                  static_call_cond(kvm_x86_apicv_post_state_restore)(vcpu);
>                  static_call_cond(kvm_x86_hwapic_irr_update)(vcpu, apic_find_highest_irr(apic));
>                  static_call_cond(kvm_x86_hwapic_isr_update)(apic_find_highest_isr(apic));
>          }
> +       __start_apic_timer(apic, APIC_TMCCT);
> +       kvm_lapic_set_reg(apic, APIC_TMCCT, 0);
>          kvm_make_request(KVM_REQ_EVENT, vcpu);
>          if (ioapic_in_kernel(vcpu->kvm))
>                  kvm_rtc_eoi_tracking_restore_one(vcpu);
> --
> 2.42.0.283.g2d96d420d3-goog
> 

Re: [PATCH] KVM: x86: Fix lapic timer interrupt lost after loading a snapshot.

[Sean Christopherson]

On Tue, Sep 12, 2023, Haitao Shan wrote:
> This issue exists in kernel version 6.3-rc1 or above. The issue is
> introduced by the commit 8e6ed96cdd50 ("KVM: x86: fire timer when it is
> migrated and expired, and in oneshot mode"). The issue occurs on Intel
> platform which APIC virtualization and posted interrupt processing.

I think the bug was actually introduced by:

  967235d32032 ("KVM: vmx: clear pending interrupts on KVM_SET_LAPIC")

Fixing the "deadline <= 0" handling just made it much easier to be hit.  E.g. if
the deadline was '1' during restore, set_target_expiration() would set tscdeadline
to T1+(1*N), where T1 is the current TSC and N is the multipler to get from nanoseconds
to cycles.  start_sw_tscdeadline() (or vmx_set_hv_timer()) would then reread the
TSC (call it T2), see T2 > T1+(1*N), and mark the timer as expired.

> The issue is first discovered when running the Android Emulator which
> is based on QEMU 2.12. I can reproduce the issue with QEMU 8.0.4 in
> Debian 12.

The above is helpful as extra context, but repeating "This issue" and "The issue"
over and over without ever actually describing what the issue actualy is makes it
quite difficult to understand what is actually being fixed.

> With the above commit, the timer gets fired immediately inside the
> KVM_SET_LAPIC call when loading the snapshot. On such Intel platforms,
> this eventually leads to setting the corresponding PIR bit. However,
> the whole PIR bits get cleared later in the same KVM_SET_LAPIC call.
> Missing lapic timer interrupt freeze the guest kernel.

Please phrase changelogs as commands and state what is actually being changed.
Again, the context on what is broken is helpful, but the changelog really, really
needs to state what is being changed.

> Signed-off-by: Haitao Shan <hshan@google.com>
> ---
>  arch/x86/kvm/lapic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index a983a16163b1..6f73406b875a 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2977,14 +2977,14 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
>  	apic_update_lvtt(apic);
>  	apic_manage_nmi_watchdog(apic, kvm_lapic_get_reg(apic, APIC_LVT0));
>  	update_divide_count(apic);
> -	__start_apic_timer(apic, APIC_TMCCT);
> -	kvm_lapic_set_reg(apic, APIC_TMCCT, 0);
>  	kvm_apic_update_apicv(vcpu);
>  	if (apic->apicv_active) {
>  		static_call_cond(kvm_x86_apicv_post_state_restore)(vcpu);
>  		static_call_cond(kvm_x86_hwapic_irr_update)(vcpu, apic_find_highest_irr(apic));
>  		static_call_cond(kvm_x86_hwapic_isr_update)(apic_find_highest_isr(apic));
>  	}
> +	__start_apic_timer(apic, APIC_TMCCT);
> +	kvm_lapic_set_reg(apic, APIC_TMCCT, 0);

I don't think this is the ordering we want.  It currently works, but it subtly
"relies" on a few things:

  1. That vmx_deliver_posted_interrupt() never "fails" when APICv is enabled,
     i.e. never puts the interrupt in the IRR instead of the PIR.

  2. The SVM, a.k.a. AVIC, doesn't ever sync from the IRR to a separate "hardware"
     virtual APIC, because unlike VMX, SVM does set the bit in the IRR.

I doubt #2 will ever change simply because that's tied to how AVIC works, and #1
shouldn't actually break anything since the fallback path in vmx_deliver_interrupt()
needs to be self-sufficient, but I don't like that the code syncs from the IRR and
_then_ potentially modifies the IRR.

I also don't like doing additional APIC state restoration _after_ invoking the
post_state_restore() hook.  Updating APICv in the middle of the restore flow is
going to be brittle and difficult to maintain, e.g. it won't be obvious what
needs to go before and what needs to go after.

IMO, vmx_apicv_post_state_restore() is blatantly broken.  It is most definitely
not doing "post state restore" stuff, it's simply purging state, i.e. belongs in
a "pre state restore" hook.

So rather than shuffle around the timer code, I think we should instead add yet
another kvm_x86_ops hook, e.g. apicv_pre_state_restore(), and have initialize
the PI descriptor there.

Aha!  And I think the new apicv_pre_state_restore() needs to be invoked even if
APICv is not active, because I don't see anything that purges the PIR when APICv
is enabled.  VMX's APICv doesn't have many inhibits that can go away, and I
highly doubt userspace will restore into a vCPU with pending posted interrupts,
so in practice this is _extremely_ unlikely to be problematic.  But it's still
wrong.

Re: [PATCH] KVM: x86: Fix lapic timer interrupt lost after loading a snapshot.

[Sean Christopherson]

Please send mail as plain text, most of the mailing lists reject HTML, and in
general HTML makes everyone grumpy.

On Tue, Sep 12, 2023, Haitao Shan wrote:
> On Tue, Sep 12, 2023 at 10:07 AM Sean Christopherson <seanjc@google.com>
> wrote:
> 
> > Aha!  And I think the new apicv_pre_state_restore() needs to be invoked
> > even if APICv is not active, because I don't see anything that purges the
> > PIR when APICv is enabled.  VMX's APICv doesn't have many inhibits that can
> > go away, and I highly doubt userspace will restore into a vCPU with pending
> > posted interrupts, so in practice this is _extremely_ unlikely to be
> > problematic.  But it's still wrong.
> >
> Thanks for sharing what you would like to fix the bug. I will write a v2 for
> that.  Actually, I am sorry that I forgot to add RFC to the title, as I
> personally did not think the proposed fix looks clean.

No worries, thanks for taking this on!

Re: [PATCH] KVM: x86: Fix lapic timer interrupt lost after loading a snapshot.

[Haitao Shan]

Sorry for sending the reply twice. The former reply got rejected since
I forgot to turn on the plain text email setting.

On Tue, Sep 12, 2023 at 10:07 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Sep 12, 2023, Haitao Shan wrote:
> > This issue exists in kernel version 6.3-rc1 or above. The issue is
> > introduced by the commit 8e6ed96cdd50 ("KVM: x86: fire timer when it is
> > migrated and expired, and in oneshot mode"). The issue occurs on Intel
> > platform which APIC virtualization and posted interrupt processing.
>
> I think the bug was actually introduced by:
>
>   967235d32032 ("KVM: vmx: clear pending interrupts on KVM_SET_LAPIC")
Thanks for pointing this out. I know commit 8e6ed96cdd50 is only a
trigger. But I
did not go one step further and find out where the bug is coming from.
>
> Fixing the "deadline <= 0" handling just made it much easier to be hit.  E.g. if
> the deadline was '1' during restore, set_target_expiration() would set tscdeadline
> to T1+(1*N), where T1 is the current TSC and N is the multipler to get from nanoseconds
> to cycles.  start_sw_tscdeadline() (or vmx_set_hv_timer()) would then reread the
> TSC (call it T2), see T2 > T1+(1*N), and mark the timer as expired.
>
> > The issue is first discovered when running the Android Emulator which
> > is based on QEMU 2.12. I can reproduce the issue with QEMU 8.0.4 in
> > Debian 12.
>
> The above is helpful as extra context, but repeating "This issue" and "The issue"
> over and over without ever actually describing what the issue actualy is makes it
> quite difficult to understand what is actually being fixed.
Got it. I will rewrite the whole commit message for v2.
>
> > With the above commit, the timer gets fired immediately inside the
> > KVM_SET_LAPIC call when loading the snapshot. On such Intel platforms,
> > this eventually leads to setting the corresponding PIR bit. However,
> > the whole PIR bits get cleared later in the same KVM_SET_LAPIC call.
> > Missing lapic timer interrupt freeze the guest kernel.
>
> Please phrase changelogs as commands and state what is actually being changed.
> Again, the context on what is broken is helpful, but the changelog really, really
> needs to state what is being changed.
Will do.
>
> > Signed-off-by: Haitao Shan <hshan@google.com>
> > ---
> >  arch/x86/kvm/lapic.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index a983a16163b1..6f73406b875a 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -2977,14 +2977,14 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
> >       apic_update_lvtt(apic);
> >       apic_manage_nmi_watchdog(apic, kvm_lapic_get_reg(apic, APIC_LVT0));
> >       update_divide_count(apic);
> > -     __start_apic_timer(apic, APIC_TMCCT);
> > -     kvm_lapic_set_reg(apic, APIC_TMCCT, 0);
> >       kvm_apic_update_apicv(vcpu);
> >       if (apic->apicv_active) {
> >               static_call_cond(kvm_x86_apicv_post_state_restore)(vcpu);
> >               static_call_cond(kvm_x86_hwapic_irr_update)(vcpu, apic_find_highest_irr(apic));
> >               static_call_cond(kvm_x86_hwapic_isr_update)(apic_find_highest_isr(apic));
> >       }
> > +     __start_apic_timer(apic, APIC_TMCCT);
> > +     kvm_lapic_set_reg(apic, APIC_TMCCT, 0);
>
> I don't think this is the ordering we want.  It currently works, but it subtly
> "relies" on a few things:
>
>   1. That vmx_deliver_posted_interrupt() never "fails" when APICv is enabled,
>      i.e. never puts the interrupt in the IRR instead of the PIR.
>
>   2. The SVM, a.k.a. AVIC, doesn't ever sync from the IRR to a separate "hardware"
>      virtual APIC, because unlike VMX, SVM does set the bit in the IRR.
>
> I doubt #2 will ever change simply because that's tied to how AVIC works, and #1
> shouldn't actually break anything since the fallback path in vmx_deliver_interrupt()
> needs to be self-sufficient, but I don't like that the code syncs from the IRR and
> _then_ potentially modifies the IRR.
>
> I also don't like doing additional APIC state restoration _after_ invoking the
> post_state_restore() hook.  Updating APICv in the middle of the restore flow is
> going to be brittle and difficult to maintain, e.g. it won't be obvious what
> needs to go before and what needs to go after.
>
> IMO, vmx_apicv_post_state_restore() is blatantly broken.  It is most definitely
> not doing "post state restore" stuff, it's simply purging state, i.e. belongs in
> a "pre state restore" hook.
>
> So rather than shuffle around the timer code, I think we should instead add yet
> another kvm_x86_ops hook, e.g. apicv_pre_state_restore(), and have initialize
> the PI descriptor there.
>
> Aha!  And I think the new apicv_pre_state_restore() needs to be invoked even if
> APICv is not active, because I don't see anything that purges the PIR when APICv
> is enabled.  VMX's APICv doesn't have many inhibits that can go away, and I
> highly doubt userspace will restore into a vCPU with pending posted interrupts,
> so in practice this is _extremely_ unlikely to be problematic.  But it's still
> wrong.
Thanks for sharing what you would like to fix the bug. I will write a
v2 for that.
Actually, I am sorry that I forgot to add RFC to the title, as I
personally did not think
the proposed fix looks clean. I am surprised that
apic_post_state_restore actually
clears the whole PIR which looks like "resetting" instead of
"restoring". However,
I am not sure whether this is the exact intended behavior and code sequence. If
it is intended, __restart_apic_timer should defer its interrupt
delivery action after
apic_post_state_restore (something like raising a request for updating PIR when
vcpu_enter_guest).

I will work on v2 following your suggestion. Thanks!

-- 
Haitao @Google