[PATCH 0/5] KVM: LAPIC: Add APIC Timer periodic/oneshot mode VMX preemption timer support

[PATCH 0/5] KVM: LAPIC: Add APIC Timer periodic/oneshot mode VMX preemption timer support

[Wanpeng Li]

Most windows guests which I have on hand currently still utilize APIC Timer
periodic/oneshot mode instead of APIC Timer tsc-deadline mode:
- windows 2008 server r2
- windows 2012 server r2
- windows 7
- windows 10

This patchset adds the APIC Timer periodic/oneshot mode VMX preemption
timer support.

I test the patchset by adding kvm-unit-test/apic_timer_latency.flat to test 100 thousands 
times APIC timer operations.

The patchset reduces ~3800+ clock cycles for each APIC timer periodic mode
operation virtualization. 

V1 -> RFC v3:
 * squash patch 3 into patch 6
 * fix switch sw to hv timer

RFC v2 -> RFC v3:
 * remove kvm_lapic_hv_timer_in_use() check in apic_get_tmcc, replace
   the hritmer_get_remaining() by target_expiration - now
 * rename expired_period to target_expiration
 * introduce set_target_expiration() helper
 * move the checking of minimal period to set_target_expiration()
 * cleanup kvm_get_lapic_target_deadline_tsc()

RFC v1 -> RFC v2:
 * remember the timeout when setting up the timer to get a correct TMCCT
 * move apic->lapic_timer.period/tscdeadline caculations to start_apic_timer()

Wanpeng Li (5):
  KVM: LAPIC: extract start_sw_period() to handle periodic/oneshot mode
  KVM: LAPIC: guarantee the timer is in tsc-deadline mode when rdmsr MSR_IA32_TSCDEADLINE
  KVM: LAPIC: introduce kvm_get_lapic_target_expiration_tsc()
  KVM: LAPIC: rename start/cancel_hv_tscdeadline to start/cancel_hv_timer
  KVM: LAPIC: add APIC Timer periodic/oneshot mode VMX preemption timer support

 arch/x86/kvm/lapic.c | 193 +++++++++++++++++++++++++++++++++------------------
 arch/x86/kvm/lapic.h |   2 +
 arch/x86/kvm/x86.c   |   2 +-
 3 files changed, 129 insertions(+), 68 deletions(-)

-- 
1.9.1

[PATCH 1/5] KVM: LAPIC: extract start_sw_period() to handle periodic/oneshot mode

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Extract start_sw_period() to handle periodic/oneshot mode, it will be
used by later patch.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/lapic.c | 89 +++++++++++++++++++++++++++-------------------------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 23b99f3..dad743e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1347,6 +1347,50 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic)
 	local_irq_restore(flags);
 }
 
+static void start_sw_period(struct kvm_lapic *apic)
+{
+	ktime_t now;
+
+	/* lapic timer in oneshot or periodic mode */
+	now = apic->lapic_timer.timer.base->get_time();
+	apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT)
+		    * APIC_BUS_CYCLE_NS * apic->divide_count;
+
+	if (!apic->lapic_timer.period)
+		return;
+	/*
+	 * Do not allow the guest to program periodic timers with small
+	 * interval, since the hrtimers are not throttled by the host
+	 * scheduler.
+	 */
+	if (apic_lvtt_period(apic)) {
+		s64 min_period = min_timer_period_us * 1000LL;
+
+		if (apic->lapic_timer.period < min_period) {
+			pr_info_ratelimited(
+			    "kvm: vcpu %i: requested %lld ns "
+			    "lapic timer period limited to %lld ns\n",
+			    apic->vcpu->vcpu_id,
+			    apic->lapic_timer.period, min_period);
+			apic->lapic_timer.period = min_period;
+		}
+	}
+
+	hrtimer_start(&apic->lapic_timer.timer,
+		      ktime_add_ns(now, apic->lapic_timer.period),
+		      HRTIMER_MODE_ABS_PINNED);
+
+	apic_debug("%s: bus cycle is %" PRId64 "ns, now 0x%016"
+		   PRIx64 ", "
+		   "timer initial count 0x%x, period %lldns, "
+		   "expire @ 0x%016" PRIx64 ".\n", __func__,
+		   APIC_BUS_CYCLE_NS, ktime_to_ns(now),
+		   kvm_lapic_get_reg(apic, APIC_TMICT),
+		   apic->lapic_timer.period,
+		   ktime_to_ns(ktime_add_ns(now,
+				apic->lapic_timer.period)));
+}
+
 bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu)
 {
 	if (!lapic_in_kernel(vcpu))
@@ -1424,50 +1468,11 @@ EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
 
 static void start_apic_timer(struct kvm_lapic *apic)
 {
-	ktime_t now;
-
 	atomic_set(&apic->lapic_timer.pending, 0);
 
-	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
-		/* lapic timer in oneshot or periodic mode */
-		now = apic->lapic_timer.timer.base->get_time();
-		apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT)
-			    * APIC_BUS_CYCLE_NS * apic->divide_count;
-
-		if (!apic->lapic_timer.period)
-			return;
-		/*
-		 * Do not allow the guest to program periodic timers with small
-		 * interval, since the hrtimers are not throttled by the host
-		 * scheduler.
-		 */
-		if (apic_lvtt_period(apic)) {
-			s64 min_period = min_timer_period_us * 1000LL;
-
-			if (apic->lapic_timer.period < min_period) {
-				pr_info_ratelimited(
-				    "kvm: vcpu %i: requested %lld ns "
-				    "lapic timer period limited to %lld ns\n",
-				    apic->vcpu->vcpu_id,
-				    apic->lapic_timer.period, min_period);
-				apic->lapic_timer.period = min_period;
-			}
-		}
-
-		hrtimer_start(&apic->lapic_timer.timer,
-			      ktime_add_ns(now, apic->lapic_timer.period),
-			      HRTIMER_MODE_ABS_PINNED);
-
-		apic_debug("%s: bus cycle is %" PRId64 "ns, now 0x%016"
-			   PRIx64 ", "
-			   "timer initial count 0x%x, period %lldns, "
-			   "expire @ 0x%016" PRIx64 ".\n", __func__,
-			   APIC_BUS_CYCLE_NS, ktime_to_ns(now),
-			   kvm_lapic_get_reg(apic, APIC_TMICT),
-			   apic->lapic_timer.period,
-			   ktime_to_ns(ktime_add_ns(now,
-					apic->lapic_timer.period)));
-	} else if (apic_lvtt_tscdeadline(apic)) {
+	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
+		start_sw_period(apic);
+	else if (apic_lvtt_tscdeadline(apic)) {
 		if (!(kvm_x86_ops->set_hv_timer && start_hv_tscdeadline(apic)))
 			start_sw_tscdeadline(apic);
 	}
-- 
1.9.1

[PATCH 2/5] KVM: LAPIC: guarantee the timer is in tsc-deadline mode

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Check apic_lvtt_tscdeadline() mode directly instead of apic_lvtt_oneshot()
and apic_lvtt_period() to guarantee the timer is in tsc-deadline mode when
rdmsr MSR_IA32_TSCDEADLINE.

Suggsted-by: Radim Krčmář <rkrcmar@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/lapic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index dad743e..dce6c0b 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1711,8 +1711,8 @@ u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!lapic_in_kernel(vcpu) || apic_lvtt_oneshot(apic) ||
-			apic_lvtt_period(apic))
+	if (!lapic_in_kernel(vcpu) ||
+		!apic_lvtt_tscdeadline(apic))
 		return 0;
 
 	return apic->lapic_timer.tscdeadline;
-- 
1.9.1

[PATCH 3/5] KVM: LAPIC: introduce kvm_get_lapic_target_expiration_tsc()

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Introdce kvm_get_lapic_target_expiration_tsc() to get APIC Timer target
deadline tsc.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/lapic.c | 9 +++++++++
 arch/x86/kvm/lapic.h | 1 +
 arch/x86/kvm/x86.c   | 2 +-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index dce6c0b..531fc28 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1706,6 +1706,15 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu)
  * LAPIC interface
  *----------------------------------------------------------------------
  */
+u64 kvm_get_lapic_target_expiration_tsc(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!lapic_in_kernel(vcpu))
+		return 0;
+
+	return apic->lapic_timer.tscdeadline;
+}
 
 u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu)
 {
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index f60d01c..031db26 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -85,6 +85,7 @@ int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
 int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
 int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu);
 
+u64 kvm_get_lapic_target_expiration_tsc(struct kvm_vcpu *vcpu);
 u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
 void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e375235..ec53c3e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2794,7 +2794,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		}
 		if (kvm_lapic_hv_timer_in_use(vcpu) &&
 				kvm_x86_ops->set_hv_timer(vcpu,
-					kvm_get_lapic_tscdeadline_msr(vcpu)))
+					kvm_get_lapic_target_expiration_tsc(vcpu)))
 			kvm_lapic_switch_to_sw_timer(vcpu);
 		/*
 		 * On a host with synchronized TSC, there is no need to update
-- 
1.9.1

[PATCH 4/5] KVM: LAPIC: rename start/cancel_hv_tscdeadline to start/cancel_hv_timer

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Rename start/cancel_hv_tscdeadline to start/cancel_hv_timer since
they will handle both APIC Timer periodic/oneshot mode and tsc-deadline
mode.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/lapic.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 531fc28..0354a79 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1400,7 +1400,7 @@ bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_hv_timer_in_use);
 
-static void cancel_hv_tscdeadline(struct kvm_lapic *apic)
+static void cancel_hv_timer(struct kvm_lapic *apic)
 {
 	kvm_x86_ops->cancel_hv_timer(apic->vcpu);
 	apic->lapic_timer.hv_timer_in_use = false;
@@ -1412,26 +1412,26 @@ void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
 
 	WARN_ON(!apic->lapic_timer.hv_timer_in_use);
 	WARN_ON(swait_active(&vcpu->wq));
-	cancel_hv_tscdeadline(apic);
+	cancel_hv_timer(apic);
 	apic_timer_expired(apic);
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
 
-static bool start_hv_tscdeadline(struct kvm_lapic *apic)
+static bool start_hv_timer(struct kvm_lapic *apic)
 {
 	u64 tscdeadline = apic->lapic_timer.tscdeadline;
 
 	if (atomic_read(&apic->lapic_timer.pending) ||
 		kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
 		if (apic->lapic_timer.hv_timer_in_use)
-			cancel_hv_tscdeadline(apic);
+			cancel_hv_timer(apic);
 	} else {
 		apic->lapic_timer.hv_timer_in_use = true;
 		hrtimer_cancel(&apic->lapic_timer.timer);
 
 		/* In case the sw timer triggered in the window */
 		if (atomic_read(&apic->lapic_timer.pending))
-			cancel_hv_tscdeadline(apic);
+			cancel_hv_timer(apic);
 	}
 	trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
 			apic->lapic_timer.hv_timer_in_use);
@@ -1445,7 +1445,7 @@ void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu)
 	WARN_ON(apic->lapic_timer.hv_timer_in_use);
 
 	if (apic_lvtt_tscdeadline(apic))
-		start_hv_tscdeadline(apic);
+		start_hv_timer(apic);
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer);
 
@@ -1457,7 +1457,7 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
 	if (!apic->lapic_timer.hv_timer_in_use)
 		return;
 
-	cancel_hv_tscdeadline(apic);
+	cancel_hv_timer(apic);
 
 	if (atomic_read(&apic->lapic_timer.pending))
 		return;
@@ -1473,7 +1473,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
 	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
 		start_sw_period(apic);
 	else if (apic_lvtt_tscdeadline(apic)) {
-		if (!(kvm_x86_ops->set_hv_timer && start_hv_tscdeadline(apic)))
+		if (!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
 			start_sw_tscdeadline(apic);
 	}
 }
-- 
1.9.1

[PATCH 5/5] KVM: LAPIC: add APIC Timer periodic/oneshot mode VMX preemption timer support

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Most windows guests still utilize APIC Timer periodic/oneshot mode
instead of tsc-deadline mode, and the APIC Timer periodic/oneshot
mode are still emulated by high overhead hrtimer on host. This patch
converts the expected expire time of the periodic/oneshot mode to
guest deadline tsc in order to leverage VMX preemption timer logic
for APIC Timer tsc-deadline mode. After each preemption timer vmexit
preemption timer is restarted to emulate LVTT current-count register
is automatically reloaded from the initial-count register when the
count reaches 0. This patch reduces ~3800 cycles for each APIC Timer
periodic mode operation virtualization.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/lapic.c | 103 ++++++++++++++++++++++++++++++++++++---------------
 arch/x86/kvm/lapic.h |   1 +
 2 files changed, 75 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 0354a79..4e77d37 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1090,7 +1090,7 @@ static void apic_send_ipi(struct kvm_lapic *apic)
 
 static u32 apic_get_tmcct(struct kvm_lapic *apic)
 {
-	ktime_t remaining;
+	ktime_t remaining, now;
 	s64 ns;
 	u32 tmcct;
 
@@ -1101,7 +1101,8 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
 		apic->lapic_timer.period == 0)
 		return 0;
 
-	remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
+	now = apic->lapic_timer.timer.base->get_time();
+	remaining = ktime_sub(apic->lapic_timer.target_expiration, now);
 	if (ktime_to_ns(remaining) < 0)
 		remaining = ktime_set(0, 0);
 
@@ -1351,13 +1352,28 @@ static void start_sw_period(struct kvm_lapic *apic)
 {
 	ktime_t now;
 
-	/* lapic timer in oneshot or periodic mode */
+	now = apic->lapic_timer.timer.base->get_time();
+
+	if (likely(ktime_compare(apic->lapic_timer.target_expiration, now)))
+		hrtimer_start(&apic->lapic_timer.timer,
+			apic->lapic_timer.target_expiration,
+			HRTIMER_MODE_ABS_PINNED);
+	else
+		apic_timer_expired(apic);
+}
+
+static bool set_target_expiration(struct kvm_lapic *apic)
+{
+	ktime_t now;
+	u64 tscl = rdtsc();
+
 	now = apic->lapic_timer.timer.base->get_time();
 	apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT)
-		    * APIC_BUS_CYCLE_NS * apic->divide_count;
+		* APIC_BUS_CYCLE_NS * apic->divide_count;
 
 	if (!apic->lapic_timer.period)
-		return;
+		return false;
+
 	/*
 	 * Do not allow the guest to program periodic timers with small
 	 * interval, since the hrtimers are not throttled by the host
@@ -1376,10 +1392,6 @@ static void start_sw_period(struct kvm_lapic *apic)
 		}
 	}
 
-	hrtimer_start(&apic->lapic_timer.timer,
-		      ktime_add_ns(now, apic->lapic_timer.period),
-		      HRTIMER_MODE_ABS_PINNED);
-
 	apic_debug("%s: bus cycle is %" PRId64 "ns, now 0x%016"
 		   PRIx64 ", "
 		   "timer initial count 0x%x, period %lldns, "
@@ -1389,6 +1401,12 @@ static void start_sw_period(struct kvm_lapic *apic)
 		   apic->lapic_timer.period,
 		   ktime_to_ns(ktime_add_ns(now,
 				apic->lapic_timer.period)));
+
+	apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
+		nsec_to_cycles(apic->vcpu, apic->lapic_timer.period);
+	apic->lapic_timer.target_expiration = ktime_add_ns(now, apic->lapic_timer.period);
+
+	return true;
 }
 
 bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu)
@@ -1406,22 +1424,12 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
 	apic->lapic_timer.hv_timer_in_use = false;
 }
 
-void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
-{
-	struct kvm_lapic *apic = vcpu->arch.apic;
-
-	WARN_ON(!apic->lapic_timer.hv_timer_in_use);
-	WARN_ON(swait_active(&vcpu->wq));
-	cancel_hv_timer(apic);
-	apic_timer_expired(apic);
-}
-EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
-
 static bool start_hv_timer(struct kvm_lapic *apic)
 {
 	u64 tscdeadline = apic->lapic_timer.tscdeadline;
 
-	if (atomic_read(&apic->lapic_timer.pending) ||
+	if ((atomic_read(&apic->lapic_timer.pending) &&
+		!apic_lvtt_period(apic)) ||
 		kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
 		if (apic->lapic_timer.hv_timer_in_use)
 			cancel_hv_timer(apic);
@@ -1430,7 +1438,8 @@ static bool start_hv_timer(struct kvm_lapic *apic)
 		hrtimer_cancel(&apic->lapic_timer.timer);
 
 		/* In case the sw timer triggered in the window */
-		if (atomic_read(&apic->lapic_timer.pending))
+		if (atomic_read(&apic->lapic_timer.pending) &&
+			!apic_lvtt_period(apic))
 			cancel_hv_timer(apic);
 	}
 	trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
@@ -1438,14 +1447,42 @@ static bool start_hv_timer(struct kvm_lapic *apic)
 	return apic->lapic_timer.hv_timer_in_use;
 }
 
+void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	WARN_ON(!apic->lapic_timer.hv_timer_in_use);
+	WARN_ON(swait_active(&vcpu->wq));
+	cancel_hv_timer(apic);
+	apic_timer_expired(apic);
+
+	if (apic_lvtt_period(apic) &&
+		set_target_expiration(apic) &&
+		!start_hv_timer(apic))
+		start_sw_period(apic);
+}
+EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
+
 void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
 	WARN_ON(apic->lapic_timer.hv_timer_in_use);
 
-	if (apic_lvtt_tscdeadline(apic))
-		start_hv_timer(apic);
+	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
+		ktime_t remaining, now;
+		u64 tscl = rdtsc();
+
+		now = apic->lapic_timer.timer.base->get_time();
+		remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
+		if (ktime_to_ns(remaining) < 0)
+			remaining = ktime_set(0, 0);
+
+		apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
+			nsec_to_cycles(apic->vcpu, ktime_to_ns(remaining));
+		apic->lapic_timer.target_expiration = ktime_add_ns(now, ktime_to_ns(remaining));
+	}
+	start_hv_timer(apic);
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer);
 
@@ -1462,7 +1499,10 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
 	if (atomic_read(&apic->lapic_timer.pending))
 		return;
 
-	start_sw_tscdeadline(apic);
+	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
+		start_sw_period(apic);
+	else if (apic_lvtt_tscdeadline(apic))
+		start_sw_tscdeadline(apic);
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
 
@@ -1470,9 +1510,11 @@ static void start_apic_timer(struct kvm_lapic *apic)
 {
 	atomic_set(&apic->lapic_timer.pending, 0);
 
-	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
-		start_sw_period(apic);
-	else if (apic_lvtt_tscdeadline(apic)) {
+	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
+		if (set_target_expiration(apic) &&
+			!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
+			start_sw_period(apic);
+	} else if (apic_lvtt_tscdeadline(apic)) {
 		if (!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
 			start_sw_tscdeadline(apic);
 	}
@@ -2005,8 +2047,11 @@ void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
 
 	if (atomic_read(&apic->lapic_timer.pending) > 0) {
 		kvm_apic_local_deliver(apic, APIC_LVTT);
-		if (apic_lvtt_tscdeadline(apic))
+		if (!(apic_lvtt_period(apic) &&
+			kvm_lapic_hv_timer_in_use(vcpu))) {
 			apic->lapic_timer.tscdeadline = 0;
+			apic->lapic_timer.target_expiration = ktime_set(0, 0);
+		}
 		atomic_set(&apic->lapic_timer.pending, 0);
 	}
 }
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 031db26..e0c8023 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -15,6 +15,7 @@
 struct kvm_timer {
 	struct hrtimer timer;
 	s64 period; 				/* unit: ns */
+	ktime_t target_expiration;
 	u32 timer_mode;
 	u32 timer_mode_mask;
 	u64 tscdeadline;
-- 
1.9.1

Re: [PATCH 5/5] KVM: LAPIC: add APIC Timer periodic/oneshot mode VMX preemption timer support

[Paolo Bonzini]

On 17/10/2016 09:45, Wanpeng Li wrote:
> +	remaining = ktime_sub(apic->lapic_timer.target_expiration, now);
>  	if (ktime_to_ns(remaining) < 0)
>  		remaining = ktime_set(0, 0);
>  
> @@ -1351,13 +1352,28 @@ static void start_sw_period(struct kvm_lapic *apic)
>  {
>  	ktime_t now;
>  
> -	/* lapic timer in oneshot or periodic mode */
> +	now = apic->lapic_timer.timer.base->get_time();
> +
> +	if (likely(ktime_compare(apic->lapic_timer.target_expiration, now)))

ktime_after, not ktime_compare.  Can be fixed on commit, I guess.

Paolo

> +		hrtimer_start(&apic->lapic_timer.timer,
> +			apic->lapic_timer.target_expiration,
> +			HRTIMER_MODE_ABS_PINNED);
> +	else
> +		apic_timer_expired(apic);
> +}
> +
> +static bool set_target_expiration(struct kvm_lapic *apic)
> +{
> +	ktime_t now;
> +	u64 tscl = rdtsc();
> +
>  	now = apic->lapic_timer.timer.base->get_time();
>  	apic->lapic_timer.period = (u64)kvm_lapic_get_reg(apic, APIC_TMICT)
> -		    * APIC_BUS_CYCLE_NS * apic->divide_count;
> +		* APIC_BUS_CYCLE_NS * apic->divide_count;
>  
>  	if (!apic->lapic_timer.period)
> -		return;
> +		return false;
>  	/*
>  	 * Do not allow the guest to program periodic timers with small
>  	 * interval, since the hrtimers are not throttled by the host
> @@ -1376,10 +1392,6 @@ static void start_sw_period(struct kvm_lapic *apic)
>  		}
>  	}
>  
> -	hrtimer_start(&apic->lapic_timer.timer,
> -		      ktime_add_ns(now, apic->lapic_timer.period),
> -		      HRTIMER_MODE_ABS_PINNED);
> -
>  	apic_debug("%s: bus cycle is %" PRId64 "ns, now 0x%016"
>  		   PRIx64 ", "
>  		   "timer initial count 0x%x, period %lldns, "
> @@ -1389,6 +1401,12 @@ static void start_sw_period(struct kvm_lapic *apic)
>  		   apic->lapic_timer.period,
>  		   ktime_to_ns(ktime_add_ns(now,
>  				apic->lapic_timer.period)));
> +
> +	apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
> +		nsec_to_cycles(apic->vcpu, apic->lapic_timer.period);
> +	apic->lapic_timer.target_expiration = ktime_add_ns(now, apic->lapic_timer.period);
> +
> +	return true;
>  }
>  
>  bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu)
> @@ -1406,22 +1424,12 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
>  	apic->lapic_timer.hv_timer_in_use = false;
>  }
>  
> -void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
> -{
> -	struct kvm_lapic *apic = vcpu->arch.apic;
> -
> -	WARN_ON(!apic->lapic_timer.hv_timer_in_use);
> -	WARN_ON(swait_active(&vcpu->wq));
> -	cancel_hv_timer(apic);
> -	apic_timer_expired(apic);
> -}
> -EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
> -
>  static bool start_hv_timer(struct kvm_lapic *apic)
>  {
>  	u64 tscdeadline = apic->lapic_timer.tscdeadline;
>  
> -	if (atomic_read(&apic->lapic_timer.pending) ||
> +	if ((atomic_read(&apic->lapic_timer.pending) &&
> +		!apic_lvtt_period(apic)) ||
>  		kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>  		if (apic->lapic_timer.hv_timer_in_use)
>  			cancel_hv_timer(apic);
> @@ -1430,7 +1438,8 @@ static bool start_hv_timer(struct kvm_lapic *apic)
>  		hrtimer_cancel(&apic->lapic_timer.timer);
>  
>  		/* In case the sw timer triggered in the window */
> -		if (atomic_read(&apic->lapic_timer.pending))
> +		if (atomic_read(&apic->lapic_timer.pending) &&
> +			!apic_lvtt_period(apic))
>  			cancel_hv_timer(apic);
>  	}
>  	trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
> @@ -1438,14 +1447,42 @@ static bool start_hv_timer(struct kvm_lapic *apic)
>  	return apic->lapic_timer.hv_timer_in_use;
>  }
>  
> +void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	WARN_ON(!apic->lapic_timer.hv_timer_in_use);
> +	WARN_ON(swait_active(&vcpu->wq));
> +	cancel_hv_timer(apic);
> +	apic_timer_expired(apic);
> +
> +	if (apic_lvtt_period(apic) &&
> +		set_target_expiration(apic) &&
> +		!start_hv_timer(apic))
> +		start_sw_period(apic);
> +}
> +EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
> +
>  void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  
>  	WARN_ON(apic->lapic_timer.hv_timer_in_use);
>  
> -	if (apic_lvtt_tscdeadline(apic))
> -		start_hv_timer(apic);
> +	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
> +		ktime_t remaining, now;
> +		u64 tscl = rdtsc();
> +
> +		now = apic->lapic_timer.timer.base->get_time();
> +		remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
> +		if (ktime_to_ns(remaining) < 0)
> +			remaining = ktime_set(0, 0);
> +
> +		apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
> +			nsec_to_cycles(apic->vcpu, ktime_to_ns(remaining));
> +		apic->lapic_timer.target_expiration = ktime_add_ns(now, ktime_to_ns(remaining));
> +	}
> +	start_hv_timer(apic);
>  }
>  EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer);
>  
> @@ -1462,7 +1499,10 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
>  	if (atomic_read(&apic->lapic_timer.pending))
>  		return;
>  
> -	start_sw_tscdeadline(apic);
> +	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
> +		start_sw_period(apic);
> +	else if (apic_lvtt_tscdeadline(apic))
> +		start_sw_tscdeadline(apic);
>  }
>  EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
>  
> @@ -1470,9 +1510,11 @@ static void start_apic_timer(struct kvm_lapic *apic)
>  {
>  	atomic_set(&apic->lapic_timer.pending, 0);
>  
> -	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
> -		start_sw_period(apic);
> -	else if (apic_lvtt_tscdeadline(apic)) {
> +	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
> +		if (set_target_expiration(apic) &&
> +			!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
> +			start_sw_period(apic);
> +	} else if (apic_lvtt_tscdeadline(apic)) {
>  		if (!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
>  			start_sw_tscdeadline(apic);
>  	}
> @@ -2005,8 +2047,11 @@ void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
>  
>  	if (atomic_read(&apic->lapic_timer.pending) > 0) {
>  		kvm_apic_local_deliver(apic, APIC_LVTT);
> -		if (apic_lvtt_tscdeadline(apic))
> +		if (!(apic_lvtt_period(apic) &&
> +			kvm_lapic_hv_timer_in_use(vcpu))) {
>  			apic->lapic_timer.tscdeadline = 0;
> +			apic->lapic_timer.target_expiration = ktime_set(0, 0);
> +		}
>  		atomic_set(&apic->lapic_timer.pending, 0);
>  	}
>  }
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 031db26..e0c8023 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -15,6 +15,7 @@
>  struct kvm_timer {
>  	struct hrtimer timer;
>  	s64 period; 				/* unit: ns */
> +	ktime_t target_expiration;
>  	u32 timer_mode;
>  	u32 timer_mode_mask;
>  	u64 tscdeadline;
> 

Re: [PATCH 5/5] KVM: LAPIC: add APIC Timer periodic/oneshot mode VMX preemption timer support

[Radim Krčmář]

2016-10-17 15:45+0800, Wanpeng Li:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> Most windows guests still utilize APIC Timer periodic/oneshot mode
> instead of tsc-deadline mode, and the APIC Timer periodic/oneshot
> mode are still emulated by high overhead hrtimer on host. This patch
> converts the expected expire time of the periodic/oneshot mode to
> guest deadline tsc in order to leverage VMX preemption timer logic
> for APIC Timer tsc-deadline mode. After each preemption timer vmexit
> preemption timer is restarted to emulate LVTT current-count register
> is automatically reloaded from the initial-count register when the
> count reaches 0. This patch reduces ~3800 cycles for each APIC Timer
> periodic mode operation virtualization.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> @@ -1090,7 +1090,7 @@ static void apic_send_ipi(struct kvm_lapic *apic)
>  
>  static u32 apic_get_tmcct(struct kvm_lapic *apic)
>  {
> -	ktime_t remaining;
> +	ktime_t remaining, now;
>  	s64 ns;
>  	u32 tmcct;
>  
> @@ -1101,7 +1101,8 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
>  		apic->lapic_timer.period == 0)
>  		return 0;
>  
> -	remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
> +	now = apic->lapic_timer.timer.base->get_time();
> +	remaining = ktime_sub(apic->lapic_timer.target_expiration, now);

Periodic timer does not advance apic->lapic_timer.target_expiration,
when rearming the hrtimer, so this would incorrectly return 0 in
subsequent periods.

>  	if (ktime_to_ns(remaining) < 0)
>  		remaining = ktime_set(0, 0);
>  
> @@ -1438,14 +1447,42 @@ static bool start_hv_timer(struct kvm_lapic *apic)
>  	return apic->lapic_timer.hv_timer_in_use;
>  }
>  
> +void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	WARN_ON(!apic->lapic_timer.hv_timer_in_use);
> +	WARN_ON(swait_active(&vcpu->wq));
> +	cancel_hv_timer(apic);
> +	apic_timer_expired(apic);
> +
> +	if (apic_lvtt_period(apic) &&
> +		set_target_expiration(apic) &&
> +		!start_hv_timer(apic))
> +		start_sw_period(apic);
> +}
> +EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
> +
>  void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  
>  	WARN_ON(apic->lapic_timer.hv_timer_in_use);
>  
> -	if (apic_lvtt_tscdeadline(apic))
> -		start_hv_timer(apic);
> +	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
> +		ktime_t remaining, now;
> +		u64 tscl = rdtsc();
> +
> +		now = apic->lapic_timer.timer.base->get_time();
> +		remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
> +		if (ktime_to_ns(remaining) < 0)
> +			remaining = ktime_set(0, 0);
> +
> +		apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
> +			nsec_to_cycles(apic->vcpu, ktime_to_ns(remaining));
> +		apic->lapic_timer.target_expiration = ktime_add_ns(now, ktime_to_ns(remaining));

For oneshot timer, there is no need to hrtimer_get_remaining(), because
apic->lapic_timer.tscdeadline and apic->lapic_timer.target_expiration
are already correct, so we could just use them.

The same could be true for the periodic timer as well, but
apic->lapic_timer.target_expiration nor apic->lapic_timer.tscdeadline is
advanced in apic_timer_fn(), so they are soon incorrect.

I think it would be better to add a function to advance the periodic
timer and use it in kvm_lapic_expired_hv_timer() and in apic_timer_fn().

The function can be simpler than set_target_expiration(), because it
just adds the period to an existing timer.  Periodic timer will also be
better then, because the period will not depend on KVM's latency when
rearming.

> +	}
> +	start_hv_timer(apic);
>  }
>  EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer);
>  
> @@ -1462,7 +1499,10 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
>  	if (atomic_read(&apic->lapic_timer.pending))
>  		return;
>  
> -	start_sw_tscdeadline(apic);
> +	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
> +		start_sw_period(apic);
> +	else if (apic_lvtt_tscdeadline(apic))
> +		start_sw_tscdeadline(apic);
>  }
>  EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
>  
> @@ -1470,9 +1510,11 @@ static void start_apic_timer(struct kvm_lapic *apic)
>  {
>  	atomic_set(&apic->lapic_timer.pending, 0);
>  
> -	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
> -		start_sw_period(apic);
> -	else if (apic_lvtt_tscdeadline(apic)) {
> +	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
> +		if (set_target_expiration(apic) &&
> +			!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
> +			start_sw_period(apic);
> +	} else if (apic_lvtt_tscdeadline(apic)) {
>  		if (!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
>  			start_sw_tscdeadline(apic);
>  	}
> @@ -2005,8 +2047,11 @@ void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
>  
>  	if (atomic_read(&apic->lapic_timer.pending) > 0) {
>  		kvm_apic_local_deliver(apic, APIC_LVTT);
> -		if (apic_lvtt_tscdeadline(apic))
> +		if (!(apic_lvtt_period(apic) &&
> +			kvm_lapic_hv_timer_in_use(vcpu))) {

This would zero apic->lapic_timer.target_expiration of
apic_lvtt_period() when !kvm_lapic_hv_timer_in_use().

I think we don't want to ever do that, so we want

  		if (!(apic_lvtt_period(apic)) {

or maybe even better

  		if (apic_lvtt_tscdeadline(apic))
  			apic->lapic_timer.tscdeadline = 0;

  		if (apic_lvtt_oneshot(apic)) {
  			apic->lapic_timer.tscdeadline = 0;
  			apic->lapic_timer.target_expiration = ktime_set(0, 0);
  		}

>  			apic->lapic_timer.tscdeadline = 0;
> +			apic->lapic_timer.target_expiration = ktime_set(0, 0);
> +		}
>  		atomic_set(&apic->lapic_timer.pending, 0);
>  	}
>  }

Re: [PATCH 5/5] KVM: LAPIC: add APIC Timer periodic/oneshot mode VMX preemption timer support

[Wanpeng Li]

2016-10-20 3:28 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> 2016-10-17 15:45+0800, Wanpeng Li:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> Most windows guests still utilize APIC Timer periodic/oneshot mode
>> instead of tsc-deadline mode, and the APIC Timer periodic/oneshot
>> mode are still emulated by high overhead hrtimer on host. This patch
>> converts the expected expire time of the periodic/oneshot mode to
>> guest deadline tsc in order to leverage VMX preemption timer logic
>> for APIC Timer tsc-deadline mode. After each preemption timer vmexit
>> preemption timer is restarted to emulate LVTT current-count register
>> is automatically reloaded from the initial-count register when the
>> count reaches 0. This patch reduces ~3800 cycles for each APIC Timer
>> periodic mode operation virtualization.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> @@ -1090,7 +1090,7 @@ static void apic_send_ipi(struct kvm_lapic *apic)
>>
>>  static u32 apic_get_tmcct(struct kvm_lapic *apic)
>>  {
>> -     ktime_t remaining;
>> +     ktime_t remaining, now;
>>       s64 ns;
>>       u32 tmcct;
>>
>> @@ -1101,7 +1101,8 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
>>               apic->lapic_timer.period == 0)
>>               return 0;
>>
>> -     remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
>> +     now = apic->lapic_timer.timer.base->get_time();
>> +     remaining = ktime_sub(apic->lapic_timer.target_expiration, now);
>
> Periodic timer does not advance apic->lapic_timer.target_expiration,
> when rearming the hrtimer, so this would incorrectly return 0 in
> subsequent periods.

Agreed.

>
>>       if (ktime_to_ns(remaining) < 0)
>>               remaining = ktime_set(0, 0);
>>
>> @@ -1438,14 +1447,42 @@ static bool start_hv_timer(struct kvm_lapic *apic)
>>       return apic->lapic_timer.hv_timer_in_use;
>>  }
>>
>> +void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
>> +{
>> +     struct kvm_lapic *apic = vcpu->arch.apic;
>> +
>> +     WARN_ON(!apic->lapic_timer.hv_timer_in_use);
>> +     WARN_ON(swait_active(&vcpu->wq));
>> +     cancel_hv_timer(apic);
>> +     apic_timer_expired(apic);
>> +
>> +     if (apic_lvtt_period(apic) &&
>> +             set_target_expiration(apic) &&
>> +             !start_hv_timer(apic))
>> +             start_sw_period(apic);
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
>> +
>>  void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu)
>>  {
>>       struct kvm_lapic *apic = vcpu->arch.apic;
>>
>>       WARN_ON(apic->lapic_timer.hv_timer_in_use);
>>
>> -     if (apic_lvtt_tscdeadline(apic))
>> -             start_hv_timer(apic);
>> +     if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
>> +             ktime_t remaining, now;
>> +             u64 tscl = rdtsc();
>> +
>> +             now = apic->lapic_timer.timer.base->get_time();
>> +             remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
>> +             if (ktime_to_ns(remaining) < 0)
>> +                     remaining = ktime_set(0, 0);
>> +
>> +             apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
>> +                     nsec_to_cycles(apic->vcpu, ktime_to_ns(remaining));
>> +             apic->lapic_timer.target_expiration = ktime_add_ns(now, ktime_to_ns(remaining));
>
> For oneshot timer, there is no need to hrtimer_get_remaining(), because
> apic->lapic_timer.tscdeadline and apic->lapic_timer.target_expiration
> are already correct, so we could just use them.

Agreed.

>
> The same could be true for the periodic timer as well, but
> apic->lapic_timer.target_expiration nor apic->lapic_timer.tscdeadline is
> advanced in apic_timer_fn(), so they are soon incorrect.

Yeah, I catch this when testing.

>
> I think it would be better to add a function to advance the periodic
> timer and use it in kvm_lapic_expired_hv_timer() and in apic_timer_fn().
>
> The function can be simpler than set_target_expiration(), because it
> just adds the period to an existing timer.  Periodic timer will also be
> better then, because the period will not depend on KVM's latency when
> rearming.

Good point, what's the function name do you like? How about
advance_target_expiration()?

>
>> +     }
>> +     start_hv_timer(apic);
>>  }
>>  EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer);
>>
>> @@ -1462,7 +1499,10 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
>>       if (atomic_read(&apic->lapic_timer.pending))
>>               return;
>>
>> -     start_sw_tscdeadline(apic);
>> +     if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
>> +             start_sw_period(apic);
>> +     else if (apic_lvtt_tscdeadline(apic))
>> +             start_sw_tscdeadline(apic);
>>  }
>>  EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
>>
>> @@ -1470,9 +1510,11 @@ static void start_apic_timer(struct kvm_lapic *apic)
>>  {
>>       atomic_set(&apic->lapic_timer.pending, 0);
>>
>> -     if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
>> -             start_sw_period(apic);
>> -     else if (apic_lvtt_tscdeadline(apic)) {
>> +     if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
>> +             if (set_target_expiration(apic) &&
>> +                     !(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
>> +                     start_sw_period(apic);
>> +     } else if (apic_lvtt_tscdeadline(apic)) {
>>               if (!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
>>                       start_sw_tscdeadline(apic);
>>       }
>> @@ -2005,8 +2047,11 @@ void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
>>
>>       if (atomic_read(&apic->lapic_timer.pending) > 0) {
>>               kvm_apic_local_deliver(apic, APIC_LVTT);
>> -             if (apic_lvtt_tscdeadline(apic))
>> +             if (!(apic_lvtt_period(apic) &&
>> +                     kvm_lapic_hv_timer_in_use(vcpu))) {
>
> This would zero apic->lapic_timer.target_expiration of
> apic_lvtt_period() when !kvm_lapic_hv_timer_in_use().
>
> I think we don't want to ever do that, so we want
>
>                 if (!(apic_lvtt_period(apic)) {
>
> or maybe even better
>
>                 if (apic_lvtt_tscdeadline(apic))
>                         apic->lapic_timer.tscdeadline = 0;
>
>                 if (apic_lvtt_oneshot(apic)) {
>                         apic->lapic_timer.tscdeadline = 0;
>                         apic->lapic_timer.target_expiration = ktime_set(0, 0);
>                 }
>
>>                       apic->lapic_timer.tscdeadline = 0;
>> +                     apic->lapic_timer.target_expiration = ktime_set(0, 0);
>> +             }

Agreed, thanks for your review. :)

Regards,
Wanpeng Li

Re: [PATCH 5/5] KVM: LAPIC: add APIC Timer periodic/oneshot mode VMX preemption timer support

[Radim Krčmář]

2016-10-20 06:57+0800, Wanpeng Li:
> 2016-10-20 3:28 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
>> I think it would be better to add a function to advance the periodic
>> timer and use it in kvm_lapic_expired_hv_timer() and in apic_timer_fn().
>>
>> The function can be simpler than set_target_expiration(), because it
>> just adds the period to an existing timer.  Periodic timer will also be
>> better then, because the period will not depend on KVM's latency when
>> rearming.
> 
> Good point, what's the function name do you like? How about
> advance_target_expiration()?

Sounds good, I'd just slap periodic somewhere,
advance_periodic_target_expiration()?

Re: [PATCH 5/5] KVM: LAPIC: add APIC Timer periodic/oneshot mode VMX preemption timer support

[Wanpeng Li]

2016-10-21 2:10 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> 2016-10-20 06:57+0800, Wanpeng Li:
>> 2016-10-20 3:28 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
>>> I think it would be better to add a function to advance the periodic
>>> timer and use it in kvm_lapic_expired_hv_timer() and in apic_timer_fn().
>>>
>>> The function can be simpler than set_target_expiration(), because it
>>> just adds the period to an existing timer.  Periodic timer will also be
>>> better then, because the period will not depend on KVM's latency when
>>> rearming.
>>
>> Good point, what's the function name do you like? How about
>> advance_target_expiration()?
>
> Sounds good, I'd just slap periodic somewhere,
> advance_periodic_target_expiration()?

Cool, thanks for your review. :)

Regards,
Wanpeng Li