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:address@hidden
> > Sent: Tuesday, June 11, 2013 6:27 PM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; Andreas Färber; address@hidden; qemu-
> > address@hidden
> > 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:address@hidden
> > > > Sent: Tuesday, June 11, 2013 6:10 PM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: Wood Scott-B07421; Andreas Färber; address@hidden; qemu-
> > > > address@hidden
> > > > 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: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.). */
> > > > 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