When running kvm-unit-tests[1] on current APIC[2], we get a failed test
case related to APIC timer

	env QEMU=build/qemu-system-x86_64 ACCEL=tcg ./run_tests.sh -g apic
	FAIL: TMCCT should stay at zero

The test case sets the timer mode to oneshot and sets the intial count, it
waits until the current count reaches 0 then change the mode to periodic.
It is expected that the timer does not start again, the current count must
stay at 0. However, in the current implementation, the write to lvt timer
entry to change to periodic mode triggers the new periodic timer.

This commit adds an additional check when the write to lvt timer entry to
change from oneshot to periodic mode happens. This check verifies if the
current count reaches 0 in oneshot mode already, then it does not start a
new timer and sets timer_stop bool flag to true. The
apic_get_current_count uses this bool flag to report the correct current
count.

[1]: This patch is applied to kvm-unit-tests before running test

	diff --git a/lib/x86/fwcfg.c b/lib/x86/fwcfg.c
	index 1734afb..f56fe1c 100644
	--- a/lib/x86/fwcfg.c
	+++ b/lib/x86/fwcfg.c
	@@ -27,6 +27,7 @@ static void read_cfg_override(void)

		if ((str = getenv("TEST_DEVICE")))
			no_test_device = !atol(str);
	+       no_test_device = true;

		if ((str = getenv("MEMLIMIT")))
			fw_override[FW_CFG_MAX_RAM] = atol(str) * 1024 * 1024;

[2]: We need to disable the APIC disable before running kvm-unit-tests
because the test try to disable and re-enable APIC which does not follow
the xAPIC disable rule in Intel SDM Section 11.4.3. Enabling or Disabling
the local APIC

"When IA32_APIC_BASE[11] is set to 0, processor APICs based on the 3-wire
APIC bus cannot be generally re-enabled until a system hardware reset"

So the current implementation does not support disable and re-enable local
APIC in xAPIC.

We need to apply this patch to run the test

	diff --git a/hw/intc/apic.c b/hw/intc/apic.c
	index ec0a20da59..5c4e0ee3bd 100644
	--- a/hw/intc/apic.c
	+++ b/hw/intc/apic.c
	@@ -293,11 +293,13 @@ static void apic_set_base(APICCommonState *s, uint64_t val)
	     s->apicbase = (val & 0xfffff000) |
		 (s->apicbase & (MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE));
	     /* if disabled, cannot be enabled again */
	+    /*
	     if (!(val & MSR_IA32_APICBASE_ENABLE)) {
		 s->apicbase &= ~MSR_IA32_APICBASE_ENABLE;
		 cpu_clear_apic_feature(&s->cpu->env);
		 s->spurious_vec &= ~APIC_SV_ENABLE;
	     }
	+    */
	 }

Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 hw/i386/kvm/apic.c              |  2 +-
 hw/intc/apic.c                  | 37 ++++++++++++++++++++++-------
 hw/intc/apic_common.c           | 41 ++++++++++++++++++++++++++++++++-
 include/hw/i386/apic_internal.h |  5 +++-
 4 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 1e89ca0899..12273ff991 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -92,7 +92,7 @@ void kvm_get_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic)
     s->count_shift = (v + 1) & 7;
 
     s->initial_count_load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    apic_next_timer(s, s->initial_count_load_time);
+    apic_next_timer(s, s->initial_count_load_time, false);
 }
 
 static void kvm_apic_set_base(APICCommonState *s, uint64_t val)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index ac3d47d231..ec0a20da59 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -619,9 +619,10 @@ int apic_accept_pic_intr(DeviceState *dev)
     return 0;
 }
 
-static void apic_timer_update(APICCommonState *s, int64_t current_time)
+static void apic_timer_update(APICCommonState *s, int64_t current_time,
+                              bool switch_to_periodic)
 {
-    if (apic_next_timer(s, current_time)) {
+    if (apic_next_timer(s, current_time, switch_to_periodic)) {
         timer_mod(s->timer, s->next_time);
     } else {
         timer_del(s->timer);
@@ -633,7 +634,7 @@ static void apic_timer(void *opaque)
     APICCommonState *s = opaque;
 
     apic_local_deliver(s, APIC_LVT_TIMER);
-    apic_timer_update(s, s->next_time);
+    apic_timer_update(s, s->next_time, false);
 }
 
 static uint64_t apic_mem_read(void *opaque, hwaddr addr, unsigned size)
@@ -814,18 +815,38 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
     case 0x32 ... 0x37:
         {
             int n = index - 0x32;
-            s->lvt[n] = val;
             if (n == APIC_LVT_TIMER) {
-                apic_timer_update(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
-            } else if (n == APIC_LVT_LINT0 && apic_check_pic(s)) {
-                apic_update_irq(s);
+                uint32_t old_val = s->lvt[n];
+
+                /* Check if we switch from one-shot to periodic mode */
+                if (!(old_val & APIC_LVT_TIMER_PERIODIC) &&
+                    (val & APIC_LVT_TIMER_PERIODIC)) {
+                    apic_timer_update(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                                      true);
+                } else {
+                    apic_timer_update(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                                      false);
+                }
+                /*
+                 * Set the lvt timer entry after we handle the switch mode
+                 * so that a concurrent apic_timer_update triggered via
+                 * apic_timer does not see the new value before we handle
+                 * the switch properly.
+                 */
+                s->lvt[n] = val;
+            } else {
+                s->lvt[n] = val;
+                if (n == APIC_LVT_LINT0 && apic_check_pic(s)) {
+                    apic_update_irq(s);
+                }
             }
         }
         break;
     case 0x38:
         s->initial_count = val;
         s->initial_count_load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        apic_timer_update(s, s->initial_count_load_time);
+        s->timer_stop = false;
+        apic_timer_update(s, s->initial_count_load_time, false);
         break;
     case 0x39:
         break;
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 4a34f03047..d21fb37a5b 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -130,7 +130,8 @@ void apic_deliver_nmi(DeviceState *dev)
     info->external_nmi(s);
 }
 
-bool apic_next_timer(APICCommonState *s, int64_t current_time)
+bool apic_next_timer(APICCommonState *s, int64_t current_time,
+                     bool switch_to_periodic)
 {
     int64_t d;
 
@@ -146,10 +147,24 @@ bool apic_next_timer(APICCommonState *s, int64_t current_time)
 
     d = (current_time - s->initial_count_load_time) >> s->count_shift;
 
+    /*
+     * The switch_to_periodic is true only when we write to the
+     * timer lvt entry to change timer mode from one-shot mode
+     * to periodic mode. In that case,  we need to check if the
+     * timer current count reaches 0. If so, don't start the new
+     * timer, only start the time when there is a write to
+     * initial count.
+     */
+    if (switch_to_periodic && d >= s->initial_count) {
+        s->timer_stop = true;
+        return false;
+    }
+
     if (s->lvt[APIC_LVT_TIMER] & APIC_LVT_TIMER_PERIODIC) {
         if (!s->initial_count) {
             return false;
         }
+
         d = ((d / ((uint64_t)s->initial_count + 1)) + 1) *
             ((uint64_t)s->initial_count + 1);
     } else {
@@ -167,6 +182,11 @@ uint32_t apic_get_current_count(APICCommonState *s)
 {
     int64_t d;
     uint32_t val;
+
+    if (s->timer_stop) {
+        return 0;
+    }
+
     d = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - s->initial_count_load_time) >>
         s->count_shift;
     if (s->lvt[APIC_LVT_TIMER] & APIC_LVT_TIMER_PERIODIC) {
@@ -282,6 +302,7 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     if (s->legacy_instance_id) {
         instance_id = VMSTATE_INSTANCE_ID_ANY;
     }
+    s->timer_stop = false;
     vmstate_register_with_alias_id(NULL, instance_id, &vmstate_apic_common,
                                    s, -1, 0, NULL);
 }
@@ -309,6 +330,7 @@ static int apic_pre_load(void *opaque)
      * absent.
      */
     s->wait_for_sipi = 0;
+    s->timer_stop = false;
     return 0;
 }
 
@@ -353,6 +375,22 @@ static const VMStateDescription vmstate_apic_common_sipi = {
     }
 };
 
+static bool apic_common_timer_stop_needed(void *opaque)
+{
+    APICCommonState *s = APIC_COMMON(opaque);
+    return s->timer_stop != 0;
+}
+
+static const VMStateDescription vmstate_apic_common_timer_stop = {
+    .name = "apic_timer_stop",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = apic_common_timer_stop_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(timer_stop, APICCommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
 static const VMStateDescription vmstate_apic_common = {
     .name = "apic",
     .version_id = 3,
@@ -385,6 +423,7 @@ static const VMStateDescription vmstate_apic_common = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmstate_apic_common_sipi,
+        &vmstate_apic_common_timer_stop,
         NULL
     }
 };
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 5f2ba24bfc..4d0a39cc72 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -187,6 +187,8 @@ struct APICCommonState {
     DeviceState *vapic;
     hwaddr vapic_paddr; /* note: persistence via kvmvapic */
     bool legacy_instance_id;
+
+    bool timer_stop;
 };
 
 typedef struct VAPICState {
@@ -199,7 +201,8 @@ typedef struct VAPICState {
 
 extern bool apic_report_tpr_access;
 
-bool apic_next_timer(APICCommonState *s, int64_t current_time);
+bool apic_next_timer(APICCommonState *s, int64_t current_time,
+                     bool switch_to_periodic);
 void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
 void apic_enable_vapic(DeviceState *d, hwaddr paddr);
 
-- 
2.25.1