Patchwork [v2] booke: timer: Deactivate timer for target_bit above 61

login
register
mail settings
Submitter Bharat Bhushan
Date June 10, 2013, 7:55 a.m.
Message ID <1370850918-18687-1-git-send-email-Bharat.Bhushan@freescale.com>
Download mbox | patch
Permalink /patch/250189/
State New
Headers show

Comments

Bharat Bhushan - June 10, 2013, 7:55 a.m.
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(-)
Peter Maydell - June 10, 2013, 9:26 a.m.
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
Bharat Bhushan - June 10, 2013, 9:53 a.m.
> -----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
Andreas Färber - June 10, 2013, 12:13 p.m.
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);
>  
>
Bharat Bhushan - June 10, 2013, 12:47 p.m.
> -----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
Alexander Graf - June 10, 2013, 2:26 p.m.
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

Patch

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);