From: Wanpeng Li <[email protected]>
According to the SDM 10.5.4.1:
A write of 0 to the initial-count register effectively stops the local
APIC timer, in both one-shot and periodic mode.
The lapic timer oneshot/periodic mode which is emulated by vmx-preemption
timer doesn't stop since vmx->hv_deadline_tsc is still set. This patch
fixes it by also cancel vmx-preemption timer when writing 0 to initial-count
register.
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/lapic.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 8120e86..20dd2ae 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1494,6 +1494,15 @@ static void limit_periodic_timer_frequency(struct kvm_lapic *apic)
static void cancel_hv_timer(struct kvm_lapic *apic);
+static void cancel_timer(struct kvm_lapic *apic)
+{
+ hrtimer_cancel(&apic->lapic_timer.timer);
+ preempt_disable();
+ if (apic->lapic_timer.hv_timer_in_use)
+ cancel_hv_timer(apic);
+ preempt_enable();
+}
+
static void apic_update_lvtt(struct kvm_lapic *apic)
{
u32 timer_mode = kvm_lapic_get_reg(apic, APIC_LVTT) &
@@ -1502,11 +1511,7 @@ static void apic_update_lvtt(struct kvm_lapic *apic)
if (apic->lapic_timer.timer_mode != timer_mode) {
if (apic_lvtt_tscdeadline(apic) != (timer_mode ==
APIC_LVT_TIMER_TSCDEADLINE)) {
- hrtimer_cancel(&apic->lapic_timer.timer);
- preempt_disable();
- if (apic->lapic_timer.hv_timer_in_use)
- cancel_hv_timer(apic);
- preempt_enable();
+ cancel_timer(apic);
kvm_lapic_set_reg(apic, APIC_TMICT, 0);
apic->lapic_timer.period = 0;
apic->lapic_timer.tscdeadline = 0;
@@ -2092,7 +2097,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
if (apic_lvtt_tscdeadline(apic))
break;
- hrtimer_cancel(&apic->lapic_timer.timer);
+ cancel_timer(apic);
kvm_lapic_set_reg(apic, APIC_TMICT, val);
start_apic_timer(apic);
break;
--
2.7.4
From: Wanpeng Li <[email protected]>
The value of current counter register after reset is 0 for both Intel
and AMD, let's do it in kvm.
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/lapic.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 20dd2ae..9ba539b 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2352,6 +2352,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
kvm_lapic_set_reg(apic, APIC_ICR2, 0);
kvm_lapic_set_reg(apic, APIC_TDCR, 0);
kvm_lapic_set_reg(apic, APIC_TMICT, 0);
+ kvm_lapic_set_reg(apic, APIC_TMCCT, 0);
for (i = 0; i < 8; i++) {
kvm_lapic_set_reg(apic, APIC_IRR + 0x10 * i, 0);
kvm_lapic_set_reg(apic, APIC_ISR + 0x10 * i, 0);
--
2.7.4
On Thu, Jun 3, 2021 at 2:01 AM Wanpeng Li <[email protected]> wrote:
>
> From: Wanpeng Li <[email protected]>
>
> The value of current counter register after reset is 0 for both Intel
> and AMD, let's do it in kvm.
>
> Signed-off-by: Wanpeng Li <[email protected]>
How did we miss that?
Reviewed-by: Jim Mattson <[email protected]>
On Thu, Jun 03, 2021, Wanpeng Li wrote:
> From: Wanpeng Li <[email protected]>
>
> According to the SDM 10.5.4.1:
>
> A write of 0 to the initial-count register effectively stops the local
> APIC timer, in both one-shot and periodic mode.
>
> The lapic timer oneshot/periodic mode which is emulated by vmx-preemption
> timer doesn't stop since vmx->hv_deadline_tsc is still set.
But the VMX preemption timer is only used for deadline, never for oneshot or
periodic. Am I missing something?
static bool start_hv_timer(struct kvm_lapic *apic)
{
struct kvm_timer *ktimer = &apic->lapic_timer;
struct kvm_vcpu *vcpu = apic->vcpu;
bool expired;
WARN_ON(preemptible());
if (!kvm_can_use_hv_timer(vcpu))
return false;
if (!ktimer->tscdeadline) <-------
return false;
...
}
On Thu, Jun 03, 2021, Jim Mattson wrote:
> On Thu, Jun 3, 2021 at 2:01 AM Wanpeng Li <[email protected]> wrote:
> >
> > From: Wanpeng Li <[email protected]>
> >
> > The value of current counter register after reset is 0 for both Intel
> > and AMD, let's do it in kvm.
> >
> > Signed-off-by: Wanpeng Li <[email protected]>
>
> How did we miss that?
I suspect it's not actually a functional issue, and that writing '0' at reset is
a glorified nop. The TMCCT is always computed on-demand and never directly
readable.
Is there an observable bug being fixed? If not, the changelog should state that
this is a cosmetic change of sorts.
static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset)
{
u32 val = 0;
if (offset >= LAPIC_MMIO_LENGTH)
return 0;
switch (offset) {
case APIC_ARBPRI:
break;
case APIC_TMCCT: /* Timer CCR */
if (apic_lvtt_tscdeadline(apic))
return 0;
val = apic_get_tmcct(apic);
break;
...
}
static u32 apic_get_tmcct(struct kvm_lapic *apic)
{
ktime_t remaining, now;
s64 ns;
u32 tmcct;
ASSERT(apic != NULL);
/* if initial count is 0, current count should also be 0 */
if (kvm_lapic_get_reg(apic, APIC_TMICT) == 0 || <------------
apic->lapic_timer.period == 0)
return 0;
now = ktime_get();
remaining = ktime_sub(apic->lapic_timer.target_expiration, now);
if (ktime_to_ns(remaining) < 0)
remaining = 0;
ns = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period);
tmcct = div64_u64(ns,
(APIC_BUS_CYCLE_NS * apic->divide_count));
return tmcct;
}
int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
{
memcpy(s->regs, vcpu->arch.apic->regs, sizeof(*s));
/*
* Get calculated timer current count for remaining timer period (if
* any) and store it in the returned register set.
*/
__kvm_lapic_set_reg(s->regs, APIC_TMCCT,
__apic_read(vcpu->arch.apic, APIC_TMCCT)); <----
return kvm_apic_state_fixup(vcpu, s, false);
}
On Thu, Jun 3, 2021 at 2:04 AM Wanpeng Li <[email protected]> wrote:
>
> From: Wanpeng Li <[email protected]>
>
> According to the SDM 10.5.4.1:
>
> A write of 0 to the initial-count register effectively stops the local
> APIC timer, in both one-shot and periodic mode.
If KVM is not correctly emulating this behavior then could you also
add a kvm-unit-test to test for the correct behavior?
>
> The lapic timer oneshot/periodic mode which is emulated by vmx-preemption
> timer doesn't stop since vmx->hv_deadline_tsc is still set. This patch
> fixes it by also cancel vmx-preemption timer when writing 0 to initial-count
> register.
>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/kvm/lapic.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 8120e86..20dd2ae 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1494,6 +1494,15 @@ static void limit_periodic_timer_frequency(struct kvm_lapic *apic)
>
> static void cancel_hv_timer(struct kvm_lapic *apic);
>
> +static void cancel_timer(struct kvm_lapic *apic)
> +{
> + hrtimer_cancel(&apic->lapic_timer.timer);
> + preempt_disable();
> + if (apic->lapic_timer.hv_timer_in_use)
> + cancel_hv_timer(apic);
> + preempt_enable();
> +}
> +
> static void apic_update_lvtt(struct kvm_lapic *apic)
> {
> u32 timer_mode = kvm_lapic_get_reg(apic, APIC_LVTT) &
> @@ -1502,11 +1511,7 @@ static void apic_update_lvtt(struct kvm_lapic *apic)
> if (apic->lapic_timer.timer_mode != timer_mode) {
> if (apic_lvtt_tscdeadline(apic) != (timer_mode ==
> APIC_LVT_TIMER_TSCDEADLINE)) {
> - hrtimer_cancel(&apic->lapic_timer.timer);
> - preempt_disable();
> - if (apic->lapic_timer.hv_timer_in_use)
> - cancel_hv_timer(apic);
> - preempt_enable();
> + cancel_timer(apic);
> kvm_lapic_set_reg(apic, APIC_TMICT, 0);
> apic->lapic_timer.period = 0;
> apic->lapic_timer.tscdeadline = 0;
> @@ -2092,7 +2097,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
> if (apic_lvtt_tscdeadline(apic))
> break;
>
> - hrtimer_cancel(&apic->lapic_timer.timer);
> + cancel_timer(apic);
> kvm_lapic_set_reg(apic, APIC_TMICT, val);
> start_apic_timer(apic);
> break;
> --
> 2.7.4
>
On Fri, 4 Jun 2021 at 07:02, David Matlack <[email protected]> wrote:
>
> On Thu, Jun 3, 2021 at 2:04 AM Wanpeng Li <[email protected]> wrote:
> >
> > From: Wanpeng Li <[email protected]>
> >
> > According to the SDM 10.5.4.1:
> >
> > A write of 0 to the initial-count register effectively stops the local
> > APIC timer, in both one-shot and periodic mode.
>
> If KVM is not correctly emulating this behavior then could you also
> add a kvm-unit-test to test for the correct behavior?
A simple test here, the test will hang after the patch since it will
not receive the spurious interrupt any more.
diff --git a/x86/apic.c b/x86/apic.c
index a7681fe..947d018 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -488,6 +488,14 @@ static void test_apic_timer_one_shot(void)
*/
report((lvtt_counter == 1) && (tsc2 - tsc1 >= interval),
"APIC LVT timer one shot");
+
+ lvtt_counter = 0;
+ apic_write(APIC_TMICT, interval);
+ apic_write(APIC_TMICT, 0);
+ while (!lvtt_counter);
+
+ report((lvtt_counter == 1),
+ "APIC LVT timer one shot spurious interrupt");
}
On Thu, 3 Jun 2021 at 23:20, Sean Christopherson <[email protected]> wrote:
>
> On Thu, Jun 03, 2021, Wanpeng Li wrote:
> > From: Wanpeng Li <[email protected]>
> >
> > According to the SDM 10.5.4.1:
> >
> > A write of 0 to the initial-count register effectively stops the local
> > APIC timer, in both one-shot and periodic mode.
> >
> > The lapic timer oneshot/periodic mode which is emulated by vmx-preemption
> > timer doesn't stop since vmx->hv_deadline_tsc is still set.
>
> But the VMX preemption timer is only used for deadline, never for oneshot or
> periodic. Am I missing something?
Yes, it is upstream.
https://lore.kernel.org/kvm/[email protected]/
Wanpeng
On Fri, Jun 04, 2021, Wanpeng Li wrote:
> On Thu, 3 Jun 2021 at 23:20, Sean Christopherson <[email protected]> wrote:
> >
> > On Thu, Jun 03, 2021, Wanpeng Li wrote:
> > > From: Wanpeng Li <[email protected]>
> > >
> > > According to the SDM 10.5.4.1:
> > >
> > > A write of 0 to the initial-count register effectively stops the local
> > > APIC timer, in both one-shot and periodic mode.
> > >
> > > The lapic timer oneshot/periodic mode which is emulated by vmx-preemption
> > > timer doesn't stop since vmx->hv_deadline_tsc is still set.
> >
> > But the VMX preemption timer is only used for deadline, never for oneshot or
> > periodic. Am I missing something?
>
> Yes, it is upstream.
Huh. I always thought 'tscdeadline' alluded to the timer being in deadline mode
and never looked closely at the arming code. Thanks!
Maybe name the new helper cancel_apic_timer() to align with start_apic_timer()
and restart_apic_timer()? With that:
Reviewed-by: Sean Christopherson <[email protected]>
On Thu, Jun 3, 2021 at 5:33 PM Wanpeng Li <[email protected]> wrote:
>
> On Fri, 4 Jun 2021 at 07:02, David Matlack <[email protected]> wrote:
> >
> > On Thu, Jun 3, 2021 at 2:04 AM Wanpeng Li <[email protected]> wrote:
> > >
> > > From: Wanpeng Li <[email protected]>
> > >
> > > According to the SDM 10.5.4.1:
> > >
> > > A write of 0 to the initial-count register effectively stops the local
> > > APIC timer, in both one-shot and periodic mode.
> >
> > If KVM is not correctly emulating this behavior then could you also
> > add a kvm-unit-test to test for the correct behavior?
>
> A simple test here, the test will hang after the patch since it will
> not receive the spurious interrupt any more.
Thanks. Can you send this as a [PATCH]? I think it would be worthwhile
so have a regression test for this bug.
>
> diff --git a/x86/apic.c b/x86/apic.c
> index a7681fe..947d018 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -488,6 +488,14 @@ static void test_apic_timer_one_shot(void)
> */
> report((lvtt_counter == 1) && (tsc2 - tsc1 >= interval),
> "APIC LVT timer one shot");
> +
> + lvtt_counter = 0;
> + apic_write(APIC_TMICT, interval);
> + apic_write(APIC_TMICT, 0);
> + while (!lvtt_counter);
> +
> + report((lvtt_counter == 1),
> + "APIC LVT timer one shot spurious interrupt");
> }
On Fri, 4 Jun 2021 at 23:37, Sean Christopherson <[email protected]> wrote:
>
> On Fri, Jun 04, 2021, Wanpeng Li wrote:
> > On Thu, 3 Jun 2021 at 23:20, Sean Christopherson <[email protected]> wrote:
> > >
> > > On Thu, Jun 03, 2021, Wanpeng Li wrote:
> > > > From: Wanpeng Li <[email protected]>
> > > >
> > > > According to the SDM 10.5.4.1:
> > > >
> > > > A write of 0 to the initial-count register effectively stops the local
> > > > APIC timer, in both one-shot and periodic mode.
> > > >
> > > > The lapic timer oneshot/periodic mode which is emulated by vmx-preemption
> > > > timer doesn't stop since vmx->hv_deadline_tsc is still set.
> > >
> > > But the VMX preemption timer is only used for deadline, never for oneshot or
> > > periodic. Am I missing something?
> >
> > Yes, it is upstream.
>
> Huh. I always thought 'tscdeadline' alluded to the timer being in deadline mode
> and never looked closely at the arming code. Thanks!
>
> Maybe name the new helper cancel_apic_timer() to align with start_apic_timer()
> and restart_apic_timer()? With that:
>
> Reviewed-by: Sean Christopherson <[email protected]>
Do it in v2, thanks.
Wanpeng
On Thu, 3 Jun 2021 at 23:34, Sean Christopherson <[email protected]> wrote:
>
> On Thu, Jun 03, 2021, Jim Mattson wrote:
> > On Thu, Jun 3, 2021 at 2:01 AM Wanpeng Li <[email protected]> wrote:
> > >
> > > From: Wanpeng Li <[email protected]>
> > >
> > > The value of current counter register after reset is 0 for both Intel
> > > and AMD, let's do it in kvm.
> > >
> > > Signed-off-by: Wanpeng Li <[email protected]>
> >
> > How did we miss that?
>
> I suspect it's not actually a functional issue, and that writing '0' at reset is
> a glorified nop. The TMCCT is always computed on-demand and never directly
> readable.
Update the patch description in v2, thanks.
Wanpeng
>
> Is there an observable bug being fixed? If not, the changelog should state that
> this is a cosmetic change of sorts.
>
> static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset)
> {
> u32 val = 0;
>
> if (offset >= LAPIC_MMIO_LENGTH)
> return 0;
>
> switch (offset) {
> case APIC_ARBPRI:
> break;
>
> case APIC_TMCCT: /* Timer CCR */
> if (apic_lvtt_tscdeadline(apic))
> return 0;
>
> val = apic_get_tmcct(apic);
> break;
> ...
> }
>
>
> static u32 apic_get_tmcct(struct kvm_lapic *apic)
> {
> ktime_t remaining, now;
> s64 ns;
> u32 tmcct;
>
> ASSERT(apic != NULL);
>
> /* if initial count is 0, current count should also be 0 */
> if (kvm_lapic_get_reg(apic, APIC_TMICT) == 0 || <------------
> apic->lapic_timer.period == 0)
> return 0;
>
> now = ktime_get();
> remaining = ktime_sub(apic->lapic_timer.target_expiration, now);
> if (ktime_to_ns(remaining) < 0)
> remaining = 0;
>
> ns = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period);
> tmcct = div64_u64(ns,
> (APIC_BUS_CYCLE_NS * apic->divide_count));
>
> return tmcct;
> }
>
> int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
> {
> memcpy(s->regs, vcpu->arch.apic->regs, sizeof(*s));
>
> /*
> * Get calculated timer current count for remaining timer period (if
> * any) and store it in the returned register set.
> */
> __kvm_lapic_set_reg(s->regs, APIC_TMCCT,
> __apic_read(vcpu->arch.apic, APIC_TMCCT)); <----
>
> return kvm_apic_state_fixup(vcpu, s, false);
> }
>
>
>