[Qemu-devel] [PATCH v1] kvm/x86: Remove Hyper-V SynIC timer stopping

[Qemu-devel] [PATCH v1] kvm/x86: Remove Hyper-V SynIC timer stopping

[Andrey Smetanin]

It's possible that guest send us Hyper-V EOM at the middle
of Hyper-V SynIC timer running, so we start processing of Hyper-V
SynIC timers in vcpu context and stop the Hyper-V SynIC timer
uncoditionally and lose time expiration which Windows 2012R2 guest
expects.

The patch fixes such situation by not stopping Hyper-V SynIC timer
at all, because it's safe to restart it without stop in vcpu context
and timer callback always returns HRTIMER_NORESTART.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
CC: Gleb Natapov <gleb@kernel.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Roman Kagan <rkagan@virtuozzo.com>
CC: Denis V. Lunev <den@openvz.org>
CC: qemu-devel@nongnu.org

---
 arch/x86/kvm/hyperv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 8ff8829..f34f666 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -598,7 +598,6 @@ void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
 	for (i = 0; i < ARRAY_SIZE(hv_vcpu->stimer); i++)
 		if (test_and_clear_bit(i, hv_vcpu->stimer_pending_bitmap)) {
 			stimer = &hv_vcpu->stimer[i];
-			stimer_stop(stimer);
 			if (stimer->config & HV_STIMER_ENABLE) {
 				time_now = get_time_ref_counter(vcpu->kvm);
 				if (time_now >= stimer->exp_time)
-- 
2.4.3

Re: [Qemu-devel] [PATCH v1] kvm/x86: Remove Hyper-V SynIC timer stopping

[Paolo Bonzini]

On 14/12/2015 16:33, Andrey Smetanin wrote:
> It's possible that guest send us Hyper-V EOM at the middle
> of Hyper-V SynIC timer running, so we start processing of Hyper-V
> SynIC timers in vcpu context and stop the Hyper-V SynIC timer
> uncoditionally and lose time expiration which Windows 2012R2 guest
> expects.
> 
> The patch fixes such situation by not stopping Hyper-V SynIC timer
> at all, because it's safe to restart it without stop in vcpu context
> and timer callback always returns HRTIMER_NORESTART.

Can you summarize with a "picture" what is the bad race?

The patch seems safe, but I'd like to have a better understanding of
what goes wrong.

Paolo

Re: [Qemu-devel] [PATCH v1] kvm/x86: Remove Hyper-V SynIC timer stopping

[Andrey Smetanin]

On 12/14/2015 07:09 PM, Paolo Bonzini wrote:
>
>
> On 14/12/2015 16:33, Andrey Smetanin wrote:
>> It's possible that guest send us Hyper-V EOM at the middle
>> of Hyper-V SynIC timer running, so we start processing of Hyper-V
>> SynIC timers in vcpu context and stop the Hyper-V SynIC timer
>> uncoditionally and lose time expiration which Windows 2012R2 guest
>> expects.
>>
>> The patch fixes such situation by not stopping Hyper-V SynIC timer
>> at all, because it's safe to restart it without stop in vcpu context
>> and timer callback always returns HRTIMER_NORESTART.
>
> Can you summarize with a "picture" what is the bad race?
>
Currently I see that guest starts periodic timer and doesn't clear 
message slot after timer expires, so timer expires again and trying to 
deliver expiration message but message slot is still busy so we set 
->msg_pending flag for guest to receive EOM. timer restarts again and 
while it's not expired guest notifies us with EOM, in this case we 
schedule timer processing in vcpu context by KVM_REQ_HV_STIMER, 
kvm_hv_process_stimers() is called in vcpu context and stops the timer
before it expires, so timer is disabled forever but guest expects it's
periodic expiration(15ms).

I do not understand why Windows doesn't clear message slot for a long 
time, it's likely need to be analyzed with debugger(and need more 
research). But we can go out from such situation by such fix.

> The patch seems safe, but I'd like to have a better understanding of
> what goes wrong.
>
> Paolo
>

Re: [Qemu-devel] [PATCH v1] kvm/x86: Remove Hyper-V SynIC timer stopping

[Andrey Smetanin]

On 12/14/2015 07:09 PM, Paolo Bonzini wrote:
>
>
> On 14/12/2015 16:33, Andrey Smetanin wrote:
>> It's possible that guest send us Hyper-V EOM at the middle
>> of Hyper-V SynIC timer running, so we start processing of Hyper-V
>> SynIC timers in vcpu context and stop the Hyper-V SynIC timer
>> uncoditionally and lose time expiration which Windows 2012R2 guest
>> expects.
>>
>> The patch fixes such situation by not stopping Hyper-V SynIC timer
>> at all, because it's safe to restart it without stop in vcpu context
>> and timer callback always returns HRTIMER_NORESTART.
>
> Can you summarize with a "picture" what is the bad race?
host					guest
					start periodic stimer
start periodic timer
timer expires after 15ms
send expiration message into guest
restart periodic timer
					....doing something....
timer expires again after 15 ms
msg slot is still not cleared so
setup ->msg_pending
restart periodic timer
					....doing something....
					process timer msg and clear slot
					so ->msg_pending was set:
						send EOM into host
received EOM
queued call of kvm_hv_process_stimers()
by KVM_REQ_HV_STIMER

kvm_hv_process_stimers():
	...
	stimer_stop()
	if (time_now >= stimer->exp_time)
         	stimer_expiration(stimer);
But time_now  < stimer->exp_time, so stimer_expiration is not called
in this case and timer is not restarted. so guest lose timer.

> The patch seems safe, but I'd like to have a better understanding of
> what goes wrong.
>
> Paolo
>

Re: [Qemu-devel] [PATCH v1] kvm/x86: Remove Hyper-V SynIC timer stopping

[Paolo Bonzini]

On 14/12/2015 18:01, Andrey Smetanin wrote:
> host                    guest
>                     start periodic stimer
> start periodic timer
> timer expires after 15ms
> send expiration message into guest
> restart periodic timer
>                     ....doing something....
> timer expires again after 15 ms
> msg slot is still not cleared so
> setup ->msg_pending
> restart periodic timer
>                     ....doing something....
>                     process timer msg and clear slot
>                     so ->msg_pending was set:
>                         send EOM into host
> received EOM
> queued call of kvm_hv_process_stimers()
> by KVM_REQ_HV_STIMER
> 
> kvm_hv_process_stimers():
>     ...
>     stimer_stop()
>     if (time_now >= stimer->exp_time)
>             stimer_expiration(stimer);
> But time_now  < stimer->exp_time, so stimer_expiration is not called
> in this case and timer is not restarted. so guest lose timer.

Great, this explains it.

Paolo