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: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
> >
> > 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<address@hidden>
> > > ---
> > > 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