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 Fri, Aug 04, 2017 at 09:48:59AM -0600, Jan Beulich wrote:
> >> Anthony PERARD <anthony.perard@xxxxxxxxxx> 08/04/17 1:38 PM >>>
> >On Thu, Aug 03, 2017 at 09:21:57AM -0600, Jan Beulich wrote:
> >> >>> Anthony PERARD <anthony.perard@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel