diff mbox series

[RFC,1/3] KVM + perf: Rename *_intel_pt_intr() for generic usage

Message ID 20220926142938.89608-2-likexu@tencent.com (mailing list archive)
State New, archived
Headers show
Series KVM: x86/pmu: Add Intel Guest Branch Trace Store Support | expand

Commit Message

Like Xu Sept. 26, 2022, 2:29 p.m. UTC 
From: Like Xu <likexu@tencent.com>

The perf_guest_info_callbacks is common to KVM, while intel_pt is not,
not even common to x86. In the VMX context, it makes sense to hook
up the intel_pt specific hook, and given the uniqueness of this usage,
calling the  generic callback in the explicit location of the perf context
is not functionally broken.

Rename a bunch of intel_pt_intr() functions to the generic guest_intr().
No functional change intended.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/events/intel/core.c    |  2 +-
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/vmx/vmx.c          |  6 +++---
 arch/x86/kvm/x86.c              |  2 +-
 include/linux/perf_event.h      | 12 +++++++-----
 kernel/events/core.c            |  9 ++++-----
 virt/kvm/kvm_main.c             |  6 +++---
 7 files changed, 20 insertions(+), 19 deletions(-)

Comments

Sean Christopherson Oct. 3, 2022, 7:39 p.m. UTC | #1 
On Mon, Sep 26, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> The perf_guest_info_callbacks is common to KVM, while intel_pt is not,
> not even common to x86. In the VMX context, it makes sense to hook
> up the intel_pt specific hook, and given the uniqueness of this usage,
> calling the  generic callback in the explicit location of the perf context
> is not functionally broken.

But it's extremely misleading.  If I were a developer writing the perf hooks for
a different architecture, I would expect perf_handle_guest_intr() to be called on
_every_ perf interrupt that occurred in the guest.

Genericizing the hook also complicates wiring up the hook and consuming the interrupt
type.  E.g. patch 3 is buggy; it wires up the VMX handler if and only if PT is in
PT_MODE_HOST_GUEST, and then takes a dependency on that buggy behavior by not
checking if Intel PT is supported in the now-generic vmx_handle_guest_intr().

This also doesn't really clean up the API from a non-x86 perspective, it just doesn't
make it any worse, i.e. other architectures are still exposed to an x86-specific hook.

Unless we anticipate ARM or RISC-V (which IIRC is gaining PMU support "soon") needing
to hook into "special" perf interrupts, it might be better to figure out a way to make
the hooks themselves more extensible for per-arch behavior.  E.g similar to
kvm_vcpu and kvm_vcpu_arch, add an embedded arch (or vice versa) struct in
perf_guest_info_callbacks plus a perf-internal arch hook to update static calls,
and use that to wire up handle_intel_pt_int for x86.  It'll require more work up
front, but in theory it will require less maintenance in the long run.

> Rename a bunch of intel_pt_intr() functions to the generic guest_intr().
> No functional change intended.

This changelog never says _why_.  Looking forward, the reason for the rename is
to piggyback the hook for BTS.
diff mbox series

Patch

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 65906241207e..48e313265a15 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2962,7 +2962,7 @@  static int handle_pmi_common(struct pt_regs *regs, u64 status)
 	 */
 	if (__test_and_clear_bit(GLOBAL_STATUS_TRACE_TOPAPMI_BIT, (unsigned long *)&status)) {
 		handled++;
-		if (!perf_guest_handle_intel_pt_intr())
+		if (!perf_handle_guest_intr())
 			intel_pt_interrupt();
 	}
 
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f1d3ae0b57bb..8cf472a4ca06 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1666,7 +1666,7 @@  struct kvm_x86_init_ops {
 	int (*disabled_by_bios)(void);
 	int (*check_processor_compatibility)(void);
 	int (*hardware_setup)(void);
-	unsigned int (*handle_intel_pt_intr)(void);
+	unsigned int (*handle_intr)(void);
 
 	struct kvm_x86_ops *runtime_ops;
 	struct kvm_pmu_ops *pmu_ops;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c9b49a09e6b5..a1856b11467d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -8374,9 +8374,9 @@  static __init int hardware_setup(void)
 	if (!enable_ept || !enable_pmu || !cpu_has_vmx_intel_pt())
 		pt_mode = PT_MODE_SYSTEM;
 	if (pt_mode == PT_MODE_HOST_GUEST)
-		vmx_init_ops.handle_intel_pt_intr = vmx_handle_intel_pt_intr;
+		vmx_init_ops.handle_intr = vmx_handle_intel_pt_intr;
 	else
-		vmx_init_ops.handle_intel_pt_intr = NULL;
+		vmx_init_ops.handle_intr = NULL;
 
 	setup_default_sgx_lepubkeyhash();
 
@@ -8405,7 +8405,7 @@  static struct kvm_x86_init_ops vmx_init_ops __initdata = {
 	.disabled_by_bios = vmx_disabled_by_bios,
 	.check_processor_compatibility = vmx_check_processor_compat,
 	.hardware_setup = hardware_setup,
-	.handle_intel_pt_intr = NULL,
+	.handle_intr = NULL,
 
 	.runtime_ops = &vmx_x86_ops,
 	.pmu_ops = &intel_pmu_ops,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6a0e5107de5c..6eff470d3f7d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11990,7 +11990,7 @@  int kvm_arch_hardware_setup(void *opaque)
 
 	kvm_ops_update(ops);
 
-	kvm_register_perf_callbacks(ops->handle_intel_pt_intr);
+	kvm_register_perf_callbacks(ops->handle_intr);
 
 	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
 		kvm_caps.supported_xss = 0;
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index ee8b9ecdc03b..6149a977bbd0 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -32,7 +32,7 @@ 
 struct perf_guest_info_callbacks {
 	unsigned int			(*state)(void);
 	unsigned long			(*get_ip)(void);
-	unsigned int			(*handle_intel_pt_intr)(void);
+	unsigned int			(*handle_intr)(void);
 };
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
@@ -1267,7 +1267,7 @@  extern struct perf_guest_info_callbacks __rcu *perf_guest_cbs;
 
 DECLARE_STATIC_CALL(__perf_guest_state, *perf_guest_cbs->state);
 DECLARE_STATIC_CALL(__perf_guest_get_ip, *perf_guest_cbs->get_ip);
-DECLARE_STATIC_CALL(__perf_guest_handle_intel_pt_intr, *perf_guest_cbs->handle_intel_pt_intr);
+DECLARE_STATIC_CALL(__perf_handle_guest_intr, *perf_guest_cbs->handle_intr);
 
 static inline unsigned int perf_guest_state(void)
 {
@@ -1277,16 +1277,18 @@  static inline unsigned long perf_guest_get_ip(void)
 {
 	return static_call(__perf_guest_get_ip)();
 }
-static inline unsigned int perf_guest_handle_intel_pt_intr(void)
+
+static inline unsigned int perf_handle_guest_intr(void)
 {
-	return static_call(__perf_guest_handle_intel_pt_intr)();
+	return static_call(__perf_handle_guest_intr)();
 }
+
 extern void perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs);
 extern void perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *cbs);
 #else
 static inline unsigned int perf_guest_state(void)		 { return 0; }
 static inline unsigned long perf_guest_get_ip(void)		 { return 0; }
-static inline unsigned int perf_guest_handle_intel_pt_intr(void) { return 0; }
+static inline unsigned int perf_handle_guest_intr(void) { return 0; }
 #endif /* CONFIG_GUEST_PERF_EVENTS */
 
 extern void perf_event_exec(void);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 2621fd24ad26..bb1d1925f153 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6510,7 +6510,7 @@  struct perf_guest_info_callbacks __rcu *perf_guest_cbs;
 
 DEFINE_STATIC_CALL_RET0(__perf_guest_state, *perf_guest_cbs->state);
 DEFINE_STATIC_CALL_RET0(__perf_guest_get_ip, *perf_guest_cbs->get_ip);
-DEFINE_STATIC_CALL_RET0(__perf_guest_handle_intel_pt_intr, *perf_guest_cbs->handle_intel_pt_intr);
+DEFINE_STATIC_CALL_RET0(__perf_handle_guest_intr, *perf_guest_cbs->handle_intr);
 
 void perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
 {
@@ -6522,9 +6522,8 @@  void perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
 	static_call_update(__perf_guest_get_ip, cbs->get_ip);
 
 	/* Implementing ->handle_intel_pt_intr is optional. */
-	if (cbs->handle_intel_pt_intr)
-		static_call_update(__perf_guest_handle_intel_pt_intr,
-				   cbs->handle_intel_pt_intr);
+	if (cbs->handle_intr)
+		static_call_update(__perf_handle_guest_intr, cbs->handle_intr);
 }
 EXPORT_SYMBOL_GPL(perf_register_guest_info_callbacks);
 
@@ -6536,7 +6535,7 @@  void perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
 	rcu_assign_pointer(perf_guest_cbs, NULL);
 	static_call_update(__perf_guest_state, (void *)&__static_call_return0);
 	static_call_update(__perf_guest_get_ip, (void *)&__static_call_return0);
-	static_call_update(__perf_guest_handle_intel_pt_intr,
+	static_call_update(__perf_handle_guest_intr,
 			   (void *)&__static_call_return0);
 	synchronize_rcu();
 }
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 584a5bab3af3..8190af3a12fa 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -5785,12 +5785,12 @@  static unsigned long kvm_guest_get_ip(void)
 static struct perf_guest_info_callbacks kvm_guest_cbs = {
 	.state			= kvm_guest_state,
 	.get_ip			= kvm_guest_get_ip,
-	.handle_intel_pt_intr	= NULL,
+	.handle_intr	= NULL,
 };
 
-void kvm_register_perf_callbacks(unsigned int (*pt_intr_handler)(void))
+void kvm_register_perf_callbacks(unsigned int (*handler)(void))
 {
-	kvm_guest_cbs.handle_intel_pt_intr = pt_intr_handler;
+	kvm_guest_cbs.handle_intr = handler;
 	perf_register_guest_info_callbacks(&kvm_guest_cbs);
 }
 void kvm_unregister_perf_callbacks(void)