[PATCH 0/5] KVM: nSVM: few fixes for the nested migration

[PATCH 0/5] KVM: nSVM: few fixes for the nested migration

[Maxim Levitsky]

Those are few fixes for issues I uncovered by doing variants of a
synthetic migration test I just created:

I modified the qemu, such that on each vm pause/resume cycle,
just prior to resuming a vCPU, qemu reads its KVM state,
then (optionaly) resets this state by uploading a
dummy reset state to KVM, and then it uploads back to KVM,
the state that this vCPU had before.

I'll try to make this test upstreamable soon, pending few details
I need to figure out.

Last patch in this series is for false positive warning
that I have seen lately when setting the nested state,
in nested_svm_vmexit, where it expects the vmcb01 to have
VMRUN vmexit, which is not true after nested migration,
as it is not fully initialized.
If you prefer the warning can be removed instead.

Best regards,
	Maxim Levitsky

Maxim Levitsky (5):
  KVM: nSVM: fix a typo in svm_leave_nested
  KVM: nSVM: fix few bugs in the vmcb02 caching logic
  KVM: nSVM: leave the guest mode prior to loading a nested state
  KVM: nSVM: force L1's GIF to 1 when setting the nested state
  KVM: nSVM: set a dummy exit reason in L1 vmcb when loading the nested
    state

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/svm/nested.c       | 29 ++++++++++++++++++++++++++---
 arch/x86/kvm/svm/svm.c          |  4 ++--
 3 files changed, 29 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH 1/5] KVM: nSVM: fix a typo in svm_leave_nested

[Maxim Levitsky]

When forcibly leaving the nested mode, we should switch to vmcb01

Fixes: 4995a3685f1b ("KVM: SVM: Use a separate vmcb for the nested L2 guest")

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 540d43ba2cf4..3321220f3deb 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -886,7 +886,7 @@ void svm_leave_nested(struct vcpu_svm *svm)
 		svm->nested.nested_run_pending = 0;
 		leave_guest_mode(vcpu);
 
-		svm_switch_vmcb(svm, &svm->nested.vmcb02);
+		svm_switch_vmcb(svm, &svm->vmcb01);
 
 		nested_svm_uninit_mmu_context(vcpu);
 		vmcb_mark_all_dirty(svm->vmcb);
-- 
2.26.2

[PATCH 2/5] KVM: nSVM: fix few bugs in the vmcb02 caching logic

[Maxim Levitsky]

* Define and use an invalid GPA (all ones) for init value of last
  and current nested vmcb physical addresses.

* Reset the current vmcb12 gpa to the invalid value when leaving
  the nested mode, similar to what is done on nested vmexit.

* Reset	the last seen vmcb12 address when disabling the nested SVM,
  as it relies on vmcb02 fields which are freed at that point.

Fixes: 4995a3685f1b ("KVM: SVM: Use a separate vmcb for the nested L2 guest")

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/svm/nested.c       | 11 +++++++++++
 arch/x86/kvm/svm/svm.c          |  4 ++--
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3e5fc80a35c8..4b95e7c4a4e3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -113,6 +113,7 @@
 #define VALID_PAGE(x) ((x) != INVALID_PAGE)
 
 #define UNMAPPED_GVA (~(gpa_t)0)
+#define INVALID_GPA (~(gpa_t)0)
 
 /* KVM Hugepage definitions for x86 */
 #define KVM_MAX_HUGEPAGE_LEVEL	PG_LEVEL_1G
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 3321220f3deb..a88c64e004c3 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -872,6 +872,15 @@ void svm_free_nested(struct vcpu_svm *svm)
 	__free_page(virt_to_page(svm->nested.vmcb02.ptr));
 	svm->nested.vmcb02.ptr = NULL;
 
+	/*
+	 * When last_vmcb12_gpa matches the current vmcb12 gpa,
+	 * some vmcb12 fields are not loaded if they are marked clean
+	 * in the vmcb12, since in this case they are up to date already.
+	 *
+	 * When the vmcb02 is freed, this optimization becomes invalid.
+	 */
+	svm->nested.last_vmcb12_gpa = INVALID_GPA;
+
 	svm->nested.initialized = false;
 }
 
@@ -884,6 +893,8 @@ void svm_leave_nested(struct vcpu_svm *svm)
 
 	if (is_guest_mode(vcpu)) {
 		svm->nested.nested_run_pending = 0;
+		svm->nested.vmcb12_gpa = INVALID_GPA;
+
 		leave_guest_mode(vcpu);
 
 		svm_switch_vmcb(svm, &svm->vmcb01);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index a7271f31df47..987173e1f954 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1235,8 +1235,8 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
 	svm->current_vmcb->asid_generation = 0;
 	svm->asid = 0;
 
-	svm->nested.vmcb12_gpa = 0;
-	svm->nested.last_vmcb12_gpa = 0;
+	svm->nested.vmcb12_gpa = INVALID_GPA;
+	svm->nested.last_vmcb12_gpa = INVALID_GPA;
 	vcpu->arch.hflags = 0;
 
 	if (!kvm_pause_in_guest(vcpu->kvm)) {
-- 
2.26.2

[PATCH 3/5] KVM: nSVM: leave the guest mode prior to loading a nested state

[Maxim Levitsky]

This allows the KVM to load the nested state more than
once without warnings.

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index a88c64e004c3..32400cba608d 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1309,12 +1309,15 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	 * L2 registers if needed are moved from the current VMCB to VMCB02.
 	 */
 
+	if (is_guest_mode(vcpu))
+		svm_leave_nested(svm);
+	else
+		svm->nested.vmcb02.ptr->save = svm->vmcb01.ptr->save;
+
 	svm->nested.nested_run_pending =
 		!!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING);
 
 	svm->nested.vmcb12_gpa = kvm_state->hdr.svm.vmcb_pa;
-	if (svm->current_vmcb == &svm->vmcb01)
-		svm->nested.vmcb02.ptr->save = svm->vmcb01.ptr->save;
 
 	svm->vmcb01.ptr->save.es = save->es;
 	svm->vmcb01.ptr->save.cs = save->cs;
-- 
2.26.2

[PATCH 4/5] KVM: nSVM: force L1's GIF to 1 when setting the nested state

[Maxim Levitsky]

While after a reset the GIF value is already 1,
it doesn't have to have this value if the nested state
is loaded later.

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 32400cba608d..12a12ae940fa 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1314,6 +1314,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	else
 		svm->nested.vmcb02.ptr->save = svm->vmcb01.ptr->save;
 
+	/* Force L1's GIF to true */
+	svm_set_gif(svm, true);
+
 	svm->nested.nested_run_pending =
 		!!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING);
 
-- 
2.26.2

[PATCH 5/5] KVM: nSVM: set a dummy exit reason in L1 vmcb when loading the nested state

[Maxim Levitsky]

Since the nested migration is a result of a VMRUN, this makes it
possible to keep a warning that L1 vmcb should always have
VMRUN exit reason when switching back to it, which
otherwise triggers incorrectly.

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 12a12ae940fa..146be4b5084b 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1338,6 +1338,12 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	svm->vmcb01.ptr->save.rip = save->rip;
 	svm->vmcb01.ptr->save.cpl = 0;
 
+	/*
+	 * For consistency sake, restore the L1 exit reason
+	 * (that happened prior to the migration) to SVM_EXIT_VMRUN.
+	 */
+	svm->vmcb->control.exit_code = SVM_EXIT_VMRUN;
+
 	nested_load_control_from_vmcb12(svm, ctl);
 
 	svm_switch_vmcb(svm, &svm->nested.vmcb02);
-- 
2.26.2

Re: [PATCH 4/5] KVM: nSVM: force L1's GIF to 1 when setting the nested state

[Paolo Bonzini]

On 03/05/21 14:54, Maxim Levitsky wrote:
> While after a reset the GIF value is already 1,
> it doesn't have to have this value if the nested state
> is loaded later.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>   arch/x86/kvm/svm/nested.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 32400cba608d..12a12ae940fa 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1314,6 +1314,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>   	else
>   		svm->nested.vmcb02.ptr->save = svm->vmcb01.ptr->save;
>   
> +	/* Force L1's GIF to true */
> +	svm_set_gif(svm, true);
> +
>   	svm->nested.nested_run_pending =
>   		!!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING);
>   
> 

Hmm, not sure about this one.  It is possible in principle to do CLGI in 
L2 with the intercept disabled.

You need to use

svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));

instead.

Paolo

Re: [PATCH 5/5] KVM: nSVM: set a dummy exit reason in L1 vmcb when loading the nested state

[Paolo Bonzini]

On 03/05/21 14:54, Maxim Levitsky wrote:
> Since the nested migration is a result of a VMRUN, this makes it
> possible to keep a warning that L1 vmcb should always have
> VMRUN exit reason when switching back to it, which
> otherwise triggers incorrectly.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

Does this fix "KVM: nSVM: If VMRUN is single-stepped, queue the #DB 
intercept in nested_svm_vmexit()"?  I don't like this, and also vmcb12 
is not initialized here (nested_load_control_from_vmcb12 is using the 
state passed in from userspace instead).

I think you should just remove the WARN instead.

Paolo

> ---
>   arch/x86/kvm/svm/nested.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 12a12ae940fa..146be4b5084b 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1338,6 +1338,12 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>   	svm->vmcb01.ptr->save.rip = save->rip;
>   	svm->vmcb01.ptr->save.cpl = 0;
>   
> +	/*
> +	 * For consistency sake, restore the L1 exit reason
> +	 * (that happened prior to the migration) to SVM_EXIT_VMRUN.
> +	 */
> +	svm->vmcb->control.exit_code = SVM_EXIT_VMRUN;
> +
>   	nested_load_control_from_vmcb12(svm, ctl);
>   
>   	svm_switch_vmcb(svm, &svm->nested.vmcb02);
> 

Re: [PATCH 0/5] KVM: nSVM: few fixes for the nested migration

[Paolo Bonzini]

On 03/05/21 14:54, Maxim Levitsky wrote:
> Those are few fixes for issues I uncovered by doing variants of a
> synthetic migration test I just created:
> 
> I modified the qemu, such that on each vm pause/resume cycle,
> just prior to resuming a vCPU, qemu reads its KVM state,
> then (optionaly) resets this state by uploading a
> dummy reset state to KVM, and then it uploads back to KVM,
> the state that this vCPU had before.
> 
> I'll try to make this test upstreamable soon, pending few details
> I need to figure out.
> 
> Last patch in this series is for false positive warning
> that I have seen lately when setting the nested state,
> in nested_svm_vmexit, where it expects the vmcb01 to have
> VMRUN vmexit, which is not true after nested migration,
> as it is not fully initialized.
> If you prefer the warning can be removed instead.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (5):
>    KVM: nSVM: fix a typo in svm_leave_nested
>    KVM: nSVM: fix few bugs in the vmcb02 caching logic
>    KVM: nSVM: leave the guest mode prior to loading a nested state
>    KVM: nSVM: force L1's GIF to 1 when setting the nested state
>    KVM: nSVM: set a dummy exit reason in L1 vmcb when loading the nested
>      state
> 
>   arch/x86/include/asm/kvm_host.h |  1 +
>   arch/x86/kvm/svm/nested.c       | 29 ++++++++++++++++++++++++++---
>   arch/x86/kvm/svm/svm.c          |  4 ++--
>   3 files changed, 29 insertions(+), 5 deletions(-)
> 

Queued patches 1-3.

Paolo

Re: [PATCH 4/5] KVM: nSVM: force L1's GIF to 1 when setting the nested state

[Maxim Levitsky]

On Mon, 2021-05-03 at 16:00 +0200, Paolo Bonzini wrote:
> On 03/05/21 14:54, Maxim Levitsky wrote:
> > While after a reset the GIF value is already 1,
> > it doesn't have to have this value if the nested state
> > is loaded later.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >   arch/x86/kvm/svm/nested.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 32400cba608d..12a12ae940fa 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -1314,6 +1314,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> >   	else
> >   		svm->nested.vmcb02.ptr->save = svm->vmcb01.ptr->save;
> >   
> > +	/* Force L1's GIF to true */
> > +	svm_set_gif(svm, true);
> > +
> >   	svm->nested.nested_run_pending =
> >   		!!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING);
> >   
> > 
> 
> Hmm, not sure about this one.  It is possible in principle to do CLGI in 
> L2 with the intercept disabled.

I need to think about this a bit more. 
In theory we have L0 GIF, the L1 GIF and the L2 GIF.
L0 GIF is always KVM's, so no problem.
L1 GIF can be toggled with L1 executing clgi/stgi, and it will be either stored in 
vmcb.int_ctl (vmcb01 or vmcb02) or in hflags depending if vGIF is enabled.
(the L1 owned bits are copied in nested_vmcb02_prepare_control)

For L2 we never advertise virtual gif and we don't let it set V_GIF_ENABLE_MASK
in int_ctl, so it either intercepts clgi/stgi and does its own businesses with it
or it doesn't intercept it in which case L2 indeed just modifies L1 GIF.


> 
> You need to use
> 
> svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));


Assuming that the above is correct, then indeed, this should be done,
so I'll send a patch for this.
Thanks a lot!!

Best regards,
	Maxim Levitsky



> 
> instead.
> 
> Paolo
> 

Re: [PATCH 5/5] KVM: nSVM: set a dummy exit reason in L1 vmcb when loading the nested state

[Maxim Levitsky]

On Mon, 2021-05-03 at 16:02 +0200, Paolo Bonzini wrote:
> On 03/05/21 14:54, Maxim Levitsky wrote:
> > Since the nested migration is a result of a VMRUN, this makes it
> > possible to keep a warning that L1 vmcb should always have
> > VMRUN exit reason when switching back to it, which
> > otherwise triggers incorrectly.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> 
> Does this fix "KVM: nSVM: If VMRUN is single-stepped, queue the #DB 
> intercept in nested_svm_vmexit()"?  I don't like this, and also vmcb12 
> is not initialized here (nested_load_control_from_vmcb12 is using the 
> state passed in from userspace instead).

> 
> I think you should just remove the WARN instead.
All right, I'll do this!

Best regards,
	Maxim Levitsky


> 
> Paolo
> 
> > ---
> >   arch/x86/kvm/svm/nested.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 12a12ae940fa..146be4b5084b 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -1338,6 +1338,12 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> >   	svm->vmcb01.ptr->save.rip = save->rip;
> >   	svm->vmcb01.ptr->save.cpl = 0;
> >   
> > +	/*
> > +	 * For consistency sake, restore the L1 exit reason
> > +	 * (that happened prior to the migration) to SVM_EXIT_VMRUN.
> > +	 */
> > +	svm->vmcb->control.exit_code = SVM_EXIT_VMRUN;
> > +
> >   	nested_load_control_from_vmcb12(svm, ctl);
> >   
> >   	svm_switch_vmcb(svm, &svm->nested.vmcb02);
> >