From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38258) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UmPA9-0000Qo-0R for qemu-devel@nongnu.org; Tue, 11 Jun 2013 10:03:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UmPA3-0000cn-Hm for qemu-devel@nongnu.org; Tue, 11 Jun 2013 10:03:04 -0400 Message-ID: <51B72E11.707@suse.de> Date: Tue, 11 Jun 2013 16:02:57 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1370884805.18413.8@snotra> <2338F1B8-5AB9-4DDF-91FD-EA6B105C5CEF@suse.de> <6A3DF150A5B70D4F9B66A25E3F7C888D07066B2F@039-SN2MPN1-012.039d.mgd.msft.net> <51B71A84.2050401@suse.de> <6A3DF150A5B70D4F9B66A25E3F7C888D07066D17@039-SN2MPN1-012.039d.mgd.msft.net> <51B71E86.60106@suse.de> <6A3DF150A5B70D4F9B66A25E3F7C888D07066D7A@039-SN2MPN1-012.039d.mgd.msft.net> In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D07066D7A@039-SN2MPN1-012.039d.mgd.msft.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bhushan Bharat-R65777 Cc: Wood Scott-B07421 , "qemu-ppc@nongnu.org" , =?ISO-8859-1?Q?Andreas_F=E4?= =?ISO-8859-1?Q?rber?= , "qemu-devel@nongnu.org" 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=E4rber; 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=E4rber; 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=E4rber; 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=E4rber [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 bit= s due to >>>>>>>>>> signedness, thus a maximum of (1<< 62), thus target_bit<=3D= 61. >>>>>>>>>> Any chance at least the comment can be worded to explain that = any >>>>>>>>>> better? Maybe also use (target-bit + 1>=3D 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 >>>> =3D >>>>>> 61. >>>>>>>>> * Also the time with this maximum target_bit (with curr= ent range >> of >>>>>>>>> * CPU frequency PowerPC supports) will be many many yea= rs. 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 th= e >>>>>> 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/disable= d (it is >> not disabled in TCR). Probably saying "do not start qemu timer" or som= ething >> similar is better than saying disabling the timer. >> >> Couldn't you simply make things obvious from the code flow without pul= ling up >> assumptions? > You yourself suggested to stop/disable timer above a threshold :) > >> Something along the lines of >> >> if () { > 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 =3D env->tb_env; > uint64_t lapse; > uint64_t tb; > - uint64_t period =3D 1<< (target_bit + 1); > + uint64_t period; > uint64_t now; > > now =3D qemu_get_clock_ns(vm_clock); > tb =3D cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset); > > - lapse =3D period - ((tb - (1<< target_bit))& (period - 1)); > - > - *next =3D now + muldiv64(lapse, get_ticks_per_sec(), tb_env->tb_fr= eq); > + if (target_bit>=3D 62) { /* This would overflow our calculation, so just max the timer out to the=20 biggest value the timer framework can handle */ > + *next =3D INT64_MAX; > + } else { > + period =3D 1ULL<< (target_bit + 1); > + lapse =3D period - ((tb - (1<< target_bit))& (period - 1)); > + *next =3D now + muldiv64(lapse, get_ticks_per_sec(), tb_env->t= b_freq); > + } Alex