* [PATCH RFC V2 0/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode
@ 2016-10-12 6:52 Wanpeng Li
2016-10-12 6:52 ` [PATCH RFC V2 1/2] KVM: lapic: introduce start_sw_period() to handle oneshot/periodic mode Wanpeng Li
2016-10-12 6:52 ` [PATCH RFC V2 2/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode Wanpeng Li
0 siblings, 2 replies; 6+ messages in thread
From: Wanpeng Li @ 2016-10-12 6:52 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Radim Krčmář, Yunhong Jiang, 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 support using the VMX preemption timer for APIC Timer
periodic/oneshot mode.
I add periodic mode testcase to kvm-unit-tests/apic.flat and observed that w/ patch
the latency is ~13% of w/o patch. I think maybe something is still not right in the
patchset. Your comments to improve the patchset is a great appreciated.
v1 -> 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 (2):
KVM: lapic: introduce start_sw_period() to handle oneshot/periodic mode
KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode
arch/x86/kvm/lapic.c | 166 ++++++++++++++++++++++++++++++++-------------------
arch/x86/kvm/lapic.h | 1 +
2 files changed, 107 insertions(+), 60 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH RFC V2 1/2] KVM: lapic: introduce start_sw_period() to handle oneshot/periodic mode
2016-10-12 6:52 [PATCH RFC V2 0/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode Wanpeng Li
@ 2016-10-12 6:52 ` Wanpeng Li
2016-10-12 6:52 ` [PATCH RFC V2 2/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode Wanpeng Li
1 sibling, 0 replies; 6+ messages in thread
From: Wanpeng Li @ 2016-10-12 6:52 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Radim Krčmář, Yunhong Jiang, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH RFC V2 2/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode
2016-10-12 6:52 [PATCH RFC V2 0/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode Wanpeng Li
2016-10-12 6:52 ` [PATCH RFC V2 1/2] KVM: lapic: introduce start_sw_period() to handle oneshot/periodic mode Wanpeng Li
@ 2016-10-12 6:52 ` Wanpeng Li
2016-10-12 17:41 ` Radim Krčmář
1 sibling, 1 reply; 6+ messages in thread
From: Wanpeng Li @ 2016-10-12 6:52 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Radim Krčmář, Yunhong Jiang, 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.
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, 73 insertions(+), 31 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index dad743e..f5a03ae 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1101,7 +1101,14 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
apic->lapic_timer.period == 0)
return 0;
- remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
+ if (kvm_lapic_hv_timer_in_use(apic->vcpu)) {
+ ktime_t now;
+
+ now = apic->lapic_timer.timer.base->get_time();
+ remaining = ktime_sub(apic->lapic_timer.expired_period, now);
+ } else
+ remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
+
if (ktime_to_ns(remaining) < 0)
remaining = ktime_set(0, 0);
@@ -1353,8 +1360,6 @@ static void start_sw_period(struct kvm_lapic *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;
@@ -1377,7 +1382,7 @@ static void start_sw_period(struct kvm_lapic *apic)
}
hrtimer_start(&apic->lapic_timer.timer,
- ktime_add_ns(now, apic->lapic_timer.period),
+ apic->lapic_timer.expired_period,
HRTIMER_MODE_ABS_PINNED);
apic_debug("%s: bus cycle is %" PRId64 "ns, now 0x%016"
@@ -1400,52 +1405,71 @@ 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;
}
-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_tscdeadline(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) ||
+ 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_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);
+ if (atomic_read(&apic->lapic_timer.pending) &&
+ !apic_lvtt_period(apic))
+ cancel_hv_timer(apic);
}
trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
apic->lapic_timer.hv_timer_in_use);
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)) {
+ ktime_t now;
+ u64 tscl = rdtsc();
+
+ 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;
+
+ apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
+ nsec_to_cycles(apic->vcpu, apic->lapic_timer.period);
+ now = apic->lapic_timer.timer.base->get_time();
+ apic->lapic_timer.expired_period = ktime_add_ns(now, apic->lapic_timer.period);
+
+ start_hv_timer(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_tscdeadline(apic);
+ start_hv_timer(apic);
}
EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer);
@@ -1457,12 +1481,15 @@ 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;
- 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,10 +1497,25 @@ 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 (!(kvm_x86_ops->set_hv_timer && start_hv_tscdeadline(apic)))
+ if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
+ ktime_t now;
+ u64 tscl = rdtsc();
+
+ 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;
+
+ apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
+ nsec_to_cycles(apic->vcpu, apic->lapic_timer.period);
+ now = apic->lapic_timer.timer.base->get_time();
+ apic->lapic_timer.expired_period = ktime_add_ns(now, apic->lapic_timer.period);
+
+ if (!(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);
}
}
@@ -1711,8 +1753,7 @@ 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))
return 0;
return apic->lapic_timer.tscdeadline;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index f60d01c..fea7602 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 expired_period;
u32 timer_mode;
u32 timer_mode_mask;
u64 tscdeadline;
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC V2 2/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode
2016-10-12 6:52 ` [PATCH RFC V2 2/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode Wanpeng Li
@ 2016-10-12 17:41 ` Radim Krčmář
2016-10-13 11:38 ` Wanpeng Li
0 siblings, 1 reply; 6+ messages in thread
From: Radim Krčmář @ 2016-10-12 17:41 UTC (permalink / raw)
To: Wanpeng Li; +Cc: linux-kernel, kvm, Paolo Bonzini, Yunhong Jiang, Wanpeng Li
2016-10-12 14:52+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.
>
> 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
> @@ -1101,7 +1101,14 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
> apic->lapic_timer.period == 0)
> return 0;
>
> - remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
> + if (kvm_lapic_hv_timer_in_use(apic->vcpu)) {
apic->lapic_timer.expired_period is set unconditionally, so the
condition seems superfluous.
> + ktime_t now;
> +
> + now = apic->lapic_timer.timer.base->get_time();
> + remaining = ktime_sub(apic->lapic_timer.expired_period, now);
The name expired_period is weird for something that can be in the future
... maybe target_ns?
> + } else
> + remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
> +
> if (ktime_to_ns(remaining) < 0)
> remaining = ktime_set(0, 0);
>
> @@ -1353,8 +1360,6 @@ static void start_sw_period(struct kvm_lapic *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;
I would gut start_sw_period() a bit more: the checking of minimal period
is a bit misplaced, because it uses now() when the timer switched to
hrtimers, but that could have been long after the timer was set, so we'd
delay for no good reason.
> if (!apic->lapic_timer.period)
> return;
Can this happen? (WARN_ONCE() if it is only a sanity check.)
> @@ -1400,52 +1405,71 @@ bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu)
> +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)) {
> + ktime_t now;
> + u64 tscl = rdtsc();
> +
> + 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;
> +
> + apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
> + nsec_to_cycles(apic->vcpu, apic->lapic_timer.period);
> + now = apic->lapic_timer.timer.base->get_time();
> + apic->lapic_timer.expired_period = ktime_add_ns(now, apic->lapic_timer.period);
> +
> + start_hv_timer(apic);
> + }
> +}
> +EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
> +
> @@ -1470,10 +1497,25 @@ 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 (!(kvm_x86_ops->set_hv_timer && start_hv_tscdeadline(apic)))
> + if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
> + ktime_t now;
> + u64 tscl = rdtsc();
> +
> + 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;
> + apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
> + nsec_to_cycles(apic->vcpu, apic->lapic_timer.period);
> + now = apic->lapic_timer.timer.base->get_time();
> + apic->lapic_timer.expired_period = ktime_add_ns(now, apic->lapic_timer.period);
This is the same code as in kvm_lapic_expired_hv_timer(), a helper
function is in order.
> +
> + if (!(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);
> }
> }
> @@ -1711,8 +1753,7 @@ 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))
rdmsr MSR_IA32_TSCDEADLINE has to return 0 when the timer is not in tsc
deadline mode, so the condition should stay. (and check for
apic_lvtt_tscdeadline(), but that is a unrelated cleanup.)
> + if (!lapic_in_kernel(vcpu))
> return 0;
>
> return apic->lapic_timer.tscdeadline;
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC V2 2/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode
2016-10-12 17:41 ` Radim Krčmář
@ 2016-10-13 11:38 ` Wanpeng Li
2016-10-13 11:52 ` Wanpeng Li
0 siblings, 1 reply; 6+ messages in thread
From: Wanpeng Li @ 2016-10-13 11:38 UTC (permalink / raw)
To: Radim Krčmář
Cc: linux-kernel, kvm, Paolo Bonzini, Yunhong Jiang, Wanpeng Li
2016-10-13 1:41 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> 2016-10-12 14:52+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.
>>
>> 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
>> @@ -1101,7 +1101,14 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
>> apic->lapic_timer.period == 0)
>> return 0;
>>
>> - remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
>> + if (kvm_lapic_hv_timer_in_use(apic->vcpu)) {
>
> apic->lapic_timer.expired_period is set unconditionally, so the
> condition seems superfluous.
Agreed.
>
>> + ktime_t now;
>> +
>> + now = apic->lapic_timer.timer.base->get_time();
>> + remaining = ktime_sub(apic->lapic_timer.expired_period, now);
>
> The name expired_period is weird for something that can be in the future
> ... maybe target_ns?
How about target_expiration? It should be ktime_t as the second
parameter of hrtimer_start().
>
>> + } else
>> + remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
>> +
>> if (ktime_to_ns(remaining) < 0)
>> remaining = ktime_set(0, 0);
>>
>> @@ -1353,8 +1360,6 @@ static void start_sw_period(struct kvm_lapic *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;
>
> I would gut start_sw_period() a bit more: the checking of minimal period
> is a bit misplaced, because it uses now() when the timer switched to
> hrtimers, but that could have been long after the timer was set, so we'd
> delay for no good reason.
Agreed.
>
>> if (!apic->lapic_timer.period)
>> return;
>
> Can this happen? (WARN_ONCE() if it is only a sanity check.)
Actually I catch several apic->lapic_timer.period == 0 here, in
addition, the oneshot mode test of kvm-unit-tests/apic.flat will fail
if change it to WARN_ONCE() instead of return directly.
>
>> @@ -1400,52 +1405,71 @@ bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu)
>> +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)) {
>> + ktime_t now;
>> + u64 tscl = rdtsc();
>> +
>> + 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;
>> +
>> + apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
>> + nsec_to_cycles(apic->vcpu, apic->lapic_timer.period);
>> + now = apic->lapic_timer.timer.base->get_time();
>> + apic->lapic_timer.expired_period = ktime_add_ns(now, apic->lapic_timer.period);
>> +
>> + start_hv_timer(apic);
>> + }
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
>> +
>> @@ -1470,10 +1497,25 @@ 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 (!(kvm_x86_ops->set_hv_timer && start_hv_tscdeadline(apic)))
>> + if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
>> + ktime_t now;
>> + u64 tscl = rdtsc();
>> +
>> + 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;
>> + apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
>> + nsec_to_cycles(apic->vcpu, apic->lapic_timer.period);
>> + now = apic->lapic_timer.timer.base->get_time();
>> + apic->lapic_timer.expired_period = ktime_add_ns(now, apic->lapic_timer.period);
>
> This is the same code as in kvm_lapic_expired_hv_timer(), a helper
> function is in order.
Agreed.
>
>> +
>> + if (!(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);
>> }
>> }
>> @@ -1711,8 +1753,7 @@ 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))
>
> rdmsr MSR_IA32_TSCDEADLINE has to return 0 when the timer is not in tsc
> deadline mode, so the condition should stay. (and check for
> apic_lvtt_tscdeadline(), but that is a unrelated cleanup.)
Agreed. I just sent out RFC V3 to handle these comments, thanks for
your active review, Radim. It is really helpful. :)
Regards,
Wanpeng Li
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC V2 2/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode
2016-10-13 11:38 ` Wanpeng Li
@ 2016-10-13 11:52 ` Wanpeng Li
0 siblings, 0 replies; 6+ messages in thread
From: Wanpeng Li @ 2016-10-13 11:52 UTC (permalink / raw)
To: Radim Krčmář
Cc: linux-kernel, kvm, Paolo Bonzini, Yunhong Jiang, Wanpeng Li
2016-10-13 19:38 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2016-10-13 1:41 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
>> 2016-10-12 14:52+0800, Wanpeng Li:
[...]
>>> @@ -1711,8 +1753,7 @@ 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))
>>
>> rdmsr MSR_IA32_TSCDEADLINE has to return 0 when the timer is not in tsc
>> deadline mode, so the condition should stay. (and check for
>> apic_lvtt_tscdeadline(), but that is a unrelated cleanup.)
Actually I modify this function to get apic->lapic_timer.tscdeadline
in kvm_arch_vcpu_load() for periodic/oneshot mode, I introduce another
function to handle this in order to avoid break rdmsr
MSR_IA32_TSCDEADLINE sanity check in RFC V3.
Regards,
Wanpeng Li
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-13 11:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-12 6:52 [PATCH RFC V2 0/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode Wanpeng Li
2016-10-12 6:52 ` [PATCH RFC V2 1/2] KVM: lapic: introduce start_sw_period() to handle oneshot/periodic mode Wanpeng Li
2016-10-12 6:52 ` [PATCH RFC V2 2/2] KVM: x86: Support using the VMX preemption timer for APIC Timer periodic/oneshot mode Wanpeng Li
2016-10-12 17:41 ` Radim Krčmář
2016-10-13 11:38 ` Wanpeng Li
2016-10-13 11:52 ` Wanpeng Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.