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

[Alexander Graf <agraf@suse.de>]

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