Fix lapic time counter read for periodic mode

Fix lapic time counter read for periodic mode

[Christian Ehrhardt]

Hi,

there is a bug in the emulation of the lapic time counter. In particular
what we are seeing is that the time counter of a periodic lapic timer
in the guest reads as zero 99% of the time. The patch below fixes that.

The emulation of the lapic timer is done with the help of a hires
timer that expires with the same frequency as the lapic counter.
New expiration times for a periodic timer are calculated incrementally
based on the last scheduled expiration time. This ensures long term
accuracy of the emulated timer close to that of the underlying clock.

The actual value of the lapic time counter is calculated from the
real time difference between current time and scheduled expiration time
of the hires timer. If this difference is negative, the hires timer
expired. For oneshot mode this is correctly translated into a zero value
for the time counter. However, in periodic mode we must use the negative
difference unmodified.

     regards   Christian

Fix lapic time counter read for periodic mode.

Signed-off-by: Christian Ehrhardt <lk@c--e.de>

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 43e9fad..eff902d 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -810,8 +810,13 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
 	if (kvm_apic_get_reg(apic, APIC_TMICT) == 0)
 		return 0;
 
+	/*
+	 * hrtimer_get_remaining returns the signed difference between
+	 * timer expiration time and current time. Keep negative return
+	 * value iff the the timer is periodic.
+	 */
 	remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
-	if (ktime_to_ns(remaining) < 0)
+	if (ktime_to_ns(remaining) < 0 && !apic_lvtt_period(apic))
 		remaining = ktime_set(0, 0);
 
 	ns = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period);

Re: Fix lapic time counter read for periodic mode

[Marcelo Tosatti]

On Sun, Nov 11, 2012 at 01:25:28PM +0100, Christian Ehrhardt wrote:
> 
> Hi,
> 
> there is a bug in the emulation of the lapic time counter. In particular
> what we are seeing is that the time counter of a periodic lapic timer
> in the guest reads as zero 99% of the time. The patch below fixes that.
> 
> The emulation of the lapic timer is done with the help of a hires
> timer that expires with the same frequency as the lapic counter.
> New expiration times for a periodic timer are calculated incrementally
> based on the last scheduled expiration time. This ensures long term
> accuracy of the emulated timer close to that of the underlying clock.
> 
> The actual value of the lapic time counter is calculated from the
> real time difference between current time and scheduled expiration time
> of the hires timer. If this difference is negative, the hires timer
> expired. For oneshot mode this is correctly translated into a zero value
> for the time counter. However, in periodic mode we must use the negative
> difference unmodified.
> 
>      regards   Christian
> 
> Fix lapic time counter read for periodic mode.

In periodic mode the hrtimer is rearmed once expired, see
apic_timer_fn. So _get_remaining should return proper value
even if the guest is not able to process timer interrupts. 

Can you describe your specific scenario in more detail?

> Signed-off-by: Christian Ehrhardt <lk@c--e.de>
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 43e9fad..eff902d 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -810,8 +810,13 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
>  	if (kvm_apic_get_reg(apic, APIC_TMICT) == 0)
>  		return 0;
>  
> +	/*
> +	 * hrtimer_get_remaining returns the signed difference between
> +	 * timer expiration time and current time. Keep negative return
> +	 * value iff the the timer is periodic.
> +	 */
>  	remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
> -	if (ktime_to_ns(remaining) < 0)
> +	if (ktime_to_ns(remaining) < 0 && !apic_lvtt_period(apic))
>  		remaining = ktime_set(0, 0);
>  
>  	ns = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period);
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fix lapic time counter read for periodic mode

[Christian Ehrhardt]

Hi,

thanks for your reply.

On Mon, Nov 12, 2012 at 07:32:37PM -0200, Marcelo Tosatti wrote:
> > there is a bug in the emulation of the lapic time counter. In particular
> > what we are seeing is that the time counter of a periodic lapic timer
> > in the guest reads as zero 99% of the time. The patch below fixes that.
> > 
> > The emulation of the lapic timer is done with the help of a hires
> > timer that expires with the same frequency as the lapic counter.
> > New expiration times for a periodic timer are calculated incrementally
> > based on the last scheduled expiration time. This ensures long term
> > accuracy of the emulated timer close to that of the underlying clock.
> > 
> > The actual value of the lapic time counter is calculated from the
> > real time difference between current time and scheduled expiration time
> > of the hires timer. If this difference is negative, the hires timer
> > expired. For oneshot mode this is correctly translated into a zero value
> > for the time counter. However, in periodic mode we must use the negative
> > difference unmodified.
> > 
> >      regards   Christian
> > 
> > Fix lapic time counter read for periodic mode.
> 
> In periodic mode the hrtimer is rearmed once expired, see
> apic_timer_fn. So _get_remaining should return proper value
> even if the guest is not able to process timer interrupts. 
> 
> Can you describe your specific scenario in more detail?

In my specific case, the host is admittedly somewhat special as it
already is a rehosted version of linux, i.e. not running directly on
native hardware. It is still unclear if the host has sufficiently accurate
timer interrupts. This is most likely part of the problems we are seeing.

However, AFAICS apic_timer_fn is only called once per jiffy (at least in
some configurations). In particular, it is not called by
hrtimer_get_remaining. Thus depending on the frequency of the LAPIC timer
in the guest there might _several_ iterations that are missed. This can
probably be mitigated by a hires timer interrupts. However, I think
the problem is still there even in that case.

Additionally, the behaviour that I want to establish matches that of the
PIT timer (in a not completely obvious way, though).

Having said that the proposed patch in my first mail is incomplete, as
the mod_64 does not work correctly for negative values. A fixed version
is below.

     regards     Christian

Signed-off-by: Christian Ehrhardt <lk@c--e.de>

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 43e9fad..ec7242c 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -810,11 +810,22 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
 	if (kvm_apic_get_reg(apic, APIC_TMICT) == 0)
 		return 0;
 
+	/*
+	 * hrtimer_get_remaining returns the signed difference between
+	 * timer expiration time and current time. Keep negative return
+	 * values iff the the timer is periodic.
+	 */
 	remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
-	if (ktime_to_ns(remaining) < 0)
-		remaining = ktime_set(0, 0);
+	ns = ktime_to_ns(remaining);
+	if (unlikely(ns < 0)) {
+		if (apic_lvtt_period(apic))
+			ns = apic->lapic_timer.period -
+				mod_64(-ns, apic->lapic_timer.period);
+		else
+			ns = 0;
+	}
 
-	ns = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period);
+	ns = mod_64(ns, apic->lapic_timer.period);
 	tmcct = div64_u64(ns,
 			 (APIC_BUS_CYCLE_NS * apic->divide_count));
 

Re: Fix lapic time counter read for periodic mode

[Marcelo Tosatti]

On Tue, Nov 13, 2012 at 08:52:54AM +0100, Christian Ehrhardt wrote:
> 
> Hi,
> 
> thanks for your reply.
> 
> On Mon, Nov 12, 2012 at 07:32:37PM -0200, Marcelo Tosatti wrote:
> > > there is a bug in the emulation of the lapic time counter. In particular
> > > what we are seeing is that the time counter of a periodic lapic timer
> > > in the guest reads as zero 99% of the time. The patch below fixes that.
> > > 
> > > The emulation of the lapic timer is done with the help of a hires
> > > timer that expires with the same frequency as the lapic counter.
> > > New expiration times for a periodic timer are calculated incrementally
> > > based on the last scheduled expiration time. This ensures long term
> > > accuracy of the emulated timer close to that of the underlying clock.
> > > 
> > > The actual value of the lapic time counter is calculated from the
> > > real time difference between current time and scheduled expiration time
> > > of the hires timer. If this difference is negative, the hires timer
> > > expired. For oneshot mode this is correctly translated into a zero value
> > > for the time counter. However, in periodic mode we must use the negative
> > > difference unmodified.
> > > 
> > >      regards   Christian
> > > 
> > > Fix lapic time counter read for periodic mode.
> > 
> > In periodic mode the hrtimer is rearmed once expired, see
> > apic_timer_fn. So _get_remaining should return proper value
> > even if the guest is not able to process timer interrupts. 
> > 
> > Can you describe your specific scenario in more detail?
> 
> In my specific case, the host is admittedly somewhat special as it
> already is a rehosted version of linux, i.e. not running directly on
> native hardware. It is still unclear if the host has sufficiently accurate
> timer interrupts. This is most likely part of the problems we are seeing.
> 
> However, AFAICS apic_timer_fn is only called once per jiffy (at least in
> some configurations). In particular, it is not called by
> hrtimer_get_remaining. Thus depending on the frequency of the LAPIC timer
> in the guest there might _several_ iterations that are missed. This can
> probably be mitigated by a hires timer interrupts. However, I think
> the problem is still there even in that case.
> 
> Additionally, the behaviour that I want to establish matches that of the
> PIT timer (in a not completely obvious way, though).
> 
> Having said that the proposed patch in my first mail is incomplete, as
> the mod_64 does not work correctly for negative values. A fixed version
> is below.
> 
>      regards     Christian
> 
> Signed-off-by: Christian Ehrhardt <lk@c--e.de>

Alright. Please add a comment from the LAPIC documentation describing
this behaviour (and a nice changelog). Thanks.

> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 43e9fad..ec7242c 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -810,11 +810,22 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
>  	if (kvm_apic_get_reg(apic, APIC_TMICT) == 0)
>  		return 0;
>  
> +	/*
> +	 * hrtimer_get_remaining returns the signed difference between
> +	 * timer expiration time and current time. Keep negative return
> +	 * values iff the the timer is periodic.
> +	 */
>  	remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
> -	if (ktime_to_ns(remaining) < 0)
> -		remaining = ktime_set(0, 0);
> +	ns = ktime_to_ns(remaining);
> +	if (unlikely(ns < 0)) {
> +		if (apic_lvtt_period(apic))
> +			ns = apic->lapic_timer.period -
> +				mod_64(-ns, apic->lapic_timer.period);
> +		else
> +			ns = 0;
> +	}
>  
> -	ns = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period);
> +	ns = mod_64(ns, apic->lapic_timer.period);
>  	tmcct = div64_u64(ns,
>  			 (APIC_BUS_CYCLE_NS * apic->divide_count));
>  

Re: Fix lapic time counter read for periodic mode

[Paolo Bonzini]

Il 13/11/2012 21:40, Marcelo Tosatti ha scritto:
> On Tue, Nov 13, 2012 at 08:52:54AM +0100, Christian Ehrhardt wrote:
>> > 
>> > Hi,
>> > 
>> > thanks for your reply.
>> > 
>> > On Mon, Nov 12, 2012 at 07:32:37PM -0200, Marcelo Tosatti wrote:
>>>> > > > there is a bug in the emulation of the lapic time counter. In particular
>>>> > > > what we are seeing is that the time counter of a periodic lapic timer
>>>> > > > in the guest reads as zero 99% of the time. The patch below fixes that.
>>>> > > > 
>>>> > > > The emulation of the lapic timer is done with the help of a hires
>>>> > > > timer that expires with the same frequency as the lapic counter.
>>>> > > > New expiration times for a periodic timer are calculated incrementally
>>>> > > > based on the last scheduled expiration time. This ensures long term
>>>> > > > accuracy of the emulated timer close to that of the underlying clock.
>>>> > > > 
>>>> > > > The actual value of the lapic time counter is calculated from the
>>>> > > > real time difference between current time and scheduled expiration time
>>>> > > > of the hires timer. If this difference is negative, the hires timer
>>>> > > > expired. For oneshot mode this is correctly translated into a zero value
>>>> > > > for the time counter. However, in periodic mode we must use the negative
>>>> > > > difference unmodified.
>>>> > > > 
>>>> > > >      regards   Christian
>>>> > > > 
>>>> > > > Fix lapic time counter read for periodic mode.
>>> > > 
>>> > > In periodic mode the hrtimer is rearmed once expired, see
>>> > > apic_timer_fn. So _get_remaining should return proper value
>>> > > even if the guest is not able to process timer interrupts. 
>>> > > 
>>> > > Can you describe your specific scenario in more detail?
>> > 
>> > In my specific case, the host is admittedly somewhat special as it
>> > already is a rehosted version of linux, i.e. not running directly on
>> > native hardware. It is still unclear if the host has sufficiently accurate
>> > timer interrupts. This is most likely part of the problems we are seeing.
>> > 
>> > However, AFAICS apic_timer_fn is only called once per jiffy (at least in
>> > some configurations). In particular, it is not called by
>> > hrtimer_get_remaining. Thus depending on the frequency of the LAPIC timer
>> > in the guest there might _several_ iterations that are missed. This can
>> > probably be mitigated by a hires timer interrupts. However, I think
>> > the problem is still there even in that case.
>> > 
>> > Additionally, the behaviour that I want to establish matches that of the
>> > PIT timer (in a not completely obvious way, though).
>> > 
>> > Having said that the proposed patch in my first mail is incomplete, as
>> > the mod_64 does not work correctly for negative values. A fixed version
>> > is below.
>> > 
>> >      regards     Christian
>> > 
>> > Signed-off-by: Christian Ehrhardt <lk@c--e.de>
> Alright. Please add a comment from the LAPIC documentation describing
> this behaviour (and a nice changelog). Thanks.
> 

Christian, did you ever resubmit the patch?

Paolo