| Message ID | 20210828003558.713983-1-seanjc@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | perf: KVM: Fix, optimize, and clean up callbacks | expand |
On Fri, Aug 27, 2021 at 05:35:45PM -0700, Sean Christopherson wrote: > Like Xu (2): > perf/core: Rework guest callbacks to prepare for static_call support > perf/core: Use static_call to optimize perf_guest_info_callbacks > > Sean Christopherson (11): > perf: Ensure perf_guest_cbs aren't reloaded between !NULL check and > deref > KVM: x86: Register perf callbacks after calling vendor's > hardware_setup() > KVM: x86: Register Processor Trace interrupt hook iff PT enabled in > guest > perf: Stop pretending that perf can handle multiple guest callbacks > perf: Force architectures to opt-in to guest callbacks > KVM: x86: Drop current_vcpu for kvm_running_vcpu + kvm_arch_vcpu > variable > KVM: x86: More precisely identify NMI from guest when handling PMI > KVM: Move x86's perf guest info callbacks to generic KVM > KVM: x86: Move Intel Processor Trace interrupt handler to vmx.c > KVM: arm64: Convert to the generic perf callbacks > KVM: arm64: Drop perf.c and fold its tiny bits of code into arm.c / > pmu.c Lets keep the whole intel_pt crud inside x86... --- Index: linux-2.6/arch/x86/events/core.c =================================================================== --- linux-2.6.orig/arch/x86/events/core.c +++ linux-2.6/arch/x86/events/core.c @@ -92,7 +92,7 @@ DEFINE_STATIC_CALL_RET0(x86_pmu_guest_ge DEFINE_STATIC_CALL_RET0(x86_guest_state, *(perf_guest_cbs->state)); DEFINE_STATIC_CALL_RET0(x86_guest_get_ip, *(perf_guest_cbs->get_ip)); -DEFINE_STATIC_CALL_RET0(x86_guest_handle_intel_pt_intr, *(perf_guest_cbs->handle_intel_pt_intr)); +DEFINE_STATIC_CALL_RET0(x86_guest_handle_intel_pt_intr, unsigned int (*)(void)); void arch_perf_update_guest_cbs(struct perf_guest_info_callbacks *guest_cbs) { @@ -103,14 +103,6 @@ void arch_perf_update_guest_cbs(struct p static_call_update(x86_guest_state, (void *)&__static_call_return0); static_call_update(x86_guest_get_ip, (void *)&__static_call_return0); } - - /* Implementing ->handle_intel_pt_intr is optional. */ - if (guest_cbs && guest_cbs->handle_intel_pt_intr) - static_call_update(x86_guest_handle_intel_pt_intr, - guest_cbs->handle_intel_pt_intr); - else - static_call_update(x86_guest_handle_intel_pt_intr, - (void *)&__static_call_return0); } u64 __read_mostly hw_cache_event_ids Index: linux-2.6/arch/x86/events/intel/core.c =================================================================== --- linux-2.6.orig/arch/x86/events/intel/core.c +++ linux-2.6/arch/x86/events/intel/core.c @@ -2782,7 +2782,7 @@ static void intel_pmu_reset(void) local_irq_restore(flags); } -DECLARE_STATIC_CALL(x86_guest_handle_intel_pt_intr, *(perf_guest_cbs->handle_intel_pt_intr)); +DECLARE_STATIC_CALL(x86_guest_handle_intel_pt_intr, unsigned int (*)(void)); static int handle_pmi_common(struct pt_regs *regs, u64 status) { Index: linux-2.6/arch/x86/kvm/x86.c =================================================================== --- linux-2.6.orig/arch/x86/kvm/x86.c +++ linux-2.6/arch/x86/kvm/x86.c @@ -10960,7 +10960,14 @@ int kvm_arch_hardware_setup(void *opaque memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops)); kvm_ops_static_call_update(); - kvm_register_perf_callbacks(ops->handle_intel_pt_intr); + kvm_register_perf_callbacks(); + if (ops->handle_intel_pt_intr) { + static_call_update(x86_guest_handle_intel_pt_intr, + ops->handle_intel_pt_intr); + } else { + static_call_update(x86_guest_handle_intel_pt_intr, + (void *)&__static_call_return0); + } if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES)) supported_xss = 0; Index: linux-2.6/include/linux/perf_event.h =================================================================== --- linux-2.6.orig/include/linux/perf_event.h +++ linux-2.6/include/linux/perf_event.h @@ -32,7 +32,6 @@ struct perf_guest_info_callbacks { unsigned int (*state)(void); unsigned long (*get_ip)(void); - unsigned int (*handle_intel_pt_intr)(void); }; #ifdef CONFIG_HAVE_HW_BREAKPOINT Index: linux-2.6/virt/kvm/kvm_main.c =================================================================== --- linux-2.6.orig/virt/kvm/kvm_main.c +++ linux-2.6/virt/kvm/kvm_main.c @@ -5374,12 +5374,10 @@ static unsigned long kvm_guest_get_ip(vo static struct perf_guest_info_callbacks kvm_guest_cbs = { .state = kvm_guest_state, .get_ip = kvm_guest_get_ip, - .handle_intel_pt_intr = NULL, }; -void kvm_register_perf_callbacks(unsigned int (*pt_intr_handler)(void)) +void kvm_register_perf_callbacks(void) { - kvm_guest_cbs.handle_intel_pt_intr = pt_intr_handler; perf_register_guest_info_callbacks(&kvm_guest_cbs); } #endif Index: linux-2.6/arch/arm64/kvm/arm.c =================================================================== --- linux-2.6.orig/arch/arm64/kvm/arm.c +++ linux-2.6/arch/arm64/kvm/arm.c @@ -1749,7 +1749,7 @@ static int init_subsystems(void) goto out; kvm_pmu_init(); - kvm_register_perf_callbacks(NULL); + kvm_register_perf_callbacks(); kvm_sys_reg_table_init(); Index: linux-2.6/include/linux/kvm_host.h =================================================================== --- linux-2.6.orig/include/linux/kvm_host.h +++ linux-2.6/include/linux/kvm_host.h @@ -1121,7 +1121,7 @@ static inline bool kvm_arch_intc_initial #ifdef __KVM_WANT_PERF_CALLBACKS unsigned long kvm_arch_vcpu_get_ip(struct kvm_vcpu *vcpu); -void kvm_register_perf_callbacks(unsigned int (*pt_intr_handler)(void)); +void kvm_register_perf_callbacks(void); static inline void kvm_unregister_perf_callbacks(void) { perf_unregister_guest_info_callbacks();
On Sat, Aug 28, 2021, Peter Zijlstra wrote: > On Fri, Aug 27, 2021 at 05:35:45PM -0700, Sean Christopherson wrote: > > Like Xu (2): > > perf/core: Rework guest callbacks to prepare for static_call support > > perf/core: Use static_call to optimize perf_guest_info_callbacks > > > > Sean Christopherson (11): > > perf: Ensure perf_guest_cbs aren't reloaded between !NULL check and > > deref > > KVM: x86: Register perf callbacks after calling vendor's > > hardware_setup() > > KVM: x86: Register Processor Trace interrupt hook iff PT enabled in > > guest > > perf: Stop pretending that perf can handle multiple guest callbacks > > perf: Force architectures to opt-in to guest callbacks > > KVM: x86: Drop current_vcpu for kvm_running_vcpu + kvm_arch_vcpu > > variable > > KVM: x86: More precisely identify NMI from guest when handling PMI > > KVM: Move x86's perf guest info callbacks to generic KVM > > KVM: x86: Move Intel Processor Trace interrupt handler to vmx.c > > KVM: arm64: Convert to the generic perf callbacks > > KVM: arm64: Drop perf.c and fold its tiny bits of code into arm.c / > > pmu.c Argh, sorry, I somehow managed to miss all of your replies. I'll get back to this series next week. Thanks for the quick response! > Lets keep the whole intel_pt crud inside x86... In theory, I like the idea of burying intel_pt inside x86 (and even in Intel+VMX code for the most part), but the actual implementation is a bit gross. Because of the whole "KVM can be a module" thing, either the static call and __static_call_return0 would need to be exported, or a new register/unregister pair would have to be exported. The unregister path would also need its own synchronize_rcu(). In general, I don't love duplicating the logic, but it's not the end of the world. Either way works for me. Paolo or Peter, do either of you have a preference? > --- > Index: linux-2.6/arch/x86/events/core.c > =================================================================== > --- linux-2.6.orig/arch/x86/events/core.c > +++ linux-2.6/arch/x86/events/core.c > @@ -92,7 +92,7 @@ DEFINE_STATIC_CALL_RET0(x86_pmu_guest_ge > > DEFINE_STATIC_CALL_RET0(x86_guest_state, *(perf_guest_cbs->state)); > DEFINE_STATIC_CALL_RET0(x86_guest_get_ip, *(perf_guest_cbs->get_ip)); > -DEFINE_STATIC_CALL_RET0(x86_guest_handle_intel_pt_intr, *(perf_guest_cbs->handle_intel_pt_intr)); > +DEFINE_STATIC_CALL_RET0(x86_guest_handle_intel_pt_intr, unsigned int (*)(void)); FWIW, the param needs to be a raw function, not a function pointer.
On Thu, Sep 16, 2021 at 09:37:43PM +0000, Sean Christopherson wrote: > On Sat, Aug 28, 2021, Peter Zijlstra wrote: > Argh, sorry, I somehow managed to miss all of your replies. I'll get back to > this series next week. Thanks for the quick response! > > > Lets keep the whole intel_pt crud inside x86... > > In theory, I like the idea of burying intel_pt inside x86 (and even in > Intel+VMX code for the most part), but the actual implementation is a > bit gross. Because of the whole "KVM can be a module" thing, ARGH!! we should really fix that. I've heard other archs have made much better choices here. > either > the static call and __static_call_return0 would need to be exported, > or a new register/unregister pair would have to be exported. So I don't mind exporting __static_call_return0, but exporting a raw static_call is much like exporting a function pointer :/ > The unregister path would also need its own synchronize_rcu(). In general, I > don't love duplicating the logic, but it's not the end of the world. > > Either way works for me. Paolo or Peter, do either of you have a preference? Can we de-feature kvm as a module and only have this PT functionality when built-in? :-) > > --- > > Index: linux-2.6/arch/x86/events/core.c > > =================================================================== > > --- linux-2.6.orig/arch/x86/events/core.c > > +++ linux-2.6/arch/x86/events/core.c > > @@ -92,7 +92,7 @@ DEFINE_STATIC_CALL_RET0(x86_pmu_guest_ge > > > > DEFINE_STATIC_CALL_RET0(x86_guest_state, *(perf_guest_cbs->state)); > > DEFINE_STATIC_CALL_RET0(x86_guest_get_ip, *(perf_guest_cbs->get_ip)); > > -DEFINE_STATIC_CALL_RET0(x86_guest_handle_intel_pt_intr, *(perf_guest_cbs->handle_intel_pt_intr)); > > +DEFINE_STATIC_CALL_RET0(x86_guest_handle_intel_pt_intr, unsigned int (*)(void)); > > FWIW, the param needs to be a raw function, not a function pointer. Yeah, I keep making that mistake.. and I wrote the bloody thing :/ I have a 'fix' for that, but I need to finish that and also flag-day change :-( https://lkml.kernel.org/r/YS+0eIeAJsRRArk4@hirez.programming.kicks-ass.net
On Fri, Sep 17, 2021, Peter Zijlstra wrote: > On Thu, Sep 16, 2021 at 09:37:43PM +0000, Sean Christopherson wrote: > So I don't mind exporting __static_call_return0, but exporting a raw > static_call is much like exporting a function pointer :/ Ya, that part is quite gross. > > The unregister path would also need its own synchronize_rcu(). In general, I > > don't love duplicating the logic, but it's not the end of the world. > > > > Either way works for me. Paolo or Peter, do either of you have a preference? > > Can we de-feature kvm as a module and only have this PT functionality > when built-in? :-) I agree that many of the for-KVM exports are ugly, especially several of the perf exports, but I will fight tooth and nail to keep KVM-as-a-module. It is invaluable for development and testing, and in the not-too-distant future there is KVM-maintenance related functionality that we'd like to implement that relies on KVM being a module. I would be more than happy to help explore approaches that reduce the for-KVM exports, but I am strongly opposed to defeaturing KVM-as-a-module. I have a few nascent ideas for eliminating a handful of a random exports, but no clever ideas for eliminating perf's for-KVM exports.
On 17/09/21 09:28, Peter Zijlstra wrote: >> In theory, I like the idea of burying intel_pt inside x86 (and even in >> Intel+VMX code for the most part), but the actual implementation is a >> bit gross. Because of the whole "KVM can be a module" thing, > > ARGH!! we should really fix that. I've heard other archs have made much > better choices here. I think that's only ARM, and even then it is only because of limitations of the hardware which mostly apply only if VHE is not in use. If anything, it's ARM that should support module build in VHE mode (Linux would still need to know whether it will be running at EL1 or EL2, but KVM's functionality is as self-contained as on x86 in the VHE case). Paolo
On Mon, 20 Sep 2021 13:05:25 +0100, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 17/09/21 09:28, Peter Zijlstra wrote: > >> In theory, I like the idea of burying intel_pt inside x86 (and even in > >> Intel+VMX code for the most part), but the actual implementation is a > >> bit gross. Because of the whole "KVM can be a module" thing, > > > > ARGH!! we should really fix that. I've heard other archs have made much > > better choices here. > > I think that's only ARM, and even then it is only because of > limitations of the hardware which mostly apply only if VHE is not in > use. > > If anything, it's ARM that should support module build in VHE mode > (Linux would still need to know whether it will be running at EL1 or > EL2, but KVM's functionality is as self-contained as on x86 in the VHE > case). I don't see this happening anytime soon. At least not before we declare the arm64 single kernel image policy to be obsolete. M.
On 20/09/21 14:22, Marc Zyngier wrote: >> I think that's only ARM, and even then it is only because of >> limitations of the hardware which mostly apply only if VHE is not in >> use. >> >> If anything, it's ARM that should support module build in VHE mode >> (Linux would still need to know whether it will be running at EL1 or >> EL2, but KVM's functionality is as self-contained as on x86 in the VHE >> case). > I don't see this happening anytime soon. At least not before we > declare the arm64 single kernel image policy to be obsolete. --verbose please. :) I am sure you're right, but I don't understand the link between the two. Paolo
On Mon, 20 Sep 2021 14:18:30 +0100, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 20/09/21 14:22, Marc Zyngier wrote: > >> I think that's only ARM, and even then it is only because of > >> limitations of the hardware which mostly apply only if VHE is not in > >> use. > >> > >> If anything, it's ARM that should support module build in VHE mode > >> (Linux would still need to know whether it will be running at EL1 or > >> EL2, but KVM's functionality is as self-contained as on x86 in the VHE > >> case). > > I don't see this happening anytime soon. At least not before we > > declare the arm64 single kernel image policy to be obsolete. > > --verbose please. :) I am sure you're right, but I don't understand > the link between the two. To start making KVM/arm64 modular, you'd have to build it such as there is no support for the nVHE hypervisor anymore. Which would mean two different configs (one that can only work with VHE, and one for the rest) and contradicts the current single kernel image policy. It is bad enough that we have to support 3 sets of page sizes. Doubling the validation space for the sake of being able to unload KVM seems a dubious prospect. M.
On 20/09/21 15:40, Marc Zyngier wrote: >>> At least not before we >>> declare the arm64 single kernel image policy to be obsolete. >> >> --verbose please.:) I am sure you're right, but I don't understand >> the link between the two. > > To start making KVM/arm64 modular, you'd have to build it such as > there is no support for the nVHE hypervisor anymore. Which would mean > two different configs (one that can only work with VHE, and one for > the rest) and contradicts the current single kernel image policy. Ah okay, I interpreted the policy as "it's possible to build a single kernel image but it would be possible to build an image for a subset of the features as well". In that case you could have one config that can work either with or without VHE (and supports y/n) and one config that can only work with VHE (and supports y/m/n). The code to enter VHE EL2 would of course always be builtin. > It is bad enough that we have to support 3 sets of page sizes. > Doubling the validation space for the sake of being able to unload KVM > seems a dubious prospect. It's not even a configuration that matches kconfig very well, since it does have a way to build something *only as a module*, but not a way to build something only as built-in. That said, if you had the possibility to unload/reload KVM, you'll quickly become unable to live without it. :) Paolo
This is a combination of ~2 series to fix bugs in the perf+KVM callbacks, optimize the callbacks by employing static_call, and do a variety of cleanup in both perf and KVM. Patch 1 fixes a mostly-theoretical bug where perf can deref a NULL pointer if KVM unregisters its callbacks while they're being accessed. In practice, compilers tend to avoid problematic reloads of the pointer and the PMI handler doesn't lose the race against module unloading, i.e doesn't hit a use-after-free. Patches 2 and 3 fix an Intel PT handling bug where KVM incorrectly eats PT interrupts when PT is supposed to be owned entirely by the host. Patches 4-7 clean up perf's callback infrastructure and switch to static_call for arm64 and x86 (the only survivors). Patches 8-13 clean up related KVM code and unify the arm64/x86 callbacks. Based on "git://git.kernel.org/pub/scm/virt/kvm/kvm.git queue", commit 680c7e3be6a3 ("KVM: x86: Exit to userspace ..."). v2 (relatively to static_call v10) - Split the patch into the semantic change (multiplexed ->state) and introduction of static_call. - Don't use '0' for "not a guest RIP". - Handle unregister path. - Drop changes for architectures that can be culled entirely. v2 (relative to v1) - Drop per-cpu approach. [Peter] - Fix mostly-theoretical reload and use-after-free with READ_ONCE(), WRITE_ONCE(), and synchronize_rcu(). [Peter] - Avoid new exports like the plague. [Peter] v1: - https://lkml.kernel.org/r/20210827005718.585190-1-seanjc@google.com v10 static_call: - https://lkml.kernel.org/r/20210806133802.3528-2-lingshan.zhu@intel.com Like Xu (2): perf/core: Rework guest callbacks to prepare for static_call support perf/core: Use static_call to optimize perf_guest_info_callbacks Sean Christopherson (11): perf: Ensure perf_guest_cbs aren't reloaded between !NULL check and deref KVM: x86: Register perf callbacks after calling vendor's hardware_setup() KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest perf: Stop pretending that perf can handle multiple guest callbacks perf: Force architectures to opt-in to guest callbacks KVM: x86: Drop current_vcpu for kvm_running_vcpu + kvm_arch_vcpu variable KVM: x86: More precisely identify NMI from guest when handling PMI KVM: Move x86's perf guest info callbacks to generic KVM KVM: x86: Move Intel Processor Trace interrupt handler to vmx.c KVM: arm64: Convert to the generic perf callbacks KVM: arm64: Drop perf.c and fold its tiny bits of code into arm.c / pmu.c arch/arm/kernel/perf_callchain.c | 28 ++------------ arch/arm64/Kconfig | 1 + arch/arm64/include/asm/kvm_host.h | 13 ++++++- arch/arm64/kernel/perf_callchain.c | 28 +++++++++++--- arch/arm64/kvm/Makefile | 2 +- arch/arm64/kvm/arm.c | 11 +++++- arch/arm64/kvm/perf.c | 62 ------------------------------ arch/arm64/kvm/pmu.c | 8 ++++ arch/csky/kernel/perf_callchain.c | 10 ----- arch/nds32/kernel/perf_event_cpu.c | 29 ++------------ arch/riscv/kernel/perf_callchain.c | 10 ----- arch/x86/Kconfig | 1 + arch/x86/events/core.c | 36 ++++++++++++++--- arch/x86/events/intel/core.c | 7 ++-- arch/x86/include/asm/kvm_host.h | 8 +++- arch/x86/kvm/pmu.c | 2 +- arch/x86/kvm/svm/svm.c | 2 +- arch/x86/kvm/vmx/vmx.c | 25 +++++++++++- arch/x86/kvm/x86.c | 58 +++++----------------------- arch/x86/kvm/x86.h | 17 ++++++-- arch/x86/xen/pmu.c | 32 +++++++-------- include/kvm/arm_pmu.h | 1 + include/linux/kvm_host.h | 10 +++++ include/linux/perf_event.h | 26 ++++++++----- init/Kconfig | 3 ++ kernel/events/core.c | 24 ++++++------ virt/kvm/kvm_main.c | 40 +++++++++++++++++++ 27 files changed, 245 insertions(+), 249 deletions(-) delete mode 100644 arch/arm64/kvm/perf.c