> -----Original Message----- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Tuesday, June 11, 2013 6:27 PM > To: Bhushan Bharat-R65777 > Cc: Wood Scott-B07421; Andreas Färber; qemu-...@nongnu.org; qemu- > de...@nongnu.org > 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:ag...@suse.de] > >> Sent: Tuesday, June 11, 2013 6:10 PM > >> To: Bhushan Bharat-R65777 > >> Cc: Wood Scott-B07421; Andreas Färber; qemu-...@nongnu.org; qemu- > >> de...@nongnu.org > >> 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:ag...@suse.de] > >>>> Sent: Monday, June 10, 2013 11:40 PM > >>>> To: Wood Scott-B07421 > >>>> Cc: Bhushan Bharat-R65777; Andreas Färber; qemu-...@nongnu.org; > >>>> qemu- de...@nongnu.org; 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:afaer...@suse.de] > >>>>>>>> Sent: Monday, June 10, 2013 5:43 PM > >>>>>>>> To: Bhushan Bharat-R65777 > >>>>>>>> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; ag...@suse.de; > >>>>>>>> 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 >