| Message ID | 6A3DF150A5B70D4F9B66A25E3F7C888D07066D7A@039-SN2MPN1-012.039d.mgd.msft.net |
|---|---|
| State | New |
| Headers | show |
On 06/11/2013 03:18 PM, Bhushan Bharat-R65777 wrote: > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Tuesday, June 11, 2013 6:27 PM >> To: Bhushan Bharat-R65777 >> Cc: Wood Scott-B07421; Andreas Färber; qemu-ppc@nongnu.org; qemu- >> devel@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:agraf@suse.de] >>>> Sent: Tuesday, June 11, 2013 6:10 PM >>>> To: Bhushan Bharat-R65777 >>>> Cc: Wood Scott-B07421; Andreas Färber; qemu-ppc@nongnu.org; qemu- >>>> devel@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:agraf@suse.de] >>>>>> Sent: Monday, June 10, 2013 11:40 PM >>>>>> To: Wood Scott-B07421 >>>>>> Cc: Bhushan Bharat-R65777; Andreas Färber; qemu-ppc@nongnu.org; >>>>>> qemu- devel@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:afaerber@suse.de] >>>>>>>>>> Sent: Monday, June 10, 2013 5:43 PM >>>>>>>>>> To: Bhushan Bharat-R65777 >>>>>>>>>> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; agraf@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? The reason you're jumping through the hoops :). > > 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) { /* This would overflow our calculation, so just max the timer out to the biggest value the timer framework can handle */ > + *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); > + } Alex
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