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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Monday, June 10, 2013 11:40 PM
> To: Wood Scott-B07421
> Cc: Bhushan Bharat-R65777; Andreas Färber; address@hidden; qemu-
> address@hidden; 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:address@hidden
> >>>> Sent: Monday, June 10, 2013 5:43 PM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: address@hidden; address@hidden; address@hidden; 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
>