Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Tuesday, June 11, 2013 6:27 PM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Andreas Färber; address@hidden; qemu-
> address@hidden
> Subject: Re: [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for
> target_bit above 61
> 
> On 06/11/2013 02:47 PM, Bhushan Bharat-R65777 wrote:
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:address@hidden
> >> Sent: Tuesday, June 11, 2013 6:10 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: Wood Scott-B07421; Andreas Färber; address@hidden; qemu-
> >> address@hidden
> >> Subject: Re: [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer
> >> for target_bit above 61
> >>
> >> On 06/11/2013 01:40 PM, Bhushan Bharat-R65777 wrote:
> >>>> -----Original Message-----
> >>>> From: Alexander Graf [mailto:address@hidden
> >>>> Sent: Monday, June 10, 2013 11:40 PM
> >>>> To: Wood Scott-B07421
> >>>> Cc: Bhushan Bharat-R65777; Andreas Färber; address@hidden;
> >>>> qemu- address@hidden; Wood Scott-B07421
> >>>> Subject: Re: [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer
> >>>> for target_bit above 61
> >>>>
> >>>>
> >>>> On 10.06.2013, at 19:20, Scott Wood wrote:
> >>>>
> >>>>> On 06/10/2013 09:26:18 AM, Alexander Graf wrote:
> >>>>>> On 06/10/2013 02:47 PM, Bhushan Bharat-R65777 wrote:
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Andreas Färber [mailto:address@hidden
> >>>>>>>> Sent: Monday, June 10, 2013 5:43 PM
> >>>>>>>> To: Bhushan Bharat-R65777
> >>>>>>>> Cc: address@hidden; address@hidden; address@hidden;
> >>>>>>>> Wood
> >>>>>>>> Scott- B07421; Bhushan Bharat-R65777
> >>>>>>>> Subject: Re: [Qemu-devel] [PATCH v2]booke: timer: Deactivate
> >>>>>>>> timer for target_bit above 61 So IIUC we can only allow 63 bits due 
> >>>>>>>> to
> >>>>>>>> signedness, thus a maximum of (1<<    62), thus target_bit<= 61.
> >>>>>>>> Any chance at least the comment can be worded to explain that any
> >>>>>>>> better? Maybe also use (target-bit + 1>= 63) or period>    INT64_MAX 
> >>>>>>>> as
> >>>> condition?
> >>>>>>> How about this:
> >>>>>>>       /* QEMU timer supports a maximum timer of INT64_MAX
> >>>> (0x7fffffff_ffffffff).
> >>>>>>>        * Run booke fit/wdog timer when
> >>>>>>>        * ((1ULL<<    target_bit + 1)<    0x40000000_00000000), i.e
> target_bit
> >> =
> >>>> 61.
> >>>>>>>        * Also the time with this maximum target_bit (with current 
> >>>>>>> range
> of
> >>>>>>>        * CPU frequency PowerPC supports) will be many many years. So 
> >>>>>>> it
> is
> >>>>>>>        * pretty safe to stop the timer above this threshold. */
> >>>>>> How about
> >>>>>>    /* This timeout will take years to trigger. Treat the timer as
> >>>>>> disabled. */
> >>>>> There should be at least a brief mention that it's because the
> >>>>> QEMU timer can't handle larger values,
> >>>> If it can't handle higher values, maybe it's better to just set the
> >>>> timer value to INT64_MAX when we detect an overflow? That would
> >>>> make the code plainly obvious.
> >>>>
> >>> What about below comment (a mix of both :)):
> >>>
> >>>       /* Timeout calculated with (target_bit + 1)>   62 will take
> >>>        * hundreds of years to trigger. Treat the timer as disabled.
> >>>        * Also this timeout is within the qemu supported maximum
> >>>        * timeout limit (INT64_MAX.). */
> >> Ok, next question: Why does return disable the timer?
> > Actually here disabled means _not_ starting the timer. This function will be
> called to start timer initially and then later it is called to restart after
> every expiry. If we do not start then it is as good as stopped/disabled (it is
> not disabled in TCR). Probably saying "do not start qemu timer" or something
> similar is better than saying disabling the timer.
> 
> Couldn't you simply make things obvious from the code flow without pulling up
> assumptions?

You yourself suggested to stop/disable timer above a threshold :)

> 
> Something along the lines of
> 
> if (<overflow>) {

What is overflow?

Do you mean something like this: 
diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
index e41b036..5b84b96 100644
--- a/hw/ppc/ppc_booke.c
+++ b/hw/ppc/ppc_booke.c
@@ -133,15 +133,19 @@ static void booke_update_fixed_timer(CPUPPCState         
*env,
     ppc_tb_t *tb_env = env->tb_env;
     uint64_t lapse;
     uint64_t tb;
-    uint64_t period = 1 << (target_bit + 1);
+    uint64_t period;
     uint64_t now;
 
     now = qemu_get_clock_ns(vm_clock);
     tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
 
-    lapse = period - ((tb - (1 << target_bit)) & (period - 1));
-
-    *next = now + muldiv64(lapse, get_ticks_per_sec(), tb_env->tb_freq);
+    if (target_bit >= 62) {
+        *next = INT64_MAX;
+    } else {
+        period = 1ULL << (target_bit + 1);
+        lapse = period - ((tb - (1 << target_bit)) & (period - 1));
+        *next = now + muldiv64(lapse, get_ticks_per_sec(), tb_env->tb_freq);
+    }
------------------------ 

Thanks
-Bharat

>      *next = INT64_MAX;
> }
> 
> qemu_mod_timer(timer, *next);
> 
> Then everyone knows what's going on, we can always assume the timer is running
> and there's no need to understand complex corner cases. It feels more like the
> timer framework would be the one to decid to ignore timeouts that take years 
> to
> finish.
> 
> 
> Alex
>