Re: [Qemu-devel] [RFC 5/5] arm: Simplify cycle counter

[Christopher Covington]

Hi Peter,

Thanks for taking a look.

On Apr 30, 2015 2:28 PM, "Peter Maydell" <address@hidden> wrote:
>
> On 30 April 2015 at 19:14, Christopher Covington
> <address@hidden> wrote:
> > Present a system with an instructions per cycle of exactly one.
> > This makes it less likely a user will mistake the cycle counter
> > values as meaningful and makes calculations involving cycles
> > trivial while preserving the necessary property of the cycle
> > counter register as monotonically increasing.
> >
> > Signed-off-by: Christopher Covington <address@hidden>
> > ---
> >  target-arm/helper.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 3e6fb0b..a027a19 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -648,8 +648,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);
> > +    temp_ticks = cpu_get_icount_raw();
>
> Are you really really sure the _raw function is the correct one?
> Nowhere else in the codebase calls it except the other wrappers
> in cpu.c which provide a sane view of the instruction count...
> (I suspect cpu_get_icount_raw() should really be static.)

I thought it wasn't being used because it was new, having been introduced by Pavel Dovgalyuk's determinism patches.

When you talk about sanity, what would this patch make insane? The instructions per second and cycles per second that would result? If so, what if seconds/timer ticks were changed in the same patch to be derived from the instruction count. All of this could potentially only apply with -icount specified.

> PS: it would be helpful if you could make sure you include
> a cover letter for future patchseries: patchsets without a
> cover letter are awkward for my workflow... (A single
> standalone patch doesn't need a coverletter.)

Will do. For this series it should have been that the first four patches are hopefully not controversial, but so far have only been tested by some very simple bare metal code (using PSCI exit instead of Angel exit at the end) and booting the Linux kernel and looking at the printks, but not yet by running `perf stat`. The last patch (this one) I realize may be controversial as it tries to make QEMU conform to certain expectations such as determinism that it has not historically met.

Thanks,
Chris