Re: [PATCH v3 00/16] perf: KVM: Fix, optimize, and clean up callbacks

Paolo Bonzini
2021-09-22T06:42:38Z

On 22/09/21 02:05, Sean Christopherson wrote:
> Peter, I left the Intel PT mess as-is. Having to pass a NULL pointer
> from KVM arm64 seemed to be a lesser evil than more exports and multiple
> registration paths.
>
> 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-9 clean up perf's callback infrastructure and switch to
> static_call for arm64 and x86 (the only survivors).
>
> Patches 10-16 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 ...").

Looks nice apart from a couple nits, I will gladly accept a topic branch
with both the perf and the KVM parts.

Thanks,

Paolo

> v3:
> - Add wrappers for guest callbacks to that stubs can be provided when
> GUEST_PERF_EVENTS=n.
> - s/HAVE_GUEST_PERF_EVENTS/GUEST_PERF_EVENTS and select it from KVM
> and XEN_PV instead of from top-level arm64/x86. [Paolo]
> - Drop an unnecessary synchronize_rcu() when registering callbacks. [Peter]
> - Retain a WARN_ON_ONCE() when unregistering callbacks if the caller
> didn't provide the correct pointer. [Peter]
> - Rework the static_call patch to move it all to common perf.
> - Add a patch to drop the (un)register stubs, made possible after
> having KVM+XEN_PV select GUEST_PERF_EVENTS.
> - Split dropping guest callback "support" for arm, csky, etc... to a
> separate patch, to make introducing GUEST_PERF_EVENTS cleaner.
>
> v2 (relative 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):
> - https://lkml.kernel.org/r/20210828003558.713983-6-seanjc@google.com
> - 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 (1):
> perf/core: Rework guest callbacks to prepare for static_call support
>
> Sean Christopherson (15):
> 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: Drop dead and useless guest "support" from arm, csky, nds32 and
> riscv
> perf: Add wrappers for invoking guest callbacks
> perf: Force architectures to opt-in to guest callbacks
> perf/core: Use static_call to optimize perf_guest_info_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
> perf: Drop guest callback (un)register stubs
>
> arch/arm/kernel/perf_callchain.c | 28 ++------------
> arch/arm64/include/asm/kvm_host.h | 9 ++++-
> arch/arm64/kernel/perf_callchain.c | 13 ++++---
> arch/arm64/kvm/Kconfig | 1 +
> 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/events/core.c | 13 ++++---
> arch/x86/events/intel/core.c | 5 +--
> arch/x86/include/asm/kvm_host.h | 7 +++-
> arch/x86/kvm/Kconfig | 1 +
> 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/Kconfig | 1 +
> arch/x86/xen/pmu.c | 32 +++++++--------
> include/kvm/arm_pmu.h | 1 +
> include/linux/kvm_host.h | 10 +++++
> include/linux/perf_event.h | 41 ++++++++++++++------
> init/Kconfig | 4 ++
> kernel/events/core.c | 39 +++++++++++++------
> virt/kvm/kvm_main.c | 44 +++++++++++++++++++++
> 28 files changed, 235 insertions(+), 250 deletions(-)
> delete mode 100644 arch/arm64/kvm/perf.c
>