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

[PATCH v2 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 mode 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-v2

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 | 131 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 99 insertions(+), 32 deletions(-)

-- 
Anthony PERARD


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

[PATCH v2 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>
---
 xen/arch/x86/hvm/vlapic.c | 79 +++++++++++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 4320c6e30a..883114c824 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -668,6 +668,57 @@ 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 expect the new value for the register TMICT to be set *before*
+ * been called.
+ * It expect the new value of LVTT to be set *after* been 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;
+    uint64_t delta;
+    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;
+
+    /* Calculate the next time the timer should trigger an interrupt. */
+    delta = period;
+
+    if ( delta )
+    {
+        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
+                        TRC_PAR_LONG(is_periodic ? period : 0LL),
+                        vlapic->pt.irq);
+
+        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;
+
+        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 +815,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 v2 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 ihe Intel SDM.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 xen/arch/x86/hvm/vlapic.c | 45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 883114c824..71a6010ee4 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;
@@ -678,18 +679,29 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
 static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
 {
     uint64_t period;
-    uint64_t delta;
-    bool is_periodic;
+    uint64_t 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;
 
     /* Calculate the next time the timer should trigger an interrupt. */
-    delta = period;
+    if ( period && vlapic->timer_last_update )
+    {
+        uint64_t time_passed = hvm_get_guest_time(current)
+            - vlapic->timer_last_update;
+
+        /* This depends of the previous mode, if a new mode is set */
+        if ( vlapic_lvtt_period(vlapic) )
+            time_passed %= period;
+        if ( time_passed < period )
+            delta = period - time_passed;
+    }
 
-    if ( delta )
+    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 : 0LL),
@@ -702,7 +714,11 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
                              is_periodic ? vlapic_pt_cb : NULL,
                              &vlapic->timer_last_update);
 
-        vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
+        /* For the case where the timer was periodic and it is now
+         * one-shot, timer_last_update should be the value of the last time
+         * the interrupt was triggered.
+         */
+        vlapic->timer_last_update = vlapic->pt.last_plt_gtime + delta - period;
 
         HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
                     "bus cycle is %uns, "
@@ -715,6 +731,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;
     }
 }
 
@@ -783,16 +805,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);
+
         /* fallthrough */
     case APIC_LVTTHMR:      /* LVT Thermal Monitor */
     case APIC_LVTPC:        /* LVT Performance Counter */
@@ -818,6 +840,7 @@ static void vlapic_reg_write(struct vcpu *v,
         if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
             break;
 
+        vlapic->timer_last_update = hvm_get_guest_time(current);
         vlapic_set_reg(vlapic, APIC_TMICT, val);
 
         vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT));
-- 
Anthony PERARD


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

[PATCH v2 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>
---
 xen/arch/x86/hvm/vlapic.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 71a6010ee4..62d171061f 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -671,12 +671,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 expect the new value for the register TMICT to be set *before*
- * been called.
+ * It expect the new value for the register TMICT and TDCR to be set *before*
+ * been called, and the previous value of TDCR to be passed as parametter.
  * It expect the new value of LVTT to be set *after* been 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,
+                                uint32_t old_divisor)
 {
     uint64_t period;
     uint64_t delta = 0;
@@ -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 ( period && vlapic->timer_last_update )
@@ -701,6 +702,13 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
             delta = period - time_passed;
     }
 
+    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;
+    }
+
     if ( delta && (is_oneshot || is_periodic) )
     {
         TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
@@ -813,7 +821,7 @@ static void vlapic_reg_write(struct vcpu *v,
         }
         vlapic->pt.irq = val & APIC_VECTOR_MASK;
 
-        vlapic_update_timer(vlapic, val);
+        vlapic_update_timer(vlapic, val, vlapic->hw.timer_divisor);
 
         /* fallthrough */
     case APIC_LVTTHMR:      /* LVT Thermal Monitor */
@@ -843,15 +851,24 @@ static void vlapic_reg_write(struct vcpu *v,
         vlapic->timer_last_update = hvm_get_guest_time(current);
         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),
+                            vlapic->hw.timer_divisor);
         break;
 
     case APIC_TDCR:
+    {
+        uint32_t current_divisor;
+
+        current_divisor = vlapic->hw.timer_divisor;
         vlapic_set_tdcr(vlapic, val & 0xb);
+
+        vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT),
+                            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 v2 1/3] x86/vlapic: Introduce vlapic_update_timer

[Jan Beulich]

>>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:10 PM >>>
>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>
>---

Missing brief revision log here.

>--- a/xen/arch/x86/hvm/vlapic.c
>+++ b/xen/arch/x86/hvm/vlapic.c
>@@ -668,6 +668,57 @@ 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 expect the new value for the register TMICT to be set *before*

expects

>+ * been called.

being

>+ * It expect the new value of LVTT to be set *after* been 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;
>+    uint64_t delta;

Why two lines (but see also below)?

>+    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;
>+
>+    /* Calculate the next time the timer should trigger an interrupt. */
>+    delta = period;

What is the point of having the same value in two variables?

>+    if ( delta )
>+    {
>+        TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
>+                        TRC_PAR_LONG(is_periodic ? period : 0LL),

I don't see the need for the LL suffix here.

>+                        vlapic->pt.irq);
>+
>+        create_periodic_time(current, &vlapic->pt,
>+                             delta,
>+                             is_periodic ? period : 0,
>+                             vlapic->pt.irq,
>+                             is_periodic ? vlapic_pt_cb : NULL,
>+                             &vlapic->timer_last_update);

Please be consistent with argument wrapping: Either always place as many on
a line as will fit, or (less desirable imo) always have just one per line.

>+        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);
>+    }
>+}
>+
>+

No double blank lines please.

Jan


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

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

[Jan Beulich]

>>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:12 PM >>>
>@@ -678,18 +679,29 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
>static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
>{
>uint64_t period;
>-    uint64_t delta;
>-    bool is_periodic;
>+    uint64_t 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;
 >
>/* Calculate the next time the timer should trigger an interrupt. */
>-    delta = period;
>+    if ( period && vlapic->timer_last_update )
>+    {
>+        uint64_t time_passed = hvm_get_guest_time(current)
>+            - vlapic->timer_last_update;
>+
>+        /* This depends of the previous mode, if a new mode is set */

It's nice that you add such a comment, but this really documents a
requirement the caller has to fulfill, so I'm afraid it's somewhat misplaced.
Also perhaps "... is being set"?

>+        if ( vlapic_lvtt_period(vlapic) )
>+            time_passed %= period;
>+        if ( time_passed < period )
>+            delta = period - time_passed;

Why is this second step not also dependent on the timer previously
having been periodic? This is particularly relevant in connection with ...

>+    }
 >
>-    if ( delta )
>+    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 : 0LL),
>@@ -702,7 +714,11 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
>is_periodic ? vlapic_pt_cb : NULL,
>&vlapic->timer_last_update);
 >
>-        vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
>+        /* For the case where the timer was periodic and it is now
>+         * one-shot, timer_last_update should be the value of the last time
>+         * the interrupt was triggered.
>+         */
>+        vlapic->timer_last_update = vlapic->pt.last_plt_gtime + delta - period;
 
,,, this. Note how the comment talks about a change from one-shot to
periodic, but how the code does not obviously alter behavior in only that
one case.

Also - comment style.

>@@ -818,6 +840,7 @@ static void vlapic_reg_write(struct vcpu *v,
>if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
>break;
 >
>+        vlapic->timer_last_update = hvm_get_guest_time(current);
>vlapic_set_reg(vlapic, APIC_TMICT, val);
 >
>vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT));

Why is this addition needed? vlapic_update_timer() sets timer_last_update
anyway. As it looks all you want is the value to be non-zero, which can be
done with less overhead and should be stated so in a comment.

Jan


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

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

[Jan Beulich]

>>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:10 PM >>>
>--- a/xen/arch/x86/hvm/vlapic.c
>+++ b/xen/arch/x86/hvm/vlapic.c
>@@ -671,12 +671,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 expect the new value for the register TMICT to be set *before*
>- * been called.
>+ * It expect the new value for the register TMICT and TDCR to be set *before*
>+ * been called, and the previous value of TDCR to be passed as parametter.

argument

>* It expect the new value of LVTT to be set *after* been 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,
>+                                uint32_t old_divisor)

To match up with "lvtt", perhaps "old_tdcr" then? Or wait, having seen
the call site, I think the parameter name is fine, but the comment shouldn't
say TDCR (as its really the derived divisor value which you care about).

>@@ -701,6 +702,13 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
>delta = period - time_passed;
>}
 >
>+    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;
>+    }

Isn't this calculation pointless when delta is zero?

>@@ -843,15 +851,24 @@ static void vlapic_reg_write(struct vcpu *v,
>vlapic->timer_last_update = hvm_get_guest_time(current);
>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),
>+                            vlapic->hw.timer_divisor);
>break;
 >
>case APIC_TDCR:
>+    {
>+        uint32_t current_divisor;
>+
>+        current_divisor = vlapic->hw.timer_divisor;

Please make this the initializer of the variable.

Jan


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

Re: [PATCH v2 1/3] x86/vlapic: Introduce vlapic_update_timer

[Anthony PERARD]

On Thu, Aug 03, 2017 at 08:59:10AM -0600, Jan Beulich wrote:
> >>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:10 PM >>>
> >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>
> >---
> 
> Missing brief revision log here.

It would have been "New patch", since I've rewrite all patches and
reorganize the serie. But the revision log is in the cover letter.

> >+static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
> >+{
> >+    uint64_t period;
> >+    uint64_t delta;
> 
> Why two lines (but see also below)?

Why not? Anyway, I'll change it.

Also I realize that delta is going to be initialize to 0 here in the
next patch, which is why I think there is two lines.

> >+    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;
> >+
> >+    /* Calculate the next time the timer should trigger an interrupt. */
> >+    delta = period;
> 
> What is the point of having the same value in two variables?

It might look like it but there are not the same values, the description
is accurate, even if the calculation at this stage is very simple.

More importantly, this line is going away in the next patch and will be
replaced by a more complexe calculation.


Thanks,

-- 
Anthony PERARD

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

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

[Anthony PERARD]

On Thu, Aug 03, 2017 at 09:21:57AM -0600, Jan Beulich wrote:
> >>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:12 PM >>>
> >@@ -678,18 +679,29 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
> >static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
> >{
> >uint64_t period;
> >-    uint64_t delta;
> >-    bool is_periodic;
> >+    uint64_t 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;
>  >
> >/* Calculate the next time the timer should trigger an interrupt. */
> >-    delta = period;
> >+    if ( period && vlapic->timer_last_update )
> >+    {
> >+        uint64_t time_passed = hvm_get_guest_time(current)
> >+            - vlapic->timer_last_update;
> >+
> >+        /* This depends of the previous mode, if a new mode is set */
> 
> It's nice that you add such a comment, but this really documents a
> requirement the caller has to fulfill, so I'm afraid it's somewhat misplaced.

It's a comment about why I'm using vlapic_lvtt_period() here instead of
the local variable is_periodic. Also, the requirement for the caller is
already documented.

> >+        if ( vlapic_lvtt_period(vlapic) )
> >+            time_passed %= period;
> >+        if ( time_passed < period )
> >+            delta = period - time_passed;
> 
> Why is this second step not also dependent on the timer previously
> having been periodic? This is particularly relevant in connection with ...

This is the same algorithme as the one used to calculate the current
value of the counter register TMCCT. The second steps only depends on
when the initial-counter register TMICT was set and what is value was.
Since the periodicity of the timer is been take care in the first step,
it does not matter any more if the timer was previously periodic or
one-shot.

"period" here is the initial value of the counter and "time_passed"
correspond to how much time have passed since TMCCT have been reset to
the value in TMICT, either because TMICT was set or because the timer
was periodic.

> >+    }
>  >
> >-    if ( delta )
> >+    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 : 0LL),
> >@@ -702,7 +714,11 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
> >is_periodic ? vlapic_pt_cb : NULL,
> >&vlapic->timer_last_update);
>  >
> >-        vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
> >+        /* For the case where the timer was periodic and it is now
> >+         * one-shot, timer_last_update should be the value of the last time
> >+         * the interrupt was triggered.
> >+         */
> >+        vlapic->timer_last_update = vlapic->pt.last_plt_gtime + delta - period;
>  
> ,,, this. Note how the comment talks about a change from one-shot to
> periodic, but how the code does not obviously alter behavior in only that
> one case.

I need to think about it.

When TMICT is changed, delta == period, so the value is not altered.

When the timer mode is changed, delta and period are going to be
different. And the difference is going to be, how much time have passed
since TMICT was set or since the last time a periodic timer reach 0.

So the comment is inappropriate here. It might be usefull in the commit
message, and I may need a better comment on why I'm doing +delta-period.

> >@@ -818,6 +840,7 @@ static void vlapic_reg_write(struct vcpu *v,
> >if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
> >break;
>  >
> >+        vlapic->timer_last_update = hvm_get_guest_time(current);
> >vlapic_set_reg(vlapic, APIC_TMICT, val);
>  >
> >vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT));
> 
> Why is this addition needed? vlapic_update_timer() sets timer_last_update
> anyway. As it looks all you want is the value to be non-zero, which can be
> done with less overhead and should be stated so in a comment.

This is not true, the value is used before been set. It is used to
calculate how much time have passed since tmict was set. Before been set
again, there is this:
time_passed = hvm_get_guest_time(current) - vlapic->timer_last_update;

-- 
Anthony PERARD

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

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

[Anthony PERARD]

On Thu, Aug 03, 2017 at 09:29:02AM -0600, Jan Beulich wrote:
> >>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:10 PM >>>
> >@@ -701,6 +702,13 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
> >delta = period - time_passed;
> >}
>  >
> >+    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;
> >+    }
> 
> Isn't this calculation pointless when delta is zero?

Yeah, I guess it is. I'll move this if block into the next one, which is
if (delta && X).

Thanks,

-- 
Anthony PERARD

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

Re: [PATCH v2 1/3] x86/vlapic: Introduce vlapic_update_timer

[Jan Beulich]

>>> Anthony PERARD <anthony.perard@citrix.com> 08/04/17 12:52 PM >>>
>On Thu, Aug 03, 2017 at 08:59:10AM -0600, Jan Beulich wrote:
>> >>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:10 PM >>>
>> >+static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
>> >+{
>> >+    uint64_t period;
>> >+    uint64_t delta;
>> 
>> Why two lines (but see also below)?
>
>Why not? Anyway, I'll change it.
>
>Also I realize that delta is going to be initialize to 0 here in the
>next patch, which is why I think there is two lines.

For both this and ...

>> >+    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;
>> >+
>> >+    /* Calculate the next time the timer should trigger an interrupt. */
>> >+    delta = period;
>> 
>> What is the point of having the same value in two variables?
>
>It might look like it but there are not the same values, the description
>is accurate, even if the calculation at this stage is very simple.
>
>More importantly, this line is going away in the next patch and will be
>replaced by a more complexe calculation.

... and this - irrespective of subsequent patches, the one here would better
be self-contained, or otherwise its description should point out that certain
things are done in a way easing subsequent ones (but only if that was really
the case, which I don't think it is here - as you say, the questionable
constructs are being touched again later anyway, so could as well be left
out).

Jan


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

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

[Jan Beulich]

>> Anthony PERARD <anthony.perard@citrix.com> 08/04/17 1:38 PM >>>
>On Thu, Aug 03, 2017 at 09:21:57AM -0600, Jan Beulich wrote:
>> >>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:12 PM >>>
>> >@@ -818,6 +840,7 @@ static void vlapic_reg_write(struct vcpu *v,
>> >if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
>> >break;
>>  >
>> >+        vlapic->timer_last_update = hvm_get_guest_time(current);
>> >vlapic_set_reg(vlapic, APIC_TMICT, val);
>>  >
>> >vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT));
>> 
>> Why is this addition needed? vlapic_update_timer() sets timer_last_update
>> anyway. As it looks all you want is the value to be non-zero, which can be
>> done with less overhead and should be stated so in a comment.
>
>This is not true, the value is used before been set. It is used to
>calculate how much time have passed since tmict was set. Before been set
>again, there is this:
>time_passed = hvm_get_guest_time(current) - vlapic->timer_last_update;

Hmm, then I'm even more puzzled - the two hvm_get_guest_time() calls
will then result in a small but non-zero delta. Is that really intended?

Jan


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

Re: [PATCH v2 1/3] x86/vlapic: Introduce vlapic_update_timer

[Anthony PERARD]

On Fri, Aug 04, 2017 at 09:40:32AM -0600, Jan Beulich wrote:
> >>> Anthony PERARD <anthony.perard@citrix.com> 08/04/17 12:52 PM >>>
> >On Thu, Aug 03, 2017 at 08:59:10AM -0600, Jan Beulich wrote:
> >> >>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:10 PM >>>
> >> >+static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
> >> >+{
> >> >+    uint64_t period;
> >> >+    uint64_t delta;
> >> 
> >> Why two lines (but see also below)?
> >
> >Why not? Anyway, I'll change it.
> >
> >Also I realize that delta is going to be initialize to 0 here in the
> >next patch, which is why I think there is two lines.
> 
> For both this and ...
> 
> >> >+    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;
> >> >+
> >> >+    /* Calculate the next time the timer should trigger an interrupt. */
> >> >+    delta = period;
> >> 
> >> What is the point of having the same value in two variables?
> >
> >It might look like it but there are not the same values, the description
> >is accurate, even if the calculation at this stage is very simple.
> >
> >More importantly, this line is going away in the next patch and will be
> >replaced by a more complexe calculation.
> 
> ... and this - irrespective of subsequent patches, the one here would better
> be self-contained, or otherwise its description should point out that certain
> things are done in a way easing subsequent ones (but only if that was really
> the case, which I don't think it is here - as you say, the questionable
> constructs are being touched again later anyway, so could as well be left
> out).

Fine, I'll get rid of delta in this patch.

-- 
Anthony PERARD

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

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

[Anthony PERARD]

On Fri, Aug 04, 2017 at 09:48:59AM -0600, Jan Beulich wrote:
> >> Anthony PERARD <anthony.perard@citrix.com> 08/04/17 1:38 PM >>>
> >On Thu, Aug 03, 2017 at 09:21:57AM -0600, Jan Beulich wrote:
> >> >>> Anthony PERARD <anthony.perard@citrix.com> 07/18/17 7:12 PM >>>
> >> >@@ -818,6 +840,7 @@ static void vlapic_reg_write(struct vcpu *v,
> >> >if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
> >> >break;
> >>  >
> >> >+        vlapic->timer_last_update = hvm_get_guest_time(current);
> >> >vlapic_set_reg(vlapic, APIC_TMICT, val);
> >>  >
> >> >vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT));
> >> 
> >> Why is this addition needed? vlapic_update_timer() sets timer_last_update
> >> anyway. As it looks all you want is the value to be non-zero, which can be
> >> done with less overhead and should be stated so in a comment.
> >
> >This is not true, the value is used before been set. It is used to
> >calculate how much time have passed since tmict was set. Before been set
> >again, there is this:
> >time_passed = hvm_get_guest_time(current) - vlapic->timer_last_update;
> 
> Hmm, then I'm even more puzzled - the two hvm_get_guest_time() calls
> will then result in a small but non-zero delta. Is that really intended?

It is not really intended, but I did not see it as an issue either. I
can try to get rid of the first assignment, but the function is going to
needs an extra argument, something that say that timer_last_update is
not accurate should not be used or that tmict as just been updated.

I'll see what I can do.

Thanks,

-- 
Anthony PERARD

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