[PATCH v2 0/3] VMX: more nested fixes

[PATCH v2 0/3] VMX: more nested fixes

[Maxim Levitsky]

This is hopefully the last fix for VMX nested migration
that finally allows my stress test of migration with a nested guest to pass.

In a nutshell after an optimization that was done in commit 7952d769c29ca,
some of vmcs02 fields which can be modified by the L2 freely while it runs
(like GSBASE and such) were not copied back to vmcs12 unless:

1. L1 tries to vmread them (update done on intercept)
2. vmclear or vmldptr on other vmcs are done.
3. nested state is read and nested guest is running.

What wasn't done was to sync these 'rare' fields when L1 is running
but still has a loaded vmcs12 which might have some stale fields,
if that vmcs was used to enter a guest already due to that optimization.

Plus I added two minor patches to improve VMX tracepoints
a bit. There is still a large room for improvement.

Best regards,
	Maxim Levitsky

Maxim Levitsky (3):
  KVM: nVMX: Always call sync_vmcs02_to_vmcs12_rare on migration
  KVM: nVMX: add kvm_nested_vmlaunch_resume tracepoint
  KVM: VMX: read idt_vectoring_info a bit earlier

 arch/x86/kvm/trace.h      | 30 ++++++++++++++++++++++++++++++
 arch/x86/kvm/vmx/nested.c | 19 ++++++++++++++-----
 arch/x86/kvm/vmx/vmx.c    |  3 ++-
 arch/x86/kvm/x86.c        |  1 +
 4 files changed, 47 insertions(+), 6 deletions(-)

-- 
2.26.2

[PATCH v2 1/3] KVM: nVMX: Always call sync_vmcs02_to_vmcs12_rare on migration

[Maxim Levitsky]

Even when we are outside the nested guest, some vmcs02 fields
are not in sync vs vmcs12.

However during the migration, the vmcs12 has to be up to date
to be able to load it later after the migration.

To fix that, call that function.

Fixes: 7952d769c29ca ("KVM: nVMX: Sync rarely accessed guest fields only when needed")

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 0fbb46990dfce..776688f9d1017 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6077,11 +6077,14 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
 	if (is_guest_mode(vcpu)) {
 		sync_vmcs02_to_vmcs12(vcpu, vmcs12);
 		sync_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
-	} else if (!vmx->nested.need_vmcs12_to_shadow_sync) {
-		if (vmx->nested.hv_evmcs)
-			copy_enlightened_to_vmcs12(vmx);
-		else if (enable_shadow_vmcs)
-			copy_shadow_to_vmcs12(vmx);
+	} else  {
+		copy_vmcs02_to_vmcs12_rare(vcpu, get_vmcs12(vcpu));
+		if (!vmx->nested.need_vmcs12_to_shadow_sync) {
+			if (vmx->nested.hv_evmcs)
+				copy_enlightened_to_vmcs12(vmx);
+			else if (enable_shadow_vmcs)
+				copy_shadow_to_vmcs12(vmx);
+		}
 	}
 
 	BUILD_BUG_ON(sizeof(user_vmx_nested_state->vmcs12) < VMCS12_SIZE);
-- 
2.26.2

Re: [PATCH v2 1/3] KVM: nVMX: Always call sync_vmcs02_to_vmcs12_rare on migration

[Sean Christopherson]

Subject is confusing, and technically wrong.  Confusing because there is no call
to sync_vmcs02_to_vmcs12_rare().  Technically wrong because sync_...() won't be
called if need_sync_vmcs02_to_vmcs12_rare==false.

Maybe something like?

KVM: nVMX: Sync unsync'd vmcs02 state to vmcs12 on migration

On Thu, Jan 14, 2021, Maxim Levitsky wrote:
> Even when we are outside the nested guest, some vmcs02 fields
> are not in sync vs vmcs12.

s/are not/may not be

It'd be helpful to provide more details in the changelog, e.g. for those not in
the loop, it's not obvious that KVM intentionally keeps those fields unsync'd,
even across nested VM-Exit.

> However during the migration, the vmcs12 has to be up to date
> to be able to load it later after the migration.
> 
> To fix that, call that function.

s/that function/copy_vmcs02_to_vmcs12_rare().  Characters are cheap, and it's
jarring to have to jump back all the way back to the subject.  And, as mentioned
above, this doesn't actually call sync_ directly.

For the code,

Reviewed-by: Sean Christopherson <seanjc@google.com> 

> 
> Fixes: 7952d769c29ca ("KVM: nVMX: Sync rarely accessed guest fields only when needed")
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 0fbb46990dfce..776688f9d1017 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -6077,11 +6077,14 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
>  	if (is_guest_mode(vcpu)) {
>  		sync_vmcs02_to_vmcs12(vcpu, vmcs12);
>  		sync_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
> -	} else if (!vmx->nested.need_vmcs12_to_shadow_sync) {
> -		if (vmx->nested.hv_evmcs)
> -			copy_enlightened_to_vmcs12(vmx);
> -		else if (enable_shadow_vmcs)
> -			copy_shadow_to_vmcs12(vmx);
> +	} else  {
> +		copy_vmcs02_to_vmcs12_rare(vcpu, get_vmcs12(vcpu));
> +		if (!vmx->nested.need_vmcs12_to_shadow_sync) {
> +			if (vmx->nested.hv_evmcs)
> +				copy_enlightened_to_vmcs12(vmx);
> +			else if (enable_shadow_vmcs)
> +				copy_shadow_to_vmcs12(vmx);
> +		}
>  	}
>  
>  	BUILD_BUG_ON(sizeof(user_vmx_nested_state->vmcs12) < VMCS12_SIZE);
> -- 
> 2.26.2
> 

Re: [PATCH v2 1/3] KVM: nVMX: Always call sync_vmcs02_to_vmcs12_rare on migration

[Paolo Bonzini]

On 15/01/21 00:58, Sean Christopherson wrote:
> Reviewed-by: Sean Christopherson<seanjc@google.com>  

New commit message:

KVM: nVMX: Sync unsync'd vmcs02 state to vmcs12 on migration

Even when we are outside the nested guest, some vmcs02 fields
may not be in sync vs vmcs12.  This is intentional, even across
nested VM-exit, because the sync can be delayed until the nested
hypervisor performs a VMCLEAR or a VMREAD/VMWRITE that affects those
rarely accessed fields.

However, during KVM_GET_NESTED_STATE, the vmcs12 has to be up to date to
be able to restore it.  To fix that, call copy_vmcs02_to_vmcs12_rare()
before the vmcs12 contents are copied to userspace.

Paolo

[PATCH v2 2/3] KVM: nVMX: add kvm_nested_vmlaunch_resume tracepoint

[Maxim Levitsky]

This is very helpful for debugging nested VMX issues.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/trace.h      | 30 ++++++++++++++++++++++++++++++
 arch/x86/kvm/vmx/nested.c |  6 ++++++
 arch/x86/kvm/x86.c        |  1 +
 3 files changed, 37 insertions(+)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 2de30c20bc264..663d1b1d8bf64 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_vmlaunch_resume,
+	    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 776688f9d1017..cd51b66480d52 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3327,6 +3327,12 @@ 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_vmlaunch_resume(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..7c6e94e32100e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11562,6 +11562,7 @@ 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_vmlaunch_resume);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmrun);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmexit);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmexit_inject);
-- 
2.26.2

Re: [PATCH v2 2/3] KVM: nVMX: add kvm_nested_vmlaunch_resume tracepoint

[Sean Christopherson]

On Thu, Jan 14, 2021, Maxim Levitsky wrote:
> This is very helpful for debugging nested VMX issues.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/trace.h      | 30 ++++++++++++++++++++++++++++++
>  arch/x86/kvm/vmx/nested.c |  6 ++++++
>  arch/x86/kvm/x86.c        |  1 +
>  3 files changed, 37 insertions(+)
> 
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index 2de30c20bc264..663d1b1d8bf64 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

VM-Enter, as below.

> + */
> +TRACE_EVENT(kvm_nested_vmlaunch_resume,

s/vmlaunc_resume/vmenter, both for consistency with other code and so that it
can sanely be reused by SVM.  IMO, trace_kvm_entry is wrong :-).

> +	    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 776688f9d1017..cd51b66480d52 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3327,6 +3327,12 @@ 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_vmlaunch_resume(kvm_rip_read(vcpu),

Hmm, won't this RIP be wrong for the migration case?  I.e. it'll be L2, not L1
as is the case for the "true" nested VM-Enter path.

> +					 vmx->nested.current_vmptr,
> +					 vmcs12->guest_rip,
> +					 vmcs12->vm_entry_intr_info_field);

The placement is a bit funky.  I assume you put it here so that calls from
vmx_set_nested_state() also get traced.  But, that also means
vmx_pre_leave_smm() will get traced, and it also creates some weirdness where
some nested VM-Enters that VM-Fail will get traced, but others will not.

Tracing vmx_pre_leave_smm() isn't necessarily bad, but it could be confusing,
especially if the debugger looks up the RIP and sees RSM.  Ditto for the
migration case.

Not sure what would be a good answer.

> +
> +
>  	/*
>  	 * 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..7c6e94e32100e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11562,6 +11562,7 @@ 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_vmlaunch_resume);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmrun);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmexit);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmexit_inject);
> -- 
> 2.26.2
> 

Re: [PATCH v2 2/3] KVM: nVMX: add kvm_nested_vmlaunch_resume tracepoint

[Paolo Bonzini]

On 15/01/21 01:14, Sean Christopherson wrote:
>> +	trace_kvm_nested_vmlaunch_resume(kvm_rip_read(vcpu),
> Hmm, won't this RIP be wrong for the migration case?  I.e. it'll be L2, not L1
> as is the case for the "true" nested VM-Enter path.

It will be the previous RIP---might as well be 0xfffffff0 depending on 
what userspace does.  I don't think you can do much better than that, 
using vmcs12->host_rip would be confusing in the SMM case.

>> +					 vmx->nested.current_vmptr,
>> +					 vmcs12->guest_rip,
>> +					 vmcs12->vm_entry_intr_info_field);
> The placement is a bit funky.  I assume you put it here so that calls from
> vmx_set_nested_state() also get traced.  But, that also means
> vmx_pre_leave_smm() will get traced, and it also creates some weirdness where
> some nested VM-Enters that VM-Fail will get traced, but others will not.
> 
> Tracing vmx_pre_leave_smm() isn't necessarily bad, but it could be confusing,
> especially if the debugger looks up the RIP and sees RSM.  Ditto for the
> migration case.

Actually tracing vmx_pre_leave_smm() is good, and pointing to RSM makes 
sense so I'm not worried about that.

Paolo

Re: [PATCH v2 2/3] KVM: nVMX: add kvm_nested_vmlaunch_resume tracepoint

[Sean Christopherson]

On Fri, Jan 15, 2021, Paolo Bonzini wrote:
> On 15/01/21 01:14, Sean Christopherson wrote:
> > > +	trace_kvm_nested_vmlaunch_resume(kvm_rip_read(vcpu),
> > Hmm, won't this RIP be wrong for the migration case?  I.e. it'll be L2, not L1
> > as is the case for the "true" nested VM-Enter path.
> 
> It will be the previous RIP---might as well be 0xfffffff0 depending on what
> userspace does.  I don't think you can do much better than that, using
> vmcs12->host_rip would be confusing in the SMM case.
> 
> > > +					 vmx->nested.current_vmptr,
> > > +					 vmcs12->guest_rip,
> > > +					 vmcs12->vm_entry_intr_info_field);
> > The placement is a bit funky.  I assume you put it here so that calls from
> > vmx_set_nested_state() also get traced.  But, that also means
> > vmx_pre_leave_smm() will get traced, and it also creates some weirdness where
> > some nested VM-Enters that VM-Fail will get traced, but others will not.
> > 
> > Tracing vmx_pre_leave_smm() isn't necessarily bad, but it could be confusing,
> > especially if the debugger looks up the RIP and sees RSM.  Ditto for the
> > migration case.
> 
> Actually tracing vmx_pre_leave_smm() is good, and pointing to RSM makes
> sense so I'm not worried about that.

Ideally there would something in the tracepoint to differentiate the various
cases.  Not that the RSM/migration cases will pop up often, but I think it's an
easily solved problem that could avoid confusion.

What if we captured vmx->nested.smm.guest_mode and from_vmentry, and explicitly
record what triggered the entry?

	TP_printk("from: %s rip: 0x%016llx vmcs: 0x%016llx nrip: 0x%016llx intr_info: 0x%08x",
		  __entry->vmenter ? "VM-Enter" : __entry->smm ? "RSM" : "SET_STATE",
		  __entry->rip, __entry->vmcs, __entry->nested_rip,
		  __entry->entry_intr_info

Side topic, can we have an "official" ruling on whether KVM tracepoints should
use colons and/or commas? And probably same question for whether or not to
prepend zeros.  E.g. kvm_entry has "vcpu %u, rip 0x%lx" versus "rip: 0x%016llx
vmcs: 0x%016llx".  It bugs me that we're so inconsistent.

Re: [PATCH v2 2/3] KVM: nVMX: add kvm_nested_vmlaunch_resume tracepoint

[Maxim Levitsky]

On Fri, 2021-01-15 at 08:30 -0800, Sean Christopherson wrote:
> On Fri, Jan 15, 2021, Paolo Bonzini wrote:
> > On 15/01/21 01:14, Sean Christopherson wrote:
> > > > +	trace_kvm_nested_vmlaunch_resume(kvm_rip_read(vcpu),
> > > Hmm, won't this RIP be wrong for the migration case?  I.e. it'll be L2, not L1
> > > as is the case for the "true" nested VM-Enter path.
> > 
> > It will be the previous RIP---might as well be 0xfffffff0 depending on what
> > userspace does.  I don't think you can do much better than that, using
> > vmcs12->host_rip would be confusing in the SMM case.
> > 
> > > > +					 vmx->nested.current_vmptr,
> > > > +					 vmcs12->guest_rip,
> > > > +					 vmcs12->vm_entry_intr_info_field);
> > > The placement is a bit funky.  I assume you put it here so that calls from
> > > vmx_set_nested_state() also get traced.  But, that also means
> > > vmx_pre_leave_smm() will get traced, and it also creates some weirdness where
> > > some nested VM-Enters that VM-Fail will get traced, but others will not.
> > > 
> > > Tracing vmx_pre_leave_smm() isn't necessarily bad, but it could be confusing,
> > > especially if the debugger looks up the RIP and sees RSM.  Ditto for the
> > > migration case.
> > 
> > Actually tracing vmx_pre_leave_smm() is good, and pointing to RSM makes
> > sense so I'm not worried about that.
> 
> Ideally there would something in the tracepoint to differentiate the various
> cases.  Not that the RSM/migration cases will pop up often, but I think it's an
> easily solved problem that could avoid confusion.
> 
> What if we captured vmx->nested.smm.guest_mode and from_vmentry, and explicitly
> record what triggered the entry?
> 
> 	TP_printk("from: %s rip: 0x%016llx vmcs: 0x%016llx nrip: 0x%016llx intr_info: 0x%08x",
> 		  __entry->vmenter ? "VM-Enter" : __entry->smm ? "RSM" : "SET_STATE",
> 		  __entry->rip, __entry->vmcs, __entry->nested_rip,
> 		  __entry->entry_intr_info

I think that this is a good idea, but should be done in a separate patch.

> 
> Side topic, can we have an "official" ruling on whether KVM tracepoints should
> use colons and/or commas? And probably same question for whether or not to
> prepend zeros.  E.g. kvm_entry has "vcpu %u, rip 0x%lx" versus "rip: 0x%016llx
> vmcs: 0x%016llx".  It bugs me that we're so inconsistent.
> 

As I said the kvm tracing has a lot of things that can be imporoved, 
and as it is often the only way to figure out complex bugs as these I had to deal with recently,
I will do more improvements in this area as time permits.

Best regards,
	Maxim Levitsky

Re: [PATCH v2 2/3] KVM: nVMX: add kvm_nested_vmlaunch_resume tracepoint

[Maxim Levitsky]

On Fri, 2021-01-15 at 14:48 +0100, Paolo Bonzini wrote:
> On 15/01/21 01:14, Sean Christopherson wrote:
> > > +	trace_kvm_nested_vmlaunch_resume(kvm_rip_read(vcpu),
> > Hmm, won't this RIP be wrong for the migration case?  I.e. it'll be L2, not L1
> > as is the case for the "true" nested VM-Enter path.

Actually in this case, the initial RIP of 0x000000000000fff0 will be printed
which isn't that bad.

A tracepoint in nested state load function would be very nice to add
to mark this explicitly. I'll do this later.

> 
> It will be the previous RIP---might as well be 0xfffffff0 depending on 
> what userspace does.  I don't think you can do much better than that, 
> using vmcs12->host_rip would be confusing in the SMM case.
> 
> > > +					 vmx->nested.current_vmptr,
> > > +					 vmcs12->guest_rip,
> > > +					 vmcs12->vm_entry_intr_info_field);
> > The placement is a bit funky.  I assume you put it here so that calls from
> > vmx_set_nested_state() also get traced.  But, that also means
> > vmx_pre_leave_smm() will get traced, and it also creates some weirdness where
> > some nested VM-Enters that VM-Fail will get traced, but others will not.
> > 
> > Tracing vmx_pre_leave_smm() isn't necessarily bad, but it could be confusing,
> > especially if the debugger looks up the RIP and sees RSM.  Ditto for the
> > migration case.
> 
> Actually tracing vmx_pre_leave_smm() is good, and pointing to RSM makes 
> sense so I'm not worried about that.
> 
> Paolo
> 

I agree with that and indeed this was my intention.

In fact I will change the svm's tracepoint to behave the same way
in the next patch series (I'll move it to enter_svm_guest_mode).

(When I wrote this patch I somehow thought that this is what SVM already does).

Best regards,
	Maxim Levitsky

Re: [PATCH v2 2/3] KVM: nVMX: add kvm_nested_vmlaunch_resume tracepoint

[Maxim Levitsky]

On Thu, 2021-01-14 at 16:14 -0800, Sean Christopherson wrote:
> On Thu, Jan 14, 2021, Maxim Levitsky wrote:
> > This is very helpful for debugging nested VMX issues.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  arch/x86/kvm/trace.h      | 30 ++++++++++++++++++++++++++++++
> >  arch/x86/kvm/vmx/nested.c |  6 ++++++
> >  arch/x86/kvm/x86.c        |  1 +
> >  3 files changed, 37 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > index 2de30c20bc264..663d1b1d8bf64 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
> 
> VM-Enter, as below.

Will do

> 
> > + */
> > +TRACE_EVENT(kvm_nested_vmlaunch_resume,
> 
> s/vmlaunc_resume/vmenter, both for consistency with other code and so that it
> can sanely be reused by SVM.  IMO, trace_kvm_entry is wrong :-).
SVM already has trace_kvm_nested_vmrun and it contains some SVM specific
stuff that doesn't exist on VMX and vise versa.
So I do want to keep these trace points separate.


> 
> > +	    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 776688f9d1017..cd51b66480d52 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -3327,6 +3327,12 @@ 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_vmlaunch_resume(kvm_rip_read(vcpu),
> 
> Hmm, won't this RIP be wrong for the migration case?  I.e. it'll be L2, not L1
> as is the case for the "true" nested VM-Enter path.

> 
> > +					 vmx->nested.current_vmptr,
> > +					 vmcs12->guest_rip,
> > +					 vmcs12->vm_entry_intr_info_field);
> 
> The placement is a bit funky.  I assume you put it here so that calls from
> vmx_set_nested_state() also get traced.  But, that also means
> vmx_pre_leave_smm() will get traced, and it also creates some weirdness where
> some nested VM-Enters that VM-Fail will get traced, but others will not.
> 
> Tracing vmx_pre_leave_smm() isn't necessarily bad, but it could be confusing,
> especially if the debugger looks up the RIP and sees RSM.  Ditto for the
> migration case.
> 
> Not sure what would be a good answer.
> 
> > +
> > +
> >  	/*
> >  	 * 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..7c6e94e32100e 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -11562,6 +11562,7 @@ 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_vmlaunch_resume);
> >  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmrun);
> >  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmexit);
> >  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmexit_inject);
> > -- 
> > 2.26.2
> > 

[PATCH v2 3/3] KVM: VMX: read idt_vectoring_info a bit earlier

[Maxim Levitsky]

This allows it to be printed correctly by the trace print
that follows.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/vmx/vmx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 2af05d3b05909..9b6e7dbf5e2bd 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6771,6 +6771,8 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	}
 
 	vmx->exit_reason = vmcs_read32(VM_EXIT_REASON);
+	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
+
 	if (unlikely((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY))
 		kvm_machine_check();
 
@@ -6780,7 +6782,6 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
 		return EXIT_FASTPATH_NONE;
 
 	vmx->loaded_vmcs->launched = 1;
-	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
 
 	vmx_recover_nmi_blocking(vmx);
 	vmx_complete_interrupts(vmx);
-- 
2.26.2

Re: [PATCH v2 3/3] KVM: VMX: read idt_vectoring_info a bit earlier

[Sean Christopherson]

On Thu, Jan 14, 2021, Maxim Levitsky wrote:
> This allows it to be printed correctly by the trace print

It'd be helpful to explicitly say which tracepoint, and explain that the value
is read by vmx_get_exit_info().  It's far from obvious how this gets consumed.

> that follows.
> 

Fixes: dcf068da7eb2 ("KVM: VMX: Introduce generic fastpath handler")

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 2af05d3b05909..9b6e7dbf5e2bd 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6771,6 +6771,8 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  	}
>  
>  	vmx->exit_reason = vmcs_read32(VM_EXIT_REASON);
> +	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);

Hrm, it probably makes sense to either do the VMREAD conditionally, or to
zero idt_vectoring_info in the vmx->fail path.  I don't care about the cycles
on VM-Exit consistency checks, just that this would hide that the field is valid
if and only if VM-Enter fully succeeded.  A third option would be to add a
comment saying that it's unnecessary if VM-Enter failed, but faster in the
common case to just do the VMREAD.

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 2af05d3b0590..3c172c05570a 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6774,13 +6774,15 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
        if (unlikely((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY))
                kvm_machine_check();

+       if (likely(!(vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)))
+               vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
+
        trace_kvm_exit(vmx->exit_reason, vcpu, KVM_ISA_VMX);

        if (unlikely(vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
                return EXIT_FASTPATH_NONE;

        vmx->loaded_vmcs->launched = 1;
-       vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);

        vmx_recover_nmi_blocking(vmx);
        vmx_complete_interrupts(vmx);


> +
>  	if (unlikely((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY))
>  		kvm_machine_check();
>  
> @@ -6780,7 +6782,6 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  		return EXIT_FASTPATH_NONE;
>  
>  	vmx->loaded_vmcs->launched = 1;
> -	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
>  
>  	vmx_recover_nmi_blocking(vmx);
>  	vmx_complete_interrupts(vmx);
> -- 
> 2.26.2
> 

Re: [PATCH v2 3/3] KVM: VMX: read idt_vectoring_info a bit earlier

[Maxim Levitsky]

On Thu, 2021-01-14 at 16:29 -0800, Sean Christopherson wrote:
> On Thu, Jan 14, 2021, Maxim Levitsky wrote:
> > This allows it to be printed correctly by the trace print
> 
> It'd be helpful to explicitly say which tracepoint, and explain that the value
> is read by vmx_get_exit_info().  It's far from obvious how this gets consumed.
> 
> > that follows.
> > 
> 
> Fixes: dcf068da7eb2 ("KVM: VMX: Introduce generic fastpath handler")
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 2af05d3b05909..9b6e7dbf5e2bd 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -6771,6 +6771,8 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
> >  	}
> >  
> >  	vmx->exit_reason = vmcs_read32(VM_EXIT_REASON);
> > +	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
> 
> Hrm, it probably makes sense to either do the VMREAD conditionally, or to
> zero idt_vectoring_info in the vmx->fail path.  I don't care about the cycles
> on VM-Exit consistency checks, just that this would hide that the field is valid
> if and only if VM-Enter fully succeeded.  A third option would be to add a
> comment saying that it's unnecessary if VM-Enter failed, but faster in the
> common case to just do the VMREAD.

Allright, I will add this.

Best regards,
	Maxim Levitsky


> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 2af05d3b0590..3c172c05570a 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6774,13 +6774,15 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
>         if (unlikely((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY))
>                 kvm_machine_check();
> 
> +       if (likely(!(vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)))
> +               vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
> +
>         trace_kvm_exit(vmx->exit_reason, vcpu, KVM_ISA_VMX);
> 
>         if (unlikely(vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
>                 return EXIT_FASTPATH_NONE;
> 
>         vmx->loaded_vmcs->launched = 1;
> -       vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
> 
>         vmx_recover_nmi_blocking(vmx);
>         vmx_complete_interrupts(vmx);
> 
> 
> > +
> >  	if (unlikely((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY))
> >  		kvm_machine_check();
> >  
> > @@ -6780,7 +6782,6 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
> >  		return EXIT_FASTPATH_NONE;
> >  
> >  	vmx->loaded_vmcs->launched = 1;
> > -	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
> >  
> >  	vmx_recover_nmi_blocking(vmx);
> >  	vmx_complete_interrupts(vmx);
> > -- 
> > 2.26.2
> > 

Re: [PATCH v2 0/3] VMX: more nested fixes

[Paolo Bonzini]

On 14/01/21 21:54, Maxim Levitsky wrote:
> This is hopefully the last fix for VMX nested migration
> that finally allows my stress test of migration with a nested guest to pass.
> 
> In a nutshell after an optimization that was done in commit 7952d769c29ca,
> some of vmcs02 fields which can be modified by the L2 freely while it runs
> (like GSBASE and such) were not copied back to vmcs12 unless:
> 
> 1. L1 tries to vmread them (update done on intercept)
> 2. vmclear or vmldptr on other vmcs are done.
> 3. nested state is read and nested guest is running.
> 
> What wasn't done was to sync these 'rare' fields when L1 is running
> but still has a loaded vmcs12 which might have some stale fields,
> if that vmcs was used to enter a guest already due to that optimization.
> 
> Plus I added two minor patches to improve VMX tracepoints
> a bit. There is still a large room for improvement.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (3):
>    KVM: nVMX: Always call sync_vmcs02_to_vmcs12_rare on migration
>    KVM: nVMX: add kvm_nested_vmlaunch_resume tracepoint
>    KVM: VMX: read idt_vectoring_info a bit earlier
> 
>   arch/x86/kvm/trace.h      | 30 ++++++++++++++++++++++++++++++
>   arch/x86/kvm/vmx/nested.c | 19 ++++++++++++++-----
>   arch/x86/kvm/vmx/vmx.c    |  3 ++-
>   arch/x86/kvm/x86.c        |  1 +
>   4 files changed, 47 insertions(+), 6 deletions(-)
> 

I committed patch 1 since I need to send it out to Linus quite soonish, 
but please adjust and resend the others based on Sean's review.

Paolo