LinuxLists.cc - [PATCH] KVM: Add extra information in kvm_page

2022-05-10 15:56:10

Subject: [PATCH] KVM: Add extra information in kvm_page_fault trace point

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 <[email protected]>
Cc: Hong Yeon Kim <[email protected]>
Cc: Taeung Song <[email protected]>
Cc: [email protected]
Signed-off-by: Wonhyuk Yang <[email protected]>
---
arch/x86/kvm/mmu/mmu.c | 2 +-
arch/x86/kvm/svm/svm.c | 2 +-
arch/x86/kvm/trace.h | 12 +++++++++---
arch/x86/kvm/vmx/vmx.c | 2 +-
4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 311e4e1d7870..b9421060efa8 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4080,7 +4080,7 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,

vcpu->arch.l1tf_flush_l1d = true;
if (!flags) {
- trace_kvm_page_fault(fault_address, error_code);
+ trace_kvm_page_fault(vcpu, fault_address, error_code);

if (kvm_event_needs_reinjection(vcpu))
kvm_mmu_unprotect_page_virt(vcpu, fault_address);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7e45d03cd018..9741cfbf47a4 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1784,7 +1784,7 @@ static int npf_interception(struct kvm_vcpu *vcpu)
u64 fault_address = svm->vmcb->control.exit_info_2;
u64 error_code = svm->vmcb->control.exit_info_1;

- trace_kvm_page_fault(fault_address, error_code);
+ trace_kvm_page_fault(vcpu, fault_address, error_code);
return kvm_mmu_page_fault(vcpu, fault_address, error_code,
static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
svm->vmcb->control.insn_bytes : NULL,
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 )
),

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",
+ __entry->vcpu_id, __entry->guest_rip,
__entry->fault_address, __entry->error_code)
);

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 610355b9ccce..0f1edd02b68b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5398,7 +5398,7 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu)
vmcs_set_bits(GUEST_INTERRUPTIBILITY_INFO, GUEST_INTR_STATE_NMI);

gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
- trace_kvm_page_fault(gpa, exit_qualification);
+ trace_kvm_page_fault(vcpu, gpa, exit_qualification);

/* Is it a read fault? */
error_code = (exit_qualification & EPT_VIOLATION_ACC_READ)
--
2.30.2

2022-08-30 19:43:41

by Sean Christopherson

[permalink] [raw]

Subject: Re: [PATCH] KVM: Add extra information in kvm_page_fault trace point

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 <[email protected]>
> Cc: Hong Yeon Kim <[email protected]>
> Cc: Taeung Song <[email protected]>
> Cc: [email protected]
> Signed-off-by: Wonhyuk Yang <[email protected]>
> ---

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 <[email protected]>
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 <[email protected]>
---
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
--

2022-08-30 22:07:46

by Sean Christopherson

[permalink] [raw]