Patchwork Re: [PATCH 3/3] mips: Expire late timers when reading cp0_count

login
register
mail settings
Submitter Edgar Iglesias
Date Jan. 18, 2011, 12:33 a.m.
Message ID <20110118003300.GA11846@laped.lan>
Download mbox | patch
Permalink /patch/79239/
State New
Headers show

Comments

Edgar Iglesias - Jan. 18, 2011, 12:33 a.m.
On Tue, Jan 18, 2011 at 12:29:42AM +0100, edgar.iglesias@gmail.com wrote:
> From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> 
> When reading cp0_count from a timer with a late trigger that should
> already have expired, expire it and raise the timer irq.
> 
> This makes it possible for guest code (e.g, Linux) that first read
> cp0_count, then compare it with cp0_compare and check for raised
> timer interrupt lines to run reliably.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>

Sorry sent the wrong version of this one. It's supposed to be the
following:

commit 139330de404209528712fd703952c0b5ad4459a1
Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Date:   Tue Jan 18 00:12:22 2011 +0100

    mips: Expire late timers when reading cp0_count
    
    When reading cp0_count from a timer with a late trigger that should
    already have expired, expire it and raise the timer irq.
    
    This makes it possible for guest code (e.g, Linux) that first read
    cp0_count, then compare it with cp0_compare and check for raised
    timer interrupt lines to run reliably.
    
    Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Aurelien Jarno - Jan. 18, 2011, 10:36 a.m.
On Tue, Jan 18, 2011 at 01:33:00AM +0100, Edgar E. Iglesias wrote:
> On Tue, Jan 18, 2011 at 12:29:42AM +0100, edgar.iglesias@gmail.com wrote:
> > From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > 
> > When reading cp0_count from a timer with a late trigger that should
> > already have expired, expire it and raise the timer irq.
> > 
> > This makes it possible for guest code (e.g, Linux) that first read
> > cp0_count, then compare it with cp0_compare and check for raised
> > timer interrupt lines to run reliably.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> 
> Sorry sent the wrong version of this one. It's supposed to be the
> following:
> 
> commit 139330de404209528712fd703952c0b5ad4459a1
> Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Date:   Tue Jan 18 00:12:22 2011 +0100
> 
>     mips: Expire late timers when reading cp0_count
>     
>     When reading cp0_count from a timer with a late trigger that should
>     already have expired, expire it and raise the timer irq.
>     
>     This makes it possible for guest code (e.g, Linux) that first read
>     cp0_count, then compare it with cp0_compare and check for raised
>     timer interrupt lines to run reliably.
>     
>     Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> 
> diff --git a/hw/mips_timer.c b/hw/mips_timer.c
> index 8c32087..9c95f28 100644
> --- a/hw/mips_timer.c
> +++ b/hw/mips_timer.c
> @@ -69,9 +69,17 @@ uint32_t cpu_mips_get_count (CPUState *env)
>      if (env->CP0_Cause & (1 << CP0Ca_DC)) {
>          return env->CP0_Count;
>      } else {
> +        uint64_t now;
> +
> +        now = qemu_get_clock(vm_clock);
> +        if (qemu_timer_pending(env->timer)
> +            && qemu_timer_expired(env->timer, now)) {
> +            /* The timer has already expired.  */
> +            cpu_mips_timer_expire(env);
> +        }
> +
>          return env->CP0_Count +
> -            (uint32_t)muldiv64(qemu_get_clock(vm_clock),
> -                               TIMER_FREQ, get_ticks_per_sec());
> +            (uint32_t)muldiv64(now, TIMER_FREQ, get_ticks_per_sec());
>      }
>  }
>  

Given the TB is now ended after this instruction (due to patch 1), isn't
the interrupt handled before starting the next TB, where the interrupt
line (I guess CP0_Cause) read?
Edgar Iglesias - Jan. 18, 2011, 10:41 a.m.
On Tue, Jan 18, 2011 at 11:36:25AM +0100, Aurelien Jarno wrote:
> On Tue, Jan 18, 2011 at 01:33:00AM +0100, Edgar E. Iglesias wrote:
> > On Tue, Jan 18, 2011 at 12:29:42AM +0100, edgar.iglesias@gmail.com wrote:
> > > From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > > 
> > > When reading cp0_count from a timer with a late trigger that should
> > > already have expired, expire it and raise the timer irq.
> > > 
> > > This makes it possible for guest code (e.g, Linux) that first read
> > > cp0_count, then compare it with cp0_compare and check for raised
> > > timer interrupt lines to run reliably.
> > > 
> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > 
> > Sorry sent the wrong version of this one. It's supposed to be the
> > following:
> > 
> > commit 139330de404209528712fd703952c0b5ad4459a1
> > Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > Date:   Tue Jan 18 00:12:22 2011 +0100
> > 
> >     mips: Expire late timers when reading cp0_count
> >     
> >     When reading cp0_count from a timer with a late trigger that should
> >     already have expired, expire it and raise the timer irq.
> >     
> >     This makes it possible for guest code (e.g, Linux) that first read
> >     cp0_count, then compare it with cp0_compare and check for raised
> >     timer interrupt lines to run reliably.
> >     
> >     Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > 
> > diff --git a/hw/mips_timer.c b/hw/mips_timer.c
> > index 8c32087..9c95f28 100644
> > --- a/hw/mips_timer.c
> > +++ b/hw/mips_timer.c
> > @@ -69,9 +69,17 @@ uint32_t cpu_mips_get_count (CPUState *env)
> >      if (env->CP0_Cause & (1 << CP0Ca_DC)) {
> >          return env->CP0_Count;
> >      } else {
> > +        uint64_t now;
> > +
> > +        now = qemu_get_clock(vm_clock);
> > +        if (qemu_timer_pending(env->timer)
> > +            && qemu_timer_expired(env->timer, now)) {
> > +            /* The timer has already expired.  */
> > +            cpu_mips_timer_expire(env);
> > +        }
> > +
> >          return env->CP0_Count +
> > -            (uint32_t)muldiv64(qemu_get_clock(vm_clock),
> > -                               TIMER_FREQ, get_ticks_per_sec());
> > +            (uint32_t)muldiv64(now, TIMER_FREQ, get_ticks_per_sec());
> >      }
> >  }
> >  
> 
> Given the TB is now ended after this instruction (due to patch 1), isn't
> the interrupt handled before starting the next TB, where the interrupt
> line (I guess CP0_Cause) read?

Hi,

The problem here is different. Due to host timing granularity, the
timer might expire later than it's precise scheduled time. If that
happens, get_count will return a count value that goes beyond the
trigger time but the interrupt may come later (when the host timer
expires).

This patch catches that case and expires the timer in-band, raising
the timer interrupt if needed.

Cheers
Aurelien Jarno - Jan. 18, 2011, 10:52 a.m.
On Tue, Jan 18, 2011 at 11:41:54AM +0100, Edgar E. Iglesias wrote:
> On Tue, Jan 18, 2011 at 11:36:25AM +0100, Aurelien Jarno wrote:
> > On Tue, Jan 18, 2011 at 01:33:00AM +0100, Edgar E. Iglesias wrote:
> > > On Tue, Jan 18, 2011 at 12:29:42AM +0100, edgar.iglesias@gmail.com wrote:
> > > > From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > > > 
> > > > When reading cp0_count from a timer with a late trigger that should
> > > > already have expired, expire it and raise the timer irq.
> > > > 
> > > > This makes it possible for guest code (e.g, Linux) that first read
> > > > cp0_count, then compare it with cp0_compare and check for raised
> > > > timer interrupt lines to run reliably.
> > > > 
> > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > > 
> > > Sorry sent the wrong version of this one. It's supposed to be the
> > > following:
> > > 
> > > commit 139330de404209528712fd703952c0b5ad4459a1
> > > Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > > Date:   Tue Jan 18 00:12:22 2011 +0100
> > > 
> > >     mips: Expire late timers when reading cp0_count
> > >     
> > >     When reading cp0_count from a timer with a late trigger that should
> > >     already have expired, expire it and raise the timer irq.
> > >     
> > >     This makes it possible for guest code (e.g, Linux) that first read
> > >     cp0_count, then compare it with cp0_compare and check for raised
> > >     timer interrupt lines to run reliably.
> > >     
> > >     Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > > 
> > > diff --git a/hw/mips_timer.c b/hw/mips_timer.c
> > > index 8c32087..9c95f28 100644
> > > --- a/hw/mips_timer.c
> > > +++ b/hw/mips_timer.c
> > > @@ -69,9 +69,17 @@ uint32_t cpu_mips_get_count (CPUState *env)
> > >      if (env->CP0_Cause & (1 << CP0Ca_DC)) {
> > >          return env->CP0_Count;
> > >      } else {
> > > +        uint64_t now;
> > > +
> > > +        now = qemu_get_clock(vm_clock);
> > > +        if (qemu_timer_pending(env->timer)
> > > +            && qemu_timer_expired(env->timer, now)) {
> > > +            /* The timer has already expired.  */
> > > +            cpu_mips_timer_expire(env);
> > > +        }
> > > +
> > >          return env->CP0_Count +
> > > -            (uint32_t)muldiv64(qemu_get_clock(vm_clock),
> > > -                               TIMER_FREQ, get_ticks_per_sec());
> > > +            (uint32_t)muldiv64(now, TIMER_FREQ, get_ticks_per_sec());
> > >      }
> > >  }
> > >  
> > 
> > Given the TB is now ended after this instruction (due to patch 1), isn't
> > the interrupt handled before starting the next TB, where the interrupt
> > line (I guess CP0_Cause) read?
> 
> Hi,
> 
> The problem here is different. Due to host timing granularity, the
> timer might expire later than it's precise scheduled time. If that
> happens, get_count will return a count value that goes beyond the
> trigger time but the interrupt may come later (when the host timer
> expires).
> 
> This patch catches that case and expires the timer in-band, raising
> the timer interrupt if needed.
> 

Ok, thanks for the explanation.

Acked-by: Aurelien Jarno <aurelien@aurel32.net>

Patch

diff --git a/hw/mips_timer.c b/hw/mips_timer.c
index 8c32087..9c95f28 100644
--- a/hw/mips_timer.c
+++ b/hw/mips_timer.c
@@ -69,9 +69,17 @@  uint32_t cpu_mips_get_count (CPUState *env)
     if (env->CP0_Cause & (1 << CP0Ca_DC)) {
         return env->CP0_Count;
     } else {
+        uint64_t now;
+
+        now = qemu_get_clock(vm_clock);
+        if (qemu_timer_pending(env->timer)
+            && qemu_timer_expired(env->timer, now)) {
+            /* The timer has already expired.  */
+            cpu_mips_timer_expire(env);
+        }
+
         return env->CP0_Count +
-            (uint32_t)muldiv64(qemu_get_clock(vm_clock),
-                               TIMER_FREQ, get_ticks_per_sec());
+            (uint32_t)muldiv64(now, TIMER_FREQ, get_ticks_per_sec());
     }
 }