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

[PATCH 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.

About the TSC-deadline mode, I've only make changes by reading the Intel
manual, I did not have real-hardward which support it.

Thanks,

[1] https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg00959.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg02533.html

Anthony PERARD (3):
  x86/vlapic: Fix vLAPIC Timer to behave more like real-hw
  x86/vlapic: Handle change of timer Divide Configuration Register
  x86/vlapic: Reset LAPIC Timer only on TSC Deadline mode change

 xen/arch/x86/hvm/vlapic.c | 147 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 116 insertions(+), 31 deletions(-)

-- 
Anthony PERARD


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

[PATCH 1/3] x86/vlapic: Fix vLAPIC Timer to behave more like real-hw

[Anthony PERARD]

This patch takes care of change of timer mode between periodic and
one-shot, because the timer is not reset this happen.

There is still change of the Divide Configuration Register that is not
handle by this patch, but will be in:
x86/vlapic: Handle change of timer Divide Configuration Register

Testing has been done with XTF+(patch "Add vlapic timer checks").

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

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 14356a78fe..97b7774b61 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -518,7 +518,11 @@ 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 be 0 as well.
+     * This happen when the timer is set to periodic with TMICT != 0, but TMCCT
+     * was already down to 0.
+     */
+    if ( tmict != 0 && vlapic->timer_last_update )
     {
         if ( vlapic_lvtt_period(vlapic) )
             counter_passed %= tmict;
@@ -666,6 +670,82 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
     vcpu_vlapic(v)->hw.tdt_msr = 0;
 }
 
+static void vlapic_update_timer(struct vlapic *vlapic,
+                                unsigned int offset,
+                                uint32_t val)
+{
+
+    uint64_t period;
+    uint64_t delta = 0;
+    bool is_oneshot, is_periodic;
+
+    switch (offset)
+    {
+    case APIC_LVTT:
+        period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
+            * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
+        is_periodic = (val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_PERIODIC;
+        is_oneshot = (val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_ONESHOT;
+
+        /* Calculate the next time the timer should trigger an interrupt. */
+        if ( period && vlapic->timer_last_update )
+        {
+            uint64_t time_passed = hvm_get_guest_time(current)
+                - vlapic->timer_last_update;
+            if ( vlapic_lvtt_period(vlapic) )
+                time_passed %= period;
+            if ( time_passed < period )
+                delta = period - time_passed;
+        }
+        break;
+    case APIC_TMICT:
+        period = (uint64_t)val * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
+        delta = period;
+
+        is_periodic = vlapic_lvtt_period(vlapic);
+        is_oneshot = vlapic_lvtt_oneshot(vlapic);
+        break;
+    default:
+        BUG();
+    }
+
+    if ( (is_oneshot || is_periodic) && delta != 0 )
+    {
+        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);
+
+        /* 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, "
+                    "initial count %u, period %"PRIu64"ns",
+                    APIC_BUS_CYCLE_NS,
+                    offset == APIC_TMICT
+                    ? val : vlapic_get_reg(vlapic, APIC_TMICT),
+                    period);
+    }
+    else
+    {
+        TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
+        destroy_periodic_time(&vlapic->pt);
+        /* From now, TMCCT should be 0 until TMICT is set. */
+        vlapic->timer_last_update = 0;
+    }
+}
+
+
 static void vlapic_reg_write(struct vcpu *v,
                              unsigned int offset, uint32_t val)
 {
@@ -733,13 +813,10 @@ static void vlapic_reg_write(struct vcpu *v,
         if ( (vlapic_get_reg(vlapic, offset) & APIC_TIMER_MODE_MASK) !=
              (val & APIC_TIMER_MODE_MASK) )
         {
-            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, APIC_LVTT, val);
         /* fallthrough */
     case APIC_LVTTHMR:      /* LVT Thermal Monitor */
     case APIC_LVTPC:        /* LVT Performance Counter */
@@ -763,34 +840,10 @@ static void vlapic_reg_write(struct vcpu *v,
 
     case APIC_TMICT:
     {
-        uint64_t period;
-
         if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
             break;
-
+        vlapic_update_timer(vlapic, APIC_TMICT, val);
         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;
 
-- 
Anthony PERARD


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

[PATCH 2/3] x86/vlapic: Handle change of timer Divide Configuration Register

[Anthony PERARD]

When the divide value change, uptade the timer according to the new
value, and keep the Counter Register (TMCCT) value the same between
before and after the divisor change.

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

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 97b7774b61..f70a25f5b9 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -705,6 +705,35 @@ static void vlapic_update_timer(struct vlapic *vlapic,
         is_periodic = vlapic_lvtt_period(vlapic);
         is_oneshot = vlapic_lvtt_oneshot(vlapic);
         break;
+    case APIC_TDCR:
+        is_periodic = vlapic_lvtt_period(vlapic);
+        is_oneshot = vlapic_lvtt_oneshot(vlapic);
+
+        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. */
+        if ( period && vlapic->timer_last_update )
+        {
+            uint64_t time_passed = hvm_get_guest_time(current)
+                - vlapic->timer_last_update;
+            if ( is_periodic )
+                time_passed %= period;
+            if ( time_passed < period )
+                delta = period - time_passed;
+        }
+
+        val = ((val & 3) | ((val & 8) >> 1)) + 1;
+        val = 1 << (val & 7);
+
+        period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
+            * APIC_BUS_CYCLE_NS * val;
+
+        /* Calculate time left until next interrupt, base on the difference
+         * between the current timer_divisor and the new one */
+        delta = delta * val / vlapic->hw.timer_divisor;
+
+        break;
     default:
         BUG();
     }
@@ -848,6 +877,7 @@ static void vlapic_reg_write(struct vcpu *v,
     break;
 
     case APIC_TDCR:
+        vlapic_update_timer(vlapic, APIC_TDCR, val);
         vlapic_set_tdcr(vlapic, val & 0xb);
         HVM_DBG_LOG(DBG_LEVEL_VLAPIC_TIMER, "timer divisor is %#x",
                     vlapic->hw.timer_divisor);
-- 
Anthony PERARD


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

[PATCH 3/3] x86/vlapic: Reset LAPIC Timer only on TSC Deadline mode change

[Anthony PERARD]

For the LAPIC timer, switching between periodic and one-shot does not
reset anything on real-hardward, but switching from TSC deadline or to
it does reset the timer, according to Intel manual.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

---
I'm not sure that TMICT should be reset, but the manuel said that in
tsc-deadline, write to TMICT are ignored.
---
 xen/arch/x86/hvm/vlapic.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index f70a25f5b9..18247bd8bb 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -839,9 +839,11 @@ 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) )
+        /* Switching between tdt and periodic|one-shot reset the other mode */
+        if ( vlapic_lvtt_tdt(vlapic) !=
+             ((val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_TSC_DEADLINE))
         {
+            vlapic_set_reg(vlapic, APIC_TMICT, 0);
             vlapic->hw.tdt_msr = 0;
         }
         vlapic->pt.irq = val & APIC_VECTOR_MASK;
-- 
Anthony PERARD


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

Re: [PATCH 1/3] x86/vlapic: Fix vLAPIC Timer to behave more like real-hw

[Jan Beulich]

>>> On 23.03.17 at 12:46, <anthony.perard@citrix.com> wrote:
> This patch takes care of change of timer mode between periodic and
> one-shot, because the timer is not reset this happen.

So I have to admit that I have some general difficulties with a
patch submission like this: The patch title is not really making
clear which aspect(s) are being changed, and the sentence
above is ambiguous according to my reading: Do you mean
the timer so far is wrongly not being reset when this happens,
or the timer should not be reset in such a case? Furthermore,
and this is really relevant in cases like this, you should clearly
spell out which part(s) of your change are addressing issues
with us not properly following the spec vs. which are based
on empirically collected information. In the case here, I
assume you mean the timer (or really TMICT) should not be
reset, but I can't seem to find anything int the SDM saying
so.

> There is still change of the Divide Configuration Register that is not
> handle by this patch, but will be in:
> x86/vlapic: Handle change of timer Divide Configuration Register
> 
> Testing has been done with XTF+(patch "Add vlapic timer checks").

On a broad range of hardware, I assume, if my observation
of you making a change not mandated by the spec is correct?

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -518,7 +518,11 @@ 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 be 0 as well.
> +     * This happen when the timer is set to periodic with TMICT != 0, but TMCCT
> +     * was already down to 0.
> +     */
> +    if ( tmict != 0 && vlapic->timer_last_update )

Please be consistent - either both sides use "!= 0", or (preferably)
both sides don't. Also please fix comment style (also elsewhere in
the patch). And finally, do you mean "this happens when ..." or
"this can happen when ..." (I assume the former, and I assume
you specifically mean a transition from one-shot to periodic)? This
needs to be spelled out without leaving any room for interpretation,
namely, as said, when the implemented behavior is not mandated
by documentation.

> @@ -666,6 +670,82 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
>      vcpu_vlapic(v)->hw.tdt_msr = 0;
>  }
>  
> +static void vlapic_update_timer(struct vlapic *vlapic,
> +                                unsigned int offset,
> +                                uint32_t val)
> +{
> +
> +    uint64_t period;
> +    uint64_t delta = 0;
> +    bool is_oneshot, is_periodic;
> +
> +    switch (offset)
> +    {
> +    case APIC_LVTT:
> +        period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
> +            * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
> +        is_periodic = (val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_PERIODIC;
> +        is_oneshot = (val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_ONESHOT;
> +
> +        /* Calculate the next time the timer should trigger an interrupt. */
> +        if ( period && vlapic->timer_last_update )
> +        {
> +            uint64_t time_passed = hvm_get_guest_time(current)
> +                - vlapic->timer_last_update;
> +            if ( vlapic_lvtt_period(vlapic) )

Blank line between declaration(s) and statements please. And then -
is this decision really to be taken based on the old LVTT value?

> +                time_passed %= period;
> +            if ( time_passed < period )
> +                delta = period - time_passed;
> +        }
> +        break;
> +    case APIC_TMICT:
> +        period = (uint64_t)val * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
> +        delta = period;
> +
> +        is_periodic = vlapic_lvtt_period(vlapic);
> +        is_oneshot = vlapic_lvtt_oneshot(vlapic);
> +        break;
> +    default:
> +        BUG();
> +    }

There are exactly two calls to this function, one each from the
code handling the writes of the respective registers handled in
the switch() above. Please move such portions of the code into
the callers (calculation of the two is_* variable can remain here,
for example, the callers would simply pass the LVTT value), using
suitable function parameters to pass the values needed here.
That way you can avoid the BUG() and make review as well as
future code inspection easier.

> +    if ( (is_oneshot || is_periodic) && delta != 0 )

Wouldn't this more obviously be

    if ( (is_oneshot && delta) || is_periodic )

(afaict delta would always non-zero here in periodic mode, if
the earlier mentioned use of the old LVTT value is indeed wrong;
otherwise delta may need forcing to period for the call to
create_periodic_time() below)?

> +    {
> +        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);
> +
> +        /* 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, "
> +                    "initial count %u, period %"PRIu64"ns",
> +                    APIC_BUS_CYCLE_NS,
> +                    offset == APIC_TMICT
> +                    ? val : vlapic_get_reg(vlapic, APIC_TMICT),
> +                    period);
> +    }
> +    else
> +    {
> +        TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
> +        destroy_periodic_time(&vlapic->pt);
> +        /* From now, TMCCT should be 0 until TMICT is set. */
> +        vlapic->timer_last_update = 0;

Hmm, the deadline case would come here too, and other TDT
related code uses the field too, so I have to ask whether this
possibly breaks TDT (then being fixed by a later patch). If so
you'll need to find a way to not transiently break other
functionality. Furthermore, with documentation again saying
nothing about TMICT when using TDT, you should also clarify
again what the intended / observed behavior is.

Jan

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

Re: [PATCH 2/3] x86/vlapic: Handle change of timer Divide Configuration Register

[Jan Beulich]

>>> On 23.03.17 at 12:47, <anthony.perard@citrix.com> wrote:
> When the divide value change, uptade the timer according to the new
> value, and keep the Counter Register (TMCCT) value the same between
> before and after the divisor change.

General and formatting comments given on patch 1 apply here too,
the logic looks okay assuming the intended / observed behavior is
that on-the-fly updates to the register take immediate effect on all
other functionality.

Jan


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

Re: [PATCH 3/3] x86/vlapic: Reset LAPIC Timer only on TSC Deadline mode change

[Jan Beulich]

>>> On 23.03.17 at 12:47, <anthony.perard@citrix.com> wrote:
> For the LAPIC timer, switching between periodic and one-shot does not
> reset anything on real-hardward, but switching from TSC deadline or to
> it does reset the timer, according to Intel manual.

Oh, I see that I've overlooked this when reviewing patch 1 (I
was looking for the acronyms only) - please ignore the respective
comment there then.

> I'm not sure that TMICT should be reset, but the manuel said that in
> tsc-deadline, write to TMICT are ignored.

Writes being ignored to me suggests that TMICT simply retains its
value while in this mode. But you should be able to verify this with
parts of the XTF test code of yours run on bare hardware, shouldn't
you?

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -839,9 +839,11 @@ 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) )
> +        /* Switching between tdt and periodic|one-shot reset the other mode */
> +        if ( vlapic_lvtt_tdt(vlapic) !=
> +             ((val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_TSC_DEADLINE))
>          {
> +            vlapic_set_reg(vlapic, APIC_TMICT, 0);
>              vlapic->hw.tdt_msr = 0;
>          }

The change is quite different from what I would expect based on
the SDM information: Did you verify that the MSR is being cleared
only when changing from/to TDT (but neither when changing
TDT->TDT nor when changing non-TDT->non-TDT)? And according
to the comment above, I'd rather expect TMICT to be left alone
here, but instead reads to not return zero but the last value
written before changing to TDT (writes already look to be ignored
as specified).

Jan


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