[PATCH 1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer

[PATCH 1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>
 
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 <wanpengli@tencent.com>
---
 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

[PATCH 2/2] KVM: LAPIC: reset TMCCT during vCPU reset

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

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 <wanpengli@tencent.com>
---
 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

Re: [PATCH 2/2] KVM: LAPIC: reset TMCCT during vCPU reset

[Jim Mattson]

On Thu, Jun 3, 2021 at 2:01 AM Wanpeng Li <kernellwp@gmail.com> wrote:
>
> From: Wanpeng Li <wanpengli@tencent.com>
>
> 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 <wanpengli@tencent.com>

How did we miss that?

Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH 2/2] KVM: LAPIC: reset TMCCT during vCPU reset

[Sean Christopherson]

On Thu, Jun 03, 2021, Jim Mattson wrote:
> On Thu, Jun 3, 2021 at 2:01 AM Wanpeng Li <kernellwp@gmail.com> wrote:
> >
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > 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 <wanpengli@tencent.com>
> 
> 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);
}

Re: [PATCH 2/2] KVM: LAPIC: reset TMCCT during vCPU reset

[Wanpeng Li]

On Thu, 3 Jun 2021 at 23:34, Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Jun 03, 2021, Jim Mattson wrote:
> > On Thu, Jun 3, 2021 at 2:01 AM Wanpeng Li <kernellwp@gmail.com> wrote:
> > >
> > > From: Wanpeng Li <wanpengli@tencent.com>
> > >
> > > 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 <wanpengli@tencent.com>
> >
> > 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);
> }
>
>
>

Re: [PATCH 1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer

[Sean Christopherson]

On Thu, Jun 03, 2021, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
>  
> 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;

	...
}

Re: [PATCH 1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer

[Wanpeng Li]

On Thu, 3 Jun 2021 at 23:20, Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Jun 03, 2021, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > 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/1477304593-3453-1-git-send-email-wanpeng.li@hotmail.com/

    Wanpeng

Re: [PATCH 1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer

[Sean Christopherson]

On Fri, Jun 04, 2021, Wanpeng Li wrote:
> On Thu, 3 Jun 2021 at 23:20, Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Jun 03, 2021, Wanpeng Li wrote:
> > > From: Wanpeng Li <wanpengli@tencent.com>
> > >
> > > 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 <seanjc@google.com>

Re: [PATCH 1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer

[Wanpeng Li]

On Fri, 4 Jun 2021 at 23:37, Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Jun 04, 2021, Wanpeng Li wrote:
> > On Thu, 3 Jun 2021 at 23:20, Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Thu, Jun 03, 2021, Wanpeng Li wrote:
> > > > From: Wanpeng Li <wanpengli@tencent.com>
> > > >
> > > > 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 <seanjc@google.com>

Do it in v2, thanks.

    Wanpeng

Re: [PATCH 1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer

[David Matlack]

On Thu, Jun 3, 2021 at 2:04 AM Wanpeng Li <kernellwp@gmail.com> wrote:
>
> From: Wanpeng Li <wanpengli@tencent.com>
>
> 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 <wanpengli@tencent.com>
> ---
>  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
>

Re: [PATCH 1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer

[Wanpeng Li]

On Fri, 4 Jun 2021 at 07:02, David Matlack <dmatlack@google.com> wrote:
>
> On Thu, Jun 3, 2021 at 2:04 AM Wanpeng Li <kernellwp@gmail.com> wrote:
> >
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > 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");
 }

Re: [PATCH 1/2] KVM: LAPIC: write 0 to TMICT should also cancel vmx-preemption timer

[David Matlack]

On Thu, Jun 3, 2021 at 5:33 PM Wanpeng Li <kernellwp@gmail.com> wrote:
>
> On Fri, 4 Jun 2021 at 07:02, David Matlack <dmatlack@google.com> wrote:
> >
> > On Thu, Jun 3, 2021 at 2:04 AM Wanpeng Li <kernellwp@gmail.com> wrote:
> > >
> > > From: Wanpeng Li <wanpengli@tencent.com>
> > >
> > > 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");
>  }