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

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

[Bharat Bhushan]

QEMU timer supports a maximum timer of INT64_MAX. So starting timer only for
time which is calculated using target_bit < 62 and deactivate/stop timer if
the target bit is above 61.

This patch also fix the time calculation from target_bit.
The code was doing (1 << (target_bit + 1)) while this
should be (1ULL << (target_bit + 1)).

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
v1->v2
 - Added "booke: timer:" in patch subject

 hw/ppc/ppc_booke.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
index e41b036..f4eda15 100644
--- a/hw/ppc/ppc_booke.c
+++ b/hw/ppc/ppc_booke.c
@@ -133,9 +133,15 @@ 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;
 
+    /* Deactivate timer for target_bit > 61 */
+    if (target_bit > 61)
+        return; 
+
+    period = 1ULL << (target_bit + 1);
+
     now = qemu_get_clock_ns(vm_clock);
     tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
 
-- 
1.7.0.4

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

[Peter Maydell]

On 10 June 2013 08:55, Bharat Bhushan <r65777@freescale.com> wrote:
> QEMU timer supports a maximum timer of INT64_MAX. So starting timer only for
> time which is calculated using target_bit < 62 and deactivate/stop timer if
> the target bit is above 61.

Is this really what the hardware does? Or do we need to set a
timer for the maximum QEMU allows and then reset it for the
residual time when the first timer expires?

thanks
-- PMM

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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Monday, June 10, 2013 2:56 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
> 
> On 10 June 2013 08:55, Bharat Bhushan <r65777@freescale.com> wrote:
> > QEMU timer supports a maximum timer of INT64_MAX. So starting timer
> > only for time which is calculated using target_bit < 62 and
> > deactivate/stop timer if the target bit is above 61.
> 
> Is this really what the hardware does? Or do we need to set a timer for the
> maximum QEMU allows and then reset it for the residual time when the first timer
> expires?

No, this is not how hardware works. But the time with the maximum target bit of 61 (with current range of CPU frequency PowerPC supports) will be many many years. So I think it is pretty safe to stop the timer.

Thanks
-Bharat

> 
> thanks
> -- PMM

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

[Andreas Färber]

Am 10.06.2013 09:55, schrieb Bharat Bhushan:
> QEMU timer supports a maximum timer of INT64_MAX. So starting timer only for
> time which is calculated using target_bit < 62 and deactivate/stop timer if
> the target bit is above 61.
> 
> This patch also fix the time calculation from target_bit.
> The code was doing (1 << (target_bit + 1)) while this
> should be (1ULL << (target_bit + 1)).
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> v1->v2
>  - Added "booke: timer:" in patch subject
> 
>  hw/ppc/ppc_booke.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
> index e41b036..f4eda15 100644
> --- a/hw/ppc/ppc_booke.c
> +++ b/hw/ppc/ppc_booke.c
> @@ -133,9 +133,15 @@ 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;
>  
> +    /* Deactivate timer for target_bit > 61 */
> +    if (target_bit > 61)
> +        return; 

Braces missing and trailing whitespace after return.

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?

Best regards,
Andreas

> +
> +    period = 1ULL << (target_bit + 1);
> +
>      now = qemu_get_clock_ns(vm_clock);
>      tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Bhushan Bharat-R65777]

> -----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
> 
> Am 10.06.2013 09:55, schrieb Bharat Bhushan:
> > QEMU timer supports a maximum timer of INT64_MAX. So starting timer
> > only for time which is calculated using target_bit < 62 and
> > deactivate/stop timer if the target bit is above 61.
> >
> > This patch also fix the time calculation from target_bit.
> > The code was doing (1 << (target_bit + 1)) while this should be (1ULL
> > << (target_bit + 1)).
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > v1->v2
> >  - Added "booke: timer:" in patch subject
> >
> >  hw/ppc/ppc_booke.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c index
> > e41b036..f4eda15 100644
> > --- a/hw/ppc/ppc_booke.c
> > +++ b/hw/ppc/ppc_booke.c
> > @@ -133,9 +133,15 @@ 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;
> >
> > +    /* Deactivate timer for target_bit > 61 */
> > +    if (target_bit > 61)
> > +        return;
> 
> Braces missing and trailing whitespace after return.

Ok, will correct

> 
> 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. */


> 
> Best regards,
> Andreas
> 
> > +
> > +    period = 1ULL << (target_bit + 1);
> > +
> >      now = qemu_get_clock_ns(vm_clock);
> >      tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
> >
> >
> 
> 
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Alexander Graf]

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
>>
>> Am 10.06.2013 09:55, schrieb Bharat Bhushan:
>>> QEMU timer supports a maximum timer of INT64_MAX. So starting timer
>>> only for time which is calculated using target_bit<  62 and
>>> deactivate/stop timer if the target bit is above 61.
>>>
>>> This patch also fix the time calculation from target_bit.
>>> The code was doing (1<<  (target_bit + 1)) while this should be (1ULL
>>> <<  (target_bit + 1)).
>>>
>>> Signed-off-by: Bharat Bhushan<bharat.bhushan@freescale.com>
>>> ---
>>> v1->v2
>>>   - Added "booke: timer:" in patch subject
>>>
>>>   hw/ppc/ppc_booke.c |    8 +++++++-
>>>   1 files changed, 7 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c index
>>> e41b036..f4eda15 100644
>>> --- a/hw/ppc/ppc_booke.c
>>> +++ b/hw/ppc/ppc_booke.c
>>> @@ -133,9 +133,15 @@ 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;
>>>
>>> +    /* Deactivate timer for target_bit>  61 */
>>> +    if (target_bit>  61)
>>> +        return;
>> Braces missing and trailing whitespace after return.
> Ok, will correct
>
>> 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. */


Alex

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

[Scott Wood]

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, with the detailed explanation in the  
changelog.  A better lower bound on the number of years would be nice  
as well (e.g. "hundreds of years").

-Scott

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

[Alexander Graf]

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.


Alex

> with the detailed explanation in the changelog.  A better lower bound on the number of years would be nice as well (e.g. "hundreds of years").
> 
> -Scott

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

[Bhushan Bharat-R65777]

> -----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.). */

-Bharat

> 
> Alex
> 
> > with the detailed explanation in the changelog.  A better lower bound on the
> number of years would be nice as well (e.g. "hundreds of years").
> >
> > -Scott
> 

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

[Alexander Graf]

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

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

[Bhushan Bharat-R65777]

> -----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.

Thanks
-Bharat

> 
> 
> Alex
> 

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

[Alexander Graf]

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

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

[Bhushan Bharat-R65777]

> -----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?

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) {
+        *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);
+    }
------------------------ 

Thanks
-Bharat

>      *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
> 

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

[Alexander Graf]

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