| Message ID | 1370850918-18687-1-git-send-email-Bharat.Bhushan@freescale.com |
|---|---|
| State | New |
| Headers | show |
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
> -----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
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); > >
> -----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
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
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);
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(-)