[Qemu-devel] [PATCH V1 0/4] Add vPMU vPMU support under TCG mode

[Qemu-devel] [PATCH V1 0/4] Add vPMU vPMU support under TCG mode

[Wei Huang]

QEMU has implemented cycle count support for guest VM under TCG mode.
But this feature is not complete. In fact using perf inside a
64-bit Linux guest VM (under TCG) can cause the following kernel panic
because some PMU registers are not implemented.

[  329.445970] [<fffffe000009e600>] armv8pmu_enable_event+0x58/0x8c
[  329.446713] [<fffffe0000621e74>] armpmu_start+0x4c/0x74

This patchset solves the problem by adding support for missing vPMU
registers. Basic perf test can work now under TCG by applying this
patchset.

[root@localhost ~]# perf stat ls
 Performance counter stats for 'ls':

        226.740256      task-clock (msec)         #    0.312 CPUs utilized    
                76      context-switches          #    0.335 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                64      page-faults               #    0.282 K/sec
       186,031,410      cycles                    #    0.820 GHz      (36.40%)
   <not supported>      stalled-cycles-frontend
   <not supported>      stalled-cycles-backend
     <not counted>      instructions               (0.00%)
   <not supported>      branches
     <not counted>      branch-misses              (0.00%)

Thanks,
-Wei

Wei Huang (4):
  target-arm: Add support for PMU register PMSELR_EL0
  target-arm: Add support for AArch64 PMU register PMXEVTYPER_EL0
  target-arm: Add support for PMU register PMINTENSET_EL1
  target-arm: Hook up TCG vPMU with CPU pmu option

 target/arm/cpu.c    |  2 +-
 target/arm/cpu.h    |  1 +
 target/arm/helper.c | 41 +++++++++++++++++++++++++++++++++++------
 3 files changed, 37 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH V1 1/4] target-arm: Add support for PMU register PMSELR_EL0

[Wei Huang]

This patch adds support for AArch64 register PMSELR_EL0. The existing
PMSELR definition is revised accordingly.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 target/arm/cpu.h    |  1 +
 target/arm/helper.c | 24 +++++++++++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index ab119e6..bd80658 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -307,6 +307,7 @@ typedef struct CPUARMState {
         uint32_t c9_pmovsr; /* perf monitor overflow status */
         uint32_t c9_pmxevtyper; /* perf monitor event type */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
+        uint32_t c9_pmselr; /* perf monitor counter selection register */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
         union { /* Memory attribute redirection */
             struct {
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8dcabbf..71adb0f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -975,6 +975,15 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return total_ticks - env->cp15.c15_ccnt;
 }
 
+static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                         uint64_t value)
+{
+    /* only cycle counter selection is supported */
+    if (value == 0x1f) {
+        env->cp15.c9_pmselr = value;
+    }
+}
+
 static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
@@ -1194,12 +1203,17 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
     /* 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 },
-    /* Since we don't implement any events, writing to PMSELR is UNPREDICTABLE.
-     * We choose to RAZ/WI.
-     */
     { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
-      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
-      .accessfn = pmreg_access },
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
+      .accessfn = pmreg_access, .writefn = pmselr_write,
+      .raw_writefn = raw_write},
+    { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 5,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .type = ARM_CP_IO,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
+      .writefn = pmselr_write, .raw_writefn = raw_write, },
 #ifndef CONFIG_USER_ONLY
     { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
       .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V1 1/4] target-arm: Add support for PMU register PMSELR_EL0

[Peter Maydell]

On 12 January 2017 at 07:04, Wei Huang <wei@redhat.com> wrote:
> This patch adds support for AArch64 register PMSELR_EL0. The existing
> PMSELR definition is revised accordingly.
>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  target/arm/cpu.h    |  1 +
>  target/arm/helper.c | 24 +++++++++++++++++++-----
>  2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index ab119e6..bd80658 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -307,6 +307,7 @@ typedef struct CPUARMState {
>          uint32_t c9_pmovsr; /* perf monitor overflow status */
>          uint32_t c9_pmxevtyper; /* perf monitor event type */
>          uint32_t c9_pmuserenr; /* perf monitor user enable */
> +        uint32_t c9_pmselr; /* perf monitor counter selection register */
>          uint32_t c9_pminten; /* perf monitor interrupt enables */
>          union { /* Memory attribute redirection */
>              struct {
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 8dcabbf..71adb0f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -975,6 +975,15 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      return total_ticks - env->cp15.c15_ccnt;
>  }
>
> +static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                         uint64_t value)
> +{
> +    /* only cycle counter selection is supported */
> +    if (value == 0x1f) {
> +        env->cp15.c9_pmselr = value;
> +    }

This looks like it's trying to add PMUv2 (and ARMv8) semantics
for PMSELR, but it doesn't quite get them right.

In the old PMUv1 (which is what we currently implement),
0x1f was reserved and thus effectively writing anything to
PMSELR in a system with no implemented counters was UNPREDICTABLE.
>From PMUv2 (and including v8) 0x1f is supported for selecting
the cycle counter and must cause:
 * PMXEVTYPER reads and writes to access PMCCFILTR
 * PMXEVCNTR access to be CONSTRAINED UNPREDICTABLE (we already
   get this right because 'RAZ/WI' is one of the permitted choices)

The SEL field is 5 bits and the rest is RAZ/WI so we should
mask value with 0x1f first.

Writing something to PMSELR which isn't 0x1f but is greater than
or equal to the number of implemented counters (so for QEMU, anything
but 0x1f) is permitted, but makes PMXEVTYPER/PMXEVCNTR accesses
CONSTRAINED UNPREDICTABLE. So you need to allow pmselr to read
back as the non-0x1f value, and have the handling of "is this
0x1f or not" happen in the read/write functions for PMXEVTYPER,
I think.

(Also I think it's nice when we're making an UNPREDICTABLE
or CONSTRAINED UNPREDICTABLE choice of behaviour to note it
in a comment.)

> +}
> +
>  static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                          uint64_t value)
>  {
> @@ -1194,12 +1203,17 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>      /* 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 },
> -    /* Since we don't implement any events, writing to PMSELR is UNPREDICTABLE.
> -     * We choose to RAZ/WI.
> -     */
>      { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> -      .accessfn = pmreg_access },
> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
> +      .accessfn = pmreg_access, .writefn = pmselr_write,
> +      .raw_writefn = raw_write},
> +    { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 5,
> +      .access = PL0_RW, .accessfn = pmreg_access,
> +      .type = ARM_CP_IO,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),

For a 64-bit register the field in the CPU struct has to be
a uint64_t, so you need to change it from the uint32_t it is
at the moment. The AArch32 regdef struct then needs to change
to use offsetoflow32() where it currently has offsetof().

> +      .writefn = pmselr_write, .raw_writefn = raw_write, },
>  #ifndef CONFIG_USER_ONLY
>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
>        .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
> --
> 1.8.3.1

thanks
-- PMM

[Qemu-devel] [PATCH V1 2/4] target-arm: Add support for AArch64 PMU register PMXEVTYPER_EL0

[Wei Huang]

In order to support Linux perf, which uses PMXEVTYPER register,
this patch adds access support for PMXEVTYPER_EL0.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 target/arm/helper.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 71adb0f..9044a33 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1233,10 +1233,17 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
       .resetvalue = 0, },
     { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
-      .access = PL0_RW,
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
       .accessfn = pmreg_access, .writefn = pmxevtyper_write,
       .raw_writefn = raw_write },
+    { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .type = ARM_CP_IO,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
+      .writefn = pmxevtyper_write, .raw_writefn = raw_write,
+      .resetvalue = 0x0 },
     /* Unimplemented, RAZ/WI. */
     { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
       .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V1 2/4] target-arm: Add support for AArch64 PMU register PMXEVTYPER_EL0

[Peter Maydell]

On 12 January 2017 at 07:04, Wei Huang <wei@redhat.com> wrote:
> In order to support Linux perf, which uses PMXEVTYPER register,
> this patch adds access support for PMXEVTYPER_EL0.
>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  target/arm/helper.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 71adb0f..9044a33 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1233,10 +1233,17 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
>        .resetvalue = 0, },
>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
> -      .access = PL0_RW,
> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
>        .accessfn = pmreg_access, .writefn = pmxevtyper_write,
>        .raw_writefn = raw_write },
> +    { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1,
> +      .access = PL0_RW, .accessfn = pmreg_access,
> +      .type = ARM_CP_IO,

Why is this marked as ARM_CP_IO ?

> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
> +      .writefn = pmxevtyper_write, .raw_writefn = raw_write,
> +      .resetvalue = 0x0 },

As with patch 1, the c9_pmxevtyper field needs to become a uint64_t
so you can use it in a 64-bit regdef.
(Also the semantics of PMXEVTYPER need to change, see comments on
patch 1.)

>      /* Unimplemented, RAZ/WI. */
>      { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
>        .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,

thanks
-- PMM

[Qemu-devel] [PATCH V1 3/4] target-arm: Add support for PMU register PMINTENSET_EL1

[Wei Huang]

This patch adds access support for PMINTENSET_EL1.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 target/arm/helper.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9044a33..22c66e3 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1261,9 +1261,17 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .writefn = pmuserenr_write, .raw_writefn = raw_write },
     { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
       .access = PL1_RW, .accessfn = access_tpm,
+      .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .resetvalue = 0,
       .writefn = pmintenset_write, .raw_writefn = raw_write },
+    { .name = "PMINTENSET_EL1", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 1,
+      .access = PL1_RW, .accessfn = access_tpm,
+      .type = ARM_CP_IO,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
+      .writefn = pmintenset_write, .raw_writefn = raw_write,
+      .resetvalue = 0x0 },
     { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
       .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V1 3/4] target-arm: Add support for PMU register PMINTENSET_EL1

[Peter Maydell]

On 12 January 2017 at 07:04, Wei Huang <wei@redhat.com> wrote:
> This patch adds access support for PMINTENSET_EL1.
>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  target/arm/helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 9044a33..22c66e3 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1261,9 +1261,17 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .writefn = pmuserenr_write, .raw_writefn = raw_write },
>      { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
>        .access = PL1_RW, .accessfn = access_tpm,
> +      .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .resetvalue = 0,
>        .writefn = pmintenset_write, .raw_writefn = raw_write },
> +    { .name = "PMINTENSET_EL1", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 1,
> +      .access = PL1_RW, .accessfn = access_tpm,
> +      .type = ARM_CP_IO,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> +      .writefn = pmintenset_write, .raw_writefn = raw_write,
> +      .resetvalue = 0x0 },
>      { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
>        .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> --

c9_pminten needs to become 64 bits.

We should add PMINTENCLR_EL1 too -- it's a bit weird to provide the
SET function but not its parter CLR operation.

thanks
-- PMM

[Qemu-devel] [PATCH V1 4/4] target-arm: Hook up TCG vPMU with CPU pmu option

[Wei Huang]

Remove the checking of kvm_enabled(). With this, .pmu option can also
control vPMU under TCG mode.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 target/arm/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index f5cb30a..2f87a4b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -692,7 +692,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         cpu->id_aa64pfr0 &= ~0xf000;
     }
 
-    if (!cpu->has_pmu || !kvm_enabled()) {
+    if (!cpu->has_pmu) {
         cpu->has_pmu = false;
         unset_feature(env, ARM_FEATURE_PMU);
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V1 4/4] target-arm: Hook up TCG vPMU with CPU pmu option

[Peter Maydell]

On 12 January 2017 at 07:04, Wei Huang <wei@redhat.com> wrote:
> Remove the checking of kvm_enabled(). With this, .pmu option can also
> control vPMU under TCG mode.
>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  target/arm/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index f5cb30a..2f87a4b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -692,7 +692,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          cpu->id_aa64pfr0 &= ~0xf000;
>      }
>
> -    if (!cpu->has_pmu || !kvm_enabled()) {
> +    if (!cpu->has_pmu) {
>          cpu->has_pmu = false;
>          unset_feature(env, ARM_FEATURE_PMU);
>      }

I notice that the ARM_FEATURE_PMU bit doesn't seem to control whether
we actually provide the TCG PMU registers, so this patch is OK but
doesn't actually change the behaviour of the emulated cpu AFAICT.

Don't we now need to remove the hack where we mask out the PMUVer field
of ID_AA64DFR1_EL1 ?

thanks
-- PMM