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

[Alexander Graf <agraf@suse.de>]

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?


Alex