[Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Wei Huang]

This patchset adds a pmu=[on/off] option to enable/disable vPMU support 
for guest VM. There are several reasons to justify this option. First,
vPMU can be problematic for cross-migration between different SoC as perf
counters are architecture-dependent. It is more flexible to have an option
to turn it on/off. Secondly this option matches the "pmu" option as
supported in libvirt. To make sure backward compatible, a PMU-related
property is added to mach-virt machine types.

The following are testing results with this patchset:
 CONFIG (qemu-system-aarch64)                       vPMU   WARNING
  -M virt-2.8/virt,accel=kvm -cpu host               YES    NO
  -M virt-2.8/virt,accel=kvm -cpu host,pmu=off       NO     NO
  -M virt-2.8/virt,accel=kvm -cpu host,pmu=on        YES    NO
  -M virt-2.7,accel=kvm -cpu host                    YES    NO
  -M virt-2.7,accel=kvm -cpu host,pmu=off            NO     NO
  -M virt-2.7,accel=kvm -cpu host,pmu=on             YES    NO
  -M virt-2.6,accel=kvm -cpu host                    NO     NO
  -M virt-2.6,accel=kvm -cpu host,pmu=off            NO     NO
  -M virt-2.6,accel=kvm -cpu host,pmu=on             NO     NO

  -M virt-2.8/virt,accel=tcg -cpu cortex-a57         NO     NO
  -M virt-2.8/virt,accel=tcg -cpu cortex-a57,pmu=off NO     NO
  -M virt-2.8/virt,accel=tcg -cpu cortex-a57,pmu=on  NO     NO
  -M virt-2.7,accel=tcg -cpu cortex-a57              NO     NO
  -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=off      NO     NO
  -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=on       NO     NO
  -M virt-2.6,accel=tcg -cpu cortex-a57              NO     NO
  -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=off      NO     NO
  -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=on       NO     NO

  -M virt-2.8/virt,accel=tcg -cpu cortex-a15         NO     NO
  -M virt-2.8/virt,accel=tcg -cpu cortex-a15,pmu=off NO     "No PMU property"
  -M virt-2.8/virt,accel=tcg -cpu cortex-a15,pmu=on  NO     "No PMU property"
  -M virt-2.7,accel=tcg -cpu cortex-a15              NO     NO
  -M virt-2.7,accel=tcg -cpu cortex-a15,pmu=off      NO     "No PMU property"
  -M virt-2.7,accel=tcg -cpu cortex-a15,pmu=on       NO     "No PMU property"
  -M virt-2.6,accel=tcg -cpu cortex-a15              NO     NO
  -M virt-2.6,accel=tcg -cpu cortex-a15,pmu=off      NO     "No PMU property"
  -M virt-2.6,accel=tcg -cpu cortex-a15,pmu=on       NO     "No PMU property"

  * "No PMU property" msg
    can't apply global cortex-a15-arm-cpu.pmu=off: Property '.pmu' not found

V7->V8:
  * add back the "pmu" property for TCG mode

V6->V7:
  * change has_pmu variable type from OnOffAuto to Boolean
  * only add "pmu" property to CPU under kvm mode, default ON
  * set no_pmu=true for machvirt-2.6

V5->V6:
  * adapt patches for new machine type 2.8

V4->V5:
  * remove comment change for has_pmu
  * remove warning msg when pmu_default_on=TRUE && has_pmu=AUTO && tcg=TRUE

V3->V4:
  * change has_pmu from Boolean to OnOffAuto to handle different cases
  * "pmu" property is re-defined as DEFINE_PROP_ON_OFF_AUTO

V2->V3:
  * revise patch 1 commit msg and if-else statement (Drew) 
  * move property field into VirtMachineClass (Drew)

V1->V2:
  * keep the original field name as "has_pmu"
  * add a warning message when PMU is turned on without KVM
  * use the feature bit to check PMU availability, instead of using has_pmu
  * add PMU compat support to mach-virt machine type

RFC->V1:
  * set default pmu=off
  * change struct ARMCPU field name "has_pmu" ==> "has_host_pmu"
  * like el3, add a new feature ARM_FEATURE_HOST_PMU
  * "pmu" property becomes CPU dependent. Only cortex-a53/cortex-a57/host
    running on kvm supports this option.

Thanks,
-Wei

Wei Huang (2):
  arm: Add an option to turn on/off vPMU support
  arm: virt: add PMU property to mach-virt machine type

 hw/arm/virt-acpi-build.c |  2 +-
 hw/arm/virt.c            |  9 ++++++++-
 target-arm/cpu.c         | 15 +++++++++++++++
 target-arm/cpu.h         |  1 +
 target-arm/cpu64.c       |  2 ++
 target-arm/kvm64.c       | 17 ++++++++++++++---
 6 files changed, 41 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH V8 1/2] arm: Add an option to turn on/off vPMU support

[Wei Huang]

This patch adds a pmu=[on/off] option to enable/disable vPMU support
in guest vCPU. It allows virt tools, such as libvirt, to determine the
exsitence of vPMU and configure it. Note this option is only available
for cortex-a57/cortex-53/ host CPUs, but unavailable on ARMv7 and other
processors. Also even though "pmu=" option is available for TCG mode,
setting it doesn't turn PMU on.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt-acpi-build.c |  2 +-
 hw/arm/virt.c            |  2 +-
 target-arm/cpu.c         | 15 +++++++++++++++
 target-arm/cpu.h         |  1 +
 target-arm/cpu64.c       |  2 ++
 target-arm/kvm64.c       | 17 ++++++++++++++---
 6 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index fa0655a..49898df 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -539,7 +539,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info)
         gicc->uid = i;
         gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
 
-        if (armcpu->has_pmu) {
+        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
             gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
         }
     }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 895446f..074d11c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -490,7 +490,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int gictype)
 
     CPU_FOREACH(cpu) {
         armcpu = ARM_CPU(cpu);
-        if (!armcpu->has_pmu ||
+        if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU) ||
             !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) {
             return;
         }
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 1b9540e..655a1f8 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "cpu.h"
 #include "internals.h"
@@ -509,6 +510,10 @@ static Property arm_cpu_rvbar_property =
 static Property arm_cpu_has_el3_property =
             DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, true);
 
+/* use property name "pmu" to match other archs and virt tools */
+static Property arm_cpu_has_pmu_property =
+            DEFINE_PROP_BOOL("pmu", ARMCPU, has_pmu, true);
+
 static Property arm_cpu_has_mpu_property =
             DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true);
 
@@ -552,6 +557,11 @@ static void arm_cpu_post_init(Object *obj)
 #endif
     }
 
+    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property,
+                                 &error_abort);
+    }
+
     if (arm_feature(&cpu->env, ARM_FEATURE_MPU)) {
         qdev_property_add_static(DEVICE(obj), &arm_cpu_has_mpu_property,
                                  &error_abort);
@@ -648,6 +658,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         cpu->id_aa64pfr0 &= ~0xf000;
     }
 
+    if (!cpu->has_pmu || !kvm_enabled()) {
+        cpu->has_pmu = false;
+        unset_feature(env, ARM_FEATURE_PMU);
+    }
+
     if (!arm_feature(env, ARM_FEATURE_EL2)) {
         /* Disable the hypervisor feature bits in the processor feature
          * registers if we don't have EL2. These are id_pfr1[15:12] and
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 2218c00..b97b93b 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1129,6 +1129,7 @@ enum arm_features {
     ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */
     ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
     ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
+    ARM_FEATURE_PMU, /* has PMU support */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index 1635deb..549cb1e 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -111,6 +111,7 @@ static void aarch64_a57_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
     set_feature(&cpu->env, ARM_FEATURE_CRC);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
+    set_feature(&cpu->env, ARM_FEATURE_PMU);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
     cpu->midr = 0x411fd070;
     cpu->revidr = 0x00000000;
@@ -166,6 +167,7 @@ static void aarch64_a53_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
     set_feature(&cpu->env, ARM_FEATURE_CRC);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
+    set_feature(&cpu->env, ARM_FEATURE_PMU);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53;
     cpu->midr = 0x410fd034;
     cpu->revidr = 0x00000000;
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 5faa76c..6111109 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -428,6 +428,11 @@ static inline void set_feature(uint64_t *features, int feature)
     *features |= 1ULL << feature;
 }
 
+static inline void unset_feature(uint64_t *features, int feature)
+{
+    *features &= ~(1ULL << feature);
+}
+
 bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
 {
     /* Identify the feature bits corresponding to the host CPU, and
@@ -469,6 +474,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
     set_feature(&features, ARM_FEATURE_VFP4);
     set_feature(&features, ARM_FEATURE_NEON);
     set_feature(&features, ARM_FEATURE_AARCH64);
+    set_feature(&features, ARM_FEATURE_PMU);
 
     ahcc->features = features;
 
@@ -482,6 +488,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
     int ret;
     uint64_t mpidr;
     ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
 
     if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
         !object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU)) {
@@ -501,10 +508,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
     if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
     }
-    if (kvm_irqchip_in_kernel() &&
-        kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
-        cpu->has_pmu = true;
+    if (!kvm_irqchip_in_kernel() ||
+        !kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
+            cpu->has_pmu = false;
+    }
+    if (cpu->has_pmu) {
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
+    } else {
+        unset_feature(&env->features, ARM_FEATURE_PMU);
     }
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
-- 
1.8.3.1

[Qemu-devel] [PATCH V8 2/2] arm: virt: add PMU property to mach-virt machine type

[Wei Huang]

CPU vPMU is now turned ON by default, but this feature wasn't introduced
until virt-2.7 machine type. To solve this problem, this patch adds a
PMU option in machine state, which is used to control CPU's vPMU status.
This PMU option is not exposed to command line and is turned off in
virt-2.6 machine type.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 074d11c..e64dc4a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -85,6 +85,7 @@ typedef struct {
     VirtBoardInfo *daughterboard;
     bool disallow_affinity_adjustment;
     bool no_its;
+    bool no_pmu;
 } VirtMachineClass;
 
 typedef struct {
@@ -1353,6 +1354,10 @@ static void machvirt_init(MachineState *machine)
             }
         }
 
+        if (vmc->no_pmu && object_property_find(cpuobj, "pmu", NULL)) {
+            object_property_set_bool(cpuobj, false, "pmu", NULL);
+        }
+
         if (object_property_find(cpuobj, "reset-cbar", NULL)) {
             object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base,
                                     "reset-cbar", &error_abort);
@@ -1588,5 +1593,7 @@ static void virt_machine_2_6_options(MachineClass *mc)
     virt_machine_2_7_options(mc);
     SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_6);
     vmc->disallow_affinity_adjustment = true;
+    /* Disable PMU for 2.6 as PMU support was first introduced in 2.7 */
+    vmc->no_pmu = true;
 }
 DEFINE_VIRT_MACHINE(2, 6)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V8 1/2] arm: Add an option to turn on/off vPMU support

[Andrew Jones]

On Wed, Oct 26, 2016 at 02:28:20AM -0400, Wei Huang wrote:
> This patch adds a pmu=[on/off] option to enable/disable vPMU support
> in guest vCPU. It allows virt tools, such as libvirt, to determine the
> exsitence of vPMU and configure it. Note this option is only available
> for cortex-a57/cortex-53/ host CPUs, but unavailable on ARMv7 and other
> processors. Also even though "pmu=" option is available for TCG mode,
> setting it doesn't turn PMU on.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c |  2 +-
>  hw/arm/virt.c            |  2 +-
>  target-arm/cpu.c         | 15 +++++++++++++++
>  target-arm/cpu.h         |  1 +
>  target-arm/cpu64.c       |  2 ++
>  target-arm/kvm64.c       | 17 ++++++++++++++---
>  6 files changed, 34 insertions(+), 5 deletions(-)

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index fa0655a..49898df 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -539,7 +539,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info)
>          gicc->uid = i;
>          gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
>  
> -        if (armcpu->has_pmu) {
> +        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>          }
>      }
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 895446f..074d11c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -490,7 +490,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int gictype)
>  
>      CPU_FOREACH(cpu) {
>          armcpu = ARM_CPU(cpu);
> -        if (!armcpu->has_pmu ||
> +        if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU) ||
>              !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) {
>              return;
>          }
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 1b9540e..655a1f8 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "internals.h"
> @@ -509,6 +510,10 @@ static Property arm_cpu_rvbar_property =
>  static Property arm_cpu_has_el3_property =
>              DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, true);
>  
> +/* use property name "pmu" to match other archs and virt tools */
> +static Property arm_cpu_has_pmu_property =
> +            DEFINE_PROP_BOOL("pmu", ARMCPU, has_pmu, true);
> +
>  static Property arm_cpu_has_mpu_property =
>              DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true);
>  
> @@ -552,6 +557,11 @@ static void arm_cpu_post_init(Object *obj)
>  #endif
>      }
>  
> +    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property,
> +                                 &error_abort);
> +    }
> +
>      if (arm_feature(&cpu->env, ARM_FEATURE_MPU)) {
>          qdev_property_add_static(DEVICE(obj), &arm_cpu_has_mpu_property,
>                                   &error_abort);
> @@ -648,6 +658,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          cpu->id_aa64pfr0 &= ~0xf000;
>      }
>  
> +    if (!cpu->has_pmu || !kvm_enabled()) {
> +        cpu->has_pmu = false;
> +        unset_feature(env, ARM_FEATURE_PMU);
> +    }
> +
>      if (!arm_feature(env, ARM_FEATURE_EL2)) {
>          /* Disable the hypervisor feature bits in the processor feature
>           * registers if we don't have EL2. These are id_pfr1[15:12] and
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 2218c00..b97b93b 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1129,6 +1129,7 @@ enum arm_features {
>      ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */
>      ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
>      ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
> +    ARM_FEATURE_PMU, /* has PMU support */
>  };
>  
>  static inline int arm_feature(CPUARMState *env, int feature)
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index 1635deb..549cb1e 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -111,6 +111,7 @@ static void aarch64_a57_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
>      set_feature(&cpu->env, ARM_FEATURE_CRC);
>      set_feature(&cpu->env, ARM_FEATURE_EL3);
> +    set_feature(&cpu->env, ARM_FEATURE_PMU);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
>      cpu->midr = 0x411fd070;
>      cpu->revidr = 0x00000000;
> @@ -166,6 +167,7 @@ static void aarch64_a53_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
>      set_feature(&cpu->env, ARM_FEATURE_CRC);
>      set_feature(&cpu->env, ARM_FEATURE_EL3);
> +    set_feature(&cpu->env, ARM_FEATURE_PMU);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53;
>      cpu->midr = 0x410fd034;
>      cpu->revidr = 0x00000000;
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 5faa76c..6111109 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -428,6 +428,11 @@ static inline void set_feature(uint64_t *features, int feature)
>      *features |= 1ULL << feature;
>  }
>  
> +static inline void unset_feature(uint64_t *features, int feature)
> +{
> +    *features &= ~(1ULL << feature);
> +}
> +
>  bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
>  {
>      /* Identify the feature bits corresponding to the host CPU, and
> @@ -469,6 +474,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
>      set_feature(&features, ARM_FEATURE_VFP4);
>      set_feature(&features, ARM_FEATURE_NEON);
>      set_feature(&features, ARM_FEATURE_AARCH64);
> +    set_feature(&features, ARM_FEATURE_PMU);
>  
>      ahcc->features = features;
>  
> @@ -482,6 +488,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      int ret;
>      uint64_t mpidr;
>      ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
>  
>      if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
>          !object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU)) {
> @@ -501,10 +508,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
>      }
> -    if (kvm_irqchip_in_kernel() &&
> -        kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
> -        cpu->has_pmu = true;
> +    if (!kvm_irqchip_in_kernel() ||
> +        !kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
> +            cpu->has_pmu = false;
> +    }
> +    if (cpu->has_pmu) {
>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> +    } else {
> +        unset_feature(&env->features, ARM_FEATURE_PMU);
>      }
>  
>      /* Do KVM_ARM_VCPU_INIT ioctl */
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH V8 2/2] arm: virt: add PMU property to mach-virt machine type

[Andrew Jones]

On Wed, Oct 26, 2016 at 02:28:21AM -0400, Wei Huang wrote:
> CPU vPMU is now turned ON by default, but this feature wasn't introduced
> until virt-2.7 machine type. To solve this problem, this patch adds a
> PMU option in machine state, which is used to control CPU's vPMU status.
> This PMU option is not exposed to command line and is turned off in
> virt-2.6 machine type.
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 074d11c..e64dc4a 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -85,6 +85,7 @@ typedef struct {
>      VirtBoardInfo *daughterboard;
>      bool disallow_affinity_adjustment;
>      bool no_its;
> +    bool no_pmu;
>  } VirtMachineClass;
>  
>  typedef struct {
> @@ -1353,6 +1354,10 @@ static void machvirt_init(MachineState *machine)
>              }
>          }
>  
> +        if (vmc->no_pmu && object_property_find(cpuobj, "pmu", NULL)) {
> +            object_property_set_bool(cpuobj, false, "pmu", NULL);
> +        }
> +
>          if (object_property_find(cpuobj, "reset-cbar", NULL)) {
>              object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base,
>                                      "reset-cbar", &error_abort);
> @@ -1588,5 +1593,7 @@ static void virt_machine_2_6_options(MachineClass *mc)
>      virt_machine_2_7_options(mc);
>      SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_6);
>      vmc->disallow_affinity_adjustment = true;
> +    /* Disable PMU for 2.6 as PMU support was first introduced in 2.7 */
> +    vmc->no_pmu = true;
>  }
>  DEFINE_VIRT_MACHINE(2, 6)
> -- 
> 1.8.3.1
> 

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Peter Maydell]

On 26 October 2016 at 07:28, Wei Huang <wei@redhat.com> wrote:
> This patchset adds a pmu=[on/off] option to enable/disable vPMU support
> for guest VM. There are several reasons to justify this option. First,
> vPMU can be problematic for cross-migration between different SoC as perf
> counters are architecture-dependent. It is more flexible to have an option
> to turn it on/off. Secondly this option matches the "pmu" option as
> supported in libvirt. To make sure backward compatible, a PMU-related
> property is added to mach-virt machine types.

(Sorry, sent this as a reply to the v7 patch; resending to
attach it to the right thread...)

So what actually are we defining as "the PMU exists" here?
The PMU exists in ARMv7 and we do actually implement it
in TCG for v7 and v8 cores (though it is a minimal
architecturally-valid implementation which implements no events,
only the cycle counter).

Should the PMU registers in TCG which we currently provide on
any v7-and-better CPU be hung off ARM_FEATURE_PMU instead ?
(this would result in the behaviour of virt-2.6 for TCG
changing in that we stop providing the PMU when we did
before, but I guess that's OK.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Wei Huang]

On 10/27/2016 09:28 AM, Peter Maydell wrote:
> On 26 October 2016 at 07:28, Wei Huang <wei@redhat.com> wrote:
>> This patchset adds a pmu=[on/off] option to enable/disable vPMU support
>> for guest VM. There are several reasons to justify this option. First,
>> vPMU can be problematic for cross-migration between different SoC as perf
>> counters are architecture-dependent. It is more flexible to have an option
>> to turn it on/off. Secondly this option matches the "pmu" option as
>> supported in libvirt. To make sure backward compatible, a PMU-related
>> property is added to mach-virt machine types.
> 
> (Sorry, sent this as a reply to the v7 patch; resending to
> attach it to the right thread...)
> 
> So what actually are we defining as "the PMU exists" here?

"PMU exists" means that PMUv3 presence can be detected and related
driver can be loaded for guest VM:
* For DT: perf_event.c checks the presence of "armv8-pmuv3" in DT and do
armv8pmu_probe_pmu().
* For ACPI: Jeremy Linton submitted an ACPI-based ARM driver for
upstream. This can be loaded when "PMU exists" is detected.

Link:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/415372.html

> The PMU exists in ARMv7 and we do actually implement it
> in TCG for v7 and v8 cores (though it is a minimal
> architecturally-valid implementation which implements no events,
> only the cycle counter).

This option shouldn't affect the existing TCG implementation. It only
touch the recent hardware-assisted vPMU implemented by Shannon. In other
words, it adds an option for users to turn off Shannon's vPMU (for the
reasons of migration compatibility, etc)

> 
> Should the PMU registers in TCG which we currently provide on
> any v7-and-better CPU be hung off ARM_FEATURE_PMU instead ?
> (this would result in the behaviour of virt-2.6 for TCG
> changing in that we stop providing the PMU when we did
> before, but I guess that's OK.)
> 

So you want to disable cycle_count for TCG mode? Reading ARM doc, I
think cycle_count is minimally required for ARM (correct me if I am
wrong). Here is the description for PMCR.N bits:

"Number of event counters. A RO field that indicates the number counters
implemented. A value of 0b00000 in this field indicates that only the
Cycle Count Register PMCCNTR_EL0 is implemented. The value of this field
is the number of event counters implemented. This value is in the range
of 0b00000 , in which case only the PMCCNTR_EL0 is  implemented, to
0b11111 , which indicates that the PMCCNTR_EL0 and 31 event counters are
implemented."

If we want to disable cycle_count, QEMU has to declare PMCR
in-accessible, right?

> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Peter Maydell]

On 27 October 2016 at 18:06, Wei Huang <wei@redhat.com> wrote:
>
>
> On 10/27/2016 09:28 AM, Peter Maydell wrote:
>> On 26 October 2016 at 07:28, Wei Huang <wei@redhat.com> wrote:
>>> This patchset adds a pmu=[on/off] option to enable/disable vPMU support
>>> for guest VM. There are several reasons to justify this option. First,
>>> vPMU can be problematic for cross-migration between different SoC as perf
>>> counters are architecture-dependent. It is more flexible to have an option
>>> to turn it on/off. Secondly this option matches the "pmu" option as
>>> supported in libvirt. To make sure backward compatible, a PMU-related
>>> property is added to mach-virt machine types.
>>
>> (Sorry, sent this as a reply to the v7 patch; resending to
>> attach it to the right thread...)
>>
>> So what actually are we defining as "the PMU exists" here?
>
> "PMU exists" means that PMUv3 presence can be detected and related
> driver can be loaded for guest VM:
> * For DT: perf_event.c checks the presence of "armv8-pmuv3" in DT and do
> armv8pmu_probe_pmu().
> * For ACPI: Jeremy Linton submitted an ACPI-based ARM driver for
> upstream. This can be loaded when "PMU exists" is detected.
>
> Link:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/415372.html
>
>> The PMU exists in ARMv7 and we do actually implement it
>> in TCG for v7 and v8 cores (though it is a minimal
>> architecturally-valid implementation which implements no events,
>> only the cycle counter).
>
> This option shouldn't affect the existing TCG implementation. It only
> touch the recent hardware-assisted vPMU implemented by Shannon. In other
> words, it adds an option for users to turn off Shannon's vPMU (for the
> reasons of migration compatibility, etc)

Right, but at the moment the commit message says there's no
PMU in TCG at all. What's missing that causes there not to be one?
(I think maybe we just don't report it in the feature register,
which might be a bug.)

>> Should the PMU registers in TCG which we currently provide on
>> any v7-and-better CPU be hung off ARM_FEATURE_PMU instead ?
>> (this would result in the behaviour of virt-2.6 for TCG
>> changing in that we stop providing the PMU when we did
>> before, but I guess that's OK.)
>>
>
> So you want to disable cycle_count for TCG mode? Reading ARM doc, I
> think cycle_count is minimally required for ARM (correct me if I am
> wrong). Here is the description for PMCR.N bits:
>
> "Number of event counters. A RO field that indicates the number counters
> implemented. A value of 0b00000 in this field indicates that only the
> Cycle Count Register PMCCNTR_EL0 is implemented. The value of this field
> is the number of event counters implemented. This value is in the range
> of 0b00000 , in which case only the PMCCNTR_EL0 is  implemented, to
> 0b11111 , which indicates that the PMCCNTR_EL0 and 31 event counters are
> implemented."
>
> If we want to disable cycle_count, QEMU has to declare PMCR
> in-accessible, right?

The cycle counter is required if the PMU exists, but the whole PMU
is optional. If the user says 'pmu=off' then shouldn't we be
disabling the implementation in TCG?

thanks
-- PMM

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Wei Huang]

On 10/27/2016 12:15 PM, Peter Maydell wrote:
> On 27 October 2016 at 18:06, Wei Huang <wei@redhat.com> wrote:
>>
>>
>> On 10/27/2016 09:28 AM, Peter Maydell wrote:
>>> On 26 October 2016 at 07:28, Wei Huang <wei@redhat.com> wrote:
>>>> This patchset adds a pmu=[on/off] option to enable/disable vPMU support
>>>> for guest VM. There are several reasons to justify this option. First,
>>>> vPMU can be problematic for cross-migration between different SoC as perf
>>>> counters are architecture-dependent. It is more flexible to have an option
>>>> to turn it on/off. Secondly this option matches the "pmu" option as
>>>> supported in libvirt. To make sure backward compatible, a PMU-related
>>>> property is added to mach-virt machine types.
>>>
>>> (Sorry, sent this as a reply to the v7 patch; resending to
>>> attach it to the right thread...)
>>>
>>> So what actually are we defining as "the PMU exists" here?
>>
>> "PMU exists" means that PMUv3 presence can be detected and related
>> driver can be loaded for guest VM:
>> * For DT: perf_event.c checks the presence of "armv8-pmuv3" in DT and do
>> armv8pmu_probe_pmu().
>> * For ACPI: Jeremy Linton submitted an ACPI-based ARM driver for
>> upstream. This can be loaded when "PMU exists" is detected.
>>
>> Link:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/415372.html
>>
>>> The PMU exists in ARMv7 and we do actually implement it
>>> in TCG for v7 and v8 cores (though it is a minimal
>>> architecturally-valid implementation which implements no events,
>>> only the cycle counter).
>>
>> This option shouldn't affect the existing TCG implementation. It only
>> touch the recent hardware-assisted vPMU implemented by Shannon. In other
>> words, it adds an option for users to turn off Shannon's vPMU (for the
>> reasons of migration compatibility, etc)
> 
> Right, but at the moment the commit message says there's no
> PMU in TCG at all. What's missing that causes there not to be one?
> (I think maybe we just don't report it in the feature register,
> which might be a bug.)

This sounds like a bug (or incomplete). In current version of QEMU, if I
forced vPMU declaration in the feature register under TCG mode, PMU
driver will be loaded in VM. But as soon as "perf stat" is executed,
guest VM panic. I think some features, like PMU IRQ, is still missing
for TCG to turn vPMU on. I will debug it further.

> 
>>> Should the PMU registers in TCG which we currently provide on
>>> any v7-and-better CPU be hung off ARM_FEATURE_PMU instead ?
>>> (this would result in the behaviour of virt-2.6 for TCG
>>> changing in that we stop providing the PMU when we did
>>> before, but I guess that's OK.)
>>>
>>
>> So you want to disable cycle_count for TCG mode? Reading ARM doc, I
>> think cycle_count is minimally required for ARM (correct me if I am
>> wrong). Here is the description for PMCR.N bits:
>>
>> "Number of event counters. A RO field that indicates the number counters
>> implemented. A value of 0b00000 in this field indicates that only the
>> Cycle Count Register PMCCNTR_EL0 is implemented. The value of this field
>> is the number of event counters implemented. This value is in the range
>> of 0b00000 , in which case only the PMCCNTR_EL0 is  implemented, to
>> 0b11111 , which indicates that the PMCCNTR_EL0 and 31 event counters are
>> implemented."
>>
>> If we want to disable cycle_count, QEMU has to declare PMCR
>> in-accessible, right?
> 
> The cycle counter is required if the PMU exists, but the whole PMU
> is optional. If the user says 'pmu=off' then shouldn't we be
> disabling the implementation in TCG?
> 

Let me clarify. Assuming we can get TCG mode to support VM's PMU driver
(see discussion above, only cycle counter supported for TCG), will the
following be sufficient?
(1) ,pmu=on
perf driver can be loaded in VM, and "perf" command can use HW counters
to profile.
(2) ,pmu=off
perf feature can't be detected, so the perf driver can't be loaded
inside VM. "perf" can't use HW perf counters. But PMU registers (PMCR,
PMCCNTR, ...) can still be accessed inside VM.

OR

You prefer to disable access permission for PMU registers in (2). Note:
this is very strict. I am not sure if current KVM already follows this rule.


> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Peter Maydell]

On 27 October 2016 at 21:53, Wei Huang <wei@redhat.com> wrote:
> On 10/27/2016 12:15 PM, Peter Maydell wrote:
>> Right, but at the moment the commit message says there's no
>> PMU in TCG at all. What's missing that causes there not to be one?
>> (I think maybe we just don't report it in the feature register,
>> which might be a bug.)
>
> This sounds like a bug (or incomplete). In current version of QEMU, if I
> forced vPMU declaration in the feature register under TCG mode, PMU
> driver will be loaded in VM. But as soon as "perf stat" is executed,
> guest VM panic. I think some features, like PMU IRQ, is still missing
> for TCG to turn vPMU on. I will debug it further.

You're right, we don't have the PMU IRQ implemented.
I wouldn't expect that to cause the guest to panic, though...

> Let me clarify. Assuming we can get TCG mode to support VM's PMU driver
> (see discussion above, only cycle counter supported for TCG), will the
> following be sufficient?
> (1) ,pmu=on
> perf driver can be loaded in VM, and "perf" command can use HW counters
> to profile.
> (2) ,pmu=off
> perf feature can't be detected, so the perf driver can't be loaded
> inside VM. "perf" can't use HW perf counters. But PMU registers (PMCR,
> PMCCNTR, ...) can still be accessed inside VM.

I think that's what we get today (with this patch set). But it
would seem more consistent if pmu=off didn't provide the PMCR,
PMCCNTR, etc to the guest under TCG in the same way that you
don't get them under KVM if you say pmu=off.

Anyway, I think that these patches are the right thing for KVM,
so I'm going to apply them to target-arm.next. We should probably
look at straightening out the TCG situation, but we can do that
with a separate patchset (and perhaps not for 2.8).

Thanks for your patience in working through all the revisions
of this series.

-- PMM