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

[Alexander Graf <agraf@suse.de>]

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?

Something along the lines of

if (<overflow>) {
     *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