Re: KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sat, Jun 18, 2016 at 02:43:51PM +0100, Alan Jenkins wrote:
> On 18/06/2016, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> >
> >Alan Jenkins reports hang at
> >https://bugzilla.redhat.com/show_bug.cgi?id=1337667.
> >
> >* APIC write: expiration = 1000.
> >* LAPIC tsc deadline code sets timer to 1000-30.
> >* Timer fires at 970.
> >* Guest writes TSC=w.
> >
> >Guest fails to VM-entry to process signal to perform
> >"vmload" in userspace.
> >
> >Case 1: w > 970:
> >Guest entry can be performed.
> >
> >Case 2: w < 970:
> >Guest entry should not be performed because "An interrupt is generated
> >when the logical processor’s time-stamp counter equals or exceeds the
> >target value in the IA32_TSC_DEADLINE MSR."
> >
> >Setting of APIC test resets all APIC state, including
> >any pending interrupts.
> >
> >Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index ea306ad..89be6e9 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -2991,6 +2991,7 @@ static int kvm_vcpu_ioctl_set_lapic(struct kvm_vcpu
> >*vcpu,
> > {
> > 	kvm_apic_post_state_restore(vcpu, s);
> > 	update_cr8_intercept(vcpu);
> >+	vcpu->arch.apic->lapic_timer.expired_tscdeadline = 0;
> >
> > 	return 0;
> > }
> 
> Hi
> 
> I'm happy to check my own test if this is considered a solution.
> (And apologies for how my own efforts go... flailing around and not
> always communicating effectively...)

Your report had many details.

> 1) I think the commit message could be improved.  Specifically it
> says the problem is due to TSC write, then that we need to fix LAPIC
> reset. It doesn't say what the connection is between the two.

Right.


> (I.e. you would have to read the bug report, & then realize that it
> _happens_ to involve a LAPIC state restore as well, because it's
> triggered by reloading the entire VM state from a snapshot)
> 
> Doing so might make the intention of this commit clearer.  I've been
> trying to work out a high quality fix, which prevents the lockups
> revealed by this case.  I think this commit would fix my bug nicely,
> but it doesn't help e.g. if the LAPIC restore was omitted from the
> sequence.
> 
> (Both TSC and the value of TSC deadline are set by MSRs, not by
> writing to the LAPIC. So I don't think the LAPIC state restore is
> necessary to trigger the issue).
> 
> 
> 2) If this approach is considered expedient, I have two comments -
> 
> i) isn't it better to set lapic_timer.expired_tscdeadline in
> lapic.c, i.e. in apic_post_state_restore() ?
> 
> ii) then, wouldn't it be more correct to set it between
> hrtimer_cancel() and start_apic_timer()?  It doesn't sound right to
> clear expired_tscdeadline, in case it was set immediately by
> start_apic_timer()
> 
> Alan

Advancing the timer expiration should only be necessary on
guest initiated writes. Lets improve the commit message.

Can you confirm the patch fixes your problem?