All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] perf/x86/intel: KVM: PT intr handler fix and cleanup
@ 2021-08-23 19:37 Sean Christopherson
  2021-08-23 19:37 ` [PATCH 1/3] KVM: x86: Register perf callbacks after calling vendor's hardware_setup() Sean Christopherson
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Sean Christopherson @ 2021-08-23 19:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Paolo Bonzini
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, linux-perf-users, linux-kernel, kvm,
	Artem Kashkanov

Patches 1 and 2 fix a bug where PT PMIs in guest are forwarded to KVM's PT
intr handler even if PT is configured for system mode, i.e. when the host
is the sole owner of PT.

Patch 3 is a related cleanup/optimization.

The PT specific stuff is effectively compile-tested only.

Sean Christopherson (3):
  KVM: x86: Register perf callbacks after calling vendor's
    hardware_setup()
  KVM: x86: Register Processor Trace interrupt hook iff PT enabled in
    guest
  perf/x86/intel: Fold current_vcpu check into KVM's PT intr handler

 arch/x86/events/intel/core.c    |  7 +++----
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/pmu.h              |  1 +
 arch/x86/kvm/vmx/vmx.c          |  1 +
 arch/x86/kvm/x86.c              | 17 ++++++++++++-----
 include/linux/perf_event.h      |  2 +-
 6 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.33.0.rc2.250.ged5fa647cd-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] KVM: x86: Register perf callbacks after calling vendor's hardware_setup()
  2021-08-23 19:37 [PATCH 0/3] perf/x86/intel: KVM: PT intr handler fix and cleanup Sean Christopherson
@ 2021-08-23 19:37 ` Sean Christopherson
  2021-08-23 19:37 ` [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest Sean Christopherson
  2021-08-23 19:37 ` [PATCH 3/3] perf/x86/intel: Fold current_vcpu check into KVM's PT intr handler Sean Christopherson
  2 siblings, 0 replies; 9+ messages in thread
From: Sean Christopherson @ 2021-08-23 19:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Paolo Bonzini
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, linux-perf-users, linux-kernel, kvm,
	Artem Kashkanov

Wait to register perf callbacks until after doing vendor hardaware setup.
VMX's hardware_setup() configures Intel Processor Trace (PT) mode, and a
future fix to register the Intel PT guest interrupt hook if and only if
Intel PT is exposed to the guest will consume the configured PT mode.

Delaying registration to hardware setup is effectively a nop as KVM's perf
hooks all pivot on the per-CPU current_vcpu, which is non-NULL only when
KVM is handling an IRQ/NMI in a VM-Exit path.  I.e. current_vcpu will be
NULL throughout both kvm_arch_init() and kvm_arch_hardware_setup().

Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Artem Kashkanov <artem.kashkanov@intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 86539c1686fa..fb6015f97f9e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8426,8 +8426,6 @@ int kvm_arch_init(void *opaque)
 
 	kvm_timer_init();
 
-	perf_register_guest_info_callbacks(&kvm_guest_cbs);
-
 	if (boot_cpu_has(X86_FEATURE_XSAVE)) {
 		host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
 		supported_xcr0 = host_xcr0 & KVM_SUPPORTED_XCR0;
@@ -8461,7 +8459,6 @@ void kvm_arch_exit(void)
 		clear_hv_tscchange_cb();
 #endif
 	kvm_lapic_exit();
-	perf_unregister_guest_info_callbacks(&kvm_guest_cbs);
 
 	if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
 		cpufreq_unregister_notifier(&kvmclock_cpufreq_notifier_block,
@@ -11064,6 +11061,8 @@ int kvm_arch_hardware_setup(void *opaque)
 	memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
 	kvm_ops_static_call_update();
 
+	perf_register_guest_info_callbacks(&kvm_guest_cbs);
+
 	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
 		supported_xss = 0;
 
@@ -11091,6 +11090,8 @@ int kvm_arch_hardware_setup(void *opaque)
 
 void kvm_arch_hardware_unsetup(void)
 {
+	perf_unregister_guest_info_callbacks(&kvm_guest_cbs);
+
 	static_call(kvm_x86_hardware_unsetup)();
 }
 
-- 
2.33.0.rc2.250.ged5fa647cd-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest
  2021-08-23 19:37 [PATCH 0/3] perf/x86/intel: KVM: PT intr handler fix and cleanup Sean Christopherson
  2021-08-23 19:37 ` [PATCH 1/3] KVM: x86: Register perf callbacks after calling vendor's hardware_setup() Sean Christopherson
@ 2021-08-23 19:37 ` Sean Christopherson
  2021-08-24  7:28   ` Alexander Shishkin
  2021-08-25  7:24   ` Like Xu
  2021-08-23 19:37 ` [PATCH 3/3] perf/x86/intel: Fold current_vcpu check into KVM's PT intr handler Sean Christopherson
  2 siblings, 2 replies; 9+ messages in thread
From: Sean Christopherson @ 2021-08-23 19:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Paolo Bonzini
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, linux-perf-users, linux-kernel, kvm,
	Artem Kashkanov

Override the Processor Trace (PT) interrupt handler for guest mode if and
only if PT is configured for host+guest mode, i.e. is being used
independently by both host and guest.  If PT is configured for system
mode, the host fully controls PT and must handle all events.

Fixes: 8479e04e7d6b ("KVM: x86: Inject PMI for KVM guest")
Cc: stable@vger.kernel.org
Reported-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reported-by: Artem Kashkanov <artem.kashkanov@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/pmu.h              | 1 +
 arch/x86/kvm/vmx/vmx.c          | 1 +
 arch/x86/kvm/x86.c              | 4 +++-
 4 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 09b256db394a..1ea4943a73d7 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1494,6 +1494,7 @@ struct kvm_x86_init_ops {
 	int (*disabled_by_bios)(void);
 	int (*check_processor_compatibility)(void);
 	int (*hardware_setup)(void);
+	bool (*intel_pt_intr_in_guest)(void);
 
 	struct kvm_x86_ops *runtime_ops;
 };
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 0e4f2b1fa9fb..b06dbbd7eeeb 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -41,6 +41,7 @@ struct kvm_pmu_ops {
 	void (*reset)(struct kvm_vcpu *vcpu);
 	void (*deliver_pmi)(struct kvm_vcpu *vcpu);
 	void (*cleanup)(struct kvm_vcpu *vcpu);
+	void (*handle_intel_pt_intr)(void);
 };
 
 static inline u64 pmc_bitmask(struct kvm_pmc *pmc)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index fada1055f325..f19d72136f77 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7896,6 +7896,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,
+	.intel_pt_intr_in_guest = vmx_pt_mode_is_host_guest,
 
 	.runtime_ops = &vmx_x86_ops,
 };
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fb6015f97f9e..b5ade47dad9c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8305,7 +8305,7 @@ static struct perf_guest_info_callbacks kvm_guest_cbs = {
 	.is_in_guest		= kvm_is_in_guest,
 	.is_user_mode		= kvm_is_user_mode,
 	.get_guest_ip		= kvm_get_guest_ip,
-	.handle_intel_pt_intr	= kvm_handle_intel_pt_intr,
+	.handle_intel_pt_intr	= NULL,
 };
 
 #ifdef CONFIG_X86_64
@@ -11061,6 +11061,8 @@ int kvm_arch_hardware_setup(void *opaque)
 	memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
 	kvm_ops_static_call_update();
 
+	if (ops->intel_pt_intr_in_guest && ops->intel_pt_intr_in_guest())
+		kvm_guest_cbs.handle_intel_pt_intr = kvm_handle_intel_pt_intr;
 	perf_register_guest_info_callbacks(&kvm_guest_cbs);
 
 	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
-- 
2.33.0.rc2.250.ged5fa647cd-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] perf/x86/intel: Fold current_vcpu check into KVM's PT intr handler
  2021-08-23 19:37 [PATCH 0/3] perf/x86/intel: KVM: PT intr handler fix and cleanup Sean Christopherson
  2021-08-23 19:37 ` [PATCH 1/3] KVM: x86: Register perf callbacks after calling vendor's hardware_setup() Sean Christopherson
  2021-08-23 19:37 ` [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest Sean Christopherson
@ 2021-08-23 19:37 ` Sean Christopherson
  2 siblings, 0 replies; 9+ messages in thread
From: Sean Christopherson @ 2021-08-23 19:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Paolo Bonzini
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, linux-perf-users, linux-kernel, kvm,
	Artem Kashkanov

Move the check for a non-NULL current_vcpu into KVM's PT intr handler
instead of relying on the caller to perform the check.  In addition to
ensuring KVM's handler won't dereference a NULL pointer (and making it
obvious that it can't), this avoids a reptoline when KVM is configured to
run PT in "system mode", in which case handle_intel_pt_intr will be NULL.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/events/intel/core.c | 7 +++----
 arch/x86/kvm/pmu.h           | 2 +-
 arch/x86/kvm/x86.c           | 6 +++++-
 include/linux/perf_event.h   | 2 +-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index fca7a6e2242f..060f1f1ebe15 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2852,10 +2852,9 @@ 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 (unlikely(perf_guest_cbs && perf_guest_cbs->is_in_guest() &&
-			perf_guest_cbs->handle_intel_pt_intr))
-			perf_guest_cbs->handle_intel_pt_intr();
-		else
+		if (likely(!perf_guest_cbs ||
+			   !perf_guest_cbs->handle_intel_pt_intr ||
+			   perf_guest_cbs->handle_intel_pt_intr()))
 			intel_pt_interrupt();
 	}
 
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index b06dbbd7eeeb..4e8a38eca72b 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -41,7 +41,7 @@ struct kvm_pmu_ops {
 	void (*reset)(struct kvm_vcpu *vcpu);
 	void (*deliver_pmi)(struct kvm_vcpu *vcpu);
 	void (*cleanup)(struct kvm_vcpu *vcpu);
-	void (*handle_intel_pt_intr)(void);
+	int (*handle_intel_pt_intr)(void);
 };
 
 static inline u64 pmc_bitmask(struct kvm_pmc *pmc)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b5ade47dad9c..3f289192f25f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8292,13 +8292,17 @@ static unsigned long kvm_get_guest_ip(void)
 	return ip;
 }
 
-static void kvm_handle_intel_pt_intr(void)
+static int kvm_handle_intel_pt_intr(void)
 {
 	struct kvm_vcpu *vcpu = __this_cpu_read(current_vcpu);
 
+	if (!vcpu)
+		return -ENXIO;
+
 	kvm_make_request(KVM_REQ_PMI, vcpu);
 	__set_bit(MSR_CORE_PERF_GLOBAL_OVF_CTRL_TRACE_TOPA_PMI_BIT,
 			(unsigned long *)&vcpu->arch.pmu.global_status);
+	return 0;
 }
 
 static struct perf_guest_info_callbacks kvm_guest_cbs = {
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 2d510ad750ed..f812c2570285 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -30,7 +30,7 @@ struct perf_guest_info_callbacks {
 	int				(*is_in_guest)(void);
 	int				(*is_user_mode)(void);
 	unsigned long			(*get_guest_ip)(void);
-	void				(*handle_intel_pt_intr)(void);
+	int				(*handle_intel_pt_intr)(void);
 };
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
-- 
2.33.0.rc2.250.ged5fa647cd-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest
  2021-08-23 19:37 ` [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest Sean Christopherson
@ 2021-08-24  7:28   ` Alexander Shishkin
  2021-08-24 14:11     ` Sean Christopherson
  2021-08-25  7:24   ` Like Xu
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Shishkin @ 2021-08-24  7:28 UTC (permalink / raw)
  To: Sean Christopherson, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Paolo Bonzini
  Cc: Mark Rutland, Jiri Olsa, Namhyung Kim, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	linux-perf-users, linux-kernel, kvm, Artem Kashkanov,
	alexander.shishkin

Sean Christopherson <seanjc@google.com> writes:

> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index 0e4f2b1fa9fb..b06dbbd7eeeb 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -41,6 +41,7 @@ struct kvm_pmu_ops {
>  	void (*reset)(struct kvm_vcpu *vcpu);
>  	void (*deliver_pmi)(struct kvm_vcpu *vcpu);
>  	void (*cleanup)(struct kvm_vcpu *vcpu);
> +	void (*handle_intel_pt_intr)(void);

What's this one for?

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest
  2021-08-24  7:28   ` Alexander Shishkin
@ 2021-08-24 14:11     ` Sean Christopherson
  0 siblings, 0 replies; 9+ messages in thread
From: Sean Christopherson @ 2021-08-24 14:11 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Paolo Bonzini, Mark Rutland, Jiri Olsa, Namhyung Kim,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	linux-perf-users, linux-kernel, kvm, Artem Kashkanov

On Tue, Aug 24, 2021, Alexander Shishkin wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> > index 0e4f2b1fa9fb..b06dbbd7eeeb 100644
> > --- a/arch/x86/kvm/pmu.h
> > +++ b/arch/x86/kvm/pmu.h
> > @@ -41,6 +41,7 @@ struct kvm_pmu_ops {
> >  	void (*reset)(struct kvm_vcpu *vcpu);
> >  	void (*deliver_pmi)(struct kvm_vcpu *vcpu);
> >  	void (*cleanup)(struct kvm_vcpu *vcpu);
> > +	void (*handle_intel_pt_intr)(void);
> 
> What's this one for?

Doh, the remnants of one of my explorations trying to figure out the least gross
way to conditionally register the handling.  I'll get it removed.

Good eyeballs, thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest
  2021-08-23 19:37 ` [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest Sean Christopherson
  2021-08-24  7:28   ` Alexander Shishkin
@ 2021-08-25  7:24   ` Like Xu
  2021-08-25 14:59     ` Sean Christopherson
  1 sibling, 1 reply; 9+ messages in thread
From: Like Xu @ 2021-08-25  7:24 UTC (permalink / raw)
  To: Sean Christopherson, Alexander Shishkin
  Cc: Mark Rutland, Jiri Olsa, Namhyung Kim, Vitaly Kuznetsov,
	Wanpeng Li, Jim Mattson, Joerg Roedel, linux-perf-users,
	linux-kernel, kvm, Artem Kashkanov, Arnaldo Carvalho de Melo,
	Paolo Bonzini, Ingo Molnar, Peter Zijlstra

On 24/8/2021 3:37 am, Sean Christopherson wrote:
> Override the Processor Trace (PT) interrupt handler for guest mode if and
> only if PT is configured for host+guest mode, i.e. is being used
> independently by both host and guest.  If PT is configured for system
> mode, the host fully controls PT and must handle all events.
> 
> Fixes: 8479e04e7d6b ("KVM: x86: Inject PMI for KVM guest")
> Cc: stable@vger.kernel.org
> Reported-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Reported-by: Artem Kashkanov <artem.kashkanov@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/include/asm/kvm_host.h | 1 +
>   arch/x86/kvm/pmu.h              | 1 +
>   arch/x86/kvm/vmx/vmx.c          | 1 +
>   arch/x86/kvm/x86.c              | 4 +++-
>   4 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 09b256db394a..1ea4943a73d7 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1494,6 +1494,7 @@ struct kvm_x86_init_ops {
>   	int (*disabled_by_bios)(void);
>   	int (*check_processor_compatibility)(void);
>   	int (*hardware_setup)(void);
> +	bool (*intel_pt_intr_in_guest)(void);
>   
>   	struct kvm_x86_ops *runtime_ops;
>   };
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index 0e4f2b1fa9fb..b06dbbd7eeeb 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -41,6 +41,7 @@ struct kvm_pmu_ops {
>   	void (*reset)(struct kvm_vcpu *vcpu);
>   	void (*deliver_pmi)(struct kvm_vcpu *vcpu);
>   	void (*cleanup)(struct kvm_vcpu *vcpu);
> +	void (*handle_intel_pt_intr)(void);
>   };
>   
>   static inline u64 pmc_bitmask(struct kvm_pmc *pmc)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index fada1055f325..f19d72136f77 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7896,6 +7896,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,
> +	.intel_pt_intr_in_guest = vmx_pt_mode_is_host_guest,
>   
>   	.runtime_ops = &vmx_x86_ops,
>   };
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fb6015f97f9e..b5ade47dad9c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8305,7 +8305,7 @@ static struct perf_guest_info_callbacks kvm_guest_cbs = {
>   	.is_in_guest		= kvm_is_in_guest,
>   	.is_user_mode		= kvm_is_user_mode,
>   	.get_guest_ip		= kvm_get_guest_ip,
> -	.handle_intel_pt_intr	= kvm_handle_intel_pt_intr,
> +	.handle_intel_pt_intr	= NULL,
>   };
>   
>   #ifdef CONFIG_X86_64
> @@ -11061,6 +11061,8 @@ int kvm_arch_hardware_setup(void *opaque)
>   	memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
>   	kvm_ops_static_call_update();
>   
> +	if (ops->intel_pt_intr_in_guest && ops->intel_pt_intr_in_guest())
> +		kvm_guest_cbs.handle_intel_pt_intr = kvm_handle_intel_pt_intr;

Emm, it's still buggy.

The guest "unknown NMI" from the host Intel PT can still be reproduced
after the following operation:

	rmmod kvm_intel
	modprobe kvm-intel pt_mode=1 ept=1
	rmmod kvm_intel
	modprobe kvm-intel pt_mode=1 ept=0

Since the handle_intel_pt_intr is not reset to NULL in kvm_arch_hardware_unsetup(),
and the previous function pointer still exists in the generic KVM data structure.

But I have to say that this fix is much better than the one I proposed [1].

[1] https://lore.kernel.org/lkml/20210514084436.848396-1-like.xu@linux.intel.com/

>   	perf_register_guest_info_callbacks(&kvm_guest_cbs);
>   
>   	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest
  2021-08-25  7:24   ` Like Xu
@ 2021-08-25 14:59     ` Sean Christopherson
  2021-08-25 20:09       ` Sean Christopherson
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Christopherson @ 2021-08-25 14:59 UTC (permalink / raw)
  To: Like Xu
  Cc: Alexander Shishkin, Mark Rutland, Jiri Olsa, Namhyung Kim,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	linux-perf-users, linux-kernel, kvm, Artem Kashkanov,
	Arnaldo Carvalho de Melo, Paolo Bonzini, Ingo Molnar,
	Peter Zijlstra

On Wed, Aug 25, 2021, Like Xu wrote:
> On 24/8/2021 3:37 am, Sean Christopherson wrote:
> > @@ -11061,6 +11061,8 @@ int kvm_arch_hardware_setup(void *opaque)
> >   	memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
> >   	kvm_ops_static_call_update();
> > +	if (ops->intel_pt_intr_in_guest && ops->intel_pt_intr_in_guest())
> > +		kvm_guest_cbs.handle_intel_pt_intr = kvm_handle_intel_pt_intr;
> 
> Emm, it's still buggy.
> 
> The guest "unknown NMI" from the host Intel PT can still be reproduced
> after the following operation:
> 
> 	rmmod kvm_intel
> 	modprobe kvm-intel pt_mode=1 ept=1
> 	rmmod kvm_intel
> 	modprobe kvm-intel pt_mode=1 ept=0
> 
> Since the handle_intel_pt_intr is not reset to NULL in kvm_arch_hardware_unsetup(),
> and the previous function pointer still exists in the generic KVM data structure.

Ooof, good catch.  Any preference between nullifying handle_intel_pt_intr in
setup() vs. unsetup()?  I think I like the idea of "unwinding" during unsetup(),
even though it splits the logic a bit.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b5ade47dad9c..ffc6c2d73508 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11093,6 +11093,7 @@ int kvm_arch_hardware_setup(void *opaque)
 void kvm_arch_hardware_unsetup(void)
 {
        perf_unregister_guest_info_callbacks(&kvm_guest_cbs);
+       kvm_guest_cbs.handle_intel_pt_intr = NULL;

        static_call(kvm_x86_hardware_unsetup)();
 }


vs.


diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b5ade47dad9c..462aa7a360e9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11063,6 +11063,8 @@ int kvm_arch_hardware_setup(void *opaque)

        if (ops->intel_pt_intr_in_guest && ops->intel_pt_intr_in_guest())
                kvm_guest_cbs.handle_intel_pt_intr = kvm_handle_intel_pt_intr;
+       else
+               kvm_guest_cbs.handle_intel_pt_intr = NULL;
        perf_register_guest_info_callbacks(&kvm_guest_cbs);

        if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))

> But I have to say that this fix is much better than the one I proposed [1].
> 
> [1] https://lore.kernel.org/lkml/20210514084436.848396-1-like.xu@linux.intel.com/
> 
> >   	perf_register_guest_info_callbacks(&kvm_guest_cbs);
> >   	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
> > 

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest
  2021-08-25 14:59     ` Sean Christopherson
@ 2021-08-25 20:09       ` Sean Christopherson
  0 siblings, 0 replies; 9+ messages in thread
From: Sean Christopherson @ 2021-08-25 20:09 UTC (permalink / raw)
  To: Like Xu
  Cc: Alexander Shishkin, Mark Rutland, Jiri Olsa, Namhyung Kim,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	linux-perf-users, linux-kernel, kvm, Artem Kashkanov,
	Arnaldo Carvalho de Melo, Paolo Bonzini, Ingo Molnar,
	Peter Zijlstra

On Wed, Aug 25, 2021, Sean Christopherson wrote:
> On Wed, Aug 25, 2021, Like Xu wrote:
> > On 24/8/2021 3:37 am, Sean Christopherson wrote:
> > > @@ -11061,6 +11061,8 @@ int kvm_arch_hardware_setup(void *opaque)
> > >   	memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
> > >   	kvm_ops_static_call_update();
> > > +	if (ops->intel_pt_intr_in_guest && ops->intel_pt_intr_in_guest())
> > > +		kvm_guest_cbs.handle_intel_pt_intr = kvm_handle_intel_pt_intr;
> > 
> > Emm, it's still buggy.
> > 
> > The guest "unknown NMI" from the host Intel PT can still be reproduced
> > after the following operation:
> > 
> > 	rmmod kvm_intel
> > 	modprobe kvm-intel pt_mode=1 ept=1
> > 	rmmod kvm_intel
> > 	modprobe kvm-intel pt_mode=1 ept=0
> > 
> > Since the handle_intel_pt_intr is not reset to NULL in kvm_arch_hardware_unsetup(),
> > and the previous function pointer still exists in the generic KVM data structure.
> 
> Ooof, good catch.  Any preference between nullifying handle_intel_pt_intr in
> setup() vs. unsetup()?  I think I like the idea of "unwinding" during unsetup(),
> even though it splits the logic a bit.

Never mind, I figured out a way to clean all this up and land the PT interrupt
handler in vmx.c where it belongs.  Getting there is a bit of a journey, but it's
very doable.  That means unwinding in unsetup() is the preferred approach,
otherwise there would be potential for leaving a dangling pointer if a different
vendor module was succesfully loaded.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-08-25 20:09 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-23 19:37 [PATCH 0/3] perf/x86/intel: KVM: PT intr handler fix and cleanup Sean Christopherson
2021-08-23 19:37 ` [PATCH 1/3] KVM: x86: Register perf callbacks after calling vendor's hardware_setup() Sean Christopherson
2021-08-23 19:37 ` [PATCH 2/3] KVM: x86: Register Processor Trace interrupt hook iff PT enabled in guest Sean Christopherson
2021-08-24  7:28   ` Alexander Shishkin
2021-08-24 14:11     ` Sean Christopherson
2021-08-25  7:24   ` Like Xu
2021-08-25 14:59     ` Sean Christopherson
2021-08-25 20:09       ` Sean Christopherson
2021-08-23 19:37 ` [PATCH 3/3] perf/x86/intel: Fold current_vcpu check into KVM's PT intr handler Sean Christopherson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.