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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Anthony PERARD <anthony.perard@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel