| Message ID | 1471067570-7503-1-git-send-email-wei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2016-08-13 at 00:52 -0500, Wei Huang wrote: > This patch adds a pmu=[on/off] option to enable/disable host vPMU > support in guest VM. There are several reasons to justify this option. > First, host-backed 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 tool. > > Note that, like "has_el3", the "pmu" option is only made available on > CPUs that support host-backed vPMU. They include: > * cortex-a53 + kvm > * cortex-a57 + kvm > * host + kvm > This option is removed in other configs where it doesn't make sense > (e.g. cortex-a57 + TCG); and the default pmu support is off. This patch > has been tested under 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 | 13 +++++++++++++ > target-arm/cpu.h | 3 ++- > target-arm/cpu64.c | 6 ++++++ > target-arm/kvm64.c | 10 +++++----- > 6 files changed, 28 insertions(+), 8 deletions(-) Did you already try driving this with libvirt? It should work out of the box. If you haven't, I will do it :) -- Andrea Bolognani / Red Hat / Virtualization
On Sat, Aug 13, 2016 at 12:52:50AM -0500, Wei Huang wrote: > This patch adds a pmu=[on/off] option to enable/disable host vPMU > support in guest VM. There are several reasons to justify this option. > First, host-backed 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 tool. > > Note that, like "has_el3", the "pmu" option is only made available on > CPUs that support host-backed vPMU. They include: > * cortex-a53 + kvm > * cortex-a57 + kvm > * host + kvm > This option is removed in other configs where it doesn't make sense > (e.g. cortex-a57 + TCG); and the default pmu support is off. This patch Some of the PMU registers are already emulated with TCG. While it doesn't make much sense to use perf counters in a TCG guest for measuring anything, it may have merit for emulation completeness and debug. With that in mind, then I think we need to add the PMU feature to all processors that support it, and thus the 'has_pmu' name is better. As the feature is optional, I agree it does make sense that it's off by default, and only available with the pmu=on property. I think there are more processors than just A53 and A57. > has been tested under 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 | 13 +++++++++++++ > target-arm/cpu.h | 3 ++- > target-arm/cpu64.c | 6 ++++++ > target-arm/kvm64.c | 10 +++++----- > 6 files changed, 28 insertions(+), 8 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 28fc59c..22fb9d9 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->has_host_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..4975f38 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->has_host_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..a527128 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -509,6 +509,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 with other archs and libvirt */ > +static Property arm_cpu_has_host_pmu_property = > + DEFINE_PROP_BOOL("pmu", ARMCPU, has_host_pmu, false); > + > static Property arm_cpu_has_mpu_property = > DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true); > > @@ -552,6 +556,11 @@ static void arm_cpu_post_init(Object *obj) > #endif > } > > + if (arm_feature(&cpu->env, ARM_FEATURE_HOST_PMU)) { > + qdev_property_add_static(DEVICE(obj), &arm_cpu_has_host_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 +657,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > cpu->id_aa64pfr0 &= ~0xf000; > } > > + if (!cpu->has_host_pmu) { > + unset_feature(env, ARM_FEATURE_HOST_PMU); > + } I think this is the right approach. But we should add the feature to all processors that can have it, and then, after checking the property's value, we remove it when it's not desired. > + > if (!arm_feature(env, ARM_FEATURE_EL2)) { > /* Disable the hypervisor feature bits in the processor feature > * registers if we don't have EL2. These are id_pfr1[15:12] and > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 76d824d..f3f6d3f 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -580,7 +580,7 @@ struct ARMCPU { > /* CPU has security extension */ > bool has_el3; > /* CPU has PMU (Performance Monitor Unit) */ > - bool has_pmu; > + bool has_host_pmu; > > /* CPU has memory protection unit */ > bool has_mpu; > @@ -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_HOST_PMU, /* PMU supported by host */ > }; > > static inline int arm_feature(CPUARMState *env, int feature) > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c > index 1635deb..19e8127 100644 > --- a/target-arm/cpu64.c > +++ b/target-arm/cpu64.c > @@ -111,6 +111,9 @@ 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); > + if (kvm_enabled()) { > + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); > + } We shouldn't need the if (kvm_enabled()) here. > cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57; > cpu->midr = 0x411fd070; > cpu->revidr = 0x00000000; > @@ -166,6 +169,9 @@ 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); > + if (kvm_enabled()) { > + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); > + } Or here. > 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..588e5e3 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -469,6 +469,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_HOST_PMU); > > ahcc->features = features; > > @@ -501,11 +502,10 @@ 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, hw capability, and user setting */ > + cpu->has_host_pmu &= kvm_irqchip_in_kernel() && > + kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3); > + cpu->kvm_init_features[0] |= cpu->has_host_pmu << KVM_ARM_VCPU_PMU_V3; Hmm... so now if the user doesn't specify pmu=on (off by default) then we silently disable it (I think that's a 2.6 compat issue), and if they do specify pmu=on, but KVM doesn't support it, then it's silently disabled, which isn't nice. I think QEMU should error out in that case, explaining that the user specified incompatible options, i.e. they selected accel=kvm and pmu=on, but their KVM version (or cpu model) doesn't support PMU emulation. > > /* Do KVM_ARM_VCPU_INIT ioctl */ > ret = kvm_arm_vcpu_init(cs); > -- > 2.4.11 > > Thanks, drew
On 8/15/16 04:31, Andrea Bolognani wrote: > > On Sat, 2016-08-13 at 00:52 -0500, Wei Huang wrote: >> This patch adds a pmu=[on/off] option to enable/disable host vPMU >> support in guest VM. There are several reasons to justify this option. >> First, host-backed 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 tool. >> >> Note that, like "has_el3", the "pmu" option is only made available on >> CPUs that support host-backed vPMU. They include: >> * cortex-a53 + kvm >> * cortex-a57 + kvm >> * host + kvm >> This option is removed in other configs where it doesn't make sense >> (e.g. cortex-a57 + TCG); and the default pmu support is off. This patch >> has been tested under 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 | 13 +++++++++++++ >> target-arm/cpu.h | 3 ++- >> target-arm/cpu64.c | 6 ++++++ >> target-arm/kvm64.c | 10 +++++----- >> 6 files changed, 28 insertions(+), 8 deletions(-) > > Did you already try driving this with libvirt? It should work > out of the box. > > If you haven't, I will do it :) No, I haven't. All tests were done based on command line to QEMU. :-) -Wei > > -- > Andrea Bolognani / Red Hat / Virtualization >
On 13 August 2016 at 06:52, Wei Huang <wei@redhat.com> wrote: > This patch adds a pmu=[on/off] option to enable/disable host vPMU > support in guest VM. There are several reasons to justify this option. > First, host-backed 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 tool. > > Note that, like "has_el3", the "pmu" option is only made available on > CPUs that support host-backed vPMU. They include: > * cortex-a53 + kvm > * cortex-a57 + kvm > * host + kvm > This option is removed in other configs where it doesn't make sense > (e.g. cortex-a57 + TCG); and the default pmu support is off. This patch > has been tested under 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 | 13 +++++++++++++ > target-arm/cpu.h | 3 ++- > target-arm/cpu64.c | 6 ++++++ > target-arm/kvm64.c | 10 +++++----- > 6 files changed, 28 insertions(+), 8 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 28fc59c..22fb9d9 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->has_host_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..4975f38 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->has_host_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..a527128 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -509,6 +509,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 with other archs and libvirt */ > +static Property arm_cpu_has_host_pmu_property = > + DEFINE_PROP_BOOL("pmu", ARMCPU, has_host_pmu, false); > + > static Property arm_cpu_has_mpu_property = > DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true); > > @@ -552,6 +556,11 @@ static void arm_cpu_post_init(Object *obj) > #endif > } > > + if (arm_feature(&cpu->env, ARM_FEATURE_HOST_PMU)) { > + qdev_property_add_static(DEVICE(obj), &arm_cpu_has_host_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 +657,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > cpu->id_aa64pfr0 &= ~0xf000; > } > > + if (!cpu->has_host_pmu) { > + unset_feature(env, ARM_FEATURE_HOST_PMU); > + } > + > if (!arm_feature(env, ARM_FEATURE_EL2)) { > /* Disable the hypervisor feature bits in the processor feature > * registers if we don't have EL2. These are id_pfr1[15:12] and > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 76d824d..f3f6d3f 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -580,7 +580,7 @@ struct ARMCPU { > /* CPU has security extension */ > bool has_el3; > /* CPU has PMU (Performance Monitor Unit) */ > - bool has_pmu; > + bool has_host_pmu; Can you explain why you renamed this? "has_pmu" matches the comment: it indicates that the emulated CPU has a PMU. has_host_pmu would be something different (and would only apply to KVM). It also looks weird in the code in the board model: we should be saying "if the guest CPU has a PMU, wire up its interrupt", not "if the host CPU has a PMU, wire up a guest CPU interrupt". > > /* CPU has memory protection unit */ > bool has_mpu; > @@ -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_HOST_PMU, /* PMU supported by host */ ARM_FEATURE_ bits are for guest CPU features, not for recording information about the host CPU. > }; > > static inline int arm_feature(CPUARMState *env, int feature) > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c > index 1635deb..19e8127 100644 > --- a/target-arm/cpu64.c > +++ b/target-arm/cpu64.c > @@ -111,6 +111,9 @@ 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); > + if (kvm_enabled()) { > + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); > + } > cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57; > cpu->midr = 0x411fd070; > cpu->revidr = 0x00000000; > @@ -166,6 +169,9 @@ 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); > + if (kvm_enabled()) { > + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); > + } This definitely looks like the wrong place to be checking kvm_enabled(). > 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..588e5e3 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -469,6 +469,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_HOST_PMU); > > ahcc->features = features; > > @@ -501,11 +502,10 @@ 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, hw capability, and user setting */ > + cpu->has_host_pmu &= kvm_irqchip_in_kernel() && > + kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3); > + cpu->kvm_init_features[0] |= cpu->has_host_pmu << KVM_ARM_VCPU_PMU_V3; > > /* Do KVM_ARM_VCPU_INIT ioctl */ > ret = kvm_arm_vcpu_init(cs); > -- > 2.4.11 > thanks -- PMM
On Mon, 2016-08-15 at 12:09 +0200, Andrew Jones wrote: > Hmm... so now if the user doesn't specify pmu=on (off by default) then we > silently disable it (I think that's a 2.6 compat issue), and if they do > specify pmu=on, but KVM doesn't support it, then it's silently disabled, > which isn't nice. I think QEMU should error out in that case, explaining > that the user specified incompatible options, i.e. they selected accel=kvm > and pmu=on, but their KVM version (or cpu model) doesn't support PMU > emulation. I agree. Asking qemu to provide a feature that it can't provide should definitely result in a big fat error. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, 2016-08-15 at 11:12 -0500, Wei Huang wrote: > > Did you already try driving this with libvirt? It should work > > out of the box. > > > > If you haven't, I will do it :) > > No, I haven't. All tests were done based on command line to QEMU. :-) Fair enough :) I did some testing on my own using libvirt to drive a QEMU binary that included your patch, and it looks like everything is working as expected: if I turn PMU on using the appropriate XML element (<pmu state='on'/>) I can access the performance counters from inside the guest; if I disable it explicitly or don't enable it, I can't. The host-side testing involved making sure the 'pmu' flag was passed, or not passed, to QEMU and the value, if any, matched the guest XML. The guest-side testing involved running $ dmesg | grep -i pmu $ perf list | grep 'Hardware event' and making sure some output was returned in both cases, and making sure the 'instructions' counter was not marked as '<not supported>' in the output of $ perf stat true If the kind of testing I performed is not good enough, please let me know and I'll do another round. This was tested only with <cpu mode='host-passthrough'/> (the libvirt equivalent of -cpu host), because the aarch64 CPU driver in libvirt is not currently capable of handling other CPU models for KVM guests. I also verified that TCG guests didn't accept the 'pmu' flag. -- Andrea Bolognani / Red Hat / Virtualization
On 08/15/2016 05:09 AM, Andrew Jones wrote: > On Sat, Aug 13, 2016 at 12:52:50AM -0500, Wei Huang wrote: >> This patch adds a pmu=[on/off] option to enable/disable host vPMU >> support in guest VM. There are several reasons to justify this option. >> First, host-backed 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 tool. >> >> Note that, like "has_el3", the "pmu" option is only made available on >> CPUs that support host-backed vPMU. They include: >> * cortex-a53 + kvm >> * cortex-a57 + kvm >> * host + kvm >> This option is removed in other configs where it doesn't make sense >> (e.g. cortex-a57 + TCG); and the default pmu support is off. This patch > > Some of the PMU registers are already emulated with TCG. While it > doesn't make much sense to use perf counters in a TCG guest for > measuring anything, it may have merit for emulation completeness > and debug. With that in mind, then I think we need to add the PMU > feature to all processors that support it, and thus the 'has_pmu' The TCG case you mentioned was one reason I changed the naming. The existing "has_pmu" variable was introduced recently and it didn't cover the case of TCG. Reading 5c0a3819, it is only turned on when KVM is enabled and PMU_V3_extension is available. If we want to extend the meaning of "has_pmu" for all PMU cases (i.e. KVM + TCG), we probably need to address cases of has_pmu=ON + TCG=ON. > name is better. As the feature is optional, I agree it does make > sense that it's off by default, and only available with the pmu=on > property. > > I think there are more processors than just A53 and A57. host-backed vPMU is only available on A53/A57/host. But TCG supports more than A53/A57. > >> has been tested under 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 | 13 +++++++++++++ >> target-arm/cpu.h | 3 ++- >> target-arm/cpu64.c | 6 ++++++ >> target-arm/kvm64.c | 10 +++++----- >> 6 files changed, 28 insertions(+), 8 deletions(-) >> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 28fc59c..22fb9d9 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->has_host_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..4975f38 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->has_host_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..a527128 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -509,6 +509,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 with other archs and libvirt */ >> +static Property arm_cpu_has_host_pmu_property = >> + DEFINE_PROP_BOOL("pmu", ARMCPU, has_host_pmu, false); >> + >> static Property arm_cpu_has_mpu_property = >> DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true); >> >> @@ -552,6 +556,11 @@ static void arm_cpu_post_init(Object *obj) >> #endif >> } >> >> + if (arm_feature(&cpu->env, ARM_FEATURE_HOST_PMU)) { >> + qdev_property_add_static(DEVICE(obj), &arm_cpu_has_host_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 +657,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) >> cpu->id_aa64pfr0 &= ~0xf000; >> } >> >> + if (!cpu->has_host_pmu) { >> + unset_feature(env, ARM_FEATURE_HOST_PMU); >> + } > > I think this is the right approach. But we should add the feature to all > processors that can have it, and then, after checking the property's > value, we remove it when it's not desired. > >> + >> if (!arm_feature(env, ARM_FEATURE_EL2)) { >> /* Disable the hypervisor feature bits in the processor feature >> * registers if we don't have EL2. These are id_pfr1[15:12] and >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 76d824d..f3f6d3f 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -580,7 +580,7 @@ struct ARMCPU { >> /* CPU has security extension */ >> bool has_el3; >> /* CPU has PMU (Performance Monitor Unit) */ >> - bool has_pmu; >> + bool has_host_pmu; >> >> /* CPU has memory protection unit */ >> bool has_mpu; >> @@ -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_HOST_PMU, /* PMU supported by host */ >> }; >> >> static inline int arm_feature(CPUARMState *env, int feature) >> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c >> index 1635deb..19e8127 100644 >> --- a/target-arm/cpu64.c >> +++ b/target-arm/cpu64.c >> @@ -111,6 +111,9 @@ 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); >> + if (kvm_enabled()) { >> + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); >> + } > > We shouldn't need the if (kvm_enabled()) here. > >> cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57; >> cpu->midr = 0x411fd070; >> cpu->revidr = 0x00000000; >> @@ -166,6 +169,9 @@ 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); >> + if (kvm_enabled()) { >> + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); >> + } > > Or here. > >> 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..588e5e3 100644 >> --- a/target-arm/kvm64.c >> +++ b/target-arm/kvm64.c >> @@ -469,6 +469,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_HOST_PMU); >> >> ahcc->features = features; >> >> @@ -501,11 +502,10 @@ 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, hw capability, and user setting */ >> + cpu->has_host_pmu &= kvm_irqchip_in_kernel() && >> + kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3); >> + cpu->kvm_init_features[0] |= cpu->has_host_pmu << KVM_ARM_VCPU_PMU_V3; > > Hmm... so now if the user doesn't specify pmu=on (off by default) then we > silently disable it (I think that's a 2.6 compat issue), and if they do ^^^^^^^^^^^^^ True. I will see if I can create a patch backward compatible with 2.6. > specify pmu=on, but KVM doesn't support it, then it's silently disabled, > which isn't nice. I think QEMU should error out in that case, explaining > that the user specified incompatible options, i.e. they selected accel=kvm > and pmu=on, but their KVM version (or cpu model) doesn't support PMU > emulation. True. This can be fixed. > >> >> /* Do KVM_ARM_VCPU_INIT ioctl */ >> ret = kvm_arm_vcpu_init(cs); >> -- >> 2.4.11 >> >> > > Thanks, > drew >
On 8/15/16 11:13, Peter Maydell wrote: > On 13 August 2016 at 06:52, Wei Huang <wei@redhat.com> wrote: >> This patch adds a pmu=[on/off] option to enable/disable host vPMU >> support in guest VM. There are several reasons to justify this option. >> First, host-backed 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 tool. >> >> Note that, like "has_el3", the "pmu" option is only made available on >> CPUs that support host-backed vPMU. They include: >> * cortex-a53 + kvm >> * cortex-a57 + kvm >> * host + kvm >> This option is removed in other configs where it doesn't make sense >> (e.g. cortex-a57 + TCG); and the default pmu support is off. This patch >> has been tested under 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 | 13 +++++++++++++ >> target-arm/cpu.h | 3 ++- >> target-arm/cpu64.c | 6 ++++++ >> target-arm/kvm64.c | 10 +++++----- >> 6 files changed, 28 insertions(+), 8 deletions(-) >> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 28fc59c..22fb9d9 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->has_host_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..4975f38 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->has_host_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..a527128 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -509,6 +509,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 with other archs and libvirt */ >> +static Property arm_cpu_has_host_pmu_property = >> + DEFINE_PROP_BOOL("pmu", ARMCPU, has_host_pmu, false); >> + >> static Property arm_cpu_has_mpu_property = >> DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true); >> >> @@ -552,6 +556,11 @@ static void arm_cpu_post_init(Object *obj) >> #endif >> } >> >> + if (arm_feature(&cpu->env, ARM_FEATURE_HOST_PMU)) { >> + qdev_property_add_static(DEVICE(obj), &arm_cpu_has_host_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 +657,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) >> cpu->id_aa64pfr0 &= ~0xf000; >> } >> >> + if (!cpu->has_host_pmu) { >> + unset_feature(env, ARM_FEATURE_HOST_PMU); >> + } >> + >> if (!arm_feature(env, ARM_FEATURE_EL2)) { >> /* Disable the hypervisor feature bits in the processor feature >> * registers if we don't have EL2. These are id_pfr1[15:12] and >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 76d824d..f3f6d3f 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -580,7 +580,7 @@ struct ARMCPU { >> /* CPU has security extension */ >> bool has_el3; >> /* CPU has PMU (Performance Monitor Unit) */ >> - bool has_pmu; >> + bool has_host_pmu; > > Can you explain why you renamed this? "has_pmu" matches > the comment: it indicates that the emulated CPU has a PMU. > has_host_pmu would be something different (and would only The existing "has_pmu" was added and only enabled under KVM. So its existing meaning is a bit misleading. It doesn't address the case of TCG. > apply to KVM). It also looks weird in the code in the board > model: we should be saying "if the guest CPU has a PMU, wire > up its interrupt", not "if the host CPU has a PMU, wire > up a guest CPU interrupt". I can change it back to "has_pmu" as suggested by you and Drew; but we need to be careful of using it, especially under TCG mode. For instance, with "-M virt,accel=tcg -cpu host,pmu=off", we expect PMU is turned off. But TCG still emulates some PMU counters (e.g. pmccntr). > >> >> /* CPU has memory protection unit */ >> bool has_mpu; >> @@ -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_HOST_PMU, /* PMU supported by host */ > > ARM_FEATURE_ bits are for guest CPU features, not for > recording information about the host CPU. OK, will address it. > >> }; >> >> static inline int arm_feature(CPUARMState *env, int feature) >> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c >> index 1635deb..19e8127 100644 >> --- a/target-arm/cpu64.c >> +++ b/target-arm/cpu64.c >> @@ -111,6 +111,9 @@ 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); >> + if (kvm_enabled()) { >> + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); >> + } >> cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57; >> cpu->midr = 0x411fd070; >> cpu->revidr = 0x00000000; >> @@ -166,6 +169,9 @@ 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); >> + if (kvm_enabled()) { >> + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); >> + } > > This definitely looks like the wrong place to be checking > kvm_enabled(). > >> 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..588e5e3 100644 >> --- a/target-arm/kvm64.c >> +++ b/target-arm/kvm64.c >> @@ -469,6 +469,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_HOST_PMU); >> >> ahcc->features = features; >> >> @@ -501,11 +502,10 @@ 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, hw capability, and user setting */ >> + cpu->has_host_pmu &= kvm_irqchip_in_kernel() && >> + kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3); >> + cpu->kvm_init_features[0] |= cpu->has_host_pmu << KVM_ARM_VCPU_PMU_V3; >> >> /* Do KVM_ARM_VCPU_INIT ioctl */ >> ret = kvm_arm_vcpu_init(cs); >> -- >> 2.4.11 >> > > thanks > -- PMM >
On 08/15/2016 11:13 AM, Peter Maydell wrote: <snip> >> static inline int arm_feature(CPUARMState *env, int feature) >> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c >> index 1635deb..19e8127 100644 >> --- a/target-arm/cpu64.c >> +++ b/target-arm/cpu64.c >> @@ -111,6 +111,9 @@ 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); >> + if (kvm_enabled()) { >> + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); >> + } >> cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57; >> cpu->midr = 0x411fd070; >> cpu->revidr = 0x00000000; >> @@ -166,6 +169,9 @@ 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); >> + if (kvm_enabled()) { >> + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); >> + } > > This definitely looks like the wrong place to be checking > kvm_enabled(). If we don't want to check kvm_enabled() in aarch64_a5x_initfn(), one viable place is to move it up to cpu.c file, like in arm_cpu_realizefn(). Do you have any other suggestions? > >> cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53; >> cpu->midr = 0x410fd034; >> cpu->revidr = 0x00000000;
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 28fc59c..22fb9d9 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->has_host_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..4975f38 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->has_host_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..a527128 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -509,6 +509,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 with other archs and libvirt */ +static Property arm_cpu_has_host_pmu_property = + DEFINE_PROP_BOOL("pmu", ARMCPU, has_host_pmu, false); + static Property arm_cpu_has_mpu_property = DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true); @@ -552,6 +556,11 @@ static void arm_cpu_post_init(Object *obj) #endif } + if (arm_feature(&cpu->env, ARM_FEATURE_HOST_PMU)) { + qdev_property_add_static(DEVICE(obj), &arm_cpu_has_host_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 +657,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) cpu->id_aa64pfr0 &= ~0xf000; } + if (!cpu->has_host_pmu) { + unset_feature(env, ARM_FEATURE_HOST_PMU); + } + if (!arm_feature(env, ARM_FEATURE_EL2)) { /* Disable the hypervisor feature bits in the processor feature * registers if we don't have EL2. These are id_pfr1[15:12] and diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 76d824d..f3f6d3f 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -580,7 +580,7 @@ struct ARMCPU { /* CPU has security extension */ bool has_el3; /* CPU has PMU (Performance Monitor Unit) */ - bool has_pmu; + bool has_host_pmu; /* CPU has memory protection unit */ bool has_mpu; @@ -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_HOST_PMU, /* PMU supported by host */ }; static inline int arm_feature(CPUARMState *env, int feature) diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c index 1635deb..19e8127 100644 --- a/target-arm/cpu64.c +++ b/target-arm/cpu64.c @@ -111,6 +111,9 @@ 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); + if (kvm_enabled()) { + set_feature(&cpu->env, ARM_FEATURE_HOST_PMU); + } cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57; cpu->midr = 0x411fd070; cpu->revidr = 0x00000000; @@ -166,6 +169,9 @@ 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); + if (kvm_enabled()) { + set_feature(&cpu->env, ARM_FEATURE_HOST_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..588e5e3 100644 --- a/target-arm/kvm64.c +++ b/target-arm/kvm64.c @@ -469,6 +469,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_HOST_PMU); ahcc->features = features; @@ -501,11 +502,10 @@ 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, hw capability, and user setting */ + cpu->has_host_pmu &= kvm_irqchip_in_kernel() && + kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3); + cpu->kvm_init_features[0] |= cpu->has_host_pmu << KVM_ARM_VCPU_PMU_V3; /* Do KVM_ARM_VCPU_INIT ioctl */ ret = kvm_arm_vcpu_init(cs);
This patch adds a pmu=[on/off] option to enable/disable host vPMU support in guest VM. There are several reasons to justify this option. First, host-backed 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 tool. Note that, like "has_el3", the "pmu" option is only made available on CPUs that support host-backed vPMU. They include: * cortex-a53 + kvm * cortex-a57 + kvm * host + kvm This option is removed in other configs where it doesn't make sense (e.g. cortex-a57 + TCG); and the default pmu support is off. This patch has been tested under 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 | 13 +++++++++++++ target-arm/cpu.h | 3 ++- target-arm/cpu64.c | 6 ++++++ target-arm/kvm64.c | 10 +++++----- 6 files changed, 28 insertions(+), 8 deletions(-)