Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 Aug 2022 19:29:13 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Wonhyuk Yang <vvghjk1234@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, Baik Song An <bsahn@etri.re.kr>,
        Hong Yeon Kim <kimhy@etri.re.kr>,
        Taeung Song <taeung@reallinux.co.kr>, linuxgeek@linuxgeek.io,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: Add extra information in kvm_page_fault trace point
Message-ID: <Yw5lCY5/SOmFGQrK@google.com>
References: <20220510071001.87169-1-vvghjk1234@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220510071001.87169-1-vvghjk1234@gmail.com>
Precedence: bulk

On Tue, May 10, 2022, Wonhyuk Yang wrote:
> Currently, kvm_page_fault trace point provide fault_address and error
> code. However it is not enough to find which cpu and instruction
> cause kvm_page_faults. So add vcpu id and instruction pointer in
> kvm_page_fault trace point.
> 
> Cc: Baik Song An <bsahn@etri.re.kr>
> Cc: Hong Yeon Kim <kimhy@etri.re.kr>
> Cc: Taeung Song <taeung@reallinux.co.kr>
> Cc: linuxgeek@linuxgeek.io
> Signed-off-by: Wonhyuk Yang <vvghjk1234@gmail.com>
> ---

Patch is good, some tangentially related FYI comments below.

> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index e3a24b8f04be..78d20d392904 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -383,20 +383,26 @@ TRACE_EVENT(kvm_inj_exception,
>   * Tracepoint for page fault.
>   */
>  TRACE_EVENT(kvm_page_fault,
> -	TP_PROTO(unsigned long fault_address, unsigned int error_code),
> -	TP_ARGS(fault_address, error_code),
> +	TP_PROTO(struct kvm_vcpu *vcpu, unsigned long fault_address,
> +		 unsigned int error_code),
> +	TP_ARGS(vcpu, fault_address, error_code),
>  
>  	TP_STRUCT__entry(
> +		__field(	unsigned int,	vcpu_id		)
> +		__field(	unsigned long,	guest_rip	)
>  		__field(	unsigned long,	fault_address	)
>  		__field(	unsigned int,	error_code	)

This tracepoint is comically bad.  The address should be a u64 since GPAs can be
64 bits even on 32-bit hosts.  Ditto for error_code since #NPF has 64-bit error
codes.

>  	),
>  
>  	TP_fast_assign(
> +		__entry->vcpu_id	= vcpu->vcpu_id;
> +		__entry->guest_rip	= kvm_rip_read(vcpu);
>  		__entry->fault_address	= fault_address;
>  		__entry->error_code	= error_code;
>  	),
>  
> -	TP_printk("address %lx error_code %x",
> +	TP_printk("vcpu %u rip 0x%lx address 0x%lx error_code %x",

And here the error code needs a "0x" prefix, especially since the majority of error
codes end up being valid decimal values, e.g. 182, 184, 181.

I also think it makes sense to force "address" to pad to 16, but not the others.
Padding error_code is wasteful most of the time, and I actually like that user vs.
kernel addresses and up with different formatting as it makes it trivial to see
where the fault originated (when running "real" guests).

       CPU 5/KVM-4145    [002] .....    86.581928: kvm_page_fault: vcpu 5 rip 0x7f08a4602116 address 0x0000000113600002 error_code 0x181
       CPU 7/KVM-4150    [001] .....    86.581936: kvm_page_fault: vcpu 7 rip 0xffffffff81511f37 address 0x0000000113674000 error_code 0x182
       CPU 5/KVM-4145    [002] .....    86.582585: kvm_page_fault: vcpu 5 rip 0xffffffff81040f72 address 0x00000000fee000b0 error_code 0x182
       CPU 1/KVM-4136    [006] .....    86.588913: kvm_page_fault: vcpu 1 rip 0xffffffff81511ba7 address 0x0000000111400000 error_code 0x182
       CPU 6/KVM-4146    [001] .....    86.594913: kvm_page_fault: vcpu 6 rip 0xffffffff81040f72 address 0x00000000fee000b0 error_code 0x182
       CPU 5/KVM-4145    [002] .....    86.595872: kvm_page_fault: vcpu 5 rip 0x7f08a4602116 address 0x0000000113810002 error_code 0x181
       CPU 5/KVM-4145    [002] .....    86.603341: kvm_page_fault: vcpu 5 rip 0x7f08a4602116 address 0x0000000113a00002 error_code 0x181

All in all, what about me adding this on top?

---
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 30 Aug 2022 12:26:24 -0700
Subject: [PATCH] KVM: x86: Use u64 for address and error code in page fault
 tracepoint

Track the address and error code as 64-bit values in the page fault
tracepoint.  When TDP is enabled, the address is a GPA and thus can be a
64-bit value even on 32-bit hosts.  And SVM's #NPF genereates 64-bit
error codes.

Opportunistically clean up the formatting.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/trace.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 331bdb0ae4b1..c369ebc7269c 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -394,15 +394,14 @@ TRACE_EVENT(kvm_inj_exception,
  * Tracepoint for page fault.
  */
 TRACE_EVENT(kvm_page_fault,
-	TP_PROTO(struct kvm_vcpu *vcpu, unsigned long fault_address,
-		 unsigned int error_code),
+	TP_PROTO(struct kvm_vcpu *vcpu, u64 fault_address, u64 error_code),
 	TP_ARGS(vcpu, fault_address, error_code),

 	TP_STRUCT__entry(
 		__field(	unsigned int,	vcpu_id		)
 		__field(	unsigned long,	guest_rip	)
-		__field(	unsigned long,	fault_address	)
-		__field(	unsigned int,	error_code	)
+		__field(	u64,		fault_address	)
+		__field(	u64,		error_code	)
 	),

 	TP_fast_assign(
@@ -412,7 +411,7 @@ TRACE_EVENT(kvm_page_fault,
 		__entry->error_code	= error_code;
 	),

-	TP_printk("vcpu %u rip 0x%lx address 0x%lx error_code %x",
+	TP_printk("vcpu %u rip 0x%lx address 0x%016llx error_code 0x%llx",
 		  __entry->vcpu_id, __entry->guest_rip,
 		  __entry->fault_address, __entry->error_code)
 );

base-commit: ca362851673d7c01c6624fff0f5a4ee192e6e56a
--