From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46142) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UmO7u-0006PG-H0 for qemu-devel@nongnu.org; Tue, 11 Jun 2013 08:56:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UmO7s-0002X8-Id for qemu-devel@nongnu.org; Tue, 11 Jun 2013 08:56:42 -0400 Message-ID: <51B71E86.60106@suse.de> Date: Tue, 11 Jun 2013 14:56:38 +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> In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D07066D17@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 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; qe= mu- >>>> 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 bits = due to >>>>>>>> signedness, thus a maximum of (1<< 62), thus target_bit<=3D 6= 1. >>>>>>>> Any chance at least the comment can be worded to explain that an= y >>>>>>>> better? Maybe also use (target-bit + 1>=3D 63) or period> INT= 64_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 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 wi= ll be called to start timer initially and then later it is called to rest= art 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=20 pulling up assumptions? Something along the lines of if () { *next =3D INT64_MAX; } qemu_mod_timer(timer, *next); Then everyone knows what's going on, we can always assume the timer is=20 running and there's no need to understand complex corner cases. It feels=20 more like the timer framework would be the one to decid to ignore=20 timeouts that take years to finish. Alex