From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alan Jenkins Subject: Re: KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time Date: Sat, 18 Jun 2016 14:43:51 +0100 Message-ID: <6bc78368-d559-fa09-7e77-389b0e87d695@gmail.com> References: <20160617234126.GA24514@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm-devel , Paolo Bonzini To: Marcelo Tosatti Return-path: Received: from mail-wm0-f65.google.com ([74.125.82.65]:33266 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751172AbcFRNnz (ORCPT ); Sat, 18 Jun 2016 09:43:55 -0400 Received: by mail-wm0-f65.google.com with SMTP id r201so3889922wme.0 for ; Sat, 18 Jun 2016 06:43:55 -0700 (PDT) In-Reply-To: <20160617234126.GA24514@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 18/06/2016, Marcelo Tosatti wrote: > > Alan Jenkins reports hang at > https://bugzilla.redhat.com/show_bug.cgi?id=3D1337667. > > * APIC write: expiration =3D 1000. > * LAPIC tsc deadline code sets timer to 1000-30. > * Timer fires at 970. > * Guest writes TSC=3Dw. > > 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 generate= d > when the logical processor=E2=80=99s time-stamp counter equals or exc= eeds 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 > > 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 =3D 0; > > return 0; > } Hi I'm happy to check my own test if this is considered a solution. (And=20 apologies for how my own efforts go... flailing around and not always=20 communicating effectively...) 1) I think the commit message could be improved. Specifically it says=20 the problem is due to TSC write, then that we need to fix LAPIC reset.=20 It doesn't say what the connection is between the two. (I.e. you would have to read the bug report, & then realize that it=20 _happens_ to involve a LAPIC state restore as well, because it's=20 triggered by reloading the entire VM state from a snapshot) Doing so might make the intention of this commit clearer. I've been=20 trying to work out a high quality fix, which prevents the lockups=20 revealed by this case. I think this commit would fix my bug nicely, bu= t=20 it doesn't help e.g. if the LAPIC restore was omitted from the sequence= =2E (Both TSC and the value of TSC deadline are set by MSRs, not by writing= =20 to the LAPIC. So I don't think the LAPIC state restore is necessary to=20 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,=20 i.e. in apic_post_state_restore() ? ii) then, wouldn't it be more correct to set it between hrtimer_cancel(= )=20 and start_apic_timer()? It doesn't sound right to clear=20 expired_tscdeadline, in case it was set immediately by start_apic_timer= () Alan