All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7]  KVM: x86: Tracepoint improvements and fixes
@ 2020-07-18  6:38 Sean Christopherson
  2020-07-18  6:38 ` [PATCH 1/7] KVM: x86: Add RIP to the kvm_entry, i.e. VM-Enter, tracepoint Sean Christopherson
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Sean Christopherson @ 2020-07-18  6:38 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Various improvements and fixes for the kvm_entry, kvm_exit and
kvm_nested_vmexit tracepoints.

  1. Capture the guest's RIP during kvm_entry for obvious reasons.

  2. Extend kvm_exit to report the same info as kvm_nested_vmexit, and
     macrofy its definition to reuse it verbatim for nested exits.

  3. Stop passing in params to kvm_nested_vmexit, and instead use the
     same approach (and now code) as kvm_exit where the tracepoint uses a
     dedicated kvm_x86_ops hook to retrieve the info.

  4. Stop reading GUEST_RIP, EXIT_QUAL, INTR_INFO, and ERROR_CODE on
     every VM-Exit from L2 (some of this comes in #3).  This saves ~100
     cycles (150+ with retpolines) on VM-Exits from L2 that are handled
     by L0, e.g. hardware interrupts.

Sean Christopherson (7):
  KVM: x86: Add RIP to the kvm_entry, i.e. VM-Enter, tracepoint
  KVM: x86: Read guest RIP from within the kvm_nested_vmexit tracepoint
  KVM: VMX: Add a helper to test for a valid error code given an intr
    info
  KVM: x86: Add intr/vectoring info and error code to kvm_exit
    tracepoint
  KVM: x86: Add macro wrapper for defining kvm_exit tracepoint
  KVM: x86: Use common definition for kvm_nested_vmexit tracepoint
  KVM: nVMX: Read EXIT_QUAL and INTR_INFO only when needed for nested
    exit

 arch/x86/include/asm/kvm_host.h |   7 ++-
 arch/x86/kvm/svm/svm.c          |  16 ++---
 arch/x86/kvm/trace.h            | 107 +++++++++++++-------------------
 arch/x86/kvm/vmx/nested.c       |  14 ++---
 arch/x86/kvm/vmx/vmcs.h         |   7 +++
 arch/x86/kvm/vmx/vmx.c          |  18 +++++-
 arch/x86/kvm/x86.c              |   2 +-
 7 files changed, 86 insertions(+), 85 deletions(-)

-- 
2.26.0


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

* [PATCH 1/7] KVM: x86: Add RIP to the kvm_entry, i.e. VM-Enter, tracepoint
  2020-07-18  6:38 [PATCH 0/7] KVM: x86: Tracepoint improvements and fixes Sean Christopherson
@ 2020-07-18  6:38 ` Sean Christopherson
  2020-07-18  6:38 ` [PATCH 2/7] KVM: x86: Read guest RIP from within the kvm_nested_vmexit tracepoint Sean Christopherson
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2020-07-18  6:38 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Add RIP to the kvm_entry tracepoint to help debug if the kvm_exit
tracepoint is disable or if VM-Enter fails, in which case the kvm_exit
tracepoint won't be hit.

Read RIP from within the tracepoint itself to avoid a potential VMREAD
and retpoline if the guest's RIP isn't available.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/trace.h | 10 ++++++----
 arch/x86/kvm/x86.c   |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index b66432b015d2e..9899ff0fa2534 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -15,18 +15,20 @@
  * Tracepoint for guest mode entry.
  */
 TRACE_EVENT(kvm_entry,
-	TP_PROTO(unsigned int vcpu_id),
-	TP_ARGS(vcpu_id),
+	TP_PROTO(struct kvm_vcpu *vcpu),
+	TP_ARGS(vcpu),
 
 	TP_STRUCT__entry(
 		__field(	unsigned int,	vcpu_id		)
+		__field(	unsigned long,	rip		)
 	),
 
 	TP_fast_assign(
-		__entry->vcpu_id	= vcpu_id;
+		__entry->vcpu_id        = vcpu->vcpu_id;
+		__entry->rip		= kvm_rip_read(vcpu);
 	),
 
-	TP_printk("vcpu %u", __entry->vcpu_id)
+	TP_printk("vcpu %u, rip 0x%lx", __entry->vcpu_id, __entry->rip)
 );
 
 /*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5f526d94c33f3..3563359316d64 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8547,7 +8547,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		kvm_x86_ops.request_immediate_exit(vcpu);
 	}
 
-	trace_kvm_entry(vcpu->vcpu_id);
+	trace_kvm_entry(vcpu);
 
 	fpregs_assert_state_consistent();
 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
-- 
2.26.0


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

* [PATCH 2/7] KVM: x86: Read guest RIP from within the kvm_nested_vmexit tracepoint
  2020-07-18  6:38 [PATCH 0/7] KVM: x86: Tracepoint improvements and fixes Sean Christopherson
  2020-07-18  6:38 ` [PATCH 1/7] KVM: x86: Add RIP to the kvm_entry, i.e. VM-Enter, tracepoint Sean Christopherson
@ 2020-07-18  6:38 ` Sean Christopherson
  2020-07-18  6:38 ` [PATCH 3/7] KVM: VMX: Add a helper to test for a valid error code given an intr info Sean Christopherson
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2020-07-18  6:38 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Use kvm_rip_read() to read the guest's RIP for the nested VM-Exit
tracepoint instead of having the caller pass in the tracepoint.  Params
that are passed into a tracepoint are evaluated even if the tracepoint
is disabled, i.e. passing in RIP for VMX incurs a VMREAD and retpoline
to retrieve a value that may never be used, e.g. if the exit is due to a
hardware interrupt.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/svm/svm.c    | 2 +-
 arch/x86/kvm/trace.h      | 6 +++---
 arch/x86/kvm/vmx/nested.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 783330d0e7b88..1fea39ff33077 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2943,7 +2943,7 @@ static int handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	if (is_guest_mode(vcpu)) {
 		int vmexit;
 
-		trace_kvm_nested_vmexit(svm->vmcb->save.rip, exit_code,
+		trace_kvm_nested_vmexit(vcpu, exit_code,
 					svm->vmcb->control.exit_info_1,
 					svm->vmcb->control.exit_info_2,
 					svm->vmcb->control.exit_int_info,
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 9899ff0fa2534..00e567378ae1f 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -571,10 +571,10 @@ TRACE_EVENT(kvm_nested_intercepts,
  * Tracepoint for #VMEXIT while nested
  */
 TRACE_EVENT(kvm_nested_vmexit,
-	    TP_PROTO(__u64 rip, __u32 exit_code,
+	    TP_PROTO(struct kvm_vcpu *vcpu, __u32 exit_code,
 		     __u64 exit_info1, __u64 exit_info2,
 		     __u32 exit_int_info, __u32 exit_int_info_err, __u32 isa),
-	    TP_ARGS(rip, exit_code, exit_info1, exit_info2,
+	    TP_ARGS(vcpu, exit_code, exit_info1, exit_info2,
 		    exit_int_info, exit_int_info_err, isa),
 
 	TP_STRUCT__entry(
@@ -588,7 +588,7 @@ TRACE_EVENT(kvm_nested_vmexit,
 	),
 
 	TP_fast_assign(
-		__entry->rip			= rip;
+		__entry->rip			= kvm_rip_read(vcpu);
 		__entry->exit_code		= exit_code;
 		__entry->exit_info1		= exit_info1;
 		__entry->exit_info2		= exit_info2;
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 4d561edf6f9ca..6f81097cbc794 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5912,7 +5912,7 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
 	exit_intr_info = vmx_get_intr_info(vcpu);
 	exit_qual = vmx_get_exit_qual(vcpu);
 
-	trace_kvm_nested_vmexit(kvm_rip_read(vcpu), exit_reason, exit_qual,
+	trace_kvm_nested_vmexit(vcpu, exit_reason, exit_qual,
 				vmx->idt_vectoring_info, exit_intr_info,
 				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
 				KVM_ISA_VMX);
-- 
2.26.0


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

* [PATCH 3/7] KVM: VMX: Add a helper to test for a valid error code given an intr info
  2020-07-18  6:38 [PATCH 0/7] KVM: x86: Tracepoint improvements and fixes Sean Christopherson
  2020-07-18  6:38 ` [PATCH 1/7] KVM: x86: Add RIP to the kvm_entry, i.e. VM-Enter, tracepoint Sean Christopherson
  2020-07-18  6:38 ` [PATCH 2/7] KVM: x86: Read guest RIP from within the kvm_nested_vmexit tracepoint Sean Christopherson
@ 2020-07-18  6:38 ` Sean Christopherson
  2020-07-18  6:38 ` [PATCH 4/7] KVM: x86: Add intr/vectoring info and error code to kvm_exit tracepoint Sean Christopherson
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2020-07-18  6:38 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Add a helper, is_exception_with_error_code(), to provide the simple but
difficult to read code of checking for a valid exception with an error
code given a vmcs.VM_EXIT_INTR_INFO value.  The helper will gain another
user, vmx_get_exit_info(), in a future patch.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 4 +---
 arch/x86/kvm/vmx/vmcs.h   | 7 +++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 6f81097cbc794..fc70644b916ca 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5931,9 +5931,7 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
 	 * need to be synthesized by querying the in-kernel LAPIC, but external
 	 * interrupts are never reflected to L1 so it's a non-issue.
 	 */
-	if ((exit_intr_info &
-	     (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) ==
-	    (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) {
+	if (is_exception_with_error_code(exit_intr_info)) {
 		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
 
 		vmcs12->vm_exit_intr_error_code =
diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
index 7a3675fddec20..1472c6c376f74 100644
--- a/arch/x86/kvm/vmx/vmcs.h
+++ b/arch/x86/kvm/vmx/vmcs.h
@@ -138,6 +138,13 @@ static inline bool is_external_intr(u32 intr_info)
 	return is_intr_type(intr_info, INTR_TYPE_EXT_INTR);
 }
 
+static inline bool is_exception_with_error_code(u32 intr_info)
+{
+	const u32 mask = INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK;
+
+	return (intr_info & mask) == mask;
+}
+
 enum vmcs_field_width {
 	VMCS_FIELD_WIDTH_U16 = 0,
 	VMCS_FIELD_WIDTH_U64 = 1,
-- 
2.26.0


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

* [PATCH 4/7] KVM: x86: Add intr/vectoring info and error code to kvm_exit tracepoint
  2020-07-18  6:38 [PATCH 0/7] KVM: x86: Tracepoint improvements and fixes Sean Christopherson
                   ` (2 preceding siblings ...)
  2020-07-18  6:38 ` [PATCH 3/7] KVM: VMX: Add a helper to test for a valid error code given an intr info Sean Christopherson
@ 2020-07-18  6:38 ` Sean Christopherson
  2020-07-18  6:38 ` [PATCH 5/7] KVM: x86: Add macro wrapper for defining " Sean Christopherson
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2020-07-18  6:38 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Extend the kvm_exit tracepoint to align it with kvm_nested_vmexit in
terms of what information is captured.  On SVM, this means adding
interrupt info and error code, and on VMX it means adding ITD vectoring
and error code.  This sets the stage for macrofying the kvm_exit
tracepoint definition so that it can be reused for kvm_nested_vmexit
without loss of information (and the nested version is more helpful).

Opportunistically stuff a zero for VM_EXIT_INTR_INFO if the VM-Enter
failed, as the field is guaranteed to be invalid.  Note, it'd be
possible to further filter the interrupt/exception fields based on the
VM-Exit reason, but the helper is intended only for tracepoints, i.e.
an extra VMREAD or two is a non-issue, the failed VM-Enter case is just
low hanging fruit.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_host.h |  7 ++++++-
 arch/x86/kvm/svm/svm.c          |  9 ++++++++-
 arch/x86/kvm/trace.h            | 12 +++++++++---
 arch/x86/kvm/vmx/vmx.c          | 18 ++++++++++++++++--
 4 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 1bab87a444d78..d8a24252e20e1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1143,7 +1143,12 @@ struct kvm_x86_ops {
 	/* Returns actual tsc_offset set in active VMCS */
 	u64 (*write_l1_tsc_offset)(struct kvm_vcpu *vcpu, u64 offset);
 
-	void (*get_exit_info)(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2);
+	/*
+	 * Retrieve somewhat arbitrary exit information.  Intended to be used
+	 * only from within tracepoints to avoid VMREADs when tracing is off.
+	 */
+	void (*get_exit_info)(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2,
+			      u32 *exit_int_info, u32 *exit_int_info_err_code);
 
 	int (*check_intercept)(struct kvm_vcpu *vcpu,
 			       struct x86_instruction_info *info,
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 1fea39ff33077..8ab3413094500 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2917,12 +2917,19 @@ static void dump_vmcb(struct kvm_vcpu *vcpu)
 	       "excp_to:", save->last_excp_to);
 }
 
-static void svm_get_exit_info(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2)
+static void svm_get_exit_info(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2,
+			      u32 *intr_info, u32 *error_code)
 {
 	struct vmcb_control_area *control = &to_svm(vcpu)->vmcb->control;
 
 	*info1 = control->exit_info_1;
 	*info2 = control->exit_info_2;
+	*intr_info = control->exit_int_info;
+	if ((*intr_info & SVM_EXITINTINFO_VALID) &&
+	    (*intr_info & SVM_EXITINTINFO_VALID_ERR))
+		*error_code = control->exit_int_info_err;
+	else
+		*error_code = 0;
 }
 
 static int handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 00e567378ae1f..4192b72c72fd8 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -248,6 +248,8 @@ TRACE_EVENT(kvm_exit,
 		__field(	u32,	        isa             )
 		__field(	u64,	        info1           )
 		__field(	u64,	        info2           )
+		__field(	u32,	        intr_info	)
+		__field(	u32,	        error_code	)
 		__field(	unsigned int,	vcpu_id         )
 	),
 
@@ -257,13 +259,17 @@ TRACE_EVENT(kvm_exit,
 		__entry->isa            = isa;
 		__entry->vcpu_id        = vcpu->vcpu_id;
 		kvm_x86_ops.get_exit_info(vcpu, &__entry->info1,
-					   &__entry->info2);
+					  &__entry->info2,
+					  &__entry->intr_info,
+					  &__entry->error_code);
 	),
 
-	TP_printk("vcpu %u reason %s%s%s rip 0x%lx info %llx %llx",
+	TP_printk("vcpu %u reason %s%s%s rip 0x%lx info1 0x%016llx "
+		  "info2 0x%016llx intr_info 0x%08x error_code 0x%08x",
 		  __entry->vcpu_id,
 		  kvm_print_exit_reason(__entry->exit_reason, __entry->isa),
-		  __entry->guest_rip, __entry->info1, __entry->info2)
+		  __entry->guest_rip, __entry->info1, __entry->info2,
+		  __entry->intr_info, __entry->error_code)
 );
 
 /*
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1bb59ae5016dc..c1448f42a9522 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5751,10 +5751,24 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 static const int kvm_vmx_max_exit_handlers =
 	ARRAY_SIZE(kvm_vmx_exit_handlers);
 
-static void vmx_get_exit_info(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2)
+static void vmx_get_exit_info(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2,
+			      u32 *intr_info, u32 *error_code)
 {
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
 	*info1 = vmx_get_exit_qual(vcpu);
-	*info2 = vmx_get_intr_info(vcpu);
+	if (!(vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)) {
+		*info2 = vmx->idt_vectoring_info;
+		*intr_info = vmx_get_intr_info(vcpu);
+		if (is_exception_with_error_code(*intr_info))
+			*error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
+		else
+			*error_code = 0;
+	} else {
+		*info2 = 0;
+		*intr_info = 0;
+		*error_code = 0;
+	}
 }
 
 static void vmx_destroy_pml_buffer(struct vcpu_vmx *vmx)
-- 
2.26.0


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

* [PATCH 5/7] KVM: x86: Add macro wrapper for defining kvm_exit tracepoint
  2020-07-18  6:38 [PATCH 0/7] KVM: x86: Tracepoint improvements and fixes Sean Christopherson
                   ` (3 preceding siblings ...)
  2020-07-18  6:38 ` [PATCH 4/7] KVM: x86: Add intr/vectoring info and error code to kvm_exit tracepoint Sean Christopherson
@ 2020-07-18  6:38 ` Sean Christopherson
  2020-07-18  6:38 ` [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint Sean Christopherson
  2020-07-18  6:38 ` [PATCH 7/7] KVM: nVMX: Read EXIT_QUAL and INTR_INFO only when needed for nested exit Sean Christopherson
  6 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2020-07-18  6:38 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Macrofy the definition of kvm_exit so that the definition can be reused
verbatim by kvm_nested_vmexit.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/trace.h | 69 +++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 4192b72c72fd8..6cb75ba494fcd 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -235,42 +235,45 @@ TRACE_EVENT(kvm_apic,
 	(isa == KVM_ISA_VMX) ?						\
 	__print_flags(exit_reason & ~0xffff, " ", VMX_EXIT_REASON_FLAGS) : ""
 
+#define TRACE_EVENT_KVM_EXIT(name)					     \
+TRACE_EVENT(name,							     \
+	TP_PROTO(unsigned int exit_reason, struct kvm_vcpu *vcpu, u32 isa),  \
+	TP_ARGS(exit_reason, vcpu, isa),				     \
+									     \
+	TP_STRUCT__entry(						     \
+		__field(	unsigned int,	exit_reason	)	     \
+		__field(	unsigned long,	guest_rip	)	     \
+		__field(	u32,	        isa             )	     \
+		__field(	u64,	        info1           )	     \
+		__field(	u64,	        info2           )	     \
+		__field(	u32,	        intr_info	)	     \
+		__field(	u32,	        error_code	)	     \
+		__field(	unsigned int,	vcpu_id         )	     \
+	),								     \
+									     \
+	TP_fast_assign(							     \
+		__entry->exit_reason	= exit_reason;			     \
+		__entry->guest_rip	= kvm_rip_read(vcpu);		     \
+		__entry->isa            = isa;				     \
+		__entry->vcpu_id        = vcpu->vcpu_id;		     \
+		kvm_x86_ops.get_exit_info(vcpu, &__entry->info1,	     \
+					  &__entry->info2,		     \
+					  &__entry->intr_info,		     \
+					  &__entry->error_code);	     \
+	),								     \
+									     \
+	TP_printk("vcpu %u reason %s%s%s rip 0x%lx info1 0x%016llx "	     \
+		  "info2 0x%016llx intr_info 0x%08x error_code 0x%08x",	     \
+		  __entry->vcpu_id,					     \
+		  kvm_print_exit_reason(__entry->exit_reason, __entry->isa), \
+		  __entry->guest_rip, __entry->info1, __entry->info2,	     \
+		  __entry->intr_info, __entry->error_code)		     \
+)
+
 /*
  * Tracepoint for kvm guest exit:
  */
-TRACE_EVENT(kvm_exit,
-	TP_PROTO(unsigned int exit_reason, struct kvm_vcpu *vcpu, u32 isa),
-	TP_ARGS(exit_reason, vcpu, isa),
-
-	TP_STRUCT__entry(
-		__field(	unsigned int,	exit_reason	)
-		__field(	unsigned long,	guest_rip	)
-		__field(	u32,	        isa             )
-		__field(	u64,	        info1           )
-		__field(	u64,	        info2           )
-		__field(	u32,	        intr_info	)
-		__field(	u32,	        error_code	)
-		__field(	unsigned int,	vcpu_id         )
-	),
-
-	TP_fast_assign(
-		__entry->exit_reason	= exit_reason;
-		__entry->guest_rip	= kvm_rip_read(vcpu);
-		__entry->isa            = isa;
-		__entry->vcpu_id        = vcpu->vcpu_id;
-		kvm_x86_ops.get_exit_info(vcpu, &__entry->info1,
-					  &__entry->info2,
-					  &__entry->intr_info,
-					  &__entry->error_code);
-	),
-
-	TP_printk("vcpu %u reason %s%s%s rip 0x%lx info1 0x%016llx "
-		  "info2 0x%016llx intr_info 0x%08x error_code 0x%08x",
-		  __entry->vcpu_id,
-		  kvm_print_exit_reason(__entry->exit_reason, __entry->isa),
-		  __entry->guest_rip, __entry->info1, __entry->info2,
-		  __entry->intr_info, __entry->error_code)
-);
+TRACE_EVENT_KVM_EXIT(kvm_exit);
 
 /*
  * Tracepoint for kvm interrupt injection:
-- 
2.26.0


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

* [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint
  2020-07-18  6:38 [PATCH 0/7] KVM: x86: Tracepoint improvements and fixes Sean Christopherson
                   ` (4 preceding siblings ...)
  2020-07-18  6:38 ` [PATCH 5/7] KVM: x86: Add macro wrapper for defining " Sean Christopherson
@ 2020-07-18  6:38 ` Sean Christopherson
  2020-07-20 16:52   ` Vitaly Kuznetsov
  2020-07-18  6:38 ` [PATCH 7/7] KVM: nVMX: Read EXIT_QUAL and INTR_INFO only when needed for nested exit Sean Christopherson
  6 siblings, 1 reply; 14+ messages in thread
From: Sean Christopherson @ 2020-07-18  6:38 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Use the newly introduced TRACE_EVENT_KVM_EXIT to define the guts of
kvm_nested_vmexit so that it captures and prints the same information as
with kvm_exit.  This has the bonus side effect of fixing the interrupt
info and error code printing for the case where they're invalid, e.g. if
the exit was a failed VM-Entry.  This also sets the stage for retrieving
EXIT_QUALIFICATION and VM_EXIT_INTR_INFO in nested_vmx_reflect_vmexit()
if and only if the VM-Exit is being routed to L1.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/svm/svm.c    |  7 +------
 arch/x86/kvm/trace.h      | 34 +---------------------------------
 arch/x86/kvm/vmx/nested.c |  5 +----
 3 files changed, 3 insertions(+), 43 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8ab3413094500..133581c5b0dc0 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2950,12 +2950,7 @@ static int handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	if (is_guest_mode(vcpu)) {
 		int vmexit;
 
-		trace_kvm_nested_vmexit(vcpu, exit_code,
-					svm->vmcb->control.exit_info_1,
-					svm->vmcb->control.exit_info_2,
-					svm->vmcb->control.exit_int_info,
-					svm->vmcb->control.exit_int_info_err,
-					KVM_ISA_SVM);
+		trace_kvm_nested_vmexit(exit_code, vcpu, KVM_ISA_SVM);
 
 		vmexit = nested_svm_exit_special(svm);
 
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 6cb75ba494fcd..e29576985e03a 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -579,39 +579,7 @@ TRACE_EVENT(kvm_nested_intercepts,
 /*
  * Tracepoint for #VMEXIT while nested
  */
-TRACE_EVENT(kvm_nested_vmexit,
-	    TP_PROTO(struct kvm_vcpu *vcpu, __u32 exit_code,
-		     __u64 exit_info1, __u64 exit_info2,
-		     __u32 exit_int_info, __u32 exit_int_info_err, __u32 isa),
-	    TP_ARGS(vcpu, exit_code, exit_info1, exit_info2,
-		    exit_int_info, exit_int_info_err, isa),
-
-	TP_STRUCT__entry(
-		__field(	__u64,		rip			)
-		__field(	__u32,		exit_code		)
-		__field(	__u64,		exit_info1		)
-		__field(	__u64,		exit_info2		)
-		__field(	__u32,		exit_int_info		)
-		__field(	__u32,		exit_int_info_err	)
-		__field(	__u32,		isa			)
-	),
-
-	TP_fast_assign(
-		__entry->rip			= kvm_rip_read(vcpu);
-		__entry->exit_code		= exit_code;
-		__entry->exit_info1		= exit_info1;
-		__entry->exit_info2		= exit_info2;
-		__entry->exit_int_info		= exit_int_info;
-		__entry->exit_int_info_err	= exit_int_info_err;
-		__entry->isa			= isa;
-	),
-	TP_printk("rip: 0x%016llx reason: %s%s%s ext_inf1: 0x%016llx "
-		  "ext_inf2: 0x%016llx ext_int: 0x%08x ext_int_err: 0x%08x",
-		  __entry->rip,
-		  kvm_print_exit_reason(__entry->exit_code, __entry->isa),
-		  __entry->exit_info1, __entry->exit_info2,
-		  __entry->exit_int_info, __entry->exit_int_info_err)
-);
+TRACE_EVENT_KVM_EXIT(kvm_nested_vmexit);
 
 /*
  * Tracepoint for #VMEXIT reinjected to the guest
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index fc70644b916ca..f437d99f4db09 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5912,10 +5912,7 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
 	exit_intr_info = vmx_get_intr_info(vcpu);
 	exit_qual = vmx_get_exit_qual(vcpu);
 
-	trace_kvm_nested_vmexit(vcpu, exit_reason, exit_qual,
-				vmx->idt_vectoring_info, exit_intr_info,
-				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
-				KVM_ISA_VMX);
+	trace_kvm_nested_vmexit(exit_reason, vcpu, KVM_ISA_VMX);
 
 	/* If L0 (KVM) wants the exit, it trumps L1's desires. */
 	if (nested_vmx_l0_wants_exit(vcpu, exit_reason))
-- 
2.26.0


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

* [PATCH 7/7] KVM: nVMX: Read EXIT_QUAL and INTR_INFO only when needed for nested exit
  2020-07-18  6:38 [PATCH 0/7] KVM: x86: Tracepoint improvements and fixes Sean Christopherson
                   ` (5 preceding siblings ...)
  2020-07-18  6:38 ` [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint Sean Christopherson
@ 2020-07-18  6:38 ` Sean Christopherson
  6 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2020-07-18  6:38 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Read vmcs.EXIT_QUALIFICATION and vmcs.VM_EXIT_INTR_INFO only when the
VM-Exit is being reflected to L1 now that they are no longer passed
directly to the kvm_nested_vmexit tracepoint.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f437d99f4db09..cc5f0085989c7 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5909,9 +5909,6 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
 		goto reflect_vmexit;
 	}
 
-	exit_intr_info = vmx_get_intr_info(vcpu);
-	exit_qual = vmx_get_exit_qual(vcpu);
-
 	trace_kvm_nested_vmexit(exit_reason, vcpu, KVM_ISA_VMX);
 
 	/* If L0 (KVM) wants the exit, it trumps L1's desires. */
@@ -5928,12 +5925,14 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
 	 * need to be synthesized by querying the in-kernel LAPIC, but external
 	 * interrupts are never reflected to L1 so it's a non-issue.
 	 */
+	exit_intr_info = vmx_get_intr_info(vcpu);
 	if (is_exception_with_error_code(exit_intr_info)) {
 		struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
 
 		vmcs12->vm_exit_intr_error_code =
 			vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
 	}
+	exit_qual = vmx_get_exit_qual(vcpu);
 
 reflect_vmexit:
 	nested_vmx_vmexit(vcpu, exit_reason, exit_intr_info, exit_qual);
-- 
2.26.0


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

* Re: [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint
  2020-07-18  6:38 ` [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint Sean Christopherson
@ 2020-07-20 16:52   ` Vitaly Kuznetsov
  2020-07-21  0:27     ` Sean Christopherson
  0 siblings, 1 reply; 14+ messages in thread
From: Vitaly Kuznetsov @ 2020-07-20 16:52 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Paolo Bonzini

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Use the newly introduced TRACE_EVENT_KVM_EXIT to define the guts of
> kvm_nested_vmexit so that it captures and prints the same information as
> with kvm_exit.  This has the bonus side effect of fixing the interrupt
> info and error code printing for the case where they're invalid, e.g. if
> the exit was a failed VM-Entry.  This also sets the stage for retrieving
> EXIT_QUALIFICATION and VM_EXIT_INTR_INFO in nested_vmx_reflect_vmexit()
> if and only if the VM-Exit is being routed to L1.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/svm/svm.c    |  7 +------
>  arch/x86/kvm/trace.h      | 34 +---------------------------------
>  arch/x86/kvm/vmx/nested.c |  5 +----
>  3 files changed, 3 insertions(+), 43 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 8ab3413094500..133581c5b0dc0 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2950,12 +2950,7 @@ static int handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  	if (is_guest_mode(vcpu)) {
>  		int vmexit;
>  
> -		trace_kvm_nested_vmexit(vcpu, exit_code,
> -					svm->vmcb->control.exit_info_1,
> -					svm->vmcb->control.exit_info_2,
> -					svm->vmcb->control.exit_int_info,
> -					svm->vmcb->control.exit_int_info_err,
> -					KVM_ISA_SVM);
> +		trace_kvm_nested_vmexit(exit_code, vcpu, KVM_ISA_SVM);
>  
>  		vmexit = nested_svm_exit_special(svm);
>  
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index 6cb75ba494fcd..e29576985e03a 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -579,39 +579,7 @@ TRACE_EVENT(kvm_nested_intercepts,
>  /*
>   * Tracepoint for #VMEXIT while nested
>   */
> -TRACE_EVENT(kvm_nested_vmexit,
> -	    TP_PROTO(struct kvm_vcpu *vcpu, __u32 exit_code,
> -		     __u64 exit_info1, __u64 exit_info2,
> -		     __u32 exit_int_info, __u32 exit_int_info_err, __u32 isa),
> -	    TP_ARGS(vcpu, exit_code, exit_info1, exit_info2,
> -		    exit_int_info, exit_int_info_err, isa),
> -
> -	TP_STRUCT__entry(
> -		__field(	__u64,		rip			)
> -		__field(	__u32,		exit_code		)
> -		__field(	__u64,		exit_info1		)
> -		__field(	__u64,		exit_info2		)
> -		__field(	__u32,		exit_int_info		)
> -		__field(	__u32,		exit_int_info_err	)
> -		__field(	__u32,		isa			)
> -	),
> -
> -	TP_fast_assign(
> -		__entry->rip			= kvm_rip_read(vcpu);
> -		__entry->exit_code		= exit_code;
> -		__entry->exit_info1		= exit_info1;
> -		__entry->exit_info2		= exit_info2;
> -		__entry->exit_int_info		= exit_int_info;
> -		__entry->exit_int_info_err	= exit_int_info_err;
> -		__entry->isa			= isa;
> -	),
> -	TP_printk("rip: 0x%016llx reason: %s%s%s ext_inf1: 0x%016llx "
> -		  "ext_inf2: 0x%016llx ext_int: 0x%08x ext_int_err: 0x%08x",
> -		  __entry->rip,
> -		  kvm_print_exit_reason(__entry->exit_code, __entry->isa),
> -		  __entry->exit_info1, __entry->exit_info2,
> -		  __entry->exit_int_info, __entry->exit_int_info_err)
> -);
> +TRACE_EVENT_KVM_EXIT(kvm_nested_vmexit);
>  
>  /*
>   * Tracepoint for #VMEXIT reinjected to the guest
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index fc70644b916ca..f437d99f4db09 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5912,10 +5912,7 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
>  	exit_intr_info = vmx_get_intr_info(vcpu);
>  	exit_qual = vmx_get_exit_qual(vcpu);
>  
> -	trace_kvm_nested_vmexit(vcpu, exit_reason, exit_qual,
> -				vmx->idt_vectoring_info, exit_intr_info,
> -				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
> -				KVM_ISA_VMX);
> +	trace_kvm_nested_vmexit(exit_reason, vcpu, KVM_ISA_VMX);
>  
>  	/* If L0 (KVM) wants the exit, it trumps L1's desires. */
>  	if (nested_vmx_l0_wants_exit(vcpu, exit_reason))

With so many lines removed I'm almost in love with the patch! However,
when testing on SVM (unrelated?) my trace log looks a bit ugly:

           <...>-315119 [010]  3733.092646: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000200000006 info2 0x0000000000641000 intr_info 0x00000000 error_code 0x00000000
           <...>-315119 [010]  3733.092655: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000100000014 info2 0x0000000000400000 intr_info 0x00000000 error_code 0x00000000

...

but after staring at this for some time I still don't see where this
comes from :-( ... but reverting this commit helps:

 qemu-system-x86-9928  [022]   379.260656: kvm_nested_vmexit:    rip 400433 reason EXIT_NPF info1 200000006 info2 641000 int_info 0 int_info_err 0
 qemu-system-x86-9928  [022]   379.260666: kvm_nested_vmexit:    rip 400433 reason EXIT_NPF info1 100000014 info2 400000 int_info 0 int_info_err 0

-- 
Vitaly


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

* Re: [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint
  2020-07-20 16:52   ` Vitaly Kuznetsov
@ 2020-07-21  0:27     ` Sean Christopherson
  2020-07-21 13:59       ` Vitaly Kuznetsov
  0 siblings, 1 reply; 14+ messages in thread
From: Sean Christopherson @ 2020-07-21  0:27 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Paolo Bonzini

On Mon, Jul 20, 2020 at 06:52:15PM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> > +TRACE_EVENT_KVM_EXIT(kvm_nested_vmexit);
> >  
> >  /*
> >   * Tracepoint for #VMEXIT reinjected to the guest
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index fc70644b916ca..f437d99f4db09 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -5912,10 +5912,7 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
> >  	exit_intr_info = vmx_get_intr_info(vcpu);
> >  	exit_qual = vmx_get_exit_qual(vcpu);
> >  
> > -	trace_kvm_nested_vmexit(vcpu, exit_reason, exit_qual,
> > -				vmx->idt_vectoring_info, exit_intr_info,
> > -				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
> > -				KVM_ISA_VMX);
> > +	trace_kvm_nested_vmexit(exit_reason, vcpu, KVM_ISA_VMX);
> >  
> >  	/* If L0 (KVM) wants the exit, it trumps L1's desires. */
> >  	if (nested_vmx_l0_wants_exit(vcpu, exit_reason))
> 
> With so many lines removed I'm almost in love with the patch! However,
> when testing on SVM (unrelated?) my trace log looks a bit ugly:
> 
>            <...>-315119 [010]  3733.092646: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000200000006 info2 0x0000000000641000 intr_info 0x00000000 error_code 0x00000000
>            <...>-315119 [010]  3733.092655: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000100000014 info2 0x0000000000400000 intr_info 0x00000000 error_code 0x00000000
> 
> ...
> 
> but after staring at this for some time I still don't see where this
> comes from :-( ... but reverting this commit helps:

The CAN'T FIND FIELD blurb comes from tools/lib/traceevent/event-parse.c.

I assume you are using tooling of some form to generate the trace, i.e. the
issue doesn't show up in /sys/kernel/debug/tracing/trace.  If that's the
case, this is more or less ABI breakage :-(
 
>  qemu-system-x86-9928  [022]   379.260656: kvm_nested_vmexit:    rip 400433 reason EXIT_NPF info1 200000006 info2 641000 int_info 0 int_info_err 0
>  qemu-system-x86-9928  [022]   379.260666: kvm_nested_vmexit:    rip 400433 reason EXIT_NPF info1 100000014 info2 400000 int_info 0 int_info_err 0
> 
> -- 
> Vitaly
> 

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

* Re: [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint
  2020-07-21  0:27     ` Sean Christopherson
@ 2020-07-21 13:59       ` Vitaly Kuznetsov
  2020-07-21 19:31         ` Sean Christopherson
  2020-08-12 18:10         ` Sean Christopherson
  0 siblings, 2 replies; 14+ messages in thread
From: Vitaly Kuznetsov @ 2020-07-21 13:59 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Paolo Bonzini

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> On Mon, Jul 20, 2020 at 06:52:15PM +0200, Vitaly Kuznetsov wrote:
>> Sean Christopherson <sean.j.christopherson@intel.com> writes:
>> > +TRACE_EVENT_KVM_EXIT(kvm_nested_vmexit);
>> >  
>> >  /*
>> >   * Tracepoint for #VMEXIT reinjected to the guest
>> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> > index fc70644b916ca..f437d99f4db09 100644
>> > --- a/arch/x86/kvm/vmx/nested.c
>> > +++ b/arch/x86/kvm/vmx/nested.c
>> > @@ -5912,10 +5912,7 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
>> >  	exit_intr_info = vmx_get_intr_info(vcpu);
>> >  	exit_qual = vmx_get_exit_qual(vcpu);
>> >  
>> > -	trace_kvm_nested_vmexit(vcpu, exit_reason, exit_qual,
>> > -				vmx->idt_vectoring_info, exit_intr_info,
>> > -				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
>> > -				KVM_ISA_VMX);
>> > +	trace_kvm_nested_vmexit(exit_reason, vcpu, KVM_ISA_VMX);
>> >  
>> >  	/* If L0 (KVM) wants the exit, it trumps L1's desires. */
>> >  	if (nested_vmx_l0_wants_exit(vcpu, exit_reason))
>> 
>> With so many lines removed I'm almost in love with the patch! However,
>> when testing on SVM (unrelated?) my trace log looks a bit ugly:
>> 
>>            <...>-315119 [010]  3733.092646: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000200000006 info2 0x0000000000641000 intr_info 0x00000000 error_code 0x00000000
>>            <...>-315119 [010]  3733.092655: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000100000014 info2 0x0000000000400000 intr_info 0x00000000 error_code 0x00000000
>> 
>> ...
>> 
>> but after staring at this for some time I still don't see where this
>> comes from :-( ... but reverting this commit helps:
>
> The CAN'T FIND FIELD blurb comes from tools/lib/traceevent/event-parse.c.
>
> I assume you are using tooling of some form to generate the trace, i.e. the
> issue doesn't show up in /sys/kernel/debug/tracing/trace.  If that's the
> case, this is more or less ABI breakage :-(
>  

Right you are,

the tool is called 'trace-cmd record -e kvm ...' / 'trace-cmd report'
but I always thought it's not any different from looking at
/sys/kernel/debug/tracing/trace directly. Apparently I was wrong. 'cat
/sys/kernel/debug/tracing/trace' seems to be OK, e.g.:

 qemu-system-x86-20263 [006] .... 75982.292657: kvm_nested_vmexit: vcpu 0 reason hypercall rip 0x40122f info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000

-- 
Vitaly


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

* Re: [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint
  2020-07-21 13:59       ` Vitaly Kuznetsov
@ 2020-07-21 19:31         ` Sean Christopherson
  2020-07-21 21:42           ` Steven Rostedt
  2020-08-12 18:10         ` Sean Christopherson
  1 sibling, 1 reply; 14+ messages in thread
From: Sean Christopherson @ 2020-07-21 19:31 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Steven Rostedt
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Paolo Bonzini

+Steve

Background: KVM has two tracepoints that effectively trace the same thing
(VM-Exit vs. nested VM-Exit), but use completely different formatting and
nomenclature for each of the existing tracepoints.  I want to add a common
macro to create the tracepoints so that they capture the exact same info
and report it with the exact same format.  But that means breaking the
"ABI" for one of the tracepoints, e.g. trace-cmd barfs on the rename of
exit_code to exit_reason.

Was there ever a verdict on whether or not tracepoints are considered ABI
and thus must retain backwards compatibility?

If not, what's the proper way to upstream changes to trace-cmd?

Thanks!

On Tue, Jul 21, 2020 at 03:59:06PM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> >> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> >> With so many lines removed I'm almost in love with the patch! However,
> >> when testing on SVM (unrelated?) my trace log looks a bit ugly:
> >> 
> >>            <...>-315119 [010]  3733.092646: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000200000006 info2 0x0000000000641000 intr_info 0x00000000 error_code 0x00000000
> >>            <...>-315119 [010]  3733.092655: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000100000014 info2 0x0000000000400000 intr_info 0x00000000 error_code 0x00000000
> >> 
> >> ...
> >> 
> >> but after staring at this for some time I still don't see where this
> >> comes from :-( ... but reverting this commit helps:
> >
> > The CAN'T FIND FIELD blurb comes from tools/lib/traceevent/event-parse.c.
> >
> > I assume you are using tooling of some form to generate the trace, i.e. the
> > issue doesn't show up in /sys/kernel/debug/tracing/trace.  If that's the
> > case, this is more or less ABI breakage :-(
> >  
> 
> Right you are,
> 
> the tool is called 'trace-cmd record -e kvm ...' / 'trace-cmd report'
> but I always thought it's not any different from looking at
> /sys/kernel/debug/tracing/trace directly. Apparently I was wrong. 'cat
> /sys/kernel/debug/tracing/trace' seems to be OK, e.g.:
> 
>  qemu-system-x86-20263 [006] .... 75982.292657: kvm_nested_vmexit: vcpu 0 reason hypercall rip 0x40122f info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
> 
> -- 
> Vitaly
> 

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

* Re: [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint
  2020-07-21 19:31         ` Sean Christopherson
@ 2020-07-21 21:42           ` Steven Rostedt
  0 siblings, 0 replies; 14+ messages in thread
From: Steven Rostedt @ 2020-07-21 21:42 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel, Paolo Bonzini

On Tue, 21 Jul 2020 12:31:30 -0700
Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> +Steve
> 
> Background: KVM has two tracepoints that effectively trace the same thing
> (VM-Exit vs. nested VM-Exit), but use completely different formatting and
> nomenclature for each of the existing tracepoints.  I want to add a common
> macro to create the tracepoints so that they capture the exact same info
> and report it with the exact same format.  But that means breaking the
> "ABI" for one of the tracepoints, e.g. trace-cmd barfs on the rename of
> exit_code to exit_reason.

Feel free to update it.

> 
> Was there ever a verdict on whether or not tracepoints are considered ABI
> and thus must retain backwards compatibility?
> 
> If not, what's the proper way to upstream changes to trace-cmd?
> 

There's a kvm plugin in the libtraceevent code.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/traceevent/plugins/plugin_kvm.c

This overrides how the events are read. In the callback handler
(e.g. kvm_nested_vmexit_handle()), you can test if "rip" is there or
not. If it is not, you can do something different. For example:

	struct tep_format_field *field;

	field = tep_find_any_field(event, "rip");
	if (field) {
		tep_print_num_field(s, "rip %llx ", event, "rip", record, 1)
		[..]
	} else {
		/* do something new */
	}

You can test if fields exist and have the plugins do different things
depending on the format of an event. This is what I do in case an event
changes in the future.

-- Steve

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

* Re: [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint
  2020-07-21 13:59       ` Vitaly Kuznetsov
  2020-07-21 19:31         ` Sean Christopherson
@ 2020-08-12 18:10         ` Sean Christopherson
  1 sibling, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2020-08-12 18:10 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Paolo Bonzini

On Tue, Jul 21, 2020 at 03:59:06PM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> > On Mon, Jul 20, 2020 at 06:52:15PM +0200, Vitaly Kuznetsov wrote:
> >> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> >> > +TRACE_EVENT_KVM_EXIT(kvm_nested_vmexit);
> >> >  
> >> >  /*
> >> >   * Tracepoint for #VMEXIT reinjected to the guest
> >> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> >> > index fc70644b916ca..f437d99f4db09 100644
> >> > --- a/arch/x86/kvm/vmx/nested.c
> >> > +++ b/arch/x86/kvm/vmx/nested.c
> >> > @@ -5912,10 +5912,7 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
> >> >  	exit_intr_info = vmx_get_intr_info(vcpu);
> >> >  	exit_qual = vmx_get_exit_qual(vcpu);
> >> >  
> >> > -	trace_kvm_nested_vmexit(vcpu, exit_reason, exit_qual,
> >> > -				vmx->idt_vectoring_info, exit_intr_info,
> >> > -				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
> >> > -				KVM_ISA_VMX);
> >> > +	trace_kvm_nested_vmexit(exit_reason, vcpu, KVM_ISA_VMX);
> >> >  
> >> >  	/* If L0 (KVM) wants the exit, it trumps L1's desires. */
> >> >  	if (nested_vmx_l0_wants_exit(vcpu, exit_reason))
> >> 
> >> With so many lines removed I'm almost in love with the patch! However,
> >> when testing on SVM (unrelated?) my trace log looks a bit ugly:
> >> 
> >>            <...>-315119 [010]  3733.092646: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000200000006 info2 0x0000000000641000 intr_info 0x00000000 error_code 0x00000000
> >>            <...>-315119 [010]  3733.092655: kvm_nested_vmexit:    CAN'T FIND FIELD "rip"<CANT FIND FIELD exit_code>vcpu 0 reason npf rip 0x400433 info1 0x0000000100000014 info2 0x0000000000400000 intr_info 0x00000000 error_code 0x00000000
> >> 
> >> ...
> >> 
> >> but after staring at this for some time I still don't see where this
> >> comes from :-( ... but reverting this commit helps:
> >
> > The CAN'T FIND FIELD blurb comes from tools/lib/traceevent/event-parse.c.
> >
> > I assume you are using tooling of some form to generate the trace, i.e. the
> > issue doesn't show up in /sys/kernel/debug/tracing/trace.  If that's the
> > case, this is more or less ABI breakage :-(
> >  
> 
> Right you are,
> 
> the tool is called 'trace-cmd record -e kvm ...' / 'trace-cmd report'

Paolo, any thoughts on how to proceed with this series?  E.g. merge KVM
first and fix trace-cmd second?  Something else?

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

end of thread, other threads:[~2020-08-12 18:10 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-18  6:38 [PATCH 0/7] KVM: x86: Tracepoint improvements and fixes Sean Christopherson
2020-07-18  6:38 ` [PATCH 1/7] KVM: x86: Add RIP to the kvm_entry, i.e. VM-Enter, tracepoint Sean Christopherson
2020-07-18  6:38 ` [PATCH 2/7] KVM: x86: Read guest RIP from within the kvm_nested_vmexit tracepoint Sean Christopherson
2020-07-18  6:38 ` [PATCH 3/7] KVM: VMX: Add a helper to test for a valid error code given an intr info Sean Christopherson
2020-07-18  6:38 ` [PATCH 4/7] KVM: x86: Add intr/vectoring info and error code to kvm_exit tracepoint Sean Christopherson
2020-07-18  6:38 ` [PATCH 5/7] KVM: x86: Add macro wrapper for defining " Sean Christopherson
2020-07-18  6:38 ` [PATCH 6/7] KVM: x86: Use common definition for kvm_nested_vmexit tracepoint Sean Christopherson
2020-07-20 16:52   ` Vitaly Kuznetsov
2020-07-21  0:27     ` Sean Christopherson
2020-07-21 13:59       ` Vitaly Kuznetsov
2020-07-21 19:31         ` Sean Christopherson
2020-07-21 21:42           ` Steven Rostedt
2020-08-12 18:10         ` Sean Christopherson
2020-07-18  6:38 ` [PATCH 7/7] KVM: nVMX: Read EXIT_QUAL and INTR_INFO only when needed for nested exit 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.