[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for
From: |
Bhushan Bharat-R65777 |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61 |
Date: |
Tue, 11 Jun 2013 13:18:26 +0000 |
> -----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?
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-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, (continued)
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Andreas Färber, 2013/06/10
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Bhushan Bharat-R65777, 2013/06/10
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Alexander Graf, 2013/06/10
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Scott Wood, 2013/06/10
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Alexander Graf, 2013/06/10
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Bhushan Bharat-R65777, 2013/06/11
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Alexander Graf, 2013/06/11
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Bhushan Bharat-R65777, 2013/06/11
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Alexander Graf, 2013/06/11
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61,
Bhushan Bharat-R65777 <=
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61, Alexander Graf, 2013/06/11