[Qemu-devel] [RFC 1/5] arm64: Add PMINTENCLR_EL1

[Qemu-devel] [RFC 1/5] arm64: Add PMINTENCLR_EL1

[Christopher Covington]

The Linux kernel accesses this register early in its setup.

Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
---
 target-arm/helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d77c6de..6aeb77c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -954,6 +954,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL1_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .resetvalue = 0, .writefn = pmintenclr_write, },
+    { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
+      .access = PL1_RW, .type = ARM_CP_ALIAS,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
+      .resetvalue = 0, .writefn = pmintenclr_write, },
     { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .writefn = vbar_write,
-- 
1.9.1

[Qemu-devel] [RFC 2/5] arm64: Add PMOVSCLR_EL0 register

[Christopher Covington]

The Linux kernel accesses this register early in its setup.

Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
---
 target-arm/helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 6aeb77c..c9463cb 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -904,6 +904,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .accessfn = pmreg_access,
       .writefn = pmovsr_write,
       .raw_writefn = raw_write },
+    { .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3,
+      .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+      .accessfn = pmreg_access,
+      .writefn = pmovsr_write,
+      .raw_writefn = raw_write },
     /* Unimplemented so WI. */
     { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
       .access = PL0_W, .accessfn = pmreg_access, .type = ARM_CP_NOP },
-- 
1.9.1

[Qemu-devel] [RFC 3/5] arm64: Add PMUSERENR_EL0 register

[Christopher Covington]

The Linux kernel accesses this register early in its setup.

Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
---
 target-arm/helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index c9463cb..863cfd0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -951,6 +951,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
       .resetvalue = 0,
       .writefn = pmuserenr_write, .raw_writefn = raw_write },
+    { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 0,
+      .access = PL0_R | PL1_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
+      .resetvalue = 0,
+      .writefn = pmuserenr_write, .raw_writefn = raw_write },
     { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
       .access = PL1_RW,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
-- 
1.9.1

[Qemu-devel] [RFC 4/5] arm64: Unmask PMU bits in debug feature register

[Christopher Covington]

The previously missing registers are now present in QEMU.

Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
---
 target-arm/helper.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 863cfd0..3e6fb0b 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3149,12 +3149,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
               .access = PL1_R, .type = ARM_CP_CONST,
-              /* We mask out the PMUVer field, because we don't currently
-               * implement the PMU. Not advertising it prevents the guest
-               * from trying to use it and getting UNDEFs on registers we
-               * don't implement.
-               */
-              .resetvalue = cpu->id_aa64dfr0 & ~0xf00 },
+              .resetvalue = cpu->id_aa64dfr0 },
             { .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
-- 
1.9.1

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

[Christopher Covington]

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 <christopher.covington@linaro.org>
---
 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();
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
@@ -687,8 +686,7 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
         return env->cp15.c15_ccnt;
     }
 
-    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
-                           get_ticks_per_sec(), 1000000);
+    total_ticks = cpu_get_icount_raw();
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
@@ -708,8 +706,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
         return;
     }
 
-    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
-                           get_ticks_per_sec(), 1000000);
+    total_ticks = cpu_get_icount_raw();
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
-- 
1.9.1

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

[Peter Maydell]

On 30 April 2015 at 19:14, Christopher Covington
<christopher.covington@linaro.org> 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 <christopher.covington@linaro.org>
> ---
>  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.)

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.)

thanks
-- PMM

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

[Christopher Covington]

[-- Attachment #1: Type: text/plain, Size: 2553 bytes --]

Hi Peter,

Thanks for taking a look.

On Apr 30, 2015 2:28 PM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> On 30 April 2015 at 19:14, Christopher Covington
> <christopher.covington@linaro.org> 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 <christopher.covington@linaro.org>
> > ---
> >  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

[-- Attachment #2: Type: text/html, Size: 3251 bytes --]

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

[Peter Maydell]

On 30 April 2015 at 22:33, Christopher Covington
<christopher.covington@linaro.org> wrote:
> On Apr 30, 2015 2:28 PM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>> 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.

I don't really know for certain how the code here works, but
it makes me very dubious when I see a function that is being
used literally nowhere else in any other target CPU, and
where every other code path to it goes via taking a lock.

Paolo: can you suggest what the right function to call to implement
a cycle counter is?

thanks
-- PMM

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

[Peter Crosthwaite]

On Thu, Apr 30, 2015 at 11:14 AM, Christopher Covington
<christopher.covington@linaro.org> 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 <christopher.covington@linaro.org>
> ---
>  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();

So I have guests (for better or worse) that make assumptions about the
rate of the PMCCNTR WRT real time. But isn't the PMCCNTR really a
clock cycle counter rather than an instruction counter? That clock
rate is well defined even if it is just the trivial
get_ticks_per_sec() at the moment. Ideally we should have a
configurable clock rate in there instead of get_ticks_per_sec(). This
is a major change in definition.

I can see your use case though, where you actually want this to mean
something WRT program performance. Should we add a switch between the
two behaviours?

Regards,
Peter

>
>      if (env->cp15.c9_pmcr & PMCRD) {
>          /* Increment once every 64 processor clock cycles */
> @@ -687,8 +686,7 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>          return env->cp15.c15_ccnt;
>      }
>
> -    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> -                           get_ticks_per_sec(), 1000000);
> +    total_ticks = cpu_get_icount_raw();
>
>      if (env->cp15.c9_pmcr & PMCRD) {
>          /* Increment once every 64 processor clock cycles */
> @@ -708,8 +706,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>          return;
>      }
>
> -    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> -                           get_ticks_per_sec(), 1000000);
> +    total_ticks = cpu_get_icount_raw();
>
>      if (env->cp15.c9_pmcr & PMCRD) {
>          /* Increment once every 64 processor clock cycles */
> --
> 1.9.1
>
>

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

[Christopher Covington]

On Thu, Apr 30, 2015 at 9:24 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Apr 30, 2015 at 11:14 AM, Christopher Covington
> <christopher.covington@linaro.org> 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 <christopher.covington@linaro.org>
>> ---
>>  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();
>
> So I have guests (for better or worse) that make assumptions about the
> rate of the PMCCNTR WRT real time. But isn't the PMCCNTR really a
> clock cycle counter rather than an instruction counter? That clock
> rate is well defined even if it is just the trivial
> get_ticks_per_sec() at the moment. Ideally we should have a
> configurable clock rate in there instead of get_ticks_per_sec(). This
> is a major change in definition.

Is this with KVM, user emulation, or system emulation mode? I'm mainly
looking at things from the TCG (and mostly system mode) perspective.
If not using KVM, would the assumptions of your guests hold if you ran
them on a hardware device instead of QEMU TCG? I'm still trying to
understand all the code involved, but I don't see get_ticks_per_sec()
or any other constants as problematic.

> I can see your use case though, where you actually want this to mean
> something WRT program performance. Should we add a switch between the
> two behaviours?

This switch already exists, in the form of -icount, and I really
should have already been using it. What initially scared me away was
qemu_icount_bias and icount_adjust(). Accounting for halting seems
like a good thing but the accounting for "speed" by referencing a host
clock doesn't make sense to me. If I have an ARM development board
hooked up to an x86 desktop, it's not querying the desktop for time.
So why does it make sense for QEMU system emulation mode behave like
that? The -icount help text makes it sound like adjustment for speed
reasons should only take place when "auto" is specified, but I have
yet to understand the code well enough to verify that.

Chris

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

[Paolo Bonzini]

On 01/05/2015 00:02, Peter Maydell wrote:
> On 30 April 2015 at 22:33, Christopher Covington
> <christopher.covington@linaro.org> wrote:
>> On Apr 30, 2015 2:28 PM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>>> 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 right function is cpu_get_ticks().  This one works without icount as
well.

At the top there is:

    if (use_icount) {
        return cpu_get_icount();
    }

and perhaps it would be correct to use cpu_get_icount_raw() if you do
not want the cycle counter to increase in halted state.  But at least
x86 wants it to increase.

Paolo

 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.
> 
> I don't really know for certain how the code here works, but
> it makes me very dubious when I see a function that is being
> used literally nowhere else in any other target CPU, and
> where every other code path to it goes via taking a lock.
> 
> Paolo: can you suggest what the right function to call to implement
> a cycle counter is?
> 
> thanks
> -- PMM
> 

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

[Peter Crosthwaite]

On Fri, May 1, 2015 at 7:35 AM, Christopher Covington
<christopher.covington@linaro.org> wrote:
> On Thu, Apr 30, 2015 at 9:24 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Thu, Apr 30, 2015 at 11:14 AM, Christopher Covington
>> <christopher.covington@linaro.org> 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 <christopher.covington@linaro.org>
>>> ---
>>>  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();
>>
>> So I have guests (for better or worse) that make assumptions about the
>> rate of the PMCCNTR WRT real time. But isn't the PMCCNTR really a
>> clock cycle counter rather than an instruction counter? That clock
>> rate is well defined even if it is just the trivial
>> get_ticks_per_sec() at the moment. Ideally we should have a
>> configurable clock rate in there instead of get_ticks_per_sec(). This
>> is a major change in definition.
>
> Is this with KVM, user emulation, or system emulation mode? I'm mainly
> looking at things from the TCG (and mostly system mode) perspective.

The same. My guest has only ever run on TCG.

> If not using KVM, would the assumptions of your guests hold if you ran
> them on a hardware device instead of QEMU TCG?

I run them as a matter of course on hardware devices.

What I am most worried about (and I need to run some tests to really
confirm facts) is what happens if a CPU WFIs. Should the PMCCNTR
continue on or hold its value? If we match instruction execution to
PMCCNTR to the PMCCNTR will freeze.

Regards,
Peter

> I'm still trying to
> understand all the code involved, but I don't see get_ticks_per_sec()
> or any other constants as problematic.
>
>> I can see your use case though, where you actually want this to mean
>> something WRT program performance. Should we add a switch between the
>> two behaviours?
>
> This switch already exists, in the form of -icount, and I really
> should have already been using it. What initially scared me away was
> qemu_icount_bias and icount_adjust(). Accounting for halting seems
> like a good thing but the accounting for "speed" by referencing a host
> clock doesn't make sense to me. If I have an ARM development board
> hooked up to an x86 desktop, it's not querying the desktop for time.
> So why does it make sense for QEMU system emulation mode behave like
> that? The -icount help text makes it sound like adjustment for speed
> reasons should only take place when "auto" is specified, but I have
> yet to understand the code well enough to verify that.
>
> Chris
>

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

[Peter Maydell]

On 6 May 2015 at 15:05, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> What I am most worried about (and I need to run some tests to really
> confirm facts) is what happens if a CPU WFIs. Should the PMCCNTR
> continue on or hold its value? If we match instruction execution to
> PMCCNTR to the PMCCNTR will freeze.

See the v8 ARM ARM D5.1.1: this doesn't count PE clock cycles, it's
linked to the hardware processor clock. The exact relationship is
IMPDEF so we have some leeway for doing whatever seems reasonable here.
Permitted things:
 * counter can stop on WFI (see D5.1.3)
 * counter can continue to run on WFI
   (it's impdef whether the PE clock is gated when in WFI, though
   I would expect that to be a popular implementation)
 * counter can read the same value if read twice in a row
 * counter can run forward a lot even if no insns executed
   (the example given is of a hyperthreading implementation)

So we should do whatever seems most convenient implementation-wise
I think...

-- PMM

[Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Christopher Covington]

cpu_get_ticks() provides a common interface across targets for
calculating CPU cycles. Using this fixes PMCCNTR reads when -icount
is specified (previously a non-increasing value was returned).

Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
 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 7dc49cb..32923fb 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -729,8 +729,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_ticks();
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
@@ -768,8 +767,7 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
         return env->cp15.c15_ccnt;
     }
 
-    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
-                           get_ticks_per_sec(), 1000000);
+    total_ticks = cpu_get_ticks();
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
@@ -789,8 +787,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
         return;
     }
 
-    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
-                           get_ticks_per_sec(), 1000000);
+    total_ticks = cpu_get_ticks();
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Alistair Francis]

On Thu, Sep 24, 2015 at 12:43 PM, Christopher Covington
<cov@codeaurora.org> wrote:
> cpu_get_ticks() provides a common interface across targets for
> calculating CPU cycles. Using this fixes PMCCNTR reads when -icount
> is specified (previously a non-increasing value was returned).
>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> ---
>  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 7dc49cb..32923fb 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -729,8 +729,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_ticks();

This patch doesn't apply anymore, you will need to rebase it.

Also I don't think this is correct. cpu_get_ticks() returns the host
CPU cycle counter, when in this case we want the guest cycles.

Thanks,

Alistair

>
>      if (env->cp15.c9_pmcr & PMCRD) {
>          /* Increment once every 64 processor clock cycles */
> @@ -768,8 +767,7 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>          return env->cp15.c15_ccnt;
>      }
>
> -    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> -                           get_ticks_per_sec(), 1000000);
> +    total_ticks = cpu_get_ticks();
>
>      if (env->cp15.c9_pmcr & PMCRD) {
>          /* Increment once every 64 processor clock cycles */
> @@ -789,8 +787,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>          return;
>      }
>
> -    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> -                           get_ticks_per_sec(), 1000000);
> +    total_ticks = cpu_get_ticks();
>
>      if (env->cp15.c9_pmcr & PMCRD) {
>          /* Increment once every 64 processor clock cycles */
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Christopher Covington]

On 09/28/2015 06:05 PM, Alistair Francis wrote:
> On Thu, Sep 24, 2015 at 12:43 PM, Christopher Covington
> <cov@codeaurora.org> wrote:
>> cpu_get_ticks() provides a common interface across targets for
>> calculating CPU cycles. Using this fixes PMCCNTR reads when -icount
>> is specified (previously a non-increasing value was returned).
>>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>> ---
>>  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 7dc49cb..32923fb 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -729,8 +729,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_ticks();

> Also I don't think this is correct. cpu_get_ticks() returns the host
> CPU cycle counter, when in this case we want the guest cycles.

I too find the use of host CPU cycles quite perplexing. Paolo suggested it
[1]. Maybe there are timeouts in some software that tend to work better in
such a mode. Perhaps it is faster, although my intuition is that it's just
providing a facade of frequency scaling to the guest.

1. https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00162.html

I like to declare guest instructions per guest CPU cycles = 1. As I understand
it, an "-icount 0" pair of parameters is how to do this in QEMU for x86. I'd
like it to work for ARM.

I wrote a simple assembly test case which I'm working on porting it to the
kvm-unit-tests framework. In the non-icount case, I saw roughly the same order
of magnitude for guest IPC before and after the patch. I'd like to also write
CPU frequency (guest CPU cycles per generic timer guest seconds) and (M)IPS
(guest instructions per generic timer guest seconds) tests.

Thanks,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Christopher Covington]

On 09/29/2015 10:07 AM, Christopher Covington wrote:
> On 09/28/2015 06:05 PM, Alistair Francis wrote:
>> On Thu, Sep 24, 2015 at 12:43 PM, Christopher Covington
>> <cov@codeaurora.org> wrote:
>>> cpu_get_ticks() provides a common interface across targets for
>>> calculating CPU cycles. Using this fixes PMCCNTR reads when -icount
>>> is specified (previously a non-increasing value was returned).
>>>
>>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>> ---
>>>  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 7dc49cb..32923fb 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -729,8 +729,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_ticks();
> 
>> Also I don't think this is correct. cpu_get_ticks() returns the host
>> CPU cycle counter, when in this case we want the guest cycles.
> 
> I too find the use of host CPU cycles quite perplexing. Paolo suggested it
> [1]. Maybe there are timeouts in some software that tend to work better in
> such a mode. Perhaps it is faster, although my intuition is that it's just
> providing a facade of frequency scaling to the guest.
> 
> 1. https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00162.html
> 
> I like to declare guest instructions per guest CPU cycles = 1. As I understand
> it, an "-icount 0" pair of parameters is how to do this in QEMU for x86. I'd
> like it to work for ARM.
> 
> I wrote a simple assembly test case which I'm working on porting it to the
> kvm-unit-tests framework. In the non-icount case, I saw roughly the same order
> of magnitude for guest IPC before and after the patch. I'd like to also write
> CPU frequency (guest CPU cycles per generic timer guest seconds) and (M)IPS
> (guest instructions per generic timer guest seconds) tests.

I've sent out the CPI test case and while exercising it I noticed that
Laurent's patch fixed -icount. So my original goal has been accomplished. I'm
happy to rebase this patch if the authorities (Peter Maydell?) think using
cpu_get_ticks() is the right thing to do. Otherwise I'll probably try to move
on to support for the instructions event in the ARM PMU.

Thanks,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Maydell]

On 2 October 2015 at 17:44, Christopher Covington <cov@codeaurora.org> wrote:
> I've sent out the CPI test case and while exercising it I noticed that
> Laurent's patch fixed -icount. So my original goal has been accomplished. I'm
> happy to rebase this patch if the authorities (Peter Maydell?) think using
> cpu_get_ticks() is the right thing to do. Otherwise I'll probably try to move
> on to support for the instructions event in the ARM PMU.

Authority here is probably Peter Crosthwaite. I can produce an
opinion but I'd have to go and research a bunch of stuff to do
that, so I'm hoping to avoid it...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Crosthwaite]

On Fri, Oct 2, 2015 at 9:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 October 2015 at 17:44, Christopher Covington <cov@codeaurora.org> wrote:
>> I've sent out the CPI test case and while exercising it I noticed that
>> Laurent's patch fixed -icount. So my original goal has been accomplished. I'm
>> happy to rebase this patch if the authorities (Peter Maydell?) think using
>> cpu_get_ticks() is the right thing to do. Otherwise I'll probably try to move
>> on to support for the instructions event in the ARM PMU.
>
> Authority here is probably Peter Crosthwaite. I can produce an
> opinion but I'd have to go and research a bunch of stuff to do
> that, so I'm hoping to avoid it...
>

So my idea here is the CPU input frequency should be a property of the CPU.

Some experimental results confirm that the PMCCNTR on many common ARM
implementations is directly connected to the input clock and can be
relied on as a straight free-running counter. I think a genuine
instruction counter is something else and this timer should be
independent of any core provider of cycle count.

Regards,
Peter

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Maydell]

On 2 October 2015 at 18:25, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> So my idea here is the CPU input frequency should be a property of the CPU.
>
> Some experimental results confirm that the PMCCNTR on many common ARM
> implementations is directly connected to the input clock and can be
> relied on as a straight free-running counter. I think a genuine
> instruction counter is something else and this timer should be
> independent of any core provider of cycle count.

Architecturally, the PMCCNTR counter is counting the hardware processor
clock. It's definitely not an instruction counter. (It's also not
counting Processor Element clock cycles, though that only makes a
difference if you have a multi-threaded hw implementation.) It is
generally subject to any hw changes in clock frequency (including if
your WFI/WFE do clock stopping).

What that means for QEMU I'm not totally sure :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Crosthwaite]

On Fri, Oct 2, 2015 at 11:08 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 October 2015 at 18:25, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> So my idea here is the CPU input frequency should be a property of the CPU.
>>
>> Some experimental results confirm that the PMCCNTR on many common ARM
>> implementations is directly connected to the input clock and can be
>> relied on as a straight free-running counter. I think a genuine
>> instruction counter is something else and this timer should be
>> independent of any core provider of cycle count.
>
> Architecturally, the PMCCNTR counter is counting the hardware processor
> clock. It's definitely not an instruction counter. (It's also not
> counting Processor Element clock cycles, though that only makes a
> difference if you have a multi-threaded hw implementation.) It is
> generally subject to any hw changes in clock frequency (including if
> your WFI/WFE do clock stopping).
>

WFI/WFE halting could be easily implemented directly as that, in
similar way to EL filtering.

> What that means for QEMU I'm not totally sure :-)

I think this all points to it being just another normal timer (like
those in hw/timer).

Regards,
Peter

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Christopher Covington]

On 10/02/2015 01:25 PM, Peter Crosthwaite wrote:
> On Fri, Oct 2, 2015 at 9:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 2 October 2015 at 17:44, Christopher Covington <cov@codeaurora.org> wrote:
>>> I've sent out the CPI test case and while exercising it I noticed that
>>> Laurent's patch fixed -icount. So my original goal has been accomplished. I'm
>>> happy to rebase this patch if the authorities (Peter Maydell?) think using
>>> cpu_get_ticks() is the right thing to do. Otherwise I'll probably try to move
>>> on to support for the instructions event in the ARM PMU.
>>
>> Authority here is probably Peter Crosthwaite. I can produce an
>> opinion but I'd have to go and research a bunch of stuff to do
>> that, so I'm hoping to avoid it...
> 
> So my idea here is the CPU input frequency should be a property of the CPU.
> 
> Some experimental results confirm that the PMCCNTR on many common ARM
> implementations is directly connected to the input clock and can be
> relied on as a straight free-running counter. I think a genuine
> instruction counter is something else

Yes, the "genuine" instruction counter is something else. The instruction
count is only relevant for folks trying to get deterministic execution by
using the -icount option. QEMU TCG mode does not emulate a cycle-level input
clock for the guest (the whole class of functional models skip this
time-consuming step) but rather operates a block at a time. By doing a little
extra, I think it also interpolates the exact instruction count. Specifying a
fixed IPC = n is the most sensible way of deterministically calculating a
PMCCNTR_EL0 value that I know of. The -icount option allows users to choose
such deterministic behavior.

> and this timer should be independent of any core provider of cycle count.

What, if anything, do you think should be hooked up to the core provider of
cycle count?

Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Crosthwaite]

On Fri, Oct 2, 2015 at 12:25 PM, Christopher Covington
<cov@codeaurora.org> wrote:
> On 10/02/2015 01:25 PM, Peter Crosthwaite wrote:
>> On Fri, Oct 2, 2015 at 9:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 2 October 2015 at 17:44, Christopher Covington <cov@codeaurora.org> wrote:
>>>> I've sent out the CPI test case and while exercising it I noticed that
>>>> Laurent's patch fixed -icount. So my original goal has been accomplished. I'm
>>>> happy to rebase this patch if the authorities (Peter Maydell?) think using
>>>> cpu_get_ticks() is the right thing to do. Otherwise I'll probably try to move
>>>> on to support for the instructions event in the ARM PMU.
>>>
>>> Authority here is probably Peter Crosthwaite. I can produce an
>>> opinion but I'd have to go and research a bunch of stuff to do
>>> that, so I'm hoping to avoid it...
>>
>> So my idea here is the CPU input frequency should be a property of the CPU.
>>
>> Some experimental results confirm that the PMCCNTR on many common ARM
>> implementations is directly connected to the input clock and can be
>> relied on as a straight free-running counter. I think a genuine
>> instruction counter is something else
>
> Yes, the "genuine" instruction counter is something else. The instruction
> count is only relevant for folks trying to get deterministic execution by
> using the -icount option. QEMU TCG mode does not emulate a cycle-level input
> clock for the guest (the whole class of functional models skip this
> time-consuming step) but rather operates a block at a time. By doing a little
> extra, I think it also interpolates the exact instruction count. Specifying a
> fixed IPC = n is the most sensible way of deterministically calculating a
> PMCCNTR_EL0 value that I know of. The -icount option allows users to choose
> such deterministic behavior.
>
>> and this timer should be independent of any core provider of cycle count.
>
> What, if anything, do you think should be hooked up to the core provider of
> cycle count?
>

Depends, Is this a virtual-machine only concept, or do you have
something with a real-hardware analogue?

Regards,
Peter

> Christopher Covington
>
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Christopher Covington]

On 10/02/2015 03:56 PM, Peter Crosthwaite wrote:
> On Fri, Oct 2, 2015 at 12:25 PM, Christopher Covington
> <cov@codeaurora.org> wrote:
>> On 10/02/2015 01:25 PM, Peter Crosthwaite wrote:
>>> On Fri, Oct 2, 2015 at 9:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 2 October 2015 at 17:44, Christopher Covington <cov@codeaurora.org> wrote:
>>>>> I've sent out the CPI test case and while exercising it I noticed that
>>>>> Laurent's patch fixed -icount. So my original goal has been accomplished. I'm
>>>>> happy to rebase this patch if the authorities (Peter Maydell?) think using
>>>>> cpu_get_ticks() is the right thing to do. Otherwise I'll probably try to move
>>>>> on to support for the instructions event in the ARM PMU.
>>>>
>>>> Authority here is probably Peter Crosthwaite. I can produce an
>>>> opinion but I'd have to go and research a bunch of stuff to do
>>>> that, so I'm hoping to avoid it...
>>>
>>> So my idea here is the CPU input frequency should be a property of the CPU.
>>>
>>> Some experimental results confirm that the PMCCNTR on many common ARM
>>> implementations is directly connected to the input clock and can be
>>> relied on as a straight free-running counter. I think a genuine
>>> instruction counter is something else
>>
>> Yes, the "genuine" instruction counter is something else. The instruction
>> count is only relevant for folks trying to get deterministic execution by
>> using the -icount option. QEMU TCG mode does not emulate a cycle-level input
>> clock for the guest (the whole class of functional models skip this
>> time-consuming step) but rather operates a block at a time. By doing a little
>> extra, I think it also interpolates the exact instruction count. Specifying a
>> fixed IPC = n is the most sensible way of deterministically calculating a
>> PMCCNTR_EL0 value that I know of. The -icount option allows users to choose
>> such deterministic behavior.
>>
>>> and this timer should be independent of any core provider of cycle count.
>>
>> What, if anything, do you think should be hooked up to the core provider of
>> cycle count?
> 
> Depends, Is this a virtual-machine only concept, or do you have
> something with a real-hardware analogue?

What I meant to ask was, do you see any reason for cpu_get_ticks() to exist?
If no architecture besides i386 wants to use it, perhaps the code should be
moved there.

Thanks,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Maydell]

On 2 October 2015 at 21:48, Christopher Covington <cov@codeaurora.org> wrote:
> What I meant to ask was, do you see any reason for cpu_get_ticks() to exist?
> If no architecture besides i386 wants to use it, perhaps the code should be
> moved there.

OTOH various non-x86 things do use the closely related cpu_get_real_ticks(),
and the implementation of cpu_get_ticks() is very closely related to
the other clock code in cpus.c.

-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Paolo Bonzini]

On 03/10/2015 00:41, Peter Maydell wrote:
> > What I meant to ask was, do you see any reason for cpu_get_ticks() to exist?
> > If no architecture besides i386 wants to use it, perhaps the code should be
> > moved there.
>
> OTOH various non-x86 things do use the closely related cpu_get_real_ticks(),
> and the implementation of cpu_get_ticks() is very closely related to
> the other clock code in cpus.c.

cpu_get_real_ticks() is returning the host cycle counter;
cpu_get_ticks() is stopping/resuming it when the VM is stopped/resumed.
 In other words, cpu_get_real_ticks() is to cpu_get_ticks() what
QEMU_CLOCK_REALTIME is to QEMU_CLOCK_VIRTUAL.

They are also similar in that both cpu_get_ticks() and
QEMU_CLOCK_VIRTUAL "morph" to the instruction count in icount mode.

cpu_get_ticks() should be the right implementation for the ARM PMCCNTR
cycle counter, since: 1) PMCCNTR is roughly the same as the x86 RDTSC;
2) cpu_get_ticks() is the only monotonically increasing value outside
icount mode.

Paolo

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Maydell]

On 5 October 2015 at 15:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 03/10/2015 00:41, Peter Maydell wrote:
>> > What I meant to ask was, do you see any reason for cpu_get_ticks() to exist?
>> > If no architecture besides i386 wants to use it, perhaps the code should be
>> > moved there.
>>
>> OTOH various non-x86 things do use the closely related cpu_get_real_ticks(),
>> and the implementation of cpu_get_ticks() is very closely related to
>> the other clock code in cpus.c.
>
> cpu_get_real_ticks() is returning the host cycle counter;
> cpu_get_ticks() is stopping/resuming it when the VM is stopped/resumed.
>  In other words, cpu_get_real_ticks() is to cpu_get_ticks() what
> QEMU_CLOCK_REALTIME is to QEMU_CLOCK_VIRTUAL.

...but it seems wrong to have anything in the simulation care
about the host cycle counter, especially since on some hosts
the underlying implementation is terrible.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Paolo Bonzini]

On 05/10/2015 16:11, Peter Maydell wrote:
> > > OTOH various non-x86 things do use the closely related cpu_get_real_ticks(),
> > > and the implementation of cpu_get_ticks() is very closely related to
> > > the other clock code in cpus.c.
> >
> > cpu_get_real_ticks() is returning the host cycle counter;
> > cpu_get_ticks() is stopping/resuming it when the VM is stopped/resumed.
> >  In other words, cpu_get_real_ticks() is to cpu_get_ticks() what
> > QEMU_CLOCK_REALTIME is to QEMU_CLOCK_VIRTUAL.
>
> ...but it seems wrong to have anything in the simulation care
> about the host cycle counter,

I agree; instruction count is much better.  Unfortunately, -icount has a
relatively hefty performance penalty.  It is also common that code using
PMCCNTR/RDTSC wants the increment between two reads to be large-ish and
at least remotely related to the number of instructions that were
executed in between.

I've also used rdtsc in the guest from time to time to benchmark changes
to TCG. Having it map to the host cycle counter is somewhat useful for
that purpose, though one might say this usecase is niche.

> especially since on some hosts
> the underlying implementation is terrible.

I remember seeing a bug where this terrible implementation caused
divisions by zero on the host.  The default implementation in
include/qemu/timer.h should be changed to
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL).

Note that in practice this terrible implementation is only used on
ARM/AArch64.  Is PMCCNTR or something similar accessible from userspace?
 I guess no, since even write access is enabled via PMUSERENR (and in
general PMUSERENR ought to be false on a preemptive-multitasking OS).

In the end I guess qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) would work just
as well for PMCCNTR, but I think non-Linux OSes still have a relatively
slow clock_gettime() so there is still an advantage in using
cpu_get_ticks().

Paolo

ps: on x86, a long time ago rdtsc was reliable and clock_gettime() was
slow, so it was a no-brainer for benchmarks and the like; then rdtsc
became unreliable and clock_gettime() became fast on Linux; but now at
least on new machines rdtsc is usually reliable again.

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Maydell]

On 5 October 2015 at 15:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
> I remember seeing a bug where this terrible implementation caused
> divisions by zero on the host.  The default implementation in
> include/qemu/timer.h should be changed to
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL).
>
> Note that in practice this terrible implementation is only used on
> ARM/AArch64.  Is PMCCNTR or something similar accessible from userspace?
>  I guess no, since even write access is enabled via PMUSERENR (and in
> general PMUSERENR ought to be false on a preemptive-multitasking OS).

Yeah, there's no guarantee on userspace access. I think the
fastest way to get some kind of count is to use a library
function that boils down to gettimeofday(2), which we will
do purely in userspace in the kernel VDSO if possible.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Paolo Bonzini]

On 06/10/2015 14:49, Peter Maydell wrote:
> Yeah, there's no guarantee on userspace access. I think the
> fastest way to get some kind of count is to use a library
> function that boils down to gettimeofday(2), which we will
> do purely in userspace in the kernel VDSO if possible.

Could we just use CNTVCT_EL0?  Which cores have it?

Paolo

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Maydell]

On 6 October 2015 at 13:58, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 06/10/2015 14:49, Peter Maydell wrote:
>> Yeah, there's no guarantee on userspace access. I think the
>> fastest way to get some kind of count is to use a library
>> function that boils down to gettimeofday(2), which we will
>> do purely in userspace in the kernel VDSO if possible.

Looking closer, clock_gettime() also has a userspace
only fastpath in the VDSO.

> Could we just use CNTVCT_EL0?  Which cores have it?

Not guaranteed to be enabled for userspace access by the kernel,
and even if it is enabled, the kernel folks don't (last time I
checked) consider this userspace ABI -- it's only there for
the benefit of the VDSO. (CNTVCT_EL0 is how the fast clock_gettime
and getimeofday paths are implemented.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Paolo Bonzini]

On 06/10/2015 15:06, Peter Maydell wrote:
> Looking closer, clock_gettime() also has a userspace
> only fastpath in the VDSO.

Yup, hence the patch that changed the default cpu_get_real_ticks()
implementation to get_clock().

>> > Could we just use CNTVCT_EL0?  Which cores have it?
>
> Not guaranteed to be enabled for userspace access by the kernel,
> and even if it is enabled, the kernel folks don't (last time I
> checked) consider this userspace ABI -- it's only there for
> the benefit of the VDSO.

Ok, thanks.

> (CNTVCT_EL0 is how the fast clock_gettime
> and getimeofday paths are implemented.)

Yes.

Paolo

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Peter Maydell]

On 24 September 2015 at 20:43, Christopher Covington <cov@codeaurora.org> wrote:
> cpu_get_ticks() provides a common interface across targets for
> calculating CPU cycles. Using this fixes PMCCNTR reads when -icount
> is specified (previously a non-increasing value was returned).
>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> ---
>  target-arm/helper.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

So, I think the conclusion from this thread was that we should
(a) change the default cpu_get_host_ticks() implementation to
call get_clock()
(b) rebase this patch and apply it

Is anybody planning to do that?

In any case, I'm taking this thread off my "must-review" list :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Use common CPU cycle infrastructure

[Christopher Covington]

On 10/13/2015 04:53 PM, Peter Maydell wrote:
> On 24 September 2015 at 20:43, Christopher Covington <cov@codeaurora.org> wrote:
>> cpu_get_ticks() provides a common interface across targets for
>> calculating CPU cycles. Using this fixes PMCCNTR reads when -icount
>> is specified (previously a non-increasing value was returned).
>>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>> ---
>>  target-arm/helper.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> So, I think the conclusion from this thread was that we should
> (a) change the default cpu_get_host_ticks() implementation to
> call get_clock()
> (b) rebase this patch and apply it
> 
> Is anybody planning to do that?

It's not in my plans at the moment.

Thanks,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project