[Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Qemu-devel] [PATCH RFC 1/1] 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. There are several reasons to justify this option. First
vpmu can be problematic for cross-migration between different SoC as
perf counters is architecture-dependent. It is more flexible to
have an option to turn it on/off. Secondly it matches the -cpu pmu
option in libivrt. This patch has been tested on both DT/ACPI modes.

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

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 28fc59c..dc5f66d 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 (armcpu->enable_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..6aea901 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 (!armcpu->enable_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..f7daf81 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = {
 };
 
 static Property arm_cpu_properties[] = {
+    DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true),
     DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
     DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
     DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 76d824d..f2341c0 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -579,8 +579,9 @@ struct ARMCPU {
     bool powered_off;
     /* CPU has security extension */
     bool has_el3;
-    /* CPU has PMU (Performance Monitor Unit) */
-    bool has_pmu;
+
+    /* CPU has vPMU (Performance Monitor Unit) support */
+    bool enable_pmu;
 
     /* CPU has memory protection unit */
     bool has_mpu;
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 5faa76c..ca21670 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -501,11 +501,11 @@ 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 vPMU based on KVM mode and hardware capability */
+    cpu->enable_pmu &= (kvm_irqchip_in_kernel() &&
+        kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3));
+    cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3;
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
     ret = kvm_arm_vcpu_init(cs);
-- 
2.4.11

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Shannon Zhao]

On 2016/7/29 0:38, Wei Huang wrote:
> This patch adds a pmu=[on/off] option to enable/disable vpmu support
> in guest vm. There are several reasons to justify this option. First
> vpmu can be problematic for cross-migration between different SoC as
> perf counters is architecture-dependent. It is more flexible to
> have an option to turn it on/off. Secondly it matches the -cpu pmu
> option in libivrt. This patch has been tested on both DT/ACPI modes.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
Reviewed-by：Shannon Zhao <shannon.zhao@linaro.org>

> ---
>  hw/arm/virt-acpi-build.c |  2 +-
>  hw/arm/virt.c            |  2 +-
>  target-arm/cpu.c         |  1 +
>  target-arm/cpu.h         |  5 +++--
>  target-arm/kvm64.c       | 10 +++++-----
>  5 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 28fc59c..dc5f66d 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 (armcpu->enable_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..6aea901 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 (!armcpu->enable_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..f7daf81 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = {
>  };
>  
>  static Property arm_cpu_properties[] = {
> +    DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true),
>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>      DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 76d824d..f2341c0 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -579,8 +579,9 @@ struct ARMCPU {
>      bool powered_off;
>      /* CPU has security extension */
>      bool has_el3;
> -    /* CPU has PMU (Performance Monitor Unit) */
> -    bool has_pmu;
> +
> +    /* CPU has vPMU (Performance Monitor Unit) support */
> +    bool enable_pmu;
>  
>      /* CPU has memory protection unit */
>      bool has_mpu;
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 5faa76c..ca21670 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -501,11 +501,11 @@ 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 vPMU based on KVM mode and hardware capability */
> +    cpu->enable_pmu &= (kvm_irqchip_in_kernel() &&
> +        kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3));
> +    cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3;
>  
>      /* Do KVM_ARM_VCPU_INIT ioctl */
>      ret = kvm_arm_vcpu_init(cs);
> 

-- 
Shannon

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Andrew Jones]

On Thu, Jul 28, 2016 at 11:38:16AM -0500, Wei Huang wrote:
> This patch adds a pmu=[on/off] option to enable/disable vpmu support
> in guest vm. There are several reasons to justify this option. First
> vpmu can be problematic for cross-migration between different SoC as
> perf counters is architecture-dependent. It is more flexible to
> have an option to turn it on/off. Secondly it matches the -cpu pmu
> option in libivrt. This patch has been tested on both DT/ACPI modes.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c |  2 +-
>  hw/arm/virt.c            |  2 +-
>  target-arm/cpu.c         |  1 +
>  target-arm/cpu.h         |  5 +++--
>  target-arm/kvm64.c       | 10 +++++-----
>  5 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 28fc59c..dc5f66d 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 (armcpu->enable_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..6aea901 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 (!armcpu->enable_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..f7daf81 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = {
>  };
>  
>  static Property arm_cpu_properties[] = {
> +    DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true),

x86's pmu property defaults to off. I'm not sure if it's necessary to
have a consistent default between x86 and arm in order for libvirt to
be able to use it in the same way. We should confirm with libvirt
people. Anyway, I think I'd prefer we default off here, and then we
can default on in machine code for configurations that we want it by
default (only AArch64 KVM). Or, maybe we don't want it by default at
all? Possibly we should only set it on by default for virt-2.6, and
then, from 2.7 on, require users to opt-in to the feature. It makes
sense to require opting-in to features that can cause problems with
migration.

>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>      DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 76d824d..f2341c0 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -579,8 +579,9 @@ struct ARMCPU {
>      bool powered_off;
>      /* CPU has security extension */
>      bool has_el3;
> -    /* CPU has PMU (Performance Monitor Unit) */
> -    bool has_pmu;
> +
> +    /* CPU has vPMU (Performance Monitor Unit) support */
> +    bool enable_pmu;
>  
>      /* CPU has memory protection unit */
>      bool has_mpu;
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 5faa76c..ca21670 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -501,11 +501,11 @@ 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 vPMU based on KVM mode and hardware capability */
> +    cpu->enable_pmu &= (kvm_irqchip_in_kernel() &&
> +        kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3));

nit: the () aren't necessary

> +    cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3;
>  
>      /* Do KVM_ARM_VCPU_INIT ioctl */
>      ret = kvm_arm_vcpu_init(cs);
> -- 
> 2.4.11
> 
>

OK, so this property will be exposed to all ARM cpu types, and if a user
turns it on, then it will stay on for all types, except when using KVM
with an aarch64 cpu type, and KVM doesn't support it. This could mislead
users to believe they'll get a pmu, by simply adding pmu=on, even when
they can't. I think we'd ideally keep has_pmu, and the current code that
sets it, and then add code like

 if (enable_pmu && !has_pmu) {
   error_report("Warning: ...")
 }

somewhere. Unfortunately I don't think there's any one place we could
add that. We'd need to add it to every ARM machine type that cares about
not misleading users. Too bad cpu properties aren't whitelisted by
machines to avoid this issue.

Anyway, all that said, I see this is just how cpu properties currently
work, so we probably don't need to worry about it for every machine. I
do still suggest we add the above warning to mach-virt though.

Thanks,
drew

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Wei Huang]

On 07/29/2016 01:54 AM, Andrew Jones wrote:
> On Thu, Jul 28, 2016 at 11:38:16AM -0500, Wei Huang wrote:
>> This patch adds a pmu=[on/off] option to enable/disable vpmu support
>> in guest vm. There are several reasons to justify this option. First
>> vpmu can be problematic for cross-migration between different SoC as
>> perf counters is architecture-dependent. It is more flexible to
>> have an option to turn it on/off. Secondly it matches the -cpu pmu
>> option in libivrt. This patch has been tested on both DT/ACPI modes.
>>
>> Signed-off-by: Wei Huang <wei@redhat.com>
>> ---
>>  hw/arm/virt-acpi-build.c |  2 +-
>>  hw/arm/virt.c            |  2 +-
>>  target-arm/cpu.c         |  1 +
>>  target-arm/cpu.h         |  5 +++--
>>  target-arm/kvm64.c       | 10 +++++-----
>>  5 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 28fc59c..dc5f66d 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 (armcpu->enable_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..6aea901 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 (!armcpu->enable_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..f7daf81 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = {
>>  };
>>  
>>  static Property arm_cpu_properties[] = {
>> +    DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true),
> 
> x86's pmu property defaults to off. I'm not sure if it's necessary to
> have a consistent default between x86 and arm in order for libvirt to
> be able to use it in the same way. We should confirm with libvirt
> people. Anyway, I think I'd prefer we default off here, and then we
> can default on in machine code for configurations that we want it by
> default (only AArch64 KVM). Or, maybe we don't want it by default at
> all? Possibly we should only set it on by default for virt-2.6, and
> then, from 2.7 on, require users to opt-in to the feature. It makes
> sense to require opting-in to features that can cause problems with
> migration.

This option is default=off on x86. I agree that it is a compatibility
related issue which can break migration. So it is reasonable to turn it
off on ARM as well. Let us wait for libvirt people to voice out.

> 
>>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>>      DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
>>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 76d824d..f2341c0 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -579,8 +579,9 @@ struct ARMCPU {
>>      bool powered_off;
>>      /* CPU has security extension */
>>      bool has_el3;
>> -    /* CPU has PMU (Performance Monitor Unit) */
>> -    bool has_pmu;
>> +
>> +    /* CPU has vPMU (Performance Monitor Unit) support */
>> +    bool enable_pmu;
>>  
>>      /* CPU has memory protection unit */
>>      bool has_mpu;
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index 5faa76c..ca21670 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -501,11 +501,11 @@ 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 vPMU based on KVM mode and hardware capability */
>> +    cpu->enable_pmu &= (kvm_irqchip_in_kernel() &&
>> +        kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3));
> 
> nit: the () aren't necessary
> 

Will fix

>> +    cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3;
>>  
>>      /* Do KVM_ARM_VCPU_INIT ioctl */
>>      ret = kvm_arm_vcpu_init(cs);
>> -- 
>> 2.4.11
>>
>>
> 
> OK, so this property will be exposed to all ARM cpu types, and if a user
> turns it on, then it will stay on for all types, except when using KVM
> with an aarch64 cpu type, and KVM doesn't support it. This could mislead
> users to believe they'll get a pmu, by simply adding pmu=on, even when
> they can't. I think we'd ideally keep has_pmu, and the current code that
> sets it, and then add code like
> 
>  if (enable_pmu && !has_pmu) {
>    error_report("Warning: ...")
>  }
> 
> somewhere. Unfortunately I don't think there's any one place we could
> add that. We'd need to add it to every ARM machine type that cares about
> not misleading users. Too bad cpu properties aren't whitelisted by
> machines to avoid this issue.
> 
> Anyway, all that said, I see this is just how cpu properties currently
> work, so we probably don't need to worry about it for every machine. I
> do still suggest we add the above warning to mach-virt though.

Will try to keep both has_pmu & enable_pmu in the next re-spin...

> 
> Thanks,
> drew
> 

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Peter Maydell]

On 29 July 2016 at 07:54, Andrew Jones <drjones@redhat.com> wrote:
> OK, so this property will be exposed to all ARM cpu types, and if a user
> turns it on, then it will stay on for all types, except when using KVM
> with an aarch64 cpu type, and KVM doesn't support it. This could mislead
> users to believe they'll get a pmu, by simply adding pmu=on, even when
> they can't. I think we'd ideally keep has_pmu, and the current code that
> sets it, and then add code like
>
>  if (enable_pmu && !has_pmu) {
>    error_report("Warning: ...")
>  }
>
> somewhere.

I think we should probably follow the existing model used by
has_el3, where the property only exists if it's valid to set it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Andrea Bolognani]

On Fri, 2016-07-29 at 08:54 +0200, Andrew Jones wrote:
> On Thu, Jul 28, 2016 at 11:38:16AM -0500, Wei Huang wrote:
> > 
> > This patch adds a pmu=[on/off] option to enable/disable vpmu support
> > in guest vm. There are several reasons to justify this option. First
> > vpmu can be problematic for cross-migration between different SoC as
> > perf counters is architecture-dependent. It is more flexible to
> > have an option to turn it on/off. Secondly it matches the -cpu pmu
> > option in libivrt. This patch has been tested on both DT/ACPI modes.
> > 
> > Signed-off-by: Wei Huang <wei@redhat.com>
> > ---
> >  hw/arm/virt-acpi-build.c |  2 +-
> >  hw/arm/virt.c            |  2 +-
> >  target-arm/cpu.c         |  1 +
> >  target-arm/cpu.h         |  5 +++--
> >  target-arm/kvm64.c       | 10 +++++-----
> >  5 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 28fc59c..dc5f66d 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 (armcpu->enable_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..6aea901 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 (!armcpu->enable_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..f7daf81 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = {
> >  };
> >  
> >  static Property arm_cpu_properties[] = {
> > +    DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true),
> 
> x86's pmu property defaults to off. I'm not sure if it's necessary to
> have a consistent default between x86 and arm in order for libvirt to
> be able to use it in the same way. We should confirm with libvirt
> people. Anyway, I think I'd prefer we default off here, and then we
> can default on in machine code for configurations that we want it by
> default (only AArch64 KVM). Or, maybe we don't want it by default at
> all? Possibly we should only set it on by default for virt-2.6, and
> then, from 2.7 on, require users to opt-in to the feature. It makes
> sense to require opting-in to features that can cause problems with
> migration.

After thinking about this a bit, I don't think it matters that
much (from libvirt's point of view) whether the default is on
or off - there are a bunch of other situations where the user
is required to specify explicitly whether he wants the feature
or not, and if he doesn't choose either side he will get
whatever QEMU uses as a default.

What's important is that the user can pick one or the other
when it matters to him, and having a pmu property like the one
x86 already has fits the bill.

That said, defaulting to off looks like it would be the least
confusing behaviour.

> > +    cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3;
> >  
> >      /* Do KVM_ARM_VCPU_INIT ioctl */
> >      ret = kvm_arm_vcpu_init(cs);
> 
> OK, so this property will be exposed to all ARM cpu types, and if a user
> turns it on, then it will stay on for all types, except when using KVM
> with an aarch64 cpu type, and KVM doesn't support it. This could mislead
> users to believe they'll get a pmu, by simply adding pmu=on, even when
> they can't. I think we'd ideally keep has_pmu, and the current code that
> sets it, and then add code like
> 
>  if (enable_pmu && !has_pmu) {
>    error_report("Warning: ...")
>  }
> 
> somewhere. Unfortunately I don't think there's any one place we could
> add that. We'd need to add it to every ARM machine type that cares about
> not misleading users. Too bad cpu properties aren't whitelisted by
> machines to avoid this issue.
> 
> Anyway, all that said, I see this is just how cpu properties currently
> work, so we probably don't need to worry about it for every machine. I
> do still suggest we add the above warning to mach-virt though.

I'm not sure a warning is enough: if I start a guest and
explicitly ask for a PMU, I expect it to be there, or for
the guest not to start at all. How does x86 behave in this
regard?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Andrew Jones]

On Mon, Aug 01, 2016 at 02:04:59PM +0200, Andrea Bolognani wrote:
> On Fri, 2016-07-29 at 08:54 +0200, Andrew Jones wrote:
> > On Thu, Jul 28, 2016 at 11:38:16AM -0500, Wei Huang wrote:
> > > 
> > > This patch adds a pmu=[on/off] option to enable/disable vpmu support
> > > in guest vm. There are several reasons to justify this option. First
> > > vpmu can be problematic for cross-migration between different SoC as
> > > perf counters is architecture-dependent. It is more flexible to
> > > have an option to turn it on/off. Secondly it matches the -cpu pmu
> > > option in libivrt. This patch has been tested on both DT/ACPI modes.
> > > 
> > > Signed-off-by: Wei Huang <wei@redhat.com>
> > > ---
> > >  hw/arm/virt-acpi-build.c |  2 +-
> > >  hw/arm/virt.c            |  2 +-
> > >  target-arm/cpu.c         |  1 +
> > >  target-arm/cpu.h         |  5 +++--
> > >  target-arm/kvm64.c       | 10 +++++-----
> > >  5 files changed, 11 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 28fc59c..dc5f66d 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 (armcpu->enable_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..6aea901 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 (!armcpu->enable_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..f7daf81 100644
> > > --- a/target-arm/cpu.c
> > > +++ b/target-arm/cpu.c
> > > @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = {
> > >  };
> > >  
> > >  static Property arm_cpu_properties[] = {
> > > +    DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true),
> > 
> > x86's pmu property defaults to off. I'm not sure if it's necessary to
> > have a consistent default between x86 and arm in order for libvirt to
> > be able to use it in the same way. We should confirm with libvirt
> > people. Anyway, I think I'd prefer we default off here, and then we
> > can default on in machine code for configurations that we want it by
> > default (only AArch64 KVM). Or, maybe we don't want it by default at
> > all? Possibly we should only set it on by default for virt-2.6, and
> > then, from 2.7 on, require users to opt-in to the feature. It makes
> > sense to require opting-in to features that can cause problems with
> > migration.
> 
> After thinking about this a bit, I don't think it matters that
> much (from libvirt's point of view) whether the default is on
> or off - there are a bunch of other situations where the user
> is required to specify explicitly whether he wants the feature
> or not, and if he doesn't choose either side he will get
> whatever QEMU uses as a default.
> 
> What's important is that the user can pick one or the other
> when it matters to him, and having a pmu property like the one
> x86 already has fits the bill.
> 
> That said, defaulting to off looks like it would be the least
> confusing behaviour.

OK, so the default is still up for debate.

Pros of ON                Cons of ON
----------                ----------
We already do it          The default instance is less migratable
Less typing on cmdline
 (libvirt covers typing for us anyway...)

Pros of OFF               Cons of OFF
-----------               -----------
See 'Cons of ON'          See 'Pros on ON' (virt-2.6 needs compat code)

> 
> > > +    cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3;
> > >  
> > >      /* Do KVM_ARM_VCPU_INIT ioctl */
> > >      ret = kvm_arm_vcpu_init(cs);
> > 
> > OK, so this property will be exposed to all ARM cpu types, and if a user
> > turns it on, then it will stay on for all types, except when using KVM
> > with an aarch64 cpu type, and KVM doesn't support it. This could mislead
> > users to believe they'll get a pmu, by simply adding pmu=on, even when
> > they can't. I think we'd ideally keep has_pmu, and the current code that
> > sets it, and then add code like
> > 
> >  if (enable_pmu && !has_pmu) {
> >    error_report("Warning: ...")
> >  }
> > 
> > somewhere. Unfortunately I don't think there's any one place we could
> > add that. We'd need to add it to every ARM machine type that cares about
> > not misleading users. Too bad cpu properties aren't whitelisted by
> > machines to avoid this issue.
> > 
> > Anyway, all that said, I see this is just how cpu properties currently
> > work, so we probably don't need to worry about it for every machine. I
> > do still suggest we add the above warning to mach-virt though.
> 
> I'm not sure a warning is enough: if I start a guest and
> explicitly ask for a PMU, I expect it to be there, or for
> the guest not to start at all. How does x86 behave in this
> regard?

Peter had a good suggestion for this. We need to wrap the property
addition in an arm_feature check like the has_el3 property. That will
remove it from all cpu types that don't support it. Then there's no
need for the enable_pmu && !has_pmu check as the has_pmu part is covered
very early with the feature flag in arm_cpu_post_init(). Peter also
suggested we keep the 'has_pmu' name, rather than change it to
'enable_pmu'. On that one I would disagree. 'has_pmu' indicates that the
feature is available at all, which it is to all cpu types that have the
arm feature, but not all users will want it enabled. 'enable_pmu', which
matches x86's naming, seems more appropriate for that.

Thanks,
drew

> 
> -- 
> Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Peter Maydell]

On 1 August 2016 at 14:08, Andrew Jones <drjones@redhat.com> wrote:
> Peter had a good suggestion for this. We need to wrap the property
> addition in an arm_feature check like the has_el3 property. That will
> remove it from all cpu types that don't support it. Then there's no
> need for the enable_pmu && !has_pmu check as the has_pmu part is covered
> very early with the feature flag in arm_cpu_post_init(). Peter also
> suggested we keep the 'has_pmu' name, rather than change it to
> 'enable_pmu'. On that one I would disagree. 'has_pmu' indicates that the
> feature is available at all, which it is to all cpu types that have the
> arm feature, but not all users will want it enabled. 'enable_pmu', which
> matches x86's naming, seems more appropriate for that.

If you create the CPU with pmu=off then it does not have a PMU,
and so has_pmu is false. I don't see any reason for the naming
convention for the PMU to diverge from what we have for EL3
and for the MPU, where the property names and the struct fields
both use 'has'.

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Andrea Bolognani]

On Mon, 2016-08-01 at 15:08 +0200, Andrew Jones wrote:
> > I'm not sure a warning is enough: if I start a guest and
> > explicitly ask for a PMU, I expect it to be there, or for
> > the guest not to start at all. How does x86 behave in this
> > regard?
> 
> Peter had a good suggestion for this. We need to wrap the property
> addition in an arm_feature check like the has_el3 property. That will
> remove it from all cpu types that don't support it.

Wouldn't that mean that you'd be unable to use

  -cpu foo,pmu=off

if CPU model 'foo' doesn't support a PMU? I'd expect that
to work.

I've played around with this a bit on x86 and it doesn't
look like it necessarily behaves the way I'd expect it to,
either, so maybe this is just a case of my expectations
being unreasonable? :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Peter Maydell]

On 1 August 2016 at 14:26, Andrea Bolognani <abologna@redhat.com> wrote:
> On Mon, 2016-08-01 at 15:08 +0200, Andrew Jones wrote:
>> > I'm not sure a warning is enough: if I start a guest and
>> > explicitly ask for a PMU, I expect it to be there, or for
>> > the guest not to start at all. How does x86 behave in this
>> > regard?
>>
>> Peter had a good suggestion for this. We need to wrap the property
>> addition in an arm_feature check like the has_el3 property. That will
>> remove it from all cpu types that don't support it.
>
> Wouldn't that mean that you'd be unable to use
>
>   -cpu foo,pmu=off
>
> if CPU model 'foo' doesn't support a PMU? I'd expect that
> to work.

The current precedent (has_el3) doesn't work like that: if
foo isn't a CPU which can support EL3 then the property doesn't
exist, and it's an error to try to set it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Andrea Bolognani]

On Mon, 2016-08-01 at 14:32 +0100, Peter Maydell wrote:
> > Wouldn't that mean that you'd be unable to use
> > 
> >   -cpu foo,pmu=off
> > 
> > if CPU model 'foo' doesn't support a PMU? I'd expect that
> > to work.
> 
> The current precedent (has_el3) doesn't work like that: if
> foo isn't a CPU which can support EL3 then the property doesn't
> exist, and it's an error to try to set it.

Doesn't look like the pmu option works like that on x86,
though, unless I'm missing something.

I have a guest running with

  -cpu pentium,pmu=on

and I can't see hardware perf events from inside the guest,
eg. dmesg reports

  Performance Events: no PMU driver, software events only.

and perf tells me

  <not supported> instructions
  <not supported> branches
  ...

I'm not sure whether that's because the PMU is being
emulated but the kernel doesn't have a driver for it, or
whether it's not being emulated at all. Any way to find
out?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Wei Huang]

On 08/01/2016 08:32 AM, Peter Maydell wrote:
> On 1 August 2016 at 14:26, Andrea Bolognani <abologna@redhat.com> wrote:
>> On Mon, 2016-08-01 at 15:08 +0200, Andrew Jones wrote:
>>>> I'm not sure a warning is enough: if I start a guest and
>>>> explicitly ask for a PMU, I expect it to be there, or for
>>>> the guest not to start at all. How does x86 behave in this
>>>> regard?
>>>
>>> Peter had a good suggestion for this. We need to wrap the property
>>> addition in an arm_feature check like the has_el3 property. That will
>>> remove it from all cpu types that don't support it.
>>
>> Wouldn't that mean that you'd be unable to use
>>
>>   -cpu foo,pmu=off
>>
>> if CPU model 'foo' doesn't support a PMU? I'd expect that
>> to work.
> 
> The current precedent (has_el3) doesn't work like that: if
> foo isn't a CPU which can support EL3 then the property doesn't
> exist, and it's an error to try to set it.

V1 sent. I tried to follow everyone's advice. See the following:

* set default pmu=off
* like el3, add a new feature ARM_FEATURE_HOST_PMU
* "pmu" property becomes CPU dependent. Only cortex-a53/cortex-a57/host
under certain mode support this option
* change struct ARMCPU field name "has_pmu" ==> "has_host_pmu" because
IMO "has_pmu" is misleading

BTW answering Andrea's question above: "-cpu foo,pmu=off" won't be
allowed in this patch if CPU "foo" doesn't support host-backed PMU. QEMU
will fail to run in this case. Maybe this is what we want?

Thanks,
-Wei

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Andrea Bolognani]

On Sat, 2016-08-13 at 01:06 -0500, Wei Huang wrote:
> > > Wouldn't that mean that you'd be unable to use
> > > 
> > >   -cpu foo,pmu=off
> > > 
> > > if CPU model 'foo' doesn't support a PMU? I'd expect that
> > > to work.
> > 
> > The current precedent (has_el3) doesn't work like that: if
> > foo isn't a CPU which can support EL3 then the property doesn't
> > exist, and it's an error to try to set it.
> 
> V1 sent. I tried to follow everyone's advice. See the following:
> 
> * set default pmu=off
> * like el3, add a new feature ARM_FEATURE_HOST_PMU
> * "pmu" property becomes CPU dependent. Only cortex-a53/cortex-a57/host
> under certain mode support this option
> * change struct ARMCPU field name "has_pmu" ==> "has_host_pmu" because
> IMO "has_pmu" is misleading
> 
> BTW answering Andrea's question above: "-cpu foo,pmu=off" won't be
> allowed in this patch if CPU "foo" doesn't support host-backed PMU. QEMU
> will fail to run in this case. Maybe this is what we want?

After discussing this a bit offline, I came to the conclusion
that there isn't a Single Right Way™ to handle this - both my
proposal and what you implemented are reasonable behaviors
one could expect.

On the other hand, what you implemented:

  * matches x86
  * is more strict than what I proposed, so there's room to
    change it later without breaking any existing guest

so I'm happy with it :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Peter Maydell]

On 28 July 2016 at 17:38, Wei Huang <wei@redhat.com> wrote:
> This patch adds a pmu=[on/off] option to enable/disable vpmu support
> in guest vm. There are several reasons to justify this option. First
> vpmu can be problematic for cross-migration between different SoC as
> perf counters is architecture-dependent. It is more flexible to
> have an option to turn it on/off. Secondly it matches the -cpu pmu
> option in libivrt. This patch has been tested on both DT/ACPI modes.


What particular two systems are you trying to migrate between?
In general we don't support migrating between different CPU
types at the moment, so the PMU sholud be the same on both ends.

(If we ever do get to supporting cross-cpu-type migration
then it will probably involve a very long and detailed command
line to specify exactly a whole lot of things like pmu yes/no,
number of hw breakpoints/watchpoints, and everything else that
can differ between implementations.)

That said, I don't have any objection to making the PMU
presence controllable (especially if we have similar
control on x86).

> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -579,8 +579,9 @@ struct ARMCPU {
>      bool powered_off;
>      /* CPU has security extension */
>      bool has_el3;
> -    /* CPU has PMU (Performance Monitor Unit) */
> -    bool has_pmu;
> +
> +    /* CPU has vPMU (Performance Monitor Unit) support */
> +    bool enable_pmu;

Why rename the flag? has_foo is what we use for other features,
as you can see in the context of this bit of the patch.

>
>      /* CPU has memory protection unit */
>      bool has_mpu;

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Wei Huang]

On 07/29/2016 02:57 AM, Peter Maydell wrote:
> On 28 July 2016 at 17:38, Wei Huang <wei@redhat.com> wrote:
>> This patch adds a pmu=[on/off] option to enable/disable vpmu support
>> in guest vm. There are several reasons to justify this option. First
>> vpmu can be problematic for cross-migration between different SoC as
>> perf counters is architecture-dependent. It is more flexible to
>> have an option to turn it on/off. Secondly it matches the -cpu pmu
>> option in libivrt. This patch has been tested on both DT/ACPI modes.
> 
> 
> What particular two systems are you trying to migrate between?

One example: APM's Mustang has 5 perf counters while AMD's Seattle has 7
counters.

> In general we don't support migrating between different CPU
> types at the moment, so the PMU sholud be the same on both ends.
> 
> (If we ever do get to supporting cross-cpu-type migration
> then it will probably involve a very long and detailed command
> line to specify exactly a whole lot of things like pmu yes/no,
> number of hw breakpoints/watchpoints, and everything else that
> can differ between implementations.)
> 
> That said, I don't have any objection to making the PMU
> presence controllable (especially if we have similar
> control on x86).
> 
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -579,8 +579,9 @@ struct ARMCPU {
>>      bool powered_off;
>>      /* CPU has security extension */
>>      bool has_el3;
>> -    /* CPU has PMU (Performance Monitor Unit) */
>> -    bool has_pmu;
>> +
>> +    /* CPU has vPMU (Performance Monitor Unit) support */
>> +    bool enable_pmu;
> 
> Why rename the flag? has_foo is what we use for other features,
> as you can see in the context of this bit of the patch.

I will fix it. Maybe follow the suggestion Drew's suggestion, keeping
has_pmu and add another option for turning it on/off.

> 
>>
>>      /* CPU has memory protection unit */
>>      bool has_mpu;
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Peter Maydell]

On 29 July 2016 at 16:08, Wei Huang <wei@redhat.com> wrote:
>
>
> On 07/29/2016 02:57 AM, Peter Maydell wrote:
>> On 28 July 2016 at 17:38, Wei Huang <wei@redhat.com> wrote:
>>> This patch adds a pmu=[on/off] option to enable/disable vpmu support
>>> in guest vm. There are several reasons to justify this option. First
>>> vpmu can be problematic for cross-migration between different SoC as
>>> perf counters is architecture-dependent. It is more flexible to
>>> have an option to turn it on/off. Secondly it matches the -cpu pmu
>>> option in libivrt. This patch has been tested on both DT/ACPI modes.
>>
>>
>> What particular two systems are you trying to migrate between?
>
> One example: APM's Mustang has 5 perf counters while AMD's Seattle has 7
> counters.

Right; this is a cross-implementation migration, which isn't
supposed to work at the moment, and more will be required for
it to work than just pmu on/off.

thanks
-- PMM