Re: [Qemu-arm] [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 <address@hidden>
> ---
>  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