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

[Qemu-devel] [PATCH V2 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 Secondly this option matches the "pmu" option
as supported in libvirt. To make sure backward compatible, a PMU property
is added to mach-virt machine types.

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.

-Wei

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

 hw/arm/virt-acpi-build.c |  2 +-
 hw/arm/virt.c            | 14 +++++++++++++-
 target-arm/cpu.c         | 22 ++++++++++++++++++++++
 target-arm/cpu.h         |  1 +
 target-arm/cpu64.c       |  2 ++
 target-arm/kvm64.c       | 17 +++++++++++++----
 6 files changed, 52 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH V2 1/2] arm64: 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 VM. The pmu option is available only for cortex-a57/cortex-53/
host under both TCG and KVM modes. Additionally pmu can only be turned
on under KVM mode, otherwise a warning message will be printed out
(e.g. "-M virt,accel=tcg -cpu cortex-a57,pmu=on"). This property isn't
available on ARMv7 and other processors. This patch has been tested under
DT and ACPI.

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

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 295ec86..8b3083e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -540,7 +540,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 a193b5a..a781ad0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -477,7 +477,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 ce8b8f4..d304597 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, false);
+
 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);
@@ -576,6 +586,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     ARMCPU *cpu = ARM_CPU(dev);
     ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
     CPUARMState *env = &cpu->env;
+    static bool pmu_warned;
 
     /* Some features automatically imply others: */
     if (arm_feature(env, ARM_FEATURE_V8)) {
@@ -648,6 +659,17 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         cpu->id_aa64pfr0 &= ~0xf000;
     }
 
+    if (cpu->has_pmu && !kvm_enabled()) {
+        cpu->has_pmu = false;
+        if (!pmu_warned) {
+            error_report("warning: pmu can't be enabled without KVM acceleration");
+            pmu_warned = true;
+        }
+    }
+    if (!cpu->has_pmu) {
+        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 76d824d..5d9e6e7 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..933b27a 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,12 @@ 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;
-        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
+    /* enable PMU based on KVM mode, hw capability, and user setting */
+    cpu->has_pmu &= kvm_irqchip_in_kernel() &&
+        kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3);
+    cpu->kvm_init_features[0] |= cpu->has_pmu << KVM_ARM_VCPU_PMU_V3;
+    if (!cpu->has_pmu) {
+        unset_feature(&env->features, ARM_FEATURE_PMU);
     }
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
-- 
1.8.3.1

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

[Wei Huang]

CPU vPMU is now turned off by default, but it was ON in 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 on in
virt-2.7 machine type to make sure it is backward compatible.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a781ad0..83cfea7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -91,6 +91,7 @@ typedef struct {
     bool secure;
     bool highmem;
     int32_t gic_version;
+    bool pmu;
 } VirtMachineState;
 
 #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
@@ -1317,6 +1318,11 @@ static void machvirt_init(MachineState *machine)
             }
         }
 
+        if (vms->pmu) {
+            /* Note: the property name is "pmu", not "has_pmu" */
+            object_property_set_bool(cpuobj, true, "pmu", NULL);
+        }
+
         if (object_property_find(cpuobj, "reset-cbar", NULL)) {
             object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base,
                                     "reset-cbar", &error_abort);
@@ -1510,6 +1516,8 @@ static void virt_2_7_instance_init(Object *obj)
     object_property_set_description(obj, "gic-version",
                                     "Set GIC version. "
                                     "Valid values are 2, 3 and host", NULL);
+    /* Default PMU is on for 2.7 */
+    vms->pmu = true;
 }
 
 static void virt_machine_2_7_options(MachineClass *mc)
@@ -1522,7 +1530,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(2, 7)
 
 static void virt_2_6_instance_init(Object *obj)
 {
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
     virt_2_7_instance_init(obj);
+    /* Default PMU is off for 2.6 */
+    vms->pmu = false;
 }
 
 static void virt_machine_2_6_options(MachineClass *mc)
-- 
1.8.3.1

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

[Andrew Jones]

On Wed, Sep 14, 2016 at 02:09:02AM -0400, Wei Huang wrote:
> This patch adds a pmu=[on/off] option to enable/disable vPMU support
> in guest VM. The pmu option is available only for cortex-a57/cortex-53/
> host under both TCG and KVM modes. Additionally pmu can only be turned
> on under KVM mode, otherwise a warning message will be printed out
> (e.g. "-M virt,accel=tcg -cpu cortex-a57,pmu=on"). This property isn't

The e.g. is a bit confusing here. Following the comment about a warning
message makes me thing it should be an example of the warning message,
but instead it's the command line parameters that generates a warning
message. I'd just drop it.

> available on ARMv7 and other processors. This patch has been tested under
> DT and ACPI.

The feature not being available everywhere is a key motivator for this
series, and maybe should be written in the commit message. Right now
the feature is only available on a57 and a53, but libvirt doesn't know
that. With a property, libvirt can a) determine the feature doesn't
exist, by the lack of property and b) enable/disable the feature. Also,
by changing the default to 'off', libvirt can simply not emit a pmu=on,
when no pmu is desired. This allows the feature-doesn't-exist and the
feature-isn't-desired cases to be consistent.

> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c |  2 +-
>  hw/arm/virt.c            |  2 +-
>  target-arm/cpu.c         | 22 ++++++++++++++++++++++
>  target-arm/cpu.h         |  1 +
>  target-arm/cpu64.c       |  2 ++
>  target-arm/kvm64.c       | 17 +++++++++++++----
>  6 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 295ec86..8b3083e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -540,7 +540,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 a193b5a..a781ad0 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -477,7 +477,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 ce8b8f4..d304597 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, false);
> +
>  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);
> @@ -576,6 +586,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      ARMCPU *cpu = ARM_CPU(dev);
>      ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
>      CPUARMState *env = &cpu->env;
> +    static bool pmu_warned;
>  
>      /* Some features automatically imply others: */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
> @@ -648,6 +659,17 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          cpu->id_aa64pfr0 &= ~0xf000;
>      }
>  
> +    if (cpu->has_pmu && !kvm_enabled()) {
> +        cpu->has_pmu = false;
> +        if (!pmu_warned) {
> +            error_report("warning: pmu can't be enabled without KVM acceleration");
> +            pmu_warned = true;
> +        }
> +    }
> +    if (!cpu->has_pmu) {
> +        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 76d824d..5d9e6e7 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..933b27a 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,12 @@ 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;
> -        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> +    /* enable PMU based on KVM mode, hw capability, and user setting */
> +    cpu->has_pmu &= kvm_irqchip_in_kernel() &&
> +        kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3);
> +    cpu->kvm_init_features[0] |= cpu->has_pmu << KVM_ARM_VCPU_PMU_V3;
> +    if (!cpu->has_pmu) {
> +        unset_feature(&env->features, ARM_FEATURE_PMU);

nit: I think I'd like the following more

 if (cpu->has_pmu) {
     cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
 } else {
     unset_feature(&env->features, ARM_FEATURE_PMU);
 }

but whatever.


>      }
>  
>      /* Do KVM_ARM_VCPU_INIT ioctl */
> -- 
> 1.8.3.1
> 
>

With some changes to the commit message

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

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

[Andrew Jones]

On Wed, Sep 14, 2016 at 02:09:03AM -0400, Wei Huang wrote:
> CPU vPMU is now turned off by default, but it was ON in 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 on in
> virt-2.7 machine type to make sure it is backward compatible.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a781ad0..83cfea7 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -91,6 +91,7 @@ typedef struct {
>      bool secure;
>      bool highmem;
>      int32_t gic_version;
> +    bool pmu;
>  } VirtMachineState;

The compat bool doesn't need to be in machine state, so it should
be in the class. See 'disallow_affinity_adjustment' in VirtMachineClass
for an example of a adding a compat bool for a different reason. Also,
I'd name it something like pmu_default_on.

>  
>  #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
> @@ -1317,6 +1318,11 @@ static void machvirt_init(MachineState *machine)
>              }
>          }
>  
> +        if (vms->pmu) {
> +            /* Note: the property name is "pmu", not "has_pmu" */

The comment is unnecessary. Please remove.

> +            object_property_set_bool(cpuobj, true, "pmu", NULL);
> +        }
> +
>          if (object_property_find(cpuobj, "reset-cbar", NULL)) {
>              object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base,
>                                      "reset-cbar", &error_abort);
> @@ -1510,6 +1516,8 @@ static void virt_2_7_instance_init(Object *obj)
>      object_property_set_description(obj, "gic-version",
>                                      "Set GIC version. "
>                                      "Valid values are 2, 3 and host", NULL);
> +    /* Default PMU is on for 2.7 */

This comment is unnecessary. Please remove.

> +    vms->pmu = true;
>  }
>  
>  static void virt_machine_2_7_options(MachineClass *mc)
> @@ -1522,7 +1530,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(2, 7)
>  
>  static void virt_2_6_instance_init(Object *obj)
>  {
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
>      virt_2_7_instance_init(obj);
> +    /* Default PMU is off for 2.6 */
> +    vms->pmu = false;

You shouldn't need to touch 2.6 code. The compat bool is false
by default (BSS init), which is the correct value for every
machine type, except the 2.7 machine type.

>  }
>  
>  static void virt_machine_2_6_options(MachineClass *mc)
> -- 
> 1.8.3.1
> 
>

Thanks,
drew

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

[Andrew Jones]

On Wed, Sep 14, 2016 at 10:37:19AM +0200, Andrew Jones wrote:
> On Wed, Sep 14, 2016 at 02:09:03AM -0400, Wei Huang wrote:
> > CPU vPMU is now turned off by default, but it was ON in 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 on in
> > virt-2.7 machine type to make sure it is backward compatible.
> > 
> > Signed-off-by: Wei Huang <wei@redhat.com>
> > ---
> >  hw/arm/virt.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index a781ad0..83cfea7 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -91,6 +91,7 @@ typedef struct {
> >      bool secure;
> >      bool highmem;
> >      int32_t gic_version;
> > +    bool pmu;
> >  } VirtMachineState;
> 
> The compat bool doesn't need to be in machine state, so it should
> be in the class. See 'disallow_affinity_adjustment' in VirtMachineClass
> for an example of a adding a compat bool for a different reason. Also,
> I'd name it something like pmu_default_on.
> 
> >  
> >  #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
> > @@ -1317,6 +1318,11 @@ static void machvirt_init(MachineState *machine)
> >              }
> >          }
> >  
> > +        if (vms->pmu) {
> > +            /* Note: the property name is "pmu", not "has_pmu" */
> 
> The comment is unnecessary. Please remove.
> 
> > +            object_property_set_bool(cpuobj, true, "pmu", NULL);
> > +        }
> > +
> >          if (object_property_find(cpuobj, "reset-cbar", NULL)) {
> >              object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base,
> >                                      "reset-cbar", &error_abort);
> > @@ -1510,6 +1516,8 @@ static void virt_2_7_instance_init(Object *obj)
> >      object_property_set_description(obj, "gic-version",
> >                                      "Set GIC version. "
> >                                      "Valid values are 2, 3 and host", NULL);
> > +    /* Default PMU is on for 2.7 */
> 
> This comment is unnecessary. Please remove.
> 
> > +    vms->pmu = true;
> >  }
> >  
> >  static void virt_machine_2_7_options(MachineClass *mc)
> > @@ -1522,7 +1530,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(2, 7)
> >  
> >  static void virt_2_6_instance_init(Object *obj)
> >  {
> > +    VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> >      virt_2_7_instance_init(obj);
> > +    /* Default PMU is off for 2.6 */
> > +    vms->pmu = false;
> 
> You shouldn't need to touch 2.6 code. The compat bool is false
> by default (BSS init), which is the correct value for every
> machine type, except the 2.7 machine type.

Eh, scratch this last comment. Only 2.8 and later don't have to
touch this bool. I momentarily forgot about our chaining. 2.6 has to
reset 2.7's selection of true back to false, as it modifies 2.7's
choices. It works that way with class state too, so 2.6 will still
need the false setting after that change. In this case, a comment is
warranted; one that says something like "PMU support was first
introduced in 2.7, ensure it is disabled by default for 2.6 and down."

Thanks,
drew

> 
> >  }
> >  
> >  static void virt_machine_2_6_options(MachineClass *mc)
> > -- 
> > 1.8.3.1
> > 
> >
> 
> Thanks,
> drew

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

[Wei Huang]

On 09/14/2016 03:17 AM, Andrew Jones wrote:
> On Wed, Sep 14, 2016 at 02:09:02AM -0400, Wei Huang wrote:
>> This patch adds a pmu=[on/off] option to enable/disable vPMU support
>> in guest VM. The pmu option is available only for cortex-a57/cortex-53/
>> host under both TCG and KVM modes. Additionally pmu can only be turned
>> on under KVM mode, otherwise a warning message will be printed out
>> (e.g. "-M virt,accel=tcg -cpu cortex-a57,pmu=on"). This property isn't
> 
> The e.g. is a bit confusing here. Following the comment about a warning
> message makes me thing it should be an example of the warning message,
> but instead it's the command line parameters that generates a warning
> message. I'd just drop it.
> 
>> available on ARMv7 and other processors. This patch has been tested under
>> DT and ACPI.
> 
> The feature not being available everywhere is a key motivator for this
> series, and maybe should be written in the commit message. Right now
> the feature is only available on a57 and a53, but libvirt doesn't know
> that. With a property, libvirt can a) determine the feature doesn't
> exist, by the lack of property and b) enable/disable the feature. Also,
> by changing the default to 'off', libvirt can simply not emit a pmu=on,
> when no pmu is desired. This allows the feature-doesn't-exist and the
> feature-isn't-desired cases to be consistent.

I will update the commit message in V3, assuming that there are comments
from others.

> 
>>
>> Signed-off-by: Wei Huang <wei@redhat.com>
>> ---
>>  hw/arm/virt-acpi-build.c |  2 +-
>>  hw/arm/virt.c            |  2 +-
>>  target-arm/cpu.c         | 22 ++++++++++++++++++++++
>>  target-arm/cpu.h         |  1 +
>>  target-arm/cpu64.c       |  2 ++
>>  target-arm/kvm64.c       | 17 +++++++++++++----
>>  6 files changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 295ec86..8b3083e 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -540,7 +540,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 a193b5a..a781ad0 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -477,7 +477,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 ce8b8f4..d304597 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, false);
>> +
>>  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);
>> @@ -576,6 +586,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>      ARMCPU *cpu = ARM_CPU(dev);
>>      ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
>>      CPUARMState *env = &cpu->env;
>> +    static bool pmu_warned;
>>  
>>      /* Some features automatically imply others: */
>>      if (arm_feature(env, ARM_FEATURE_V8)) {
>> @@ -648,6 +659,17 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>          cpu->id_aa64pfr0 &= ~0xf000;
>>      }
>>  
>> +    if (cpu->has_pmu && !kvm_enabled()) {
>> +        cpu->has_pmu = false;
>> +        if (!pmu_warned) {
>> +            error_report("warning: pmu can't be enabled without KVM acceleration");
>> +            pmu_warned = true;
>> +        }
>> +    }
>> +    if (!cpu->has_pmu) {
>> +        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 76d824d..5d9e6e7 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..933b27a 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,12 @@ 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;
>> -        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
>> +    /* enable PMU based on KVM mode, hw capability, and user setting */
>> +    cpu->has_pmu &= kvm_irqchip_in_kernel() &&
>> +        kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3);
>> +    cpu->kvm_init_features[0] |= cpu->has_pmu << KVM_ARM_VCPU_PMU_V3;
>> +    if (!cpu->has_pmu) {
>> +        unset_feature(&env->features, ARM_FEATURE_PMU);
> 
> nit: I think I'd like the following more
> 
>  if (cpu->has_pmu) {
>      cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
>  } else {
>      unset_feature(&env->features, ARM_FEATURE_PMU);
>  }

Will correct it.

> 
> but whatever.
> 
> 
>>      }
>>  
>>      /* Do KVM_ARM_VCPU_INIT ioctl */
>> -- 
>> 1.8.3.1
>>
>>
> 
> With some changes to the commit message
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 

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

[Wei Huang]

On 09/14/2016 03:37 AM, Andrew Jones wrote:
> On Wed, Sep 14, 2016 at 02:09:03AM -0400, Wei Huang wrote:
>> CPU vPMU is now turned off by default, but it was ON in 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 on in
>> virt-2.7 machine type to make sure it is backward compatible.
>>
>> Signed-off-by: Wei Huang <wei@redhat.com>
>> ---
>>  hw/arm/virt.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index a781ad0..83cfea7 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -91,6 +91,7 @@ typedef struct {
>>      bool secure;
>>      bool highmem;
>>      int32_t gic_version;
>> +    bool pmu;
>>  } VirtMachineState;
> 
> The compat bool doesn't need to be in machine state, so it should
> be in the class. See 'disallow_affinity_adjustment' in VirtMachineClass
> for an example of a adding a compat bool for a different reason. Also,
> I'd name it something like pmu_default_on.

This should be an easy fix.

> 
>>  
>>  #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
>> @@ -1317,6 +1318,11 @@ static void machvirt_init(MachineState *machine)
>>              }
>>          }
>>  
>> +        if (vms->pmu) {
>> +            /* Note: the property name is "pmu", not "has_pmu" */
> 
> The comment is unnecessary. Please remove.
> 

I might still say something here. :-) The reason is the confusion of
"pmu" as a command line property and "has_pmu" as a property of ARMCPU.
Most other properties use the same naming, so they are OK. But PMU, due
to naming difference, did cause trouble for me and took a while to debug.

>> +            object_property_set_bool(cpuobj, true, "pmu", NULL);
>> +        }
>> +
>>          if (object_property_find(cpuobj, "reset-cbar", NULL)) {
>>              object_property_set_int(cpuobj, vbi->memmap[VIRT_CPUPERIPHS].base,
>>                                      "reset-cbar", &error_abort);
>> @@ -1510,6 +1516,8 @@ static void virt_2_7_instance_init(Object *obj)
>>      object_property_set_description(obj, "gic-version",
>>                                      "Set GIC version. "
>>                                      "Valid values are 2, 3 and host", NULL);
>> +    /* Default PMU is on for 2.7 */
> 
> This comment is unnecessary. Please remove.

OK

> 
>> +    vms->pmu = true;
>>  }
>>  
>>  static void virt_machine_2_7_options(MachineClass *mc)
>> @@ -1522,7 +1530,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(2, 7)
>>  
>>  static void virt_2_6_instance_init(Object *obj)
>>  {
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>>      virt_2_7_instance_init(obj);
>> +    /* Default PMU is off for 2.6 */
>> +    vms->pmu = false;
> 
> You shouldn't need to touch 2.6 code. The compat bool is false
> by default (BSS init), which is the correct value for every
> machine type, except the 2.7 machine type.

I will fix the comments, using the next email from you.

> 
>>  }
>>  
>>  static void virt_machine_2_6_options(MachineClass *mc)
>> -- 
>> 1.8.3.1
>>
>>
> 
> Thanks,
> drew
>