[PATCH v3 0/3] Rework vlapic timer to behave more like real-hardware

[PATCH v3 0/3] Rework vlapic timer to behave more like real-hardware

[Anthony PERARD]

Hi,

When developing PVH for OVMF, I've used the lapic timer. It turns out that the
way it is used by OVMF did not work with Xen [1]. I tried to find out how
real-hw behave, and write a XTF tests [2]. And this patch series tries to fix
the behavior of the vlapic timer.

The OVMF driver for the APIC timer initialize the timer like this:
  write to TMICT (initial counter)
  write to TMDCR (divide configuration)
  enable the timer (this may change timer mode from one-shot to periodic)
It turns out that TMICT is set to 0 on the last step, but OVMF expect the timer
to run.

Here is some description of the APIC timer, base on observation as well as read
of the Intel SDM. The description is also patch of patch description
(reworded).

Maybe a way of thinking how the APIC timer is evaluated, is to think of how
hardward will do it. There is a counter TMCCT which always keeps counting down.

Setting TMICT also set TMCCT, nothing else matter.
Setting LVTT does not change anything right away.
Setting TMDCR does not change much.

Now TMCCT keeps counting down, by a value related to TMDCR.
Once, TMCCT reach 0, it is only at this time that LVTT is taken into account.
Is there an interrupt to deliver? Should the timer restart counting from the
value in TMICT?

In the Intel SDM, there is the word "disarm" of the timer used. I guess the
easier way to disarm the APIC timer (when in periodic or one-shot) is to set
TMICT to 0. But if we take TSC-Deadline mode out of the picture, there is
nothing in the manual that say that the timer is disarm or stopped when
changing timer mode (there is only two modes left, period and one-shot).

As for the TSC-deadline timer mode, observation shown that changing to it (or
from it) does reset and disarm both timers, so effectively TMICT and the
tscdeadline are set to 0.

There is a XTF patch series that check the emulation of the vlapic timer.
"[XTF PATCH V2 0/3] Testing vlapic timer"

This patch series can be found at:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git
tag: vlapic-timer-v3

Changes in V3:
- details in patches.

Changes in V2:
- patches have been reworked.
- vlapic_update_timer does not care anymore which register is been changed.
- more comments, hopefully also better.

Thanks,

[1] https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg00959.html
[2] v1: https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg02533.html
    v2: look for "[XTF PATCH V2 0/3] Testing vlapic timer"

Anthony PERARD (3):
  x86/vlapic: Introduce vlapic_update_timer
  x86/vlapic: Keep timer running when switching between one-shot and
    periodic mode
  x86/vlapic: Apply change to TDCR right away to the timer

 xen/arch/x86/hvm/vlapic.c | 126 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 94 insertions(+), 32 deletions(-)

-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 1/3] x86/vlapic: Introduce vlapic_update_timer

[Anthony PERARD]

There should not be any functionality change with this patch.

This function is used when the APIC_TMICT register is updated.

vlapic_update_timer is introduce as it will be use also when the
registers APIC_LVTT and APIC_TDCR are updated.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes in V3:
- removed variable delta, which had the same value as period.
---
 xen/arch/x86/hvm/vlapic.c | 72 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 4320c6e30a..587ef8defe 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -668,6 +668,50 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
     vcpu_vlapic(v)->hw.tdt_msr = 0;
 }
 
+/*
+ * This function is used when a register related to the APIC timer is updated.
+ * It expects the new value for the register TMICT to be set *before*
+ * being called.
+ * It expect the new value of LVTT to be set *after* being called, with this
+ * new values passed as parameter (only APIC_TIMER_MODE_MASK bits matter).
+ */
+static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt)
+{
+    uint64_t period;
+    bool is_periodic;
+
+    is_periodic = (lvtt & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_PERIODIC;
+
+    period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
+        * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
+
+    if ( period )
+    {
+        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(period),
+                        TRC_PAR_LONG(is_periodic ? period : 0),
+                        vlapic->pt.irq);
+
+        create_periodic_time(current, &vlapic->pt, period,
+                             is_periodic ? period : 0, vlapic->pt.irq,
+                             is_periodic ? vlapic_pt_cb : NULL,
+                             &vlapic->timer_last_update);
+
+        vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
+
+        HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
+                    "bus cycle is %uns, "
+                    "initial count %u, period %"PRIu64"ns",
+                    APIC_BUS_CYCLE_NS,
+                    vlapic_get_reg(vlapic, APIC_TMICT),
+                    period);
+    }
+    else
+    {
+        TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
+        destroy_periodic_time(&vlapic->pt);
+    }
+}
+
 static void vlapic_reg_write(struct vcpu *v,
                              unsigned int offset, uint32_t val)
 {
@@ -764,37 +808,13 @@ static void vlapic_reg_write(struct vcpu *v,
         break;
 
     case APIC_TMICT:
-    {
-        uint64_t period;
-
         if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
             break;
 
         vlapic_set_reg(vlapic, APIC_TMICT, val);
-        if ( val == 0 )
-        {
-            TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
-            destroy_periodic_time(&vlapic->pt);
-            break;
-        }
 
-        period = (uint64_t)APIC_BUS_CYCLE_NS * val * vlapic->hw.timer_divisor;
-        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(period),
-                 TRC_PAR_LONG(vlapic_lvtt_period(vlapic) ? period : 0LL),
-                 vlapic->pt.irq);
-        create_periodic_time(current, &vlapic->pt, period, 
-                             vlapic_lvtt_period(vlapic) ? period : 0,
-                             vlapic->pt.irq,
-                             vlapic_lvtt_period(vlapic) ? vlapic_pt_cb : NULL,
-                             &vlapic->timer_last_update);
-        vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
-
-        HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
-                    "bus cycle is %uns, "
-                    "initial count %u, period %"PRIu64"ns",
-                    APIC_BUS_CYCLE_NS, val, period);
-    }
-    break;
+        vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT));
+        break;
 
     case APIC_TDCR:
         vlapic_set_tdcr(vlapic, val & 0xb);
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 2/3] x86/vlapic: Keep timer running when switching between one-shot and periodic mode

[Anthony PERARD]

If we take TSC-deadline mode timer out of the picture, the Intel SDM
does not say that the timer is disable when the timer mode is change,
either from one-shot to periodic or vice versa.

After this patch, the timer is no longer disarmed on change of mode, so
the counter (TMCCT) keeps counting down.

So what does a write to LVTT changes ? On baremetal, the change of mode
is probably taken into account only when the counter reach 0. When this
happen, LVTT is use to figure out if the counter should restard counting
down from TMICT (so periodic mode) or stop counting (if one-shot mode).

This also mean that if the counter reach 0 and the mode is one-shot, a
change to periodic would not restart the timer. This is achieve by
setting vlapic->timer_last_update=0.

This patch is based on observation of the behavior of the APIC timer on
baremetal as well as check that they does not go against the description
written in the Intel SDM.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes in V3:
- new argument for vlapic_update_timer: tmict_updated.
  To avoid setting timer_last_update twice when TMICT is been updated.
- use values of period and delta to calculate timer_last_update only
  when register other than TMICT are been updated
---
 xen/arch/x86/hvm/vlapic.c | 52 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 587ef8defe..7a5fbb40cd 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -520,7 +520,8 @@ static uint32_t vlapic_get_tmcct(struct vlapic *vlapic)
     counter_passed = ((hvm_get_guest_time(v) - vlapic->timer_last_update)
                       / (APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor));
 
-    if ( tmict != 0 )
+    /* If timer_last_update is 0, then TMCCT should return 0 as well.  */
+    if ( tmict && vlapic->timer_last_update )
     {
         if ( vlapic_lvtt_period(vlapic) )
             counter_passed %= tmict;
@@ -675,28 +676,47 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
  * It expect the new value of LVTT to be set *after* being called, with this
  * new values passed as parameter (only APIC_TIMER_MODE_MASK bits matter).
  */
-static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt)
+static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt,
+                                bool tmict_updated)
 {
-    uint64_t period;
-    bool is_periodic;
+    uint64_t period, delta = 0;
+    bool is_oneshot, is_periodic;
 
     is_periodic = (lvtt & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_PERIODIC;
+    is_oneshot = (lvtt & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_ONESHOT;
 
     period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
         * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
 
-    if ( period )
+    /* Calculate the next time the timer should trigger an interrupt. */
+    if ( tmict_updated )
+        delta = period;
+    else if ( period && vlapic->timer_last_update )
     {
-        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(period),
+        uint64_t time_passed = hvm_get_guest_time(current)
+            - vlapic->timer_last_update;
+
+        /* This depends of the previous mode, if a new mode is being set */
+        if ( vlapic_lvtt_period(vlapic) )
+            time_passed %= period;
+        if ( time_passed < period )
+            delta = period - time_passed;
+    }
+
+    if ( delta && (is_oneshot || is_periodic) )
+    {
+        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
                         TRC_PAR_LONG(is_periodic ? period : 0),
                         vlapic->pt.irq);
 
-        create_periodic_time(current, &vlapic->pt, period,
+        create_periodic_time(current, &vlapic->pt, delta,
                              is_periodic ? period : 0, vlapic->pt.irq,
                              is_periodic ? vlapic_pt_cb : NULL,
                              &vlapic->timer_last_update);
 
         vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
+        if ( !tmict_updated )
+            vlapic->timer_last_update -= period - delta;
 
         HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
                     "bus cycle is %uns, "
@@ -709,6 +729,12 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt)
     {
         TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
         destroy_periodic_time(&vlapic->pt);
+        /*
+         * From now, TMCCT should return 0 until TMICT is set again.
+         * This is because the timer mode was one-shot when the counter reach 0
+         * or just because the timer is disable.
+         */
+        vlapic->timer_last_update = 0;
     }
 }
 
@@ -776,16 +802,16 @@ static void vlapic_reg_write(struct vcpu *v,
         break;
 
     case APIC_LVTT:         /* LVT Timer Reg */
-        if ( (vlapic_get_reg(vlapic, offset) & APIC_TIMER_MODE_MASK) !=
-             (val & APIC_TIMER_MODE_MASK) )
+        if ( vlapic_lvtt_tdt(vlapic) !=
+             ((val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_TSC_DEADLINE))
         {
-            TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
-            destroy_periodic_time(&vlapic->pt);
             vlapic_set_reg(vlapic, APIC_TMICT, 0);
-            vlapic_set_reg(vlapic, APIC_TMCCT, 0);
             vlapic->hw.tdt_msr = 0;
         }
         vlapic->pt.irq = val & APIC_VECTOR_MASK;
+
+        vlapic_update_timer(vlapic, val, false);
+
         /* fallthrough */
     case APIC_LVTTHMR:      /* LVT Thermal Monitor */
     case APIC_LVTPC:        /* LVT Performance Counter */
@@ -813,7 +839,7 @@ static void vlapic_reg_write(struct vcpu *v,
 
         vlapic_set_reg(vlapic, APIC_TMICT, val);
 
-        vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT));
+        vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT), true);
         break;
 
     case APIC_TDCR:
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 3/3] x86/vlapic: Apply change to TDCR right away to the timer

[Anthony PERARD]

The description in the Intel SDM of how the divide configuration
register is used: "The APIC timer frequency will be the processor's bus
clock or core crystal clock frequency divided by the value specified in
the divide configuration register."

Observation of baremetal shown that when the TDCR is change, the TMCCT
does not change or make a big jump in value, but the rate at which it
count down change.

The patch update the emulation to APIC timer to so that a change to the
divide configuration would be reflected in the value of the counter and
when the next interrupt is triggered.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes in V3:
- do the calculation when the divisor is change only if delta is !0.
---
 xen/arch/x86/hvm/vlapic.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 7a5fbb40cd..4bfc53eab7 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -672,12 +672,13 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
 /*
  * This function is used when a register related to the APIC timer is updated.
  * It expects the new value for the register TMICT to be set *before*
- * being called.
+ * being called, and the previous value of the divisor (calculated from TDCR)
+ * to be passed as argument.
  * It expect the new value of LVTT to be set *after* being called, with this
  * new values passed as parameter (only APIC_TIMER_MODE_MASK bits matter).
  */
 static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt,
-                                bool tmict_updated)
+                                bool tmict_updated, uint32_t old_divisor)
 {
     uint64_t period, delta = 0;
     bool is_oneshot, is_periodic;
@@ -686,7 +687,7 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt,
     is_oneshot = (lvtt & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_ONESHOT;
 
     period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
-        * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
+        * APIC_BUS_CYCLE_NS * old_divisor;
 
     /* Calculate the next time the timer should trigger an interrupt. */
     if ( tmict_updated )
@@ -705,6 +706,13 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt,
 
     if ( delta && (is_oneshot || is_periodic) )
     {
+        if ( vlapic->hw.timer_divisor != old_divisor )
+        {
+            period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
+                * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
+            delta = delta * vlapic->hw.timer_divisor / old_divisor;
+        }
+
         TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
                         TRC_PAR_LONG(is_periodic ? period : 0),
                         vlapic->pt.irq);
@@ -810,7 +818,7 @@ static void vlapic_reg_write(struct vcpu *v,
         }
         vlapic->pt.irq = val & APIC_VECTOR_MASK;
 
-        vlapic_update_timer(vlapic, val, false);
+        vlapic_update_timer(vlapic, val, false, vlapic->hw.timer_divisor);
 
         /* fallthrough */
     case APIC_LVTTHMR:      /* LVT Thermal Monitor */
@@ -839,15 +847,23 @@ static void vlapic_reg_write(struct vcpu *v,
 
         vlapic_set_reg(vlapic, APIC_TMICT, val);
 
-        vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT), true);
+        vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT), true,
+                            vlapic->hw.timer_divisor);
         break;
 
     case APIC_TDCR:
+    {
+        uint32_t current_divisor = vlapic->hw.timer_divisor;
+
         vlapic_set_tdcr(vlapic, val & 0xb);
+
+        vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT), false,
+                            current_divisor);
         HVM_DBG_LOG(DBG_LEVEL_VLAPIC_TIMER, "timer divisor is %#x",
                     vlapic->hw.timer_divisor);
         break;
     }
+    }
 }
 
 static int vlapic_write(struct vcpu *v, unsigned long address,
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 0/3] Rework vlapic timer to behave more like real-hardware

[Jan Beulich]

>>> On 08.08.17 at 15:32, <anthony.perard@citrix.com> wrote:
> Anthony PERARD (3):
>   x86/vlapic: Introduce vlapic_update_timer
>   x86/vlapic: Keep timer running when switching between one-shot and
>     periodic mode
>   x86/vlapic: Apply change to TDCR right away to the timer
> 
>  xen/arch/x86/hvm/vlapic.c | 126 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 94 insertions(+), 32 deletions(-)

Reviewed-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel