[PATCH v1 0/6] KVM: Hyper-V SynIC timers migration fixes

[PATCH v1 0/6] KVM: Hyper-V SynIC timers migration fixes

[Andrey Smetanin]

During testing of Windows 2012R2 guest migration with
Hyper-V SynIC timers enabled we found several bugs
which lead to restoring guest in a hung state.

This patch series provides several fixes to make the
migration of guest with Hyper-V SynIC timers enabled
succeed.

The series applies on top of
'kvm/x86: Remove Hyper-V SynIC timer stopping'
previously sent.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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

Andrey Smetanin (6):
  kvm/x86: Drop stimer_stop() function
  kvm/x86: Hyper-V unify stimer_start() and stimer_restart()
  kvm/x86: Reorg stimer_expiration() to better control timer restart
  kvm/x86: Hyper-V fix SynIC timer disabling condition
  kvm/x86: Skip SynIC vector check for QEMU side
  kvm/x86: Update SynIC timers on guest entry only

 arch/x86/kvm/hyperv.c | 112 +++++++++++++++++++++++---------------------------
 arch/x86/kvm/x86.c    |   6 +++
 2 files changed, 58 insertions(+), 60 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v1 0/6] KVM: Hyper-V SynIC timers migration fixes

[Andrey Smetanin]

During testing of Windows 2012R2 guest migration with
Hyper-V SynIC timers enabled we found several bugs
which lead to restoring guest in a hung state.

This patch series provides several fixes to make the
migration of guest with Hyper-V SynIC timers enabled
succeed.

The series applies on top of
'kvm/x86: Remove Hyper-V SynIC timer stopping'
previously sent.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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

Andrey Smetanin (6):
  kvm/x86: Drop stimer_stop() function
  kvm/x86: Hyper-V unify stimer_start() and stimer_restart()
  kvm/x86: Reorg stimer_expiration() to better control timer restart
  kvm/x86: Hyper-V fix SynIC timer disabling condition
  kvm/x86: Skip SynIC vector check for QEMU side
  kvm/x86: Update SynIC timers on guest entry only

 arch/x86/kvm/hyperv.c | 112 +++++++++++++++++++++++---------------------------
 arch/x86/kvm/x86.c    |   6 +++
 2 files changed, 58 insertions(+), 60 deletions(-)

-- 
2.4.3

[PATCH v1 1/6] kvm/x86: Drop stimer_stop() function

[Andrey Smetanin]

The function stimer_stop() is called in one place
so remove the function and replace it's call by function
content.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index f34f666..ec3a900 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -400,16 +400,11 @@ static void stimer_mark_expired(struct kvm_vcpu_hv_stimer *stimer,
 		kvm_vcpu_kick(vcpu);
 }
 
-static void stimer_stop(struct kvm_vcpu_hv_stimer *stimer)
-{
-	hrtimer_cancel(&stimer->timer);
-}
-
 static void stimer_cleanup(struct kvm_vcpu_hv_stimer *stimer)
 {
 	struct kvm_vcpu *vcpu = stimer_to_vcpu(stimer);
 
-	stimer_stop(stimer);
+	hrtimer_cancel(&stimer->timer);
 	clear_bit(stimer->index,
 		  vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap);
 	stimer->msg_pending = false;
-- 
2.4.3

[Qemu-devel] [PATCH v1 1/6] kvm/x86: Drop stimer_stop() function

[Andrey Smetanin]

The function stimer_stop() is called in one place
so remove the function and replace it's call by function
content.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index f34f666..ec3a900 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -400,16 +400,11 @@ static void stimer_mark_expired(struct kvm_vcpu_hv_stimer *stimer,
 		kvm_vcpu_kick(vcpu);
 }
 
-static void stimer_stop(struct kvm_vcpu_hv_stimer *stimer)
-{
-	hrtimer_cancel(&stimer->timer);
-}
-
 static void stimer_cleanup(struct kvm_vcpu_hv_stimer *stimer)
 {
 	struct kvm_vcpu *vcpu = stimer_to_vcpu(stimer);
 
-	stimer_stop(stimer);
+	hrtimer_cancel(&stimer->timer);
 	clear_bit(stimer->index,
 		  vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap);
 	stimer->msg_pending = false;
-- 
2.4.3

[PATCH v1 2/6] kvm/x86: Hyper-V unify stimer_start() and stimer_restart()

[Andrey Smetanin]

This will be used in future to start Hyper-V SynIC timer
in several places by one logic in one function.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index ec3a900..8623aa6 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -408,6 +408,7 @@ static void stimer_cleanup(struct kvm_vcpu_hv_stimer *stimer)
 	clear_bit(stimer->index,
 		  vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap);
 	stimer->msg_pending = false;
+	stimer->exp_time = 0;
 }
 
 static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
@@ -420,24 +421,6 @@ static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
 	return HRTIMER_NORESTART;
 }
 
-static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
-{
-	u64 time_now;
-	ktime_t ktime_now;
-	u64 remainder;
-
-	time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
-	ktime_now = ktime_get();
-
-	div64_u64_rem(time_now - stimer->exp_time, stimer->count, &remainder);
-	stimer->exp_time = time_now + (stimer->count - remainder);
-
-	hrtimer_start(&stimer->timer,
-		      ktime_add_ns(ktime_now,
-				   100 * (stimer->exp_time - time_now)),
-		      HRTIMER_MODE_ABS);
-}
-
 static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 {
 	u64 time_now;
@@ -450,9 +433,21 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 		if (stimer->count == 0)
 			return -EINVAL;
 
-		stimer->exp_time = time_now + stimer->count;
+		if (stimer->exp_time) {
+			if (time_now >= stimer->exp_time) {
+				u64 remainder;
+
+				div64_u64_rem(time_now - stimer->exp_time,
+					      stimer->count, &remainder);
+				stimer->exp_time =
+					time_now + (stimer->count - remainder);
+			}
+		} else
+			stimer->exp_time = time_now + stimer->count;
+
 		hrtimer_start(&stimer->timer,
-			      ktime_add_ns(ktime_now, 100 * stimer->count),
+			      ktime_add_ns(ktime_now,
+					   100 * (stimer->exp_time - time_now)),
 			      HRTIMER_MODE_ABS);
 		return 0;
 	}
@@ -580,7 +575,7 @@ static void stimer_expiration(struct kvm_vcpu_hv_stimer *stimer)
 	if (!(stimer->config & HV_STIMER_PERIODIC))
 		stimer->config |= ~HV_STIMER_ENABLE;
 	else
-		stimer_restart(stimer);
+		stimer_start(stimer);
 }
 
 void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
-- 
2.4.3

[Qemu-devel] [PATCH v1 2/6] kvm/x86: Hyper-V unify stimer_start() and stimer_restart()

[Andrey Smetanin]

This will be used in future to start Hyper-V SynIC timer
in several places by one logic in one function.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index ec3a900..8623aa6 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -408,6 +408,7 @@ static void stimer_cleanup(struct kvm_vcpu_hv_stimer *stimer)
 	clear_bit(stimer->index,
 		  vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap);
 	stimer->msg_pending = false;
+	stimer->exp_time = 0;
 }
 
 static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
@@ -420,24 +421,6 @@ static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
 	return HRTIMER_NORESTART;
 }
 
-static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
-{
-	u64 time_now;
-	ktime_t ktime_now;
-	u64 remainder;
-
-	time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
-	ktime_now = ktime_get();
-
-	div64_u64_rem(time_now - stimer->exp_time, stimer->count, &remainder);
-	stimer->exp_time = time_now + (stimer->count - remainder);
-
-	hrtimer_start(&stimer->timer,
-		      ktime_add_ns(ktime_now,
-				   100 * (stimer->exp_time - time_now)),
-		      HRTIMER_MODE_ABS);
-}
-
 static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 {
 	u64 time_now;
@@ -450,9 +433,21 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 		if (stimer->count == 0)
 			return -EINVAL;
 
-		stimer->exp_time = time_now + stimer->count;
+		if (stimer->exp_time) {
+			if (time_now >= stimer->exp_time) {
+				u64 remainder;
+
+				div64_u64_rem(time_now - stimer->exp_time,
+					      stimer->count, &remainder);
+				stimer->exp_time =
+					time_now + (stimer->count - remainder);
+			}
+		} else
+			stimer->exp_time = time_now + stimer->count;
+
 		hrtimer_start(&stimer->timer,
-			      ktime_add_ns(ktime_now, 100 * stimer->count),
+			      ktime_add_ns(ktime_now,
+					   100 * (stimer->exp_time - time_now)),
 			      HRTIMER_MODE_ABS);
 		return 0;
 	}
@@ -580,7 +575,7 @@ static void stimer_expiration(struct kvm_vcpu_hv_stimer *stimer)
 	if (!(stimer->config & HV_STIMER_PERIODIC))
 		stimer->config |= ~HV_STIMER_ENABLE;
 	else
-		stimer_restart(stimer);
+		stimer_start(stimer);
 }
 
 void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
-- 
2.4.3

[PATCH v1 3/6] kvm/x86: Reorg stimer_expiration() to better control timer restart

[Andrey Smetanin]

Split stimer_expiration() into two parts - timer expiration message
sending and timer restart/cleanup based on timer state(config).

This also fixes a bug where a one-shot timer message whose delivery
failed once would get lost for good.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 8623aa6..ce17529 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -552,30 +552,27 @@ static int synic_deliver_msg(struct kvm_vcpu_hv_synic *synic, u32 sint,
 	return r;
 }
 
-static void stimer_send_msg(struct kvm_vcpu_hv_stimer *stimer)
+static int stimer_send_msg(struct kvm_vcpu_hv_stimer *stimer)
 {
 	struct kvm_vcpu *vcpu = stimer_to_vcpu(stimer);
 	struct hv_message *msg = &stimer->msg;
 	struct hv_timer_message_payload *payload =
 			(struct hv_timer_message_payload *)&msg->u.payload;
-	int r;
 
-	stimer->msg_pending = true;
 	payload->expiration_time = stimer->exp_time;
 	payload->delivery_time = get_time_ref_counter(vcpu->kvm);
-	r = synic_deliver_msg(vcpu_to_synic(vcpu),
-			      HV_STIMER_SINT(stimer->config), msg);
-	if (!r)
-		stimer->msg_pending = false;
+	return synic_deliver_msg(vcpu_to_synic(vcpu),
+				 HV_STIMER_SINT(stimer->config), msg);
 }
 
 static void stimer_expiration(struct kvm_vcpu_hv_stimer *stimer)
 {
-	stimer_send_msg(stimer);
-	if (!(stimer->config & HV_STIMER_PERIODIC))
-		stimer->config |= ~HV_STIMER_ENABLE;
-	else
-		stimer_start(stimer);
+	stimer->msg_pending = true;
+	if (!stimer_send_msg(stimer)) {
+		stimer->msg_pending = false;
+		if (!(stimer->config & HV_STIMER_PERIODIC))
+			stimer->config |= ~HV_STIMER_ENABLE;
+	}
 }
 
 void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
@@ -592,6 +589,11 @@ void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
 				time_now = get_time_ref_counter(vcpu->kvm);
 				if (time_now >= stimer->exp_time)
 					stimer_expiration(stimer);
+
+				if (stimer->config & HV_STIMER_ENABLE)
+					stimer_start(stimer);
+				else
+					stimer_cleanup(stimer);
 			}
 		}
 }
-- 
2.4.3

[Qemu-devel] [PATCH v1 3/6] kvm/x86: Reorg stimer_expiration() to better control timer restart

[Andrey Smetanin]

Split stimer_expiration() into two parts - timer expiration message
sending and timer restart/cleanup based on timer state(config).

This also fixes a bug where a one-shot timer message whose delivery
failed once would get lost for good.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 8623aa6..ce17529 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -552,30 +552,27 @@ static int synic_deliver_msg(struct kvm_vcpu_hv_synic *synic, u32 sint,
 	return r;
 }
 
-static void stimer_send_msg(struct kvm_vcpu_hv_stimer *stimer)
+static int stimer_send_msg(struct kvm_vcpu_hv_stimer *stimer)
 {
 	struct kvm_vcpu *vcpu = stimer_to_vcpu(stimer);
 	struct hv_message *msg = &stimer->msg;
 	struct hv_timer_message_payload *payload =
 			(struct hv_timer_message_payload *)&msg->u.payload;
-	int r;
 
-	stimer->msg_pending = true;
 	payload->expiration_time = stimer->exp_time;
 	payload->delivery_time = get_time_ref_counter(vcpu->kvm);
-	r = synic_deliver_msg(vcpu_to_synic(vcpu),
-			      HV_STIMER_SINT(stimer->config), msg);
-	if (!r)
-		stimer->msg_pending = false;
+	return synic_deliver_msg(vcpu_to_synic(vcpu),
+				 HV_STIMER_SINT(stimer->config), msg);
 }
 
 static void stimer_expiration(struct kvm_vcpu_hv_stimer *stimer)
 {
-	stimer_send_msg(stimer);
-	if (!(stimer->config & HV_STIMER_PERIODIC))
-		stimer->config |= ~HV_STIMER_ENABLE;
-	else
-		stimer_start(stimer);
+	stimer->msg_pending = true;
+	if (!stimer_send_msg(stimer)) {
+		stimer->msg_pending = false;
+		if (!(stimer->config & HV_STIMER_PERIODIC))
+			stimer->config |= ~HV_STIMER_ENABLE;
+	}
 }
 
 void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
@@ -592,6 +589,11 @@ void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
 				time_now = get_time_ref_counter(vcpu->kvm);
 				if (time_now >= stimer->exp_time)
 					stimer_expiration(stimer);
+
+				if (stimer->config & HV_STIMER_ENABLE)
+					stimer_start(stimer);
+				else
+					stimer_cleanup(stimer);
 			}
 		}
 }
-- 
2.4.3

[PATCH v1 4/6] kvm/x86: Hyper-V fix SynIC timer disabling condition

[Andrey Smetanin]

Hypervisor Function Specification(HFS) doesn't require
to disable SynIC timer at timer config write if timer->count = 0.

So drop this check, this allow to load timers MSR's
during migration restore, because config are set before count
in QEMU side.

Also fix condition according to HFS doc(15.3.1):
"It is not permitted to set the SINTx field to zero for an
enabled timer. If attempted, the timer will be
marked disabled (that is, bit 0 cleared) immediately."

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index ce17529..b203ce3 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -472,7 +472,7 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 			     bool host)
 {
-	if (stimer->count == 0 || HV_STIMER_SINT(config) == 0)
+	if ((stimer->config & HV_STIMER_ENABLE) && HV_STIMER_SINT(config) == 0)
 		config &= ~HV_STIMER_ENABLE;
 	stimer->config = config;
 	stimer_cleanup(stimer);
-- 
2.4.3

[Qemu-devel] [PATCH v1 4/6] kvm/x86: Hyper-V fix SynIC timer disabling condition

[Andrey Smetanin]

Hypervisor Function Specification(HFS) doesn't require
to disable SynIC timer at timer config write if timer->count = 0.

So drop this check, this allow to load timers MSR's
during migration restore, because config are set before count
in QEMU side.

Also fix condition according to HFS doc(15.3.1):
"It is not permitted to set the SINTx field to zero for an
enabled timer. If attempted, the timer will be
marked disabled (that is, bit 0 cleared) immediately."

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index ce17529..b203ce3 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -472,7 +472,7 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 			     bool host)
 {
-	if (stimer->count == 0 || HV_STIMER_SINT(config) == 0)
+	if ((stimer->config & HV_STIMER_ENABLE) && HV_STIMER_SINT(config) == 0)
 		config &= ~HV_STIMER_ENABLE;
 	stimer->config = config;
 	stimer_cleanup(stimer);
-- 
2.4.3

[PATCH v1 5/6] kvm/x86: Skip SynIC vector check for QEMU side

[Andrey Smetanin]

QEMU zero-inits Hyper-V SynIC vectors. We should allow that,
and don't reject zero values if set by the host.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index b203ce3..d7e6651 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -72,12 +72,13 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
 	return false;
 }
 
-static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, u64 data)
+static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
+			  u64 data, bool host)
 {
 	int vector;
 
 	vector = data & HV_SYNIC_SINT_VECTOR_MASK;
-	if (vector < 16)
+	if (vector < 16 && !host)
 		return 1;
 	/*
 	 * Guest may configure multiple SINTs to use the same vector, so
@@ -247,7 +248,7 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
 		break;
 	}
 	case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:
-		ret = synic_set_sint(synic, msr - HV_X64_MSR_SINT0, data);
+		ret = synic_set_sint(synic, msr - HV_X64_MSR_SINT0, data, host);
 		break;
 	default:
 		ret = 1;
-- 
2.4.3

[Qemu-devel] [PATCH v1 5/6] kvm/x86: Skip SynIC vector check for QEMU side

[Andrey Smetanin]

QEMU zero-inits Hyper-V SynIC vectors. We should allow that,
and don't reject zero values if set by the host.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index b203ce3..d7e6651 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -72,12 +72,13 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
 	return false;
 }
 
-static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, u64 data)
+static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
+			  u64 data, bool host)
 {
 	int vector;
 
 	vector = data & HV_SYNIC_SINT_VECTOR_MASK;
-	if (vector < 16)
+	if (vector < 16 && !host)
 		return 1;
 	/*
 	 * Guest may configure multiple SINTs to use the same vector, so
@@ -247,7 +248,7 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
 		break;
 	}
 	case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:
-		ret = synic_set_sint(synic, msr - HV_X64_MSR_SINT0, data);
+		ret = synic_set_sint(synic, msr - HV_X64_MSR_SINT0, data, host);
 		break;
 	default:
 		ret = 1;
-- 
2.4.3

[PATCH v1 6/6] kvm/x86: Update SynIC timers on guest entry only

[Andrey Smetanin]

Consolidate updating the Hyper-V SynIC timers in a
single place: on guest entry in processing KVM_REQ_HV_STIMER
request.  This simplifies the overall logic, and makes sure
the most current state of msrs and guest clock is used for
arming the timers (to achieve that, KVM_REQ_HV_STIMER
has to be processed after KVM_REQ_CLOCK_UPDATE).

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 35 +++++++++++++++++------------------
 arch/x86/kvm/x86.c    |  6 ++++++
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index d7e6651..7857329 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -389,7 +389,7 @@ static u64 get_time_ref_counter(struct kvm *kvm)
 	return div_u64(get_kernel_ns() + kvm->arch.kvmclock_offset, 100);
 }
 
-static void stimer_mark_expired(struct kvm_vcpu_hv_stimer *stimer,
+static void stimer_mark_pending(struct kvm_vcpu_hv_stimer *stimer,
 				bool vcpu_kick)
 {
 	struct kvm_vcpu *vcpu = stimer_to_vcpu(stimer);
@@ -417,7 +417,7 @@ static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
 	struct kvm_vcpu_hv_stimer *stimer;
 
 	stimer = container_of(timer, struct kvm_vcpu_hv_stimer, timer);
-	stimer_mark_expired(stimer, true);
+	stimer_mark_pending(stimer, true);
 
 	return HRTIMER_NORESTART;
 }
@@ -460,7 +460,7 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 		 * "If a one shot is enabled and the specified count is in
 		 * the past, it will expire immediately."
 		 */
-		stimer_mark_expired(stimer, false);
+		stimer_mark_pending(stimer, false);
 		return 0;
 	}
 
@@ -473,30 +473,24 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 			     bool host)
 {
+	stimer_cleanup(stimer);
 	if ((stimer->config & HV_STIMER_ENABLE) && HV_STIMER_SINT(config) == 0)
 		config &= ~HV_STIMER_ENABLE;
 	stimer->config = config;
-	stimer_cleanup(stimer);
-	if (stimer->config & HV_STIMER_ENABLE)
-		if (stimer_start(stimer))
-			return 1;
+	stimer_mark_pending(stimer, false);
 	return 0;
 }
 
 static int stimer_set_count(struct kvm_vcpu_hv_stimer *stimer, u64 count,
 			    bool host)
 {
-	stimer->count = count;
-
 	stimer_cleanup(stimer);
+	stimer->count = count;
 	if (stimer->count == 0)
 		stimer->config &= ~HV_STIMER_ENABLE;
-	else if (stimer->config & HV_STIMER_AUTOENABLE) {
+	else if (stimer->config & HV_STIMER_AUTOENABLE)
 		stimer->config |= HV_STIMER_ENABLE;
-		if (stimer_start(stimer))
-			return 1;
-	}
-
+	stimer_mark_pending(stimer, false);
 	return 0;
 }
 
@@ -580,16 +574,21 @@ void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
 {
 	struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
 	struct kvm_vcpu_hv_stimer *stimer;
-	u64 time_now;
+	u64 time_now, exp_time;
 	int i;
 
 	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];
 			if (stimer->config & HV_STIMER_ENABLE) {
-				time_now = get_time_ref_counter(vcpu->kvm);
-				if (time_now >= stimer->exp_time)
-					stimer_expiration(stimer);
+				exp_time = stimer->exp_time;
+
+				if (exp_time) {
+					time_now =
+						get_time_ref_counter(vcpu->kvm);
+					if (time_now >= exp_time)
+						stimer_expiration(stimer);
+				}
 
 				if (stimer->config & HV_STIMER_ENABLE)
 					stimer_start(stimer);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b6102c1..795c14c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6492,6 +6492,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			r = 0;
 			goto out;
 		}
+
+		/*
+		 * KVM_REQ_HV_STIMER has to be processed after
+		 * KVM_REQ_CLOCK_UPDATE, because Hyper-V SynIC timers
+		 * depend on the guest clock being up-to-date
+		 */
 		if (kvm_check_request(KVM_REQ_HV_STIMER, vcpu))
 			kvm_hv_process_stimers(vcpu);
 	}
-- 
2.4.3

[Qemu-devel] [PATCH v1 6/6] kvm/x86: Update SynIC timers on guest entry only

[Andrey Smetanin]

Consolidate updating the Hyper-V SynIC timers in a
single place: on guest entry in processing KVM_REQ_HV_STIMER
request.  This simplifies the overall logic, and makes sure
the most current state of msrs and guest clock is used for
arming the timers (to achieve that, KVM_REQ_HV_STIMER
has to be processed after KVM_REQ_CLOCK_UPDATE).

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@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 | 35 +++++++++++++++++------------------
 arch/x86/kvm/x86.c    |  6 ++++++
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index d7e6651..7857329 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -389,7 +389,7 @@ static u64 get_time_ref_counter(struct kvm *kvm)
 	return div_u64(get_kernel_ns() + kvm->arch.kvmclock_offset, 100);
 }
 
-static void stimer_mark_expired(struct kvm_vcpu_hv_stimer *stimer,
+static void stimer_mark_pending(struct kvm_vcpu_hv_stimer *stimer,
 				bool vcpu_kick)
 {
 	struct kvm_vcpu *vcpu = stimer_to_vcpu(stimer);
@@ -417,7 +417,7 @@ static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
 	struct kvm_vcpu_hv_stimer *stimer;
 
 	stimer = container_of(timer, struct kvm_vcpu_hv_stimer, timer);
-	stimer_mark_expired(stimer, true);
+	stimer_mark_pending(stimer, true);
 
 	return HRTIMER_NORESTART;
 }
@@ -460,7 +460,7 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 		 * "If a one shot is enabled and the specified count is in
 		 * the past, it will expire immediately."
 		 */
-		stimer_mark_expired(stimer, false);
+		stimer_mark_pending(stimer, false);
 		return 0;
 	}
 
@@ -473,30 +473,24 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 			     bool host)
 {
+	stimer_cleanup(stimer);
 	if ((stimer->config & HV_STIMER_ENABLE) && HV_STIMER_SINT(config) == 0)
 		config &= ~HV_STIMER_ENABLE;
 	stimer->config = config;
-	stimer_cleanup(stimer);
-	if (stimer->config & HV_STIMER_ENABLE)
-		if (stimer_start(stimer))
-			return 1;
+	stimer_mark_pending(stimer, false);
 	return 0;
 }
 
 static int stimer_set_count(struct kvm_vcpu_hv_stimer *stimer, u64 count,
 			    bool host)
 {
-	stimer->count = count;
-
 	stimer_cleanup(stimer);
+	stimer->count = count;
 	if (stimer->count == 0)
 		stimer->config &= ~HV_STIMER_ENABLE;
-	else if (stimer->config & HV_STIMER_AUTOENABLE) {
+	else if (stimer->config & HV_STIMER_AUTOENABLE)
 		stimer->config |= HV_STIMER_ENABLE;
-		if (stimer_start(stimer))
-			return 1;
-	}
-
+	stimer_mark_pending(stimer, false);
 	return 0;
 }
 
@@ -580,16 +574,21 @@ void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
 {
 	struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
 	struct kvm_vcpu_hv_stimer *stimer;
-	u64 time_now;
+	u64 time_now, exp_time;
 	int i;
 
 	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];
 			if (stimer->config & HV_STIMER_ENABLE) {
-				time_now = get_time_ref_counter(vcpu->kvm);
-				if (time_now >= stimer->exp_time)
-					stimer_expiration(stimer);
+				exp_time = stimer->exp_time;
+
+				if (exp_time) {
+					time_now =
+						get_time_ref_counter(vcpu->kvm);
+					if (time_now >= exp_time)
+						stimer_expiration(stimer);
+				}
 
 				if (stimer->config & HV_STIMER_ENABLE)
 					stimer_start(stimer);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b6102c1..795c14c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6492,6 +6492,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			r = 0;
 			goto out;
 		}
+
+		/*
+		 * KVM_REQ_HV_STIMER has to be processed after
+		 * KVM_REQ_CLOCK_UPDATE, because Hyper-V SynIC timers
+		 * depend on the guest clock being up-to-date
+		 */
 		if (kvm_check_request(KVM_REQ_HV_STIMER, vcpu))
 			kvm_hv_process_stimers(vcpu);
 	}
-- 
2.4.3

Re: [PATCH v1 2/6] kvm/x86: Hyper-V unify stimer_start() and stimer_restart()

[Paolo Bonzini]

On 23/12/2015 12:28, Andrey Smetanin wrote:
> This will be used in future to start Hyper-V SynIC timer
> in several places by one logic in one function.
> 
> Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@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 | 37 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index ec3a900..8623aa6 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -408,6 +408,7 @@ static void stimer_cleanup(struct kvm_vcpu_hv_stimer *stimer)
>  	clear_bit(stimer->index,
>  		  vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap);
>  	stimer->msg_pending = false;
> +	stimer->exp_time = 0;
>  }
>  
>  static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
> @@ -420,24 +421,6 @@ static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
>  	return HRTIMER_NORESTART;
>  }
>  
> -static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
> -{
> -	u64 time_now;
> -	ktime_t ktime_now;
> -	u64 remainder;
> -
> -	time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
> -	ktime_now = ktime_get();
> -
> -	div64_u64_rem(time_now - stimer->exp_time, stimer->count, &remainder);
> -	stimer->exp_time = time_now + (stimer->count - remainder);
> -
> -	hrtimer_start(&stimer->timer,
> -		      ktime_add_ns(ktime_now,
> -				   100 * (stimer->exp_time - time_now)),
> -		      HRTIMER_MODE_ABS);
> -}
> -
>  static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
>  {
>  	u64 time_now;
> @@ -450,9 +433,21 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
>  		if (stimer->count == 0)
>  			return -EINVAL;
>  
> -		stimer->exp_time = time_now + stimer->count;
> +		if (stimer->exp_time) {
> +			if (time_now >= stimer->exp_time) {

Just for my education, is it possible to have this function called with
stimer->exp_time != 0 && time_now < stimer->exp_time?

Paolo

> +				u64 remainder;
> +
> +				div64_u64_rem(time_now - stimer->exp_time,
> +					      stimer->count, &remainder);
> +				stimer->exp_time =
> +					time_now + (stimer->count - remainder);
> +			}
> +		} else
> +			stimer->exp_time = time_now + stimer->count;
> +
>  		hrtimer_start(&stimer->timer,
> -			      ktime_add_ns(ktime_now, 100 * stimer->count),
> +			      ktime_add_ns(ktime_now,
> +					   100 * (stimer->exp_time - time_now)),
>  			      HRTIMER_MODE_ABS);
>  		return 0;
>  	}
> @@ -580,7 +575,7 @@ static void stimer_expiration(struct kvm_vcpu_hv_stimer *stimer)
>  	if (!(stimer->config & HV_STIMER_PERIODIC))
>  		stimer->config |= ~HV_STIMER_ENABLE;
>  	else
> -		stimer_restart(stimer);
> +		stimer_start(stimer);
>  }
>  
>  void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
> 

Re: [Qemu-devel] [PATCH v1 2/6] kvm/x86: Hyper-V unify stimer_start() and stimer_restart()

[Paolo Bonzini]

On 23/12/2015 12:28, Andrey Smetanin wrote:
> This will be used in future to start Hyper-V SynIC timer
> in several places by one logic in one function.
> 
> Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@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 | 37 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index ec3a900..8623aa6 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -408,6 +408,7 @@ static void stimer_cleanup(struct kvm_vcpu_hv_stimer *stimer)
>  	clear_bit(stimer->index,
>  		  vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap);
>  	stimer->msg_pending = false;
> +	stimer->exp_time = 0;
>  }
>  
>  static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
> @@ -420,24 +421,6 @@ static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
>  	return HRTIMER_NORESTART;
>  }
>  
> -static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
> -{
> -	u64 time_now;
> -	ktime_t ktime_now;
> -	u64 remainder;
> -
> -	time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
> -	ktime_now = ktime_get();
> -
> -	div64_u64_rem(time_now - stimer->exp_time, stimer->count, &remainder);
> -	stimer->exp_time = time_now + (stimer->count - remainder);
> -
> -	hrtimer_start(&stimer->timer,
> -		      ktime_add_ns(ktime_now,
> -				   100 * (stimer->exp_time - time_now)),
> -		      HRTIMER_MODE_ABS);
> -}
> -
>  static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
>  {
>  	u64 time_now;
> @@ -450,9 +433,21 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
>  		if (stimer->count == 0)
>  			return -EINVAL;
>  
> -		stimer->exp_time = time_now + stimer->count;
> +		if (stimer->exp_time) {
> +			if (time_now >= stimer->exp_time) {

Just for my education, is it possible to have this function called with
stimer->exp_time != 0 && time_now < stimer->exp_time?

Paolo

> +				u64 remainder;
> +
> +				div64_u64_rem(time_now - stimer->exp_time,
> +					      stimer->count, &remainder);
> +				stimer->exp_time =
> +					time_now + (stimer->count - remainder);
> +			}
> +		} else
> +			stimer->exp_time = time_now + stimer->count;
> +
>  		hrtimer_start(&stimer->timer,
> -			      ktime_add_ns(ktime_now, 100 * stimer->count),
> +			      ktime_add_ns(ktime_now,
> +					   100 * (stimer->exp_time - time_now)),
>  			      HRTIMER_MODE_ABS);
>  		return 0;
>  	}
> @@ -580,7 +575,7 @@ static void stimer_expiration(struct kvm_vcpu_hv_stimer *stimer)
>  	if (!(stimer->config & HV_STIMER_PERIODIC))
>  		stimer->config |= ~HV_STIMER_ENABLE;
>  	else
> -		stimer_restart(stimer);
> +		stimer_start(stimer);
>  }
>  
>  void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
> 

Re: [PATCH v1 3/6] kvm/x86: Reorg stimer_expiration() to better control timer restart

[Paolo Bonzini]

On 23/12/2015 12:28, Andrey Smetanin wrote:
> -	stimer_send_msg(stimer);
> -	if (!(stimer->config & HV_STIMER_PERIODIC))
> -		stimer->config |= ~HV_STIMER_ENABLE;
> -	else
> -		stimer_start(stimer);
> +	stimer->msg_pending = true;
> +	if (!stimer_send_msg(stimer)) {
> +		stimer->msg_pending = false;
> +		if (!(stimer->config & HV_STIMER_PERIODIC))
> +			stimer->config |= ~HV_STIMER_ENABLE;

Just because this is curious: sure it shouldn't be "&="?

Paolo

> +	}

Re: [Qemu-devel] [PATCH v1 3/6] kvm/x86: Reorg stimer_expiration() to better control timer restart

[Paolo Bonzini]

On 23/12/2015 12:28, Andrey Smetanin wrote:
> -	stimer_send_msg(stimer);
> -	if (!(stimer->config & HV_STIMER_PERIODIC))
> -		stimer->config |= ~HV_STIMER_ENABLE;
> -	else
> -		stimer_start(stimer);
> +	stimer->msg_pending = true;
> +	if (!stimer_send_msg(stimer)) {
> +		stimer->msg_pending = false;
> +		if (!(stimer->config & HV_STIMER_PERIODIC))
> +			stimer->config |= ~HV_STIMER_ENABLE;

Just because this is curious: sure it shouldn't be "&="?

Paolo

> +	}

Re: [PATCH v1 0/6] KVM: Hyper-V SynIC timers migration fixes

[Paolo Bonzini]

On 23/12/2015 12:28, Andrey Smetanin wrote:
> During testing of Windows 2012R2 guest migration with
> Hyper-V SynIC timers enabled we found several bugs
> which lead to restoring guest in a hung state.
> 
> This patch series provides several fixes to make the
> migration of guest with Hyper-V SynIC timers enabled
> succeed.
> 
> The series applies on top of
> 'kvm/x86: Remove Hyper-V SynIC timer stopping'
> previously sent.
> 
> Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@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
> 
> Andrey Smetanin (6):
>   kvm/x86: Drop stimer_stop() function
>   kvm/x86: Hyper-V unify stimer_start() and stimer_restart()
>   kvm/x86: Reorg stimer_expiration() to better control timer restart
>   kvm/x86: Hyper-V fix SynIC timer disabling condition
>   kvm/x86: Skip SynIC vector check for QEMU side
>   kvm/x86: Update SynIC timers on guest entry only
> 
>  arch/x86/kvm/hyperv.c | 112 +++++++++++++++++++++++---------------------------
>  arch/x86/kvm/x86.c    |   6 +++
>  2 files changed, 58 insertions(+), 60 deletions(-)
> 

Applied, let me know if I should fix up patch 3 (the bug is preexisting
anyway, if it is a bug as I suspect).

Paolo

Re: [Qemu-devel] [PATCH v1 0/6] KVM: Hyper-V SynIC timers migration fixes

[Paolo Bonzini]

On 23/12/2015 12:28, Andrey Smetanin wrote:
> During testing of Windows 2012R2 guest migration with
> Hyper-V SynIC timers enabled we found several bugs
> which lead to restoring guest in a hung state.
> 
> This patch series provides several fixes to make the
> migration of guest with Hyper-V SynIC timers enabled
> succeed.
> 
> The series applies on top of
> 'kvm/x86: Remove Hyper-V SynIC timer stopping'
> previously sent.
> 
> Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@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
> 
> Andrey Smetanin (6):
>   kvm/x86: Drop stimer_stop() function
>   kvm/x86: Hyper-V unify stimer_start() and stimer_restart()
>   kvm/x86: Reorg stimer_expiration() to better control timer restart
>   kvm/x86: Hyper-V fix SynIC timer disabling condition
>   kvm/x86: Skip SynIC vector check for QEMU side
>   kvm/x86: Update SynIC timers on guest entry only
> 
>  arch/x86/kvm/hyperv.c | 112 +++++++++++++++++++++++---------------------------
>  arch/x86/kvm/x86.c    |   6 +++
>  2 files changed, 58 insertions(+), 60 deletions(-)
> 

Applied, let me know if I should fix up patch 3 (the bug is preexisting
anyway, if it is a bug as I suspect).

Paolo

Re: [PATCH v1 3/6] kvm/x86: Reorg stimer_expiration() to better control timer restart

[Andrey Smetanin]

On 01/07/2016 07:34 PM, Paolo Bonzini wrote:
>
>
> On 23/12/2015 12:28, Andrey Smetanin wrote:
>> -	stimer_send_msg(stimer);
>> -	if (!(stimer->config & HV_STIMER_PERIODIC))
>> -		stimer->config |= ~HV_STIMER_ENABLE;
>> -	else
>> -		stimer_start(stimer);
>> +	stimer->msg_pending = true;
>> +	if (!stimer_send_msg(stimer)) {
>> +		stimer->msg_pending = false;
>> +		if (!(stimer->config & HV_STIMER_PERIODIC))
>> +			stimer->config |= ~HV_STIMER_ENABLE;
>
> Just because this is curious: sure it shouldn't be "&="?
You are right, we found it too late - only by testing Linux guest 
Hyper-v timers which are one-shot timers. We have fixed it in v2.
>
> Paolo
>
>> +	}

Re: [Qemu-devel] [PATCH v1 3/6] kvm/x86: Reorg stimer_expiration() to better control timer restart

[Andrey Smetanin]

On 01/07/2016 07:34 PM, Paolo Bonzini wrote:
>
>
> On 23/12/2015 12:28, Andrey Smetanin wrote:
>> -	stimer_send_msg(stimer);
>> -	if (!(stimer->config & HV_STIMER_PERIODIC))
>> -		stimer->config |= ~HV_STIMER_ENABLE;
>> -	else
>> -		stimer_start(stimer);
>> +	stimer->msg_pending = true;
>> +	if (!stimer_send_msg(stimer)) {
>> +		stimer->msg_pending = false;
>> +		if (!(stimer->config & HV_STIMER_PERIODIC))
>> +			stimer->config |= ~HV_STIMER_ENABLE;
>
> Just because this is curious: sure it shouldn't be "&="?
You are right, we found it too late - only by testing Linux guest 
Hyper-v timers which are one-shot timers. We have fixed it in v2.
>
> Paolo
>
>> +	}

Re: [PATCH v1 2/6] kvm/x86: Hyper-V unify stimer_start() and stimer_restart()

[Andrey Smetanin]

On 01/07/2016 07:32 PM, Paolo Bonzini wrote:
>
>
> On 23/12/2015 12:28, Andrey Smetanin wrote:
>> This will be used in future to start Hyper-V SynIC timer
>> in several places by one logic in one function.
>>
>> Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
>> Reviewed-by: Roman Kagan <rkagan@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 | 37 ++++++++++++++++---------------------
>>   1 file changed, 16 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index ec3a900..8623aa6 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -408,6 +408,7 @@ static void stimer_cleanup(struct kvm_vcpu_hv_stimer *stimer)
>>   	clear_bit(stimer->index,
>>   		  vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap);
>>   	stimer->msg_pending = false;
>> +	stimer->exp_time = 0;
>>   }
>>
>>   static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
>> @@ -420,24 +421,6 @@ static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
>>   	return HRTIMER_NORESTART;
>>   }
>>
>> -static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
>> -{
>> -	u64 time_now;
>> -	ktime_t ktime_now;
>> -	u64 remainder;
>> -
>> -	time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
>> -	ktime_now = ktime_get();
>> -
>> -	div64_u64_rem(time_now - stimer->exp_time, stimer->count, &remainder);
>> -	stimer->exp_time = time_now + (stimer->count - remainder);
>> -
>> -	hrtimer_start(&stimer->timer,
>> -		      ktime_add_ns(ktime_now,
>> -				   100 * (stimer->exp_time - time_now)),
>> -		      HRTIMER_MODE_ABS);
>> -}
>> -
>>   static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
>>   {
>>   	u64 time_now;
>> @@ -450,9 +433,21 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
>>   		if (stimer->count == 0)
>>   			return -EINVAL;
>>
>> -		stimer->exp_time = time_now + stimer->count;
>> +		if (stimer->exp_time) {
>> +			if (time_now >= stimer->exp_time) {
>
> Just for my education, is it possible to have this function called with
> stimer->exp_time != 0 && time_now < stimer->exp_time?
I think it's possible, assume the following situation:
GUEST                           HOST
periodic timer setup & start    start timer
			        timer expiration
				    send timer expiration message
guest is busy and		.....
do not processing timer		timer expiration
message for longer than timer  	  try to send timer expiration message
period				    since message slot is still occupied
.....                               by guest set slot->msg_pending = 1
guest wake and now
  processed timer message,
   since slot->msg_pending = 1
    do wrmsr(HV_X64_MSR_EOM)
				handle HV_X64_MSR_EOM
				  kvm_hv_notify_acked_sint()
			           schedule KVM_REQ_HV_STIMER
				    kvm_hv_process_stimers()
				      exp_time != 0 && now < exp_time

				....
				timer expiration
			
In this case we just start hrtimer again with
the same(previous) target exp_time, so I do not see any side effects.
>
> Paolo
>
>> +				u64 remainder;
>> +
>> +				div64_u64_rem(time_now - stimer->exp_time,
>> +					      stimer->count, &remainder);
>> +				stimer->exp_time =
>> +					time_now + (stimer->count - remainder);
>> +			}
>> +		} else
>> +			stimer->exp_time = time_now + stimer->count;
>> +
>>   		hrtimer_start(&stimer->timer,
>> -			      ktime_add_ns(ktime_now, 100 * stimer->count),
>> +			      ktime_add_ns(ktime_now,
>> +					   100 * (stimer->exp_time - time_now)),
>>   			      HRTIMER_MODE_ABS);
>>   		return 0;
>>   	}
>> @@ -580,7 +575,7 @@ static void stimer_expiration(struct kvm_vcpu_hv_stimer *stimer)
>>   	if (!(stimer->config & HV_STIMER_PERIODIC))
>>   		stimer->config |= ~HV_STIMER_ENABLE;
>>   	else
>> -		stimer_restart(stimer);
>> +		stimer_start(stimer);
>>   }
>>
>>   void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
>>

Re: [Qemu-devel] [PATCH v1 2/6] kvm/x86: Hyper-V unify stimer_start() and stimer_restart()

[Andrey Smetanin]

On 01/07/2016 07:32 PM, Paolo Bonzini wrote:
>
>
> On 23/12/2015 12:28, Andrey Smetanin wrote:
>> This will be used in future to start Hyper-V SynIC timer
>> in several places by one logic in one function.
>>
>> Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
>> Reviewed-by: Roman Kagan <rkagan@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 | 37 ++++++++++++++++---------------------
>>   1 file changed, 16 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index ec3a900..8623aa6 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -408,6 +408,7 @@ static void stimer_cleanup(struct kvm_vcpu_hv_stimer *stimer)
>>   	clear_bit(stimer->index,
>>   		  vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap);
>>   	stimer->msg_pending = false;
>> +	stimer->exp_time = 0;
>>   }
>>
>>   static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
>> @@ -420,24 +421,6 @@ static enum hrtimer_restart stimer_timer_callback(struct hrtimer *timer)
>>   	return HRTIMER_NORESTART;
>>   }
>>
>> -static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
>> -{
>> -	u64 time_now;
>> -	ktime_t ktime_now;
>> -	u64 remainder;
>> -
>> -	time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
>> -	ktime_now = ktime_get();
>> -
>> -	div64_u64_rem(time_now - stimer->exp_time, stimer->count, &remainder);
>> -	stimer->exp_time = time_now + (stimer->count - remainder);
>> -
>> -	hrtimer_start(&stimer->timer,
>> -		      ktime_add_ns(ktime_now,
>> -				   100 * (stimer->exp_time - time_now)),
>> -		      HRTIMER_MODE_ABS);
>> -}
>> -
>>   static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
>>   {
>>   	u64 time_now;
>> @@ -450,9 +433,21 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
>>   		if (stimer->count == 0)
>>   			return -EINVAL;
>>
>> -		stimer->exp_time = time_now + stimer->count;
>> +		if (stimer->exp_time) {
>> +			if (time_now >= stimer->exp_time) {
>
> Just for my education, is it possible to have this function called with
> stimer->exp_time != 0 && time_now < stimer->exp_time?
I think it's possible, assume the following situation:
GUEST                           HOST
periodic timer setup & start    start timer
			        timer expiration
				    send timer expiration message
guest is busy and		.....
do not processing timer		timer expiration
message for longer than timer  	  try to send timer expiration message
period				    since message slot is still occupied
.....                               by guest set slot->msg_pending = 1
guest wake and now
  processed timer message,
   since slot->msg_pending = 1
    do wrmsr(HV_X64_MSR_EOM)
				handle HV_X64_MSR_EOM
				  kvm_hv_notify_acked_sint()
			           schedule KVM_REQ_HV_STIMER
				    kvm_hv_process_stimers()
				      exp_time != 0 && now < exp_time

				....
				timer expiration
			
In this case we just start hrtimer again with
the same(previous) target exp_time, so I do not see any side effects.
>
> Paolo
>
>> +				u64 remainder;
>> +
>> +				div64_u64_rem(time_now - stimer->exp_time,
>> +					      stimer->count, &remainder);
>> +				stimer->exp_time =
>> +					time_now + (stimer->count - remainder);
>> +			}
>> +		} else
>> +			stimer->exp_time = time_now + stimer->count;
>> +
>>   		hrtimer_start(&stimer->timer,
>> -			      ktime_add_ns(ktime_now, 100 * stimer->count),
>> +			      ktime_add_ns(ktime_now,
>> +					   100 * (stimer->exp_time - time_now)),
>>   			      HRTIMER_MODE_ABS);
>>   		return 0;
>>   	}
>> @@ -580,7 +575,7 @@ static void stimer_expiration(struct kvm_vcpu_hv_stimer *stimer)
>>   	if (!(stimer->config & HV_STIMER_PERIODIC))
>>   		stimer->config |= ~HV_STIMER_ENABLE;
>>   	else
>> -		stimer_restart(stimer);
>> +		stimer_start(stimer);
>>   }
>>
>>   void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
>>