Re: [Qemu-devel] [PATCH] replaced get_ticks_per_sec() with constant

[Stefan Hajnoczi]

On Tue, Mar 31, 2015 at 09:46:32AM +1000, Peter Crosthwaite wrote:
> On Tue, Mar 31, 2015 at 7:52 AM, Peter Maydell <address@hidden> wrote:
> > On 30 March 2015 at 14:01, Stefan Hajnoczi <address@hidden> wrote:
> >> On Sat, Mar 28, 2015 at 06:22:11PM +0200, Emil Condrea wrote:
> >>> diff --git a/target-arm/helper.c b/target-arm/helper.c
> >>> index 10886c5..504530a 100644
> >>> --- a/target-arm/helper.c
> >>> +++ b/target-arm/helper.c
> >>> @@ -649,7 +649,7 @@ void pmccntr_sync(CPUARMState *env)
> >>>      uint64_t temp_ticks;
> >>>
> >>>      temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> >>> -                          get_ticks_per_sec(), 1000000);
> >>> +                          NSEC_PER_SEC, 1000000);
> >>>
> >>>      if (env->cp15.c9_pmcr & PMCRD) {
> >>>          /* Increment once every 64 processor clock cycles */
> >>> @@ -688,7 +688,7 @@ static uint64_t pmccntr_read(CPUARMState *env, const 
> >>> ARMCPRegInfo *ri)
> >>>      }
> >>>
> >>>      total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> >>> -                           get_ticks_per_sec(), 1000000);
> >>> +                           NSEC_PER_SEC, 1000000);
> >>>
> >>>      if (env->cp15.c9_pmcr & PMCRD) {
> >>>          /* Increment once every 64 processor clock cycles */
> >>> @@ -709,7 +709,7 @@ static void pmccntr_write(CPUARMState *env, const 
> >>> ARMCPRegInfo *ri,
> >>>      }
> >>>
> >>>      total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> >>> -                           get_ticks_per_sec(), 1000000);
> >>> +                           NSEC_PER_SEC, 1000000);
> >>
> >> Peter: I wonder why the muldiv64() expression is necessary.  Is it
> >> intentionally returning the microsecond clock converted to nanoseconds?
> >>
> 
> That is the effect but not the intention. In this context,
> get_tick_per_sec() is really trying to be a clock signal frequency
> rate and QEMU ARM is just hardcoding to 1GHz. Because its fixed and in
> terms of real time, your proposed cancellation is possible.
> 
> For ARM CPU we are simplifying the CPU clock pin as ns (basically 1GHz
> clock). Ideally this should be parameterisable as this clock is
> controlled outside of the ARM processor.
> 
> I think the form of the current code has value for self documentation,
> and maybe should have a comment explaining this 1GHz harcodedness.
> When we get around to parameterising the ARM CPU frequency as a QOM
> property this code should be fixed by doing:
> 
> s/NSEC_PER_SEC/s->clock_freq_hz/
> 
> So this muldiv in its current form is really preparation for that. Can
> we keep it? Your patch as-is looks good to me.
> 
> Acked-by: Peter Crosthwaite <address@hidden>

Thanks for explaining.

Stefan

pgp9nq9tHMQJw.pgp
Description: PGP signature