KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time

KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time

[Marcelo Tosatti]

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;
 }

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

[Alan Jenkins]

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...)


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.

(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

[PATCH v2] KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time

[Marcelo Tosatti]

Alan Jenkins reports hang at
https://bugzilla.redhat.com/show_bug.cgi?id=1337667,
due to guest TSC being set far behind than
lapic_timer.expired_tscdeadline, when restoring VM state
on top of currently active VM.

It is not possible to disable LAPIC timer advancement 
(by setting lapic_timer.expired_tscdeadline = 0), at 
guest TSC write because:

* 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."

In case 2, hardware would not fire an interrupt.

To fix the problem, disable timer advancement when 
userspace sets the LAPIC state. Setting of APIC 
resets all APIC state, including 
any pending interrupt.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Reported-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>

---

v2: improve commit message

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;
 }

Re: [PATCH v2] KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time

[Alan Jenkins]

On 20/06/16 14:05, Marcelo Tosatti wrote:
> Alan Jenkins reports hang at
> https://bugzilla.redhat.com/show_bug.cgi?id=1337667,
> due to guest TSC being set far behind than
> lapic_timer.expired_tscdeadline, when restoring VM state
> on top of currently active VM.
>
> It is not possible to disable LAPIC timer advancement
> (by setting lapic_timer.expired_tscdeadline = 0), at
> guest TSC write

I like that it acknowledges (though only implicitly) the guest can 
trigger arbitrary lockups of host CPUs.

> because:
>
> * 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."
>
> In case 2, hardware would not fire an interrupt.
>
> To fix the problem, disable timer advancement when
> userspace sets the LAPIC state. Setting of APIC
> resets all APIC state, including
> any pending interrupt.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Reported-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>

However I feel this doesn't admit (even implicitly) that host software 
(not necessarily root) can still hard-lockup the CPU. It depends on the 
sequence of operations, and the message doesn't show that sequence 
explicitly. I now understand what the sequence that _is_ in the message 
shows, but it's unfortunately distracting.

I.e. if you restore the LAPIC first (or omit to do so at all), then 
restore the TSC deadline MSR, then the TSC MSR.

The patch assumes that the LAPIC is restored after the MSRs so it can 
clear the incorrect value of expired_tscdeadline, right?

I didn't know whether this patch would work until I tested it, because I 
didn't try to nail down the exact sequence QEMU is using.

> 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;
>   }

Re: [PATCH v2] KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time

[Marcelo Tosatti]

On Mon, Jun 20, 2016 at 04:20:56PM +0100, Alan Jenkins wrote:
> On 20/06/16 14:05, Marcelo Tosatti wrote:
> >Alan Jenkins reports hang at
> >https://bugzilla.redhat.com/show_bug.cgi?id=1337667,
> >due to guest TSC being set far behind than
> >lapic_timer.expired_tscdeadline, when restoring VM state
> >on top of currently active VM.
> >
> >It is not possible to disable LAPIC timer advancement
> >(by setting lapic_timer.expired_tscdeadline = 0), at
> >guest TSC write
> 
> I like that it acknowledges (though only implicitly) the guest can
> trigger arbitrary lockups of host CPUs.
> 
> >because:
> >
> >* 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."
> >
> >In case 2, hardware would not fire an interrupt.
> >
> >To fix the problem, disable timer advancement when
> >userspace sets the LAPIC state. Setting of APIC
> >resets all APIC state, including
> >any pending interrupt.
> >
> >Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >Reported-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>
> 
> However I feel this doesn't admit (even implicitly) that host
> software (not necessarily root) can still hard-lockup the CPU. It
> depends on the sequence of operations, and the message doesn't show
> that sequence explicitly. I now understand what the sequence that
> _is_ in the message shows, but it's unfortunately distracting.
> 
> I.e. if you restore the LAPIC first (or omit to do so at all), then
> restore the TSC deadline MSR, then the TSC MSR.
> 
> The patch assumes that the LAPIC is restored after the MSRs so it
> can clear the incorrect value of expired_tscdeadline, right?

Yes.

> I didn't know whether this patch would work until I tested it,
> because I didn't try to nail down the exact sequence QEMU is using.

You are right about the host lockups -- sent another patch to fix that
one.

Thanks for the reports.

Re: [PATCH v2] KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time

[Paolo Bonzini]

On 20/06/2016 15:05, Marcelo Tosatti wrote:
> 
> 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;
>  

I think this is not correct.  You have programmed the host timer to an
early value when kvm_apic_post_state_restore called start_apic_timer.

I think that:

1) post_state_restore should cancel the timer and clear
lapic_timer.pending before writing the registers, not afterwards.
lapic_timer.expired_tscdeadline can be cleared at the same time.

2) kvm_write_tsc should do the same and restart the timer afterwards.

Thanks,

Paolo

Re: [PATCH v2] KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time

[Marcelo Tosatti]

On Tue, Jun 21, 2016 at 09:50:54AM +0200, Paolo Bonzini wrote:
> 
> 
> On 20/06/2016 15:05, Marcelo Tosatti wrote:
> > 
> > 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;
> >  
> 
> I think this is not correct.  You have programmed the host timer to an
> early value when kvm_apic_post_state_restore called start_apic_timer.
> 
> I think that:
> 
> 1) post_state_restore should cancel the timer and clear
> lapic_timer.pending before writing the registers, not afterwards.
> lapic_timer.expired_tscdeadline can be cleared at the same time.
> 
> 2) kvm_write_tsc should do the same and restart the timer afterwards.
> 
> Thanks,
> 
> Paolo

Actually, the max should deal with the problem Alan is having with
vmload as well (one delay of tenths of microseconds, or 0, per vmload
should be irrelevant).

Re: [PATCH v2] KVM: x86: reset lapic_timer.expired_tscdeadline at SET_LAPIC time

[Paolo Bonzini]

On 21/06/2016 13:35, Marcelo Tosatti wrote:
> On Tue, Jun 21, 2016 at 09:50:54AM +0200, Paolo Bonzini wrote:
>>
>>
>> On 20/06/2016 15:05, Marcelo Tosatti wrote:
>>>
>>> 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;
>>>  
>>
>> I think this is not correct.  You have programmed the host timer to an
>> early value when kvm_apic_post_state_restore called start_apic_timer.
>>
>> I think that:
>>
>> 1) post_state_restore should cancel the timer and clear
>> lapic_timer.pending before writing the registers, not afterwards.
>> lapic_timer.expired_tscdeadline can be cleared at the same time.
>>
>> 2) kvm_write_tsc should do the same and restart the timer afterwards.
>>
>> Thanks,
> 
> Actually, the max should deal with the problem Alan is having with
> vmload as well (one delay of tenths of microseconds, or 0, per vmload
> should be irrelevant).

Yes, it does.  Though I think it's a bug that we don't clear .pending on
KVM_SET_LAPIC, and I'm happy if you do a v2 of this patch to fix it. :)

Paolo

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

[Marcelo Tosatti]

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?

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

[Alan Jenkins]

On 20/06/16 14:11, Marcelo Tosatti wrote:
> On Sat, Jun 18, 2016 at 02:43:51PM +0100, Alan Jenkins wrote:
>> On 18/06/2016, Marcelo Tosatti <mtosatti@redhat.com> wrote:
>>> 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.

Good, let's hope I will not report too many irrelevant details ("bury 
the lede") :).

>
>> 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.

The timer expiration is still advanced, this patch just avoids the 
busywait. I think you mean it's ok if the guest would be interrupted 
early after a host-initiated write.

I think it'd be fine in practice; even interrupting before observed TSC 
reaches the TSC deadline isn't a problem in itself. When restoring CPU 
state, qemu will already have burnt the ~10ns advance in terms of *real* 
time.  And if there actually were external observers, they would already 
be confused by the guest being rewound.  I just can't code something 
like that without leaving a comment.

> Can you confirm the patch fixes your problem?

That did it, yes.