KVM: arm64: Prevent spurious PMU accesses when no
Marc Zyngier
maintainerv2 cover KVM: arm64: Prevent spurious PMU accesses when no
Yet another PMU bug that is only likely to hit under Nested Virt: we unconditionally access PMU registers without checking whether it actually is present.
Given that we already have a predicate for this, promote it to a static key, and use that in the world switch.
Thanks to Andre for the heads up!
* From v1:
- Fix compilation when CONFIG_ARM_PMU isn't selected
Marc Zyngier (2):
KVM: arm64: Turn kvm_arm_support_pmu_v3() into a static key
KVM: arm64: Don't access PMSELR_EL0/PMUSERENR_EL0 when no PMU is
available
arch/arm64/kernel/image-vars.h | 3 +++ arch/arm64/kvm/hyp/include/hyp/switch.h | 9 ++++++--- arch/arm64/kvm/perf.c | 10 ++++++++++ arch/arm64/kvm/pmu-emul.c | 10 ---------- include/kvm/arm_pmu.h | 9 +++++++-- 5 files changed, 26 insertions(+), 15 deletions(-)
Signature · 6 lines
2.29.2 _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Marc Zyngier
maintainerPATCH v2 1/2 KVM: arm64: Turn kvm_arm_support_pmu_v3() into a static key
We currently find out about the presence of a HW PMU (or the handling of that PMU by perf, which amounts to the same thing) in a fairly roundabout way, by checking the number of counters available to perf. That's good enough for now, but we will soon need to find about about that on paths where perf is out of reach (in the world switch).
Instead, let's turn kvm_arm_support_pmu_v3() into a static key.
--- arch/arm64/kvm/perf.c | 10 ++++++++++ arch/arm64/kvm/pmu-emul.c | 10 ---------- include/kvm/arm_pmu.h | 9 +++++++-- 3 files changed, 17 insertions(+), 12 deletions(-)
arch/arm64/kvm/perf.c +10 -0
@@ -11,6 +11,8 @@#include <asm/kvm_emulate.h> +DEFINE_STATIC_KEY_FALSE (kvm_arm_pmu_available); +static int kvm_is_in_guest (void ) {return kvm_get_running_vcpu () !=NULL ;
@@ -48,6 +50,14 @@ static struct perf_guest_info_callbacks kvm_guest_cbs = {int kvm_perf_init (void ) { +/* +* Check if HW_PERF_EVENTS are supported by checking the number of +* hardware performance counters. This could ensure the presence of +* a physical PMU and CONFIG_PERF_EVENT is selected. +*/ +if (IS_ENABLED (CONFIG_ARM_PMU ) &&perf_num_counters () >0 ) +static_branch_enable (&kvm_arm_pmu_available); +return perf_register_guest_info_callbacks (&kvm_guest_cbs); }
arch/arm64/kvm/pmu-emul.c +0 -10
@@ -813,16 +813,6 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)return val & mask; } -bool kvm_arm_support_pmu_v3 (void ) -{ -/* -* Check if HW_PERF_EVENTS are supported by checking the number of -* hardware performance counters. This could ensure the presence of -* a physical PMU and CONFIG_PERF_EVENT is selected. -*/ -return (perf_num_counters () >0 ); -} -int kvm_arm_pmu_v3_enable (struct kvm_vcpu *vcpu) {if (!kvm_vcpu_has_pmu (vcpu))
include/kvm/arm_pmu.h +7 -2
@@ -13,6 +13,13 @@#define ARMV8_PMU_CYCLE_IDX (ARMV8_PMU_MAX_COUNTERS - 1)#define ARMV8_PMU_MAX_COUNTER_PAIRS ((ARMV8_PMU_MAX_COUNTERS + 1) >> 1) +DECLARE_STATIC_KEY_FALSE (kvm_arm_pmu_available); + +static __always_inline bool kvm_arm_support_pmu_v3(void) +{ +return static_branch_likely (&kvm_arm_pmu_available); +} +#ifdef CONFIG_HW_PERF_EVENTS struct kvm_pmc {
@@ -47,7 +54,6 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val);void kvm_pmu_handle_pmcr (struct kvm_vcpu *vcpu,u64 val);void kvm_pmu_set_counter_event_type (struct kvm_vcpu *vcpu,u64 data,u64 select_idx); -bool kvm_arm_support_pmu_v3 (void );int kvm_arm_pmu_v3_set_attr (struct kvm_vcpu *vcpu,struct kvm_device_attr *attr); int kvm_arm_pmu_v3_get_attr(struct kvm_vcpu *vcpu,
@@ -87,7 +93,6 @@ static inline void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val) {}static inline void kvm_pmu_handle_pmcr (struct kvm_vcpu *vcpu,u64 val) {}static inline void kvm_pmu_set_counter_event_type (struct kvm_vcpu *vcpu,u64 data,u64 select_idx) {} -static inline bool kvm_arm_support_pmu_v3 (void ) {return false; }static inline int kvm_arm_pmu_v3_set_attr (struct kvm_vcpu *vcpu,struct kvm_device_attr *attr) {
Signature · 6 lines
2.29.2 _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/2] KVM: arm64: Turn kvm_arm_support_pmu_v3() into a static key
Hi Marc,
Quoted 7 lines · Marc Zyngier
We currently find out about the presence of a HW PMU (or the handling of that PMU by perf, which amounts to the same thing) in a fairly roundabout way, by checking the number of counters available to perf. That's good enough for now, but we will soon need to find about about that on paths where perf is out of reach (in the world switch).
Instead, let's turn
kvm_arm_support_pmu_v3()into a static key.
The patch looks correct to me. I compiled the kernel with CONFIG_ARM_PMU set and unset, saw no problems. The IS_ENABLED(CONFIG_ARM_PMU) condition does prevent the compile error reported by Andre, as removing it triggered an error. I also double checked that static keys are initialized before KVM, which in hindsight was obvious, since the GIC is initialized before KVM, and the GIC driver makes use of static keys:
Thanks,
Alex
Quoted 84 lines
Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kvm/perf.c | 10 ++++++++++ arch/arm64/kvm/pmu-emul.c | 10 ---------- include/kvm/arm_pmu.h | 9 +++++++-- 3 files changed, 17 insertions(+), 12 deletions(-)arch/arm64/kvm/perf.c +10
-0@@ -11,6 +11,8 @@#include <asm/kvm_emulate.h> +DEFINE_STATIC_KEY_FALSE (kvm_arm_pmu_available); +static int kvm_is_in_guest (void ) {return kvm_get_running_vcpu () !=NULL ;@@ -48,6 +50,14 @@ static struct perf_guest_info_callbacks kvm_guest_cbs = {int kvm_perf_init (void ) { +/* +* Check if HW_PERF_EVENTS are supported by checking the number of +* hardware performance counters. This could ensure the presence of +* a physical PMU and CONFIG_PERF_EVENT is selected. +*/ +if (IS_ENABLED (CONFIG_ARM_PMU ) &&perf_num_counters () >0 ) +static_branch_enable (&kvm_arm_pmu_available); +return perf_register_guest_info_callbacks (&kvm_guest_cbs); }arch/arm64/kvm/pmu-emul.c +0
-10@@ -813,16 +813,6 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)return val & mask; } -bool kvm_arm_support_pmu_v3 (void ) -{ -/* -* Check if HW_PERF_EVENTS are supported by checking the number of -* hardware performance counters. This could ensure the presence of -* a physical PMU and CONFIG_PERF_EVENT is selected. -*/ -return (perf_num_counters () >0 ); -} -int kvm_arm_pmu_v3_enable (struct kvm_vcpu *vcpu) {if (!kvm_vcpu_has_pmu (vcpu))include/kvm/arm_pmu.h +7
-1@@ -13,6 +13,13 @@#define ARMV8_PMU_CYCLE_IDX (ARMV8_PMU_MAX_COUNTERS - 1)#define ARMV8_PMU_MAX_COUNTER_PAIRS ((ARMV8_PMU_MAX_COUNTERS + 1) >> 1) +DECLARE_STATIC_KEY_FALSE (kvm_arm_pmu_available); + +static __always_inline bool kvm_arm_support_pmu_v3(void) +{ +return static_branch_likely (&kvm_arm_pmu_available); +} +#ifdef CONFIG_HW_PERF_EVENTS struct kvm_pmc {@@ -47,7 +54,6 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val);void kvm_pmu_handle_pmcr (struct kvm_vcpu *vcpu,u64 val);void kvm_pmu_set_counter_event_type (struct kvm_vcpu *vcpu,u64 data,u64 select_idx); -bool kvm_arm_support_pmu_v3 (void );int kvm_arm_pmu_v3_set_attr (struct kvm_vcpu *vcpu,struct kvm_device_attr *attr); int kvm_arm_pmu_v3_get_attr(struct kvm_vcpu *vcpu,
include/kvm/arm_pmu.h static inline void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val) {} · line 98
Quoted 7 lines
include/kvm/arm_pmu.h +0
-1@@ -87,7 +93,6 @@ static inline void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val) {}static inline void kvm_pmu_handle_pmcr (struct kvm_vcpu *vcpu,u64 val) {}static inline void kvm_pmu_set_counter_event_type (struct kvm_vcpu *vcpu,u64 data,u64 select_idx) {} -static inline bool kvm_arm_support_pmu_v3 (void ) {return false; }static inline int kvm_arm_pmu_v3_set_attr (struct kvm_vcpu *vcpu,struct kvm_device_attr *attr) {
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Marc Zyngier
maintainerPATCH v2 2/2 KVM: arm64: Don't access PMSELR_EL0/PMUSERENR_EL0 when no PMU is available
In reply to Marc ZyngierWhen running under a nesting hypervisor, it isn't guaranteed that the virtual HW will include a PMU. In which case, let's not try to access the PMU registers in the world switch, as that'd be deadly.
--- arch/arm64/kernel/image-vars.h | 3 +++ arch/arm64/kvm/hyp/include/hyp/switch.h | 9 ++++++--- 2 files changed, 9 insertions(+), 3 deletions(-)
arch/arm64/kernel/image-vars.h +3 -0
@@ -102,6 +102,9 @@ KVM_NVHE_ALIAS(__stop___kvm_ex_table);/* Array containing bases of nVHE per-CPU memory regions. */ KVM_NVHE_ALIAS (kvm_arm_hyp_percpu_base); +/* PMU available static key */ +KVM_NVHE_ALIAS (kvm_arm_pmu_available); +#endif /* CONFIG_KVM */ #endif /* __ARM64_KERNEL_IMAGE_VARS_H */
arch/arm64/kvm/hyp/include/hyp/switch.h +6 -3
@@ -90,15 +90,18 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)* counter, which could make a PMXEVCNTR_EL0 access UNDEF at * EL1 instead of being trapped to EL2. */ -write_sysreg (0 , pmselr_el0); -write_sysreg (ARMV8_PMU_USERENR_MASK , pmuserenr_el0); +if (kvm_arm_support_pmu_v3 ()) { +write_sysreg (0 , pmselr_el0); +write_sysreg (ARMV8_PMU_USERENR_MASK , pmuserenr_el0); + }write_sysreg (vcpu->arch.mdcr_el2, mdcr_el2); }static inline void __deactivate_traps_common (void ) {write_sysreg (0 , hstr_el2); -write_sysreg (0 , pmuserenr_el0); +if (kvm_arm_support_pmu_v3 ()) +write_sysreg (0 , pmuserenr_el0); }static inline void ___activate_traps (struct kvm_vcpu *vcpu)
Signature · 6 lines
2.29.2 _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/2] KVM: arm64: Don't access PMSELR_EL0/PMUSERENR_EL0 when no PMU is available
Hi Marc,
Quoted 42 lines · Marc Zyngier
When running under a nesting hypervisor, it isn't guaranteed that the virtual HW will include a PMU. In which case, let's not try to access the PMU registers in the world switch, as that'd be deadly.
Reported-by Andre Przywara <[email protected]>Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kernel/image-vars.h | 3 +++ arch/arm64/kvm/hyp/include/hyp/switch.h | 9 ++++++--- 2 files changed, 9 insertions(+), 3 deletions(-)arch/arm64/kernel/image-vars.h +3
-0@@ -102,6 +102,9 @@ KVM_NVHE_ALIAS(__stop___kvm_ex_table);/* Array containing bases of nVHE per-CPU memory regions. */ KVM_NVHE_ALIAS (kvm_arm_hyp_percpu_base); +/* PMU available static key */ +KVM_NVHE_ALIAS (kvm_arm_pmu_available); +#endif /* CONFIG_KVM */ #endif /* __ARM64_KERNEL_IMAGE_VARS_H */ arch/arm64/kvm/hyp/include/hyp/switch.h +4
-2@@ -90,15 +90,18 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)* counter, which could make a PMXEVCNTR_EL0 access UNDEF at * EL1 instead of being trapped to EL2. */ -write_sysreg (0 , pmselr_el0); -write_sysreg (ARMV8_PMU_USERENR_MASK , pmuserenr_el0); +if (kvm_arm_support_pmu_v3 ()) { +write_sysreg (0 , pmselr_el0); +write_sysreg (ARMV8_PMU_USERENR_MASK , pmuserenr_el0); + }write_sysreg (vcpu->arch.mdcr_el2, mdcr_el2); }
arch/arm64/kvm/hyp/include/hyp/switch.h static inline void __activate_traps_common(struct kvm_vcpu *vcpu) · line 107
Quoted 10 lines
arch/arm64/kvm/hyp/include/hyp/switch.h +2
-1static inline void __deactivate_traps_common (void ) {write_sysreg (0 , hstr_el2); -write_sysreg (0 , pmuserenr_el0); +if (kvm_arm_support_pmu_v3 ()) +write_sysreg (0 , pmuserenr_el0); }static inline void ___activate_traps (struct kvm_vcpu *vcpu)
It looks to me like this indeed fixes the issue with accessing PMU registers even if the PMU is not present. Found another instance in the world switch where the PMU is accessed, in __kvm_vcpu_run() in the nvhe code, but in that case the registers were accessed only if there were events that needed to be context switched. No PMU on the host, no PMU on the guest and no events for either, so that looks fine to me:
Thanks,
Alex
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Quoted 8 lines · Marc Zyngier
Yet another PMU bug that is only likely to hit under Nested Virt: we unconditionally access PMU registers without checking whether it actually is present.
Given that we already have a predicate for this, promote it to a static key, and use that in the world switch.
[...]
Applied to kvmarm-master/fixes, thanks!
[1/2] KVM: arm64: Turn kvm_arm_support_pmu_v3() into a static key
commit: 502e5f9a6985898b5318ebb5978a54c3ebf3dfe1
[2/2] KVM: arm64: Don't access PMSELR_EL0/PMUSERENR_EL0 when no PMU is available
commit: 7b85a9313e6cad22a66027151b6d54d1ce44543fCheers,
M.
Signature · 7 lines
Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm