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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Aug 03, 2017 at 09:21:57AM -0600, Jan Beulich wrote:
> >>> 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.

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel