* [PATCH 0/2] VMX: few tracing improvements
@ 2021-01-21 17:10 Maxim Levitsky
2021-01-21 17:10 ` [PATCH 1/2] KVM: nSVM: move nested vmrun tracepoint to enter_svm_guest_mode Maxim Levitsky
2021-01-21 17:10 ` [PATCH 2/2] KVM: nVMX: trace nested vm entry Maxim Levitsky
0 siblings, 2 replies; 7+ messages in thread
From: Maxim Levitsky @ 2021-01-21 17:10 UTC (permalink / raw)
To: kvm
Cc: Borislav Petkov, Paolo Bonzini, x86, Wanpeng Li, linux-kernel,
Sean Christopherson, Jim Mattson, Thomas Gleixner, Ingo Molnar,
Vitaly Kuznetsov, H. Peter Anvin, Joerg Roedel, Maxim Levitsky
Since the fix for the bug in nested migration on VMX is
already merged by Paulo, those are the remaining
patches in this series.
I added a new patch to trace SVM nested entries from
SMM and nested state load as well.
Best regards,
Maxim Levitsky
Maxim Levitsky (2):
KVM: nSVM: move nested vmrun tracepoint to enter_svm_guest_mode
KVM: nVMX: trace nested vm entry
arch/x86/kvm/svm/nested.c | 26 ++++++++++++++------------
arch/x86/kvm/trace.h | 30 ++++++++++++++++++++++++++++++
arch/x86/kvm/vmx/nested.c | 5 +++++
arch/x86/kvm/x86.c | 3 ++-
4 files changed, 51 insertions(+), 13 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] KVM: nSVM: move nested vmrun tracepoint to enter_svm_guest_mode
2021-01-21 17:10 [PATCH 0/2] VMX: few tracing improvements Maxim Levitsky
@ 2021-01-21 17:10 ` Maxim Levitsky
2021-01-21 17:10 ` [PATCH 2/2] KVM: nVMX: trace nested vm entry Maxim Levitsky
1 sibling, 0 replies; 7+ messages in thread
From: Maxim Levitsky @ 2021-01-21 17:10 UTC (permalink / raw)
To: kvm
Cc: Borislav Petkov, Paolo Bonzini, x86, Wanpeng Li, linux-kernel,
Sean Christopherson, Jim Mattson, Thomas Gleixner, Ingo Molnar,
Vitaly Kuznetsov, H. Peter Anvin, Joerg Roedel, Maxim Levitsky
This way trace will capture all the nested mode entries
(including entries after migration, and from smm)
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
arch/x86/kvm/svm/nested.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index cb4c6ee10029c..a6a14f7b56278 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -440,6 +440,20 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb12_gpa,
{
int ret;
+ trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb12_gpa,
+ vmcb12->save.rip,
+ vmcb12->control.int_ctl,
+ vmcb12->control.event_inj,
+ vmcb12->control.nested_ctl);
+
+ trace_kvm_nested_intercepts(vmcb12->control.intercepts[INTERCEPT_CR] & 0xffff,
+ vmcb12->control.intercepts[INTERCEPT_CR] >> 16,
+ vmcb12->control.intercepts[INTERCEPT_EXCEPTION],
+ vmcb12->control.intercepts[INTERCEPT_WORD3],
+ vmcb12->control.intercepts[INTERCEPT_WORD4],
+ vmcb12->control.intercepts[INTERCEPT_WORD5]);
+
+
svm->nested.vmcb12_gpa = vmcb12_gpa;
load_nested_vmcb_control(svm, &vmcb12->control);
nested_prepare_vmcb_save(svm, vmcb12);
@@ -493,18 +507,6 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
goto out;
}
- trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb12_gpa,
- vmcb12->save.rip,
- vmcb12->control.int_ctl,
- vmcb12->control.event_inj,
- vmcb12->control.nested_ctl);
-
- trace_kvm_nested_intercepts(vmcb12->control.intercepts[INTERCEPT_CR] & 0xffff,
- vmcb12->control.intercepts[INTERCEPT_CR] >> 16,
- vmcb12->control.intercepts[INTERCEPT_EXCEPTION],
- vmcb12->control.intercepts[INTERCEPT_WORD3],
- vmcb12->control.intercepts[INTERCEPT_WORD4],
- vmcb12->control.intercepts[INTERCEPT_WORD5]);
/* Clear internal status */
kvm_clear_exception_queue(&svm->vcpu);
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] KVM: nVMX: trace nested vm entry
2021-01-21 17:10 [PATCH 0/2] VMX: few tracing improvements Maxim Levitsky
2021-01-21 17:10 ` [PATCH 1/2] KVM: nSVM: move nested vmrun tracepoint to enter_svm_guest_mode Maxim Levitsky
@ 2021-01-21 17:10 ` Maxim Levitsky
2021-01-21 22:27 ` Sean Christopherson
1 sibling, 1 reply; 7+ messages in thread
From: Maxim Levitsky @ 2021-01-21 17:10 UTC (permalink / raw)
To: kvm
Cc: Borislav Petkov, Paolo Bonzini, x86, Wanpeng Li, linux-kernel,
Sean Christopherson, Jim Mattson, Thomas Gleixner, Ingo Molnar,
Vitaly Kuznetsov, H. Peter Anvin, Joerg Roedel, Maxim Levitsky
This is very helpful to debug nested VMX issues.
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
arch/x86/kvm/trace.h | 30 ++++++++++++++++++++++++++++++
arch/x86/kvm/vmx/nested.c | 5 +++++
arch/x86/kvm/x86.c | 3 ++-
3 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 2de30c20bc264..ec75efdac3560 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -554,6 +554,36 @@ TRACE_EVENT(kvm_nested_vmrun,
__entry->npt ? "on" : "off")
);
+
+/*
+ * Tracepoint for nested VMLAUNCH/VMRESUME
+ */
+TRACE_EVENT(kvm_nested_vmenter,
+ TP_PROTO(__u64 rip, __u64 vmcs, __u64 nested_rip,
+ __u32 entry_intr_info),
+ TP_ARGS(rip, vmcs, nested_rip, entry_intr_info),
+
+ TP_STRUCT__entry(
+ __field( __u64, rip )
+ __field( __u64, vmcs )
+ __field( __u64, nested_rip )
+ __field( __u32, entry_intr_info )
+ ),
+
+ TP_fast_assign(
+ __entry->rip = rip;
+ __entry->vmcs = vmcs;
+ __entry->nested_rip = nested_rip;
+ __entry->entry_intr_info = entry_intr_info;
+ ),
+
+ TP_printk("rip: 0x%016llx vmcs: 0x%016llx nrip: 0x%016llx "
+ "entry_intr_info: 0x%08x",
+ __entry->rip, __entry->vmcs, __entry->nested_rip,
+ __entry->entry_intr_info)
+);
+
+
TRACE_EVENT(kvm_nested_intercepts,
TP_PROTO(__u16 cr_read, __u16 cr_write, __u32 exceptions,
__u32 intercept1, __u32 intercept2, __u32 intercept3),
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 0fbb46990dfce..20b0954f31bda 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3327,6 +3327,11 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS))
vmx->nested.vmcs01_guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS);
+ trace_kvm_nested_vmenter(kvm_rip_read(vcpu),
+ vmx->nested.current_vmptr,
+ vmcs12->guest_rip,
+ vmcs12->vm_entry_intr_info_field);
+
/*
* Overwrite vmcs01.GUEST_CR3 with L1's CR3 if EPT is disabled *and*
* nested early checks are disabled. In the event of a "late" VM-Fail,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a480804ae27a3..757f4f88072af 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11562,11 +11562,12 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_inj_virq);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_page_fault);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_msr);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_cr);
+EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmenter);
+EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmenter_failed);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmrun);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmexit);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmexit_inject);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_intr_vmexit);
-EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmenter_failed);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_invlpga);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_skinit);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_intercepts);
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] KVM: nVMX: trace nested vm entry
2021-01-21 17:10 ` [PATCH 2/2] KVM: nVMX: trace nested vm entry Maxim Levitsky
@ 2021-01-21 22:27 ` Sean Christopherson
2021-01-25 13:22 ` Thoughts on sharing KVM tracepoints [was:Re: [PATCH 2/2] KVM: nVMX: trace nested vm entry] Maxim Levitsky
0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2021-01-21 22:27 UTC (permalink / raw)
To: Maxim Levitsky
Cc: kvm, Borislav Petkov, Paolo Bonzini, x86, Wanpeng Li,
linux-kernel, Jim Mattson, Thomas Gleixner, Ingo Molnar,
Vitaly Kuznetsov, H. Peter Anvin, Joerg Roedel
On Thu, Jan 21, 2021, Maxim Levitsky wrote:
> This is very helpful to debug nested VMX issues.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> arch/x86/kvm/trace.h | 30 ++++++++++++++++++++++++++++++
> arch/x86/kvm/vmx/nested.c | 5 +++++
> arch/x86/kvm/x86.c | 3 ++-
> 3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index 2de30c20bc264..ec75efdac3560 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -554,6 +554,36 @@ TRACE_EVENT(kvm_nested_vmrun,
> __entry->npt ? "on" : "off")
> );
>
> +
> +/*
> + * Tracepoint for nested VMLAUNCH/VMRESUME
> + */
> +TRACE_EVENT(kvm_nested_vmenter,
> + TP_PROTO(__u64 rip, __u64 vmcs, __u64 nested_rip,
> + __u32 entry_intr_info),
> + TP_ARGS(rip, vmcs, nested_rip, entry_intr_info),
> +
> + TP_STRUCT__entry(
> + __field( __u64, rip )
> + __field( __u64, vmcs )
> + __field( __u64, nested_rip )
> + __field( __u32, entry_intr_info )
> + ),
> +
> + TP_fast_assign(
> + __entry->rip = rip;
> + __entry->vmcs = vmcs;
> + __entry->nested_rip = nested_rip;
> + __entry->entry_intr_info = entry_intr_info;
> + ),
> +
> + TP_printk("rip: 0x%016llx vmcs: 0x%016llx nrip: 0x%016llx "
> + "entry_intr_info: 0x%08x",
> + __entry->rip, __entry->vmcs, __entry->nested_rip,
> + __entry->entry_intr_info)
I still don't see why VMX can't share this with SVM. "npt' can easily be "tdp",
differentiating between VMCB and VMCS can be down with ISA, and VMX can give 0
for int_ctl (or throw in something else interesting/relevant).
trace_kvm_nested_vmenter(kvm_rip_read(vcpu),
vmx->nested.current_vmptr,
vmcs12->guest_rip,
0,
vmcs12->vm_entry_intr_info_field,
nested_cpu_has_ept(vmcs12),
KVM_ISA_VMX);
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 2de30c20bc26..90f7cdb31fc1 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -522,12 +522,12 @@ TRACE_EVENT(kvm_pv_eoi,
);
/*
- * Tracepoint for nested VMRUN
+ * Tracepoint for nested VM-Enter. Note, vmcb==vmcs on VMX.
*/
-TRACE_EVENT(kvm_nested_vmrun,
+TRACE_EVENT(kvm_nested_vmenter,
TP_PROTO(__u64 rip, __u64 vmcb, __u64 nested_rip, __u32 int_ctl,
- __u32 event_inj, bool npt),
- TP_ARGS(rip, vmcb, nested_rip, int_ctl, event_inj, npt),
+ __u32 event_inj, bool tdp, __u32 isa),
+ TP_ARGS(rip, vmcb, nested_rip, int_ctl, event_inj, tdp, isa),
TP_STRUCT__entry(
__field( __u64, rip )
@@ -535,7 +535,8 @@ TRACE_EVENT(kvm_nested_vmrun,
__field( __u64, nested_rip )
__field( __u32, int_ctl )
__field( __u32, event_inj )
- __field( bool, npt )
+ __field( bool, tdp )
+ __field( __u32, isa )
),
TP_fast_assign(
@@ -544,14 +545,16 @@ TRACE_EVENT(kvm_nested_vmrun,
__entry->nested_rip = nested_rip;
__entry->int_ctl = int_ctl;
__entry->event_inj = event_inj;
- __entry->npt = npt;
+ __entry->tdp = tdp;
+ __entry->isa = isa;
),
- TP_printk("rip: 0x%016llx vmcb: 0x%016llx nrip: 0x%016llx int_ctl: 0x%08x "
- "event_inj: 0x%08x npt: %s",
- __entry->rip, __entry->vmcb, __entry->nested_rip,
+ TP_printk("rip: 0x%016llx %s: 0x%016llx nrip: 0x%016llx int_ctl: 0x%08x "
+ "event_inj: 0x%08x tdp: %s",
+ __entry->rip, __entry->isa == KVM_ISA_VMX ? "vmcs" : "vmcb",
+ __entry->vmcb, __entry->nested_rip,
__entry->int_ctl, __entry->event_inj,
- __entry->npt ? "on" : "off")
+ __entry->tdp ? "on" : "off")
);
TRACE_EVENT(kvm_nested_intercepts,
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Thoughts on sharing KVM tracepoints [was:Re: [PATCH 2/2] KVM: nVMX: trace nested vm entry]
2021-01-21 22:27 ` Sean Christopherson
@ 2021-01-25 13:22 ` Maxim Levitsky
2021-01-25 21:01 ` Sean Christopherson
0 siblings, 1 reply; 7+ messages in thread
From: Maxim Levitsky @ 2021-01-25 13:22 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm, Borislav Petkov, Paolo Bonzini, x86, Wanpeng Li,
linux-kernel, Jim Mattson, Thomas Gleixner, Ingo Molnar,
Vitaly Kuznetsov, H. Peter Anvin, Joerg Roedel
On Thu, 2021-01-21 at 14:27 -0800, Sean Christopherson wrote:
> On Thu, Jan 21, 2021, Maxim Levitsky wrote:
> > This is very helpful to debug nested VMX issues.
> >
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> > arch/x86/kvm/trace.h | 30 ++++++++++++++++++++++++++++++
> > arch/x86/kvm/vmx/nested.c | 5 +++++
> > arch/x86/kvm/x86.c | 3 ++-
> > 3 files changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > index 2de30c20bc264..ec75efdac3560 100644
> > --- a/arch/x86/kvm/trace.h
> > +++ b/arch/x86/kvm/trace.h
> > @@ -554,6 +554,36 @@ TRACE_EVENT(kvm_nested_vmrun,
> > __entry->npt ? "on" : "off")
> > );
> >
> > +
> > +/*
> > + * Tracepoint for nested VMLAUNCH/VMRESUME
> > + */
> > +TRACE_EVENT(kvm_nested_vmenter,
> > + TP_PROTO(__u64 rip, __u64 vmcs, __u64 nested_rip,
> > + __u32 entry_intr_info),
> > + TP_ARGS(rip, vmcs, nested_rip, entry_intr_info),
> > +
> > + TP_STRUCT__entry(
> > + __field( __u64, rip )
> > + __field( __u64, vmcs )
> > + __field( __u64, nested_rip )
> > + __field( __u32, entry_intr_info )
> > + ),
> > +
> > + TP_fast_assign(
> > + __entry->rip = rip;
> > + __entry->vmcs = vmcs;
> > + __entry->nested_rip = nested_rip;
> > + __entry->entry_intr_info = entry_intr_info;
> > + ),
> > +
> > + TP_printk("rip: 0x%016llx vmcs: 0x%016llx nrip: 0x%016llx "
> > + "entry_intr_info: 0x%08x",
> > + __entry->rip, __entry->vmcs, __entry->nested_rip,
> > + __entry->entry_intr_info)
>
> I still don't see why VMX can't share this with SVM. "npt' can easily be "tdp",
> differentiating between VMCB and VMCS can be down with ISA, and VMX can give 0
> for int_ctl (or throw in something else interesting/relevant).
I understand very well your point, and I don't strongly disagree with you.
However let me voice my own thoughts on this:
I think that sharing tracepoints between SVM and VMX isn't necessarily a good idea.
It does make sense in some cases but not in all of them.
The trace points are primarily intended for developers, thus they should capture as
much as possible relevant info but not everything because traces can get huge.
Also despite the fact that a developer will look at the traces, some usability is welcome
as well (e.g for new developers), and looking at things like info1/info2/intr_info/error_code
isn't very usable (btw the error_code should at least be called intr_info_error_code, and
of course both it and intr_info are VMX specific).
So I don't even like the fact that kvm_entry/kvm_exit are shared, and neither I want
to add even more shared trace points.
I understand that there are some benefits of sharing, namely a userspace tool can use
the same event to *profile* kvm, but I am not sure that this is worth it.
What we could have done is to have ISA (and maybe even x86) agnostic kvm_exit/kvm_entry
tracepoints that would have no data attached to them, or have very little (like maybe RIP),
and then have ISA specific tracepoints with the reset of the info.
Same could be applied to kvm_nested_vmenter, although for this one I don't think that we
need an ISA agnostic tracepoint.
Having said all that, I am not hell bent on this. If you really want it to be this way,
I won't argue that much.
Thoughts?
Best regards,
Maxim Levitsky
>
> trace_kvm_nested_vmenter(kvm_rip_read(vcpu),
> vmx->nested.current_vmptr,
> vmcs12->guest_rip,
> 0,
> vmcs12->vm_entry_intr_info_field,
> nested_cpu_has_ept(vmcs12),
> KVM_ISA_VMX);
>
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index 2de30c20bc26..90f7cdb31fc1 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -522,12 +522,12 @@ TRACE_EVENT(kvm_pv_eoi,
> );
>
> /*
> - * Tracepoint for nested VMRUN
> + * Tracepoint for nested VM-Enter. Note, vmcb==vmcs on VMX.
> */
> -TRACE_EVENT(kvm_nested_vmrun,
> +TRACE_EVENT(kvm_nested_vmenter,
> TP_PROTO(__u64 rip, __u64 vmcb, __u64 nested_rip, __u32 int_ctl,
> - __u32 event_inj, bool npt),
> - TP_ARGS(rip, vmcb, nested_rip, int_ctl, event_inj, npt),
> + __u32 event_inj, bool tdp, __u32 isa),
> + TP_ARGS(rip, vmcb, nested_rip, int_ctl, event_inj, tdp, isa),
>
> TP_STRUCT__entry(
> __field( __u64, rip )
> @@ -535,7 +535,8 @@ TRACE_EVENT(kvm_nested_vmrun,
> __field( __u64, nested_rip )
> __field( __u32, int_ctl )
> __field( __u32, event_inj )
> - __field( bool, npt )
> + __field( bool, tdp )
> + __field( __u32, isa )
> ),
>
> TP_fast_assign(
> @@ -544,14 +545,16 @@ TRACE_EVENT(kvm_nested_vmrun,
> __entry->nested_rip = nested_rip;
> __entry->int_ctl = int_ctl;
> __entry->event_inj = event_inj;
> - __entry->npt = npt;
> + __entry->tdp = tdp;
> + __entry->isa = isa;
> ),
>
> - TP_printk("rip: 0x%016llx vmcb: 0x%016llx nrip: 0x%016llx int_ctl: 0x%08x "
> - "event_inj: 0x%08x npt: %s",
> - __entry->rip, __entry->vmcb, __entry->nested_rip,
> + TP_printk("rip: 0x%016llx %s: 0x%016llx nrip: 0x%016llx int_ctl: 0x%08x "
> + "event_inj: 0x%08x tdp: %s",
> + __entry->rip, __entry->isa == KVM_ISA_VMX ? "vmcs" : "vmcb",
> + __entry->vmcb, __entry->nested_rip,
> __entry->int_ctl, __entry->event_inj,
> - __entry->npt ? "on" : "off")
> + __entry->tdp ? "on" : "off")
> );
>
> TRACE_EVENT(kvm_nested_intercepts,
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Thoughts on sharing KVM tracepoints [was:Re: [PATCH 2/2] KVM: nVMX: trace nested vm entry]
2021-01-25 13:22 ` Thoughts on sharing KVM tracepoints [was:Re: [PATCH 2/2] KVM: nVMX: trace nested vm entry] Maxim Levitsky
@ 2021-01-25 21:01 ` Sean Christopherson
2021-01-27 10:35 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2021-01-25 21:01 UTC (permalink / raw)
To: Maxim Levitsky
Cc: kvm, Borislav Petkov, Paolo Bonzini, x86, Wanpeng Li,
linux-kernel, Jim Mattson, Thomas Gleixner, Ingo Molnar,
Vitaly Kuznetsov, H. Peter Anvin, Joerg Roedel
On Mon, Jan 25, 2021, Maxim Levitsky wrote:
> On Thu, 2021-01-21 at 14:27 -0800, Sean Christopherson wrote:
> > I still don't see why VMX can't share this with SVM. "npt' can easily be "tdp",
> > differentiating between VMCB and VMCS can be down with ISA, and VMX can give 0
> > for int_ctl (or throw in something else interesting/relevant).
>
> I understand very well your point, and I don't strongly disagree with you.
> However let me voice my own thoughts on this:
>
> I think that sharing tracepoints between SVM and VMX isn't necessarily a good idea.
> It does make sense in some cases but not in all of them.
>
> The trace points are primarily intended for developers, thus they should capture as
> much as possible relevant info but not everything because traces can get huge.
>
> Also despite the fact that a developer will look at the traces, some usability is welcome
> as well (e.g for new developers), and looking at things like info1/info2/intr_info/error_code
> isn't very usable
I'm not opposed to printing different names on VMX, e.g. exit_qual and
idt_vec_info, but I 100% think that VMX and SVM should share the bulk of the
code. Improvements to VMX almost always apply in some way to SVM, and vice
versa. It's all but guaranteed that splitting flows will eventually cause
divergence in a bad way. Divergence in tracepoints is likely to be minor at
worst, but I don't think that's a good reason to intentionally split the code
when it's quite easy to share.
> (btw the error_code should at least be called intr_info_error_code, and
Heh, I disagree even on this. IMO, after debugging a few times, associating
error_code with the event being injected is second nature. Prepending
intr_info_ would just add extra characters and slow down mental processing.
> of course both it and intr_info are VMX specific).
Not really, SVM has the exact same fields with slightly different names.
> So I don't even like the fact that kvm_entry/kvm_exit are shared, and neither I want
> to add even more shared trace points.
>
> I understand that there are some benefits of sharing, namely a userspace tool can use
> the same event to *profile* kvm, but I am not sure that this is worth it.
Why is it not worth it? It's a small amount of one-time kernel pain that allows
all users/developers to reuse scripts and tools across VMX and SVM. Even manual
usage benefits, e.g. I don't have to remember that a tracepoint is 'x' on VMX
but 'y' on SVM.
> What we could have done is to have ISA (and maybe even x86) agnostic kvm_exit/kvm_entry
> tracepoints that would have no data attached to them, or have very little (like maybe RIP),
> and then have ISA specific tracepoints with the reset of the info.
That would probably end up as the least user friendly combination. Usually I
enable a tracepoint to get more info, rarely am I interested in _just_ the
logging of the tracepoint itself. The generic tracepoint would either be
useless and never enabled, or even worse would cause people to overlook the
vendor-specific variant.
> Same could be applied to kvm_nested_vmenter, although for this one I don't think that we
> need an ISA agnostic tracepoint.
>
> Having said all that, I am not hell bent on this. If you really want it to be this way,
> I won't argue that much.
>
> Thoughts?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Thoughts on sharing KVM tracepoints [was:Re: [PATCH 2/2] KVM: nVMX: trace nested vm entry]
2021-01-25 21:01 ` Sean Christopherson
@ 2021-01-27 10:35 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2021-01-27 10:35 UTC (permalink / raw)
To: Sean Christopherson, Maxim Levitsky
Cc: kvm, Borislav Petkov, x86, Wanpeng Li, linux-kernel, Jim Mattson,
Thomas Gleixner, Ingo Molnar, Vitaly Kuznetsov, H. Peter Anvin,
Joerg Roedel
On 25/01/21 22:01, Sean Christopherson wrote:
> I 100% think that VMX and SVM should share the bulk of the
> code. Improvements to VMX almost always apply in some way to SVM, and vice
> versa.
I agree.
> IMO, after debugging a few times, associating
> error_code with the event being injected is second nature. Prepending
> intr_info_ would just add extra characters and slow down mental processing.
>
>> of course both it and intr_info are VMX specific).
>
> Not really, SVM has the exact same fields with slightly different names.
>
I slightly prefer the SVM names, using eventinj and eventinjerr in the
trace points wouldn't be bad.
Having too many tracepoints are a problem. Having a lot of info in a
single tracepoint is not a problem, though.
Paolo
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-27 11:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-21 17:10 [PATCH 0/2] VMX: few tracing improvements Maxim Levitsky
2021-01-21 17:10 ` [PATCH 1/2] KVM: nSVM: move nested vmrun tracepoint to enter_svm_guest_mode Maxim Levitsky
2021-01-21 17:10 ` [PATCH 2/2] KVM: nVMX: trace nested vm entry Maxim Levitsky
2021-01-21 22:27 ` Sean Christopherson
2021-01-25 13:22 ` Thoughts on sharing KVM tracepoints [was:Re: [PATCH 2/2] KVM: nVMX: trace nested vm entry] Maxim Levitsky
2021-01-25 21:01 ` Sean Christopherson
2021-01-27 10:35 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).