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

[Qemu-devel] [PATCH V7 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 PMU property"
  -M virt-2.8/virt,accel=tcg -cpu cortex-a57,pmu=on  NO     "No PMU property"
  -M virt-2.7,accel=tcg -cpu cortex-a57              NO     NO
  -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=off      NO     "No PMU property"
  -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=on       NO     "No PMU property"
  -M virt-2.6,accel=tcg -cpu cortex-a57              NO     NO
  -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=off      NO     "No PMU property"
  -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=on       NO     "No PMU property"

  -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, e.g.
    can't apply global cortex-a15-arm-cpu.pmu=off: Property '.pmu' not found

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):
  arm64: 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         | 14 ++++++++++++++
 target-arm/cpu.h         |  1 +
 target-arm/cpu64.c       |  2 ++
 target-arm/kvm64.c       | 17 ++++++++++++++---
 6 files changed, 40 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH V7 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. This option is only available for cortex-a57/cortex-53/
host under both KVM mode, but unavailable on ARMv7 and other
processors. It allows virt tools, such as libvirt, to determine the
exsitence of vPMU and configure it.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt-acpi-build.c |  2 +-
 hw/arm/virt.c            |  2 +-
 target-arm/cpu.c         | 14 ++++++++++++++
 target-arm/cpu.h         |  1 +
 target-arm/cpu64.c       |  2 ++
 target-arm/kvm64.c       | 17 ++++++++++++++---
 6 files changed, 33 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..8e62c6e 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) && kvm_enabled()) {
+        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,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         cpu->id_aa64pfr0 &= ~0xf000;
     }
 
+    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 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 V7 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.

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 V7 0/2] Add option to configure guest vPMU

[Andrew Jones]

On Fri, Oct 21, 2016 at 05:53:00PM -0400, Wei Huang 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.
> 
> 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 PMU property"
>   -M virt-2.8/virt,accel=tcg -cpu cortex-a57,pmu=on  NO     "No PMU property"
>   -M virt-2.7,accel=tcg -cpu cortex-a57              NO     NO
>   -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=off      NO     "No PMU property"
>   -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=on       NO     "No PMU property"
>   -M virt-2.6,accel=tcg -cpu cortex-a57              NO     NO
>   -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=off      NO     "No PMU property"
>   -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=on       NO     "No PMU property"
> 
>   -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, e.g.
>     can't apply global cortex-a15-arm-cpu.pmu=off: Property '.pmu' not found
> 
> V6->V7:
>   * change has_pmu variable type from OnOffAuto to Boolean
>   * only add "pmu" property to CPU under kvm mode, default ON

Hmm, if we don't allow the property with TCG then switching a guest from
KVM to TCG will require more than just an accelerator switch. That's a
bit annoying and I think we'd have to teach it to libvirt too. I'd prefer

 -M virt-2.8,accel=tcg -cpu cortex-a57         NO     NO
 -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=off NO     NO
 -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=on  NO     "Warning: PMU not
yet supported with TCG" (or something)


>   * 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):
>   arm64: 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         | 14 ++++++++++++++
>  target-arm/cpu.h         |  1 +
>  target-arm/cpu64.c       |  2 ++
>  target-arm/kvm64.c       | 17 ++++++++++++++---
>  6 files changed, 40 insertions(+), 5 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> 

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

[Andrew Jones]

On Fri, Oct 21, 2016 at 05:53:02PM -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.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt.c | 7 +++++++
>  1 file changed, 7 insertions(+)

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

> 
> 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 V7 0/2] Add option to configure guest vPMU

[Wei Huang]

On 10/24/2016 03:49 AM, Andrew Jones wrote:
> On Fri, Oct 21, 2016 at 05:53:00PM -0400, Wei Huang 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.
>>
>> 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 PMU property"
>>   -M virt-2.8/virt,accel=tcg -cpu cortex-a57,pmu=on  NO     "No PMU property"
>>   -M virt-2.7,accel=tcg -cpu cortex-a57              NO     NO
>>   -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=off      NO     "No PMU property"
>>   -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=on       NO     "No PMU property"
>>   -M virt-2.6,accel=tcg -cpu cortex-a57              NO     NO
>>   -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=off      NO     "No PMU property"
>>   -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=on       NO     "No PMU property"
>>
>>   -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, e.g.
>>     can't apply global cortex-a15-arm-cpu.pmu=off: Property '.pmu' not found
>>
>> V6->V7:
>>   * change has_pmu variable type from OnOffAuto to Boolean
>>   * only add "pmu" property to CPU under kvm mode, default ON
> 
> Hmm, if we don't allow the property with TCG then switching a guest from
> KVM to TCG will require more than just an accelerator switch. That's a
> bit annoying and I think we'd have to teach it to libvirt too. I'd prefer
> 
>  -M virt-2.8,accel=tcg -cpu cortex-a57         NO     NO
>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=off NO     NO
>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=on  NO     "Warning: PMU not
> yet supported with TCG" (or something)

I am fine with this request. But note that, if we enforce
pmu-default=ON, we can't tell "-cpu cortex-a57" apart from
"cortex-a57,pmu=on", implying that we have to print the warning msg for
the case of "cortex-a57" as well (this was why we switch to tri-state
before). To solve this problem, we have to switch from pmu-default=ON to
pmu-default=OFF under TCG mode, something like:

    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
        qdev_property_add_static(DEVICE(obj),
            &arm_cpu_has_pmu_property, &error_abort);
        if (!kvm_enabled())
            object_property_set_bool(obj, false, "pmu", NULL);
    }

Then we do:
    if (cpu->has_pmu && !kvm_enabled()) {
        cpu->has_pmu = false;
        if (!pmu_warned && !qtest_enabled()) {
            error_report("warning: pmu not supported under TCG");
            pmu_warned = true;
        }
    }

This will work. Are you and Peter OK with this solution?

-Wei


> 
> 
>>   * 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):
>>   arm64: 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         | 14 ++++++++++++++
>>  target-arm/cpu.h         |  1 +
>>  target-arm/cpu64.c       |  2 ++
>>  target-arm/kvm64.c       | 17 ++++++++++++++---
>>  6 files changed, 40 insertions(+), 5 deletions(-)
>>
>> -- 
>> 1.8.3.1
>>
>>

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

[Andrew Jones]

On Mon, Oct 24, 2016 at 11:39:59PM -0500, Wei Huang wrote:
> >> V6->V7:
> >>   * change has_pmu variable type from OnOffAuto to Boolean
> >>   * only add "pmu" property to CPU under kvm mode, default ON
> > 
> > Hmm, if we don't allow the property with TCG then switching a guest from
> > KVM to TCG will require more than just an accelerator switch. That's a
> > bit annoying and I think we'd have to teach it to libvirt too. I'd prefer
> > 
> >  -M virt-2.8,accel=tcg -cpu cortex-a57         NO     NO
> >  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=off NO     NO
> >  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=on  NO     "Warning: PMU not
> > yet supported with TCG" (or something)
> 
> I am fine with this request. But note that, if we enforce
> pmu-default=ON, we can't tell "-cpu cortex-a57" apart from
> "cortex-a57,pmu=on", implying that we have to print the warning msg for
> the case of "cortex-a57" as well (this was why we switch to tri-state
> before). To solve this problem, we have to switch from pmu-default=ON to
> pmu-default=OFF under TCG mode, something like:
> 
>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>         qdev_property_add_static(DEVICE(obj),
>             &arm_cpu_has_pmu_property, &error_abort);
>         if (!kvm_enabled())
>             object_property_set_bool(obj, false, "pmu", NULL);
>     }
> 
> Then we do:
>     if (cpu->has_pmu && !kvm_enabled()) {
>         cpu->has_pmu = false;
>         if (!pmu_warned && !qtest_enabled()) {
>             error_report("warning: pmu not supported under TCG");
>             pmu_warned = true;
>         }
>     }
> 
> This will work. Are you and Peter OK with this solution?
>

Yes, I prefer it to not being able to easily switch a command line from
KVM to TCG. Granted, right now we mostly need to run with -cpu host for
KVM, which also needs to be switched for TCG. However that will hopefully
change with Shannon's work.

Thanks,
drew

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

[Peter Maydell]

On 25 October 2016 at 08:33, Andrew Jones <drjones@redhat.com> wrote:
> On Mon, Oct 24, 2016 at 11:39:59PM -0500, Wei Huang wrote:
>> >> V6->V7:
>> >>   * change has_pmu variable type from OnOffAuto to Boolean
>> >>   * only add "pmu" property to CPU under kvm mode, default ON
>> >
>> > Hmm, if we don't allow the property with TCG then switching a guest from
>> > KVM to TCG will require more than just an accelerator switch. That's a
>> > bit annoying and I think we'd have to teach it to libvirt too. I'd prefer
>> >
>> >  -M virt-2.8,accel=tcg -cpu cortex-a57         NO     NO
>> >  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=off NO     NO
>> >  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=on  NO     "Warning: PMU not
>> > yet supported with TCG" (or something)
>>
>> I am fine with this request. But note that, if we enforce
>> pmu-default=ON, we can't tell "-cpu cortex-a57" apart from
>> "cortex-a57,pmu=on", implying that we have to print the warning msg for
>> the case of "cortex-a57" as well (this was why we switch to tri-state
>> before). To solve this problem, we have to switch from pmu-default=ON to
>> pmu-default=OFF under TCG mode, something like:
>>
>>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>>         qdev_property_add_static(DEVICE(obj),
>>             &arm_cpu_has_pmu_property, &error_abort);
>>         if (!kvm_enabled())
>>             object_property_set_bool(obj, false, "pmu", NULL);
>>     }
>>
>> Then we do:
>>     if (cpu->has_pmu && !kvm_enabled()) {
>>         cpu->has_pmu = false;
>>         if (!pmu_warned && !qtest_enabled()) {
>>             error_report("warning: pmu not supported under TCG");
>>             pmu_warned = true;
>>         }
>>     }
>>
>> This will work. Are you and Peter OK with this solution?
>>
>
> Yes, I prefer it to not being able to easily switch a command line from
> KVM to TCG. Granted, right now we mostly need to run with -cpu host for
> KVM, which also needs to be switched for TCG. However that will hopefully
> change with Shannon's work.

If the TCG default is different from the KVM default then you
will have problems with switching it anyway, because it will
silently change behaviour underfoot. I think we should avoid that...

thanks
-- PMM

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

[Peter Maydell]

On 25 October 2016 at 16:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 October 2016 at 08:33, Andrew Jones <drjones@redhat.com> wrote:
>> Yes, I prefer it to not being able to easily switch a command line from
>> KVM to TCG. Granted, right now we mostly need to run with -cpu host for
>> KVM, which also needs to be switched for TCG. However that will hopefully
>> change with Shannon's work.
>
> If the TCG default is different from the KVM default then you
> will have problems with switching it anyway, because it will
> silently change behaviour underfoot. I think we should avoid that...

So here's my suggestion: we should just not worry about printing
a warning for not actually honouring the "please give me a PMU"
flag under TCG. At some point we'll actually implement it,
at which point the problem goes away. In the meantime (a) not
providing a PMU and not bothering to say so is what we were
doing before, and (b) there's lots of bits of the CPU that we
don't actually yet implement in TCG but which will appear as
we fix bugs in the emulation.

thanks
-- PMM

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

[Wei Huang]

On 10/25/2016 10:26 AM, Peter Maydell wrote:
> On 25 October 2016 at 08:33, Andrew Jones <drjones@redhat.com> wrote:
>> On Mon, Oct 24, 2016 at 11:39:59PM -0500, Wei Huang wrote:
>>>>> V6->V7:
>>>>>   * change has_pmu variable type from OnOffAuto to Boolean
>>>>>   * only add "pmu" property to CPU under kvm mode, default ON
>>>>
>>>> Hmm, if we don't allow the property with TCG then switching a guest from
>>>> KVM to TCG will require more than just an accelerator switch. That's a
>>>> bit annoying and I think we'd have to teach it to libvirt too. I'd prefer
>>>>
>>>>  -M virt-2.8,accel=tcg -cpu cortex-a57         NO     NO
>>>>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=off NO     NO
>>>>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=on  NO     "Warning: PMU not
>>>> yet supported with TCG" (or something)
>>>
>>> I am fine with this request. But note that, if we enforce
>>> pmu-default=ON, we can't tell "-cpu cortex-a57" apart from
>>> "cortex-a57,pmu=on", implying that we have to print the warning msg for
>>> the case of "cortex-a57" as well (this was why we switch to tri-state
>>> before). To solve this problem, we have to switch from pmu-default=ON to
>>> pmu-default=OFF under TCG mode, something like:
>>>
>>>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>>>         qdev_property_add_static(DEVICE(obj),
>>>             &arm_cpu_has_pmu_property, &error_abort);
>>>         if (!kvm_enabled())
>>>             object_property_set_bool(obj, false, "pmu", NULL);
>>>     }
>>>
>>> Then we do:
>>>     if (cpu->has_pmu && !kvm_enabled()) {
>>>         cpu->has_pmu = false;
>>>         if (!pmu_warned && !qtest_enabled()) {
>>>             error_report("warning: pmu not supported under TCG");
>>>             pmu_warned = true;
>>>         }
>>>     }
>>>
>>> This will work. Are you and Peter OK with this solution?
>>>
>>
>> Yes, I prefer it to not being able to easily switch a command line from
>> KVM to TCG. Granted, right now we mostly need to run with -cpu host for
>> KVM, which also needs to be switched for TCG. However that will hopefully
>> change with Shannon's work.
> 
> If the TCG default is different from the KVM default then you
> will have problems with switching it anyway, because it will
> silently change behaviour underfoot. I think we should avoid that...

Compared with V7, my proposed solution above isn't so different as we
thought. Details below. In V7,

* has_pmu=off by default. It is turned on in target-arm/cpu.c (see
below). Apparently it is turned on only under KVM mode; UNDER TCG, IT
REMAINS OFF.

    if (arm_feature(&cpu->env, ARM_FEATURE_PMU) && kvm_enabled()) {
        qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property,
                                 &error_abort);
    }
* We then remove ARM_FEATURE_PMU if has_pmu=off. The rest of code will
know if PMU is on/off using arm_feature(&cpu->env, ARM_FEATURE_PMU).

With this implementation, V7 can
* disable PMU under TCG mode
* print out an error when "pmu=on|off" property is specified under TCG.

Drew didn't like it, he wanted a warning when "pmu=on", and didn't want
it when "pmu=off". This is fair. To do that, we need to add
arm_cpu_has_pmu_property for both KVM and TCG. But, in this case,
printing a warning message only for "pmu=on" isn't possible without
tri-state, because we can't tell if pmu is on by default OR turned on by
users using ",pmu=on".

To solve this problem, in my proposed solution above, we still add
arm_cpu_has_pmu_property under TCG, but turned it off (see below).

    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
        qdev_property_add_static(DEVICE(obj),
            &arm_cpu_has_pmu_property, &error_abort);
        if (!kvm_enabled())
            object_property_set_bool(obj, false, "pmu", NULL);
    }

Because has_pmu=off now, we remove ARM_FEATURE_PMU under TCG. THIS IS
SAME AS V7. If pmu=on is detected later, we know end-users turn it on
intentionally and can flag a warning message. With this approach, we can:
* disable PMU under TCG mode
* print a warning when ",pmu=on" is specified; it remains silent under
other cases.

I think removing tri-state and printing out a warning only for ",pmu=on"
contradict each other. We need some trick to solve this problem. The
proposed solution didn't change the behavior underfoot as it might sound
like. If you have other suggestion, I want to know the details.

Thanks,
-Wei

> 
> thanks
> -- PMM
> 

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

[Wei Huang]

On 10/25/2016 10:59 AM, Peter Maydell wrote:
> On 25 October 2016 at 16:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 25 October 2016 at 08:33, Andrew Jones <drjones@redhat.com> wrote:
>>> Yes, I prefer it to not being able to easily switch a command line from
>>> KVM to TCG. Granted, right now we mostly need to run with -cpu host for
>>> KVM, which also needs to be switched for TCG. However that will hopefully
>>> change with Shannon's work.
>>
>> If the TCG default is different from the KVM default then you
>> will have problems with switching it anyway, because it will
>> silently change behaviour underfoot. I think we should avoid that...
> 
> So here's my suggestion: we should just not worry about printing
> a warning for not actually honouring the "please give me a PMU"
> flag under TCG. At some point we'll actually implement it,
> at which point the problem goes away. In the meantime (a) not
> providing a PMU and not bothering to say so is what we were
> doing before, and (b) there's lots of bits of the CPU that we
> don't actually yet implement in TCG but which will appear as
> we fix bugs in the emulation.

This is easy to implement. If Drew is OK with silence under TCG mode, I
will send V8 out.

> 
> thanks
> -- PMM
> 

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

[Andrew Jones]

On Tue, Oct 25, 2016 at 11:55:05AM -0500, Wei Huang wrote:
> 
> 
> On 10/25/2016 10:26 AM, Peter Maydell wrote:
> > On 25 October 2016 at 08:33, Andrew Jones <drjones@redhat.com> wrote:
> >> On Mon, Oct 24, 2016 at 11:39:59PM -0500, Wei Huang wrote:
> >>>>> V6->V7:
> >>>>>   * change has_pmu variable type from OnOffAuto to Boolean
> >>>>>   * only add "pmu" property to CPU under kvm mode, default ON
> >>>>
> >>>> Hmm, if we don't allow the property with TCG then switching a guest from
> >>>> KVM to TCG will require more than just an accelerator switch. That's a
> >>>> bit annoying and I think we'd have to teach it to libvirt too. I'd prefer
> >>>>
> >>>>  -M virt-2.8,accel=tcg -cpu cortex-a57         NO     NO
> >>>>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=off NO     NO
> >>>>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=on  NO     "Warning: PMU not
> >>>> yet supported with TCG" (or something)
> >>>
> >>> I am fine with this request. But note that, if we enforce
> >>> pmu-default=ON, we can't tell "-cpu cortex-a57" apart from
> >>> "cortex-a57,pmu=on", implying that we have to print the warning msg for
> >>> the case of "cortex-a57" as well (this was why we switch to tri-state
> >>> before). To solve this problem, we have to switch from pmu-default=ON to
> >>> pmu-default=OFF under TCG mode, something like:
> >>>
> >>>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
> >>>         qdev_property_add_static(DEVICE(obj),
> >>>             &arm_cpu_has_pmu_property, &error_abort);
> >>>         if (!kvm_enabled())
> >>>             object_property_set_bool(obj, false, "pmu", NULL);
> >>>     }
> >>>
> >>> Then we do:
> >>>     if (cpu->has_pmu && !kvm_enabled()) {
> >>>         cpu->has_pmu = false;
> >>>         if (!pmu_warned && !qtest_enabled()) {
> >>>             error_report("warning: pmu not supported under TCG");
> >>>             pmu_warned = true;
> >>>         }
> >>>     }
> >>>
> >>> This will work. Are you and Peter OK with this solution?
> >>>
> >>
> >> Yes, I prefer it to not being able to easily switch a command line from
> >> KVM to TCG. Granted, right now we mostly need to run with -cpu host for
> >> KVM, which also needs to be switched for TCG. However that will hopefully
> >> change with Shannon's work.
> > 
> > If the TCG default is different from the KVM default then you
> > will have problems with switching it anyway, because it will
> > silently change behaviour underfoot. I think we should avoid that...
> 
> Compared with V7, my proposed solution above isn't so different as we
> thought. Details below. In V7,
> 
> * has_pmu=off by default. It is turned on in target-arm/cpu.c (see
> below). Apparently it is turned on only under KVM mode; UNDER TCG, IT
> REMAINS OFF.
> 
>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU) && kvm_enabled()) {
>         qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property,
>                                  &error_abort);
>     }
> * We then remove ARM_FEATURE_PMU if has_pmu=off. The rest of code will
> know if PMU is on/off using arm_feature(&cpu->env, ARM_FEATURE_PMU).
> 
> With this implementation, V7 can
> * disable PMU under TCG mode
> * print out an error when "pmu=on|off" property is specified under TCG.
> 
> Drew didn't like it, he wanted a warning when "pmu=on", and didn't want
> it when "pmu=off". This is fair. To do that, we need to add
> arm_cpu_has_pmu_property for both KVM and TCG. But, in this case,
> printing a warning message only for "pmu=on" isn't possible without
> tri-state, because we can't tell if pmu is on by default OR turned on by
> users using ",pmu=on".
> 
> To solve this problem, in my proposed solution above, we still add
> arm_cpu_has_pmu_property under TCG, but turned it off (see below).
> 
>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>         qdev_property_add_static(DEVICE(obj),
>             &arm_cpu_has_pmu_property, &error_abort);
>         if (!kvm_enabled())
>             object_property_set_bool(obj, false, "pmu", NULL);
>     }
> 
> Because has_pmu=off now, we remove ARM_FEATURE_PMU under TCG. THIS IS
> SAME AS V7. If pmu=on is detected later, we know end-users turn it on
> intentionally and can flag a warning message. With this approach, we can:
> * disable PMU under TCG mode
> * print a warning when ",pmu=on" is specified; it remains silent under
> other cases.
> 
> I think removing tri-state and printing out a warning only for ",pmu=on"
> contradict each other. We need some trick to solve this problem. The
> proposed solution didn't change the behavior underfoot as it might sound
> like. If you have other suggestion, I want to know the details.
>

I'm a bit lost as to which proposal is which now, but what I didn't like
was QEMU failing to run with TCG when the same command line worked for
KVM. If the property doesn't exist when using TCG then QEMU fails when a
command line including ,pmu=on/off is used.

Peter says he's not worried about the PMU not working for TCG right now,
and thus isn't worried about warning that it doesn't work. I'm not sure
if he was proposing to keep your v7 - fail for tcg when the property was
used, or if he was proposing to just not worry about the property for tcg.
I believe the later case would be the proposed change to v7, but without
any warning.

Anyway, whatever you guys work out is fine by me. I can live with changing
the command line between KVM and TCG runs. Eventually I won't have to when
PMU support comes to TCG. I'm also fine with ,pmu=on silently not actually
providing a PMU when run under TCG. It's like Peter says, many things
under TCG don't work, but none of them warn about it. Only, in this case,
I feel a warning for ,pmu=on would be better, because the feature would be
getting asked for explicitly.

Thanks,
drew

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

[Wei Huang]

On 10/25/2016 12:56 PM, Andrew Jones wrote:
<snip>
>>
>> Compared with V7, my proposed solution above isn't so different as we
>> thought. Details below. In V7,
>>
>> * has_pmu=off by default. It is turned on in target-arm/cpu.c (see
>> below). Apparently it is turned on only under KVM mode; UNDER TCG, IT
>> REMAINS OFF.
>>
>>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU) && kvm_enabled()) {
>>         qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property,
>>                                  &error_abort);
>>     }
>> * We then remove ARM_FEATURE_PMU if has_pmu=off. The rest of code will
>> know if PMU is on/off using arm_feature(&cpu->env, ARM_FEATURE_PMU).
>>
>> With this implementation, V7 can
>> * disable PMU under TCG mode
>> * print out an error when "pmu=on|off" property is specified under TCG.
>>
>> Drew didn't like it, he wanted a warning when "pmu=on", and didn't want
>> it when "pmu=off". This is fair. To do that, we need to add
>> arm_cpu_has_pmu_property for both KVM and TCG. But, in this case,
>> printing a warning message only for "pmu=on" isn't possible without
>> tri-state, because we can't tell if pmu is on by default OR turned on by
>> users using ",pmu=on".
>>
>> To solve this problem, in my proposed solution above, we still add
>> arm_cpu_has_pmu_property under TCG, but turned it off (see below).
>>
>>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>>         qdev_property_add_static(DEVICE(obj),
>>             &arm_cpu_has_pmu_property, &error_abort);
>>         if (!kvm_enabled())
>>             object_property_set_bool(obj, false, "pmu", NULL);
>>     }
>>
>> Because has_pmu=off now, we remove ARM_FEATURE_PMU under TCG. THIS IS
>> SAME AS V7. If pmu=on is detected later, we know end-users turn it on
>> intentionally and can flag a warning message. With this approach, we can:
>> * disable PMU under TCG mode
>> * print a warning when ",pmu=on" is specified; it remains silent under
>> other cases.
>>
>> I think removing tri-state and printing out a warning only for ",pmu=on"
>> contradict each other. We need some trick to solve this problem. The
>> proposed solution didn't change the behavior underfoot as it might sound
>> like. If you have other suggestion, I want to know the details.
>>
> 
> I'm a bit lost as to which proposal is which now, but what I didn't like
> was QEMU failing to run with TCG when the same command line worked for
> KVM. If the property doesn't exist when using TCG then QEMU fails when a
> command line including ,pmu=on/off is used.
> 
> Peter says he's not worried about the PMU not working for TCG right now,
> and thus isn't worried about warning that it doesn't work. I'm not sure
> if he was proposing to keep your v7 - fail for tcg when the property was
> used, or if he was proposing to just not worry about the property for tcg.
> I believe the later case would be the proposed change to v7, but without
> any warning.

I think Peter means the later case, which requires a fix to v7. It will
keep the same command line for both TCG and KVM; but TCG will ignore
silently without a warning when ",pmu=on".

> 
> Anyway, whatever you guys work out is fine by me. I can live with changing
> the command line between KVM and TCG runs. Eventually I won't have to when
> PMU support comes to TCG. I'm also fine with ,pmu=on silently not actually
> providing a PMU when run under TCG. It's like Peter says, many things

I prefer the second 2nd one. If you are OK, I will send in a new spin (V8).

> under TCG don't work, but none of them warn about it. Only, in this case,
> I feel a warning for ,pmu=on would be better, because the feature would be
> getting asked for explicitly.
> 
> Thanks,
> drew
> 

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

[Andrew Jones]

On Tue, Oct 25, 2016 at 01:50:01PM -0500, Wei Huang wrote:
> 
> 
> On 10/25/2016 12:56 PM, Andrew Jones wrote:
> <snip>
> >>
> >> Compared with V7, my proposed solution above isn't so different as we
> >> thought. Details below. In V7,
> >>
> >> * has_pmu=off by default. It is turned on in target-arm/cpu.c (see
> >> below). Apparently it is turned on only under KVM mode; UNDER TCG, IT
> >> REMAINS OFF.
> >>
> >>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU) && kvm_enabled()) {
> >>         qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property,
> >>                                  &error_abort);
> >>     }
> >> * We then remove ARM_FEATURE_PMU if has_pmu=off. The rest of code will
> >> know if PMU is on/off using arm_feature(&cpu->env, ARM_FEATURE_PMU).
> >>
> >> With this implementation, V7 can
> >> * disable PMU under TCG mode
> >> * print out an error when "pmu=on|off" property is specified under TCG.
> >>
> >> Drew didn't like it, he wanted a warning when "pmu=on", and didn't want
> >> it when "pmu=off". This is fair. To do that, we need to add
> >> arm_cpu_has_pmu_property for both KVM and TCG. But, in this case,
> >> printing a warning message only for "pmu=on" isn't possible without
> >> tri-state, because we can't tell if pmu is on by default OR turned on by
> >> users using ",pmu=on".
> >>
> >> To solve this problem, in my proposed solution above, we still add
> >> arm_cpu_has_pmu_property under TCG, but turned it off (see below).
> >>
> >>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
> >>         qdev_property_add_static(DEVICE(obj),
> >>             &arm_cpu_has_pmu_property, &error_abort);
> >>         if (!kvm_enabled())
> >>             object_property_set_bool(obj, false, "pmu", NULL);
> >>     }
> >>
> >> Because has_pmu=off now, we remove ARM_FEATURE_PMU under TCG. THIS IS
> >> SAME AS V7. If pmu=on is detected later, we know end-users turn it on
> >> intentionally and can flag a warning message. With this approach, we can:
> >> * disable PMU under TCG mode
> >> * print a warning when ",pmu=on" is specified; it remains silent under
> >> other cases.
> >>
> >> I think removing tri-state and printing out a warning only for ",pmu=on"
> >> contradict each other. We need some trick to solve this problem. The
> >> proposed solution didn't change the behavior underfoot as it might sound
> >> like. If you have other suggestion, I want to know the details.
> >>
> > 
> > I'm a bit lost as to which proposal is which now, but what I didn't like
> > was QEMU failing to run with TCG when the same command line worked for
> > KVM. If the property doesn't exist when using TCG then QEMU fails when a
> > command line including ,pmu=on/off is used.
> > 
> > Peter says he's not worried about the PMU not working for TCG right now,
> > and thus isn't worried about warning that it doesn't work. I'm not sure
> > if he was proposing to keep your v7 - fail for tcg when the property was
> > used, or if he was proposing to just not worry about the property for tcg.
> > I believe the later case would be the proposed change to v7, but without
> > any warning.
> 
> I think Peter means the later case, which requires a fix to v7. It will
> keep the same command line for both TCG and KVM; but TCG will ignore
> silently without a warning when ",pmu=on".
> 
> > 
> > Anyway, whatever you guys work out is fine by me. I can live with changing
> > the command line between KVM and TCG runs. Eventually I won't have to when
> > PMU support comes to TCG. I'm also fine with ,pmu=on silently not actually
> > providing a PMU when run under TCG. It's like Peter says, many things
> 
> I prefer the second 2nd one. If you are OK, I will send in a new spin (V8).

Works for me.


> 
> > under TCG don't work, but none of them warn about it. Only, in this case,
> > I feel a warning for ,pmu=on would be better, because the feature would be
> > getting asked for explicitly.
> > 
> > Thanks,
> > drew
> > 
> 

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

[Peter Maydell]

On 21 October 2016 at 22:53, Wei Huang <wei@redhat.com> wrote:
> This patch adds a pmu=[on/off] option to enable/disable vPMU support
> in guest vCPU. This option is only available for cortex-a57/cortex-53/
> host under both KVM mode, but unavailable on ARMv7 and other
> processors. It allows virt tools, such as libvirt, to determine the
> exsitence of vPMU and configure it.

So what actually are we defining as "the PMU exists" here?
The PMU exists in ARMv7 and we actually implement at least some
of it in TCG including for 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 ?

thanks
-- PMM