* [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true
@ 2021-02-18 16:28 Paolo Bonzini
2021-02-18 17:42 ` Sean Christopherson
2021-02-22 15:25 ` Vitaly Kuznetsov
0 siblings, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-02-18 16:28 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: jroedel, seanjc, mlevitsk
Right now, enter_svm_guest_mode is calling nested_prepare_vmcb_save and
nested_prepare_vmcb_control. This results in is_guest_mode being false
until the end of nested_prepare_vmcb_control.
This is a problem because nested_prepare_vmcb_save can in turn cause
changes to the intercepts and these have to be applied to the "host VMCB"
(stored in svm->nested.hsave) and then merged with the VMCB12 intercepts
into svm->vmcb.
In particular, without this change we forget to set the CR0 read and CR0
write intercepts when running a real mode L2 guest with NPT disabled.
The guest is therefore able to see the CR0.PG bit that KVM sets to
enable "paged real mode". This patch fixes the svm.flat mode_switch
test case with npt=0. There are no other problematic calls in
nested_prepare_vmcb_save.
The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
state only when guest state is complete", 2010-04-25). Unfortunately,
it is not clear from the commit message what issue exactly led to the
change back then. It was probably related to svm_set_cr0 however because
the patch series cover letter[1] mentioned lazy FPU switching.
[1] https://lore.kernel.org/kvm/1266493115-28386-1-git-send-email-joerg.roedel@amd.com/
Signed-off-by: Paolo Bonzini <pbonzini@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 92d3aaaac612..35891d9a1099 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -469,8 +469,8 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb12_gpa,
svm->nested.vmcb12_gpa = vmcb12_gpa;
load_nested_vmcb_control(svm, &vmcb12->control);
- nested_prepare_vmcb_save(svm, vmcb12);
nested_prepare_vmcb_control(svm);
+ nested_prepare_vmcb_save(svm, vmcb12);
ret = nested_svm_load_cr3(&svm->vcpu, vmcb12->save.cr3,
nested_npt_enabled(svm));
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true
2021-02-18 16:28 [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true Paolo Bonzini
@ 2021-02-18 17:42 ` Sean Christopherson
2021-02-18 18:00 ` Paolo Bonzini
2021-02-22 15:25 ` Vitaly Kuznetsov
1 sibling, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2021-02-18 17:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, jroedel, mlevitsk
On Thu, Feb 18, 2021, Paolo Bonzini wrote:
> Right now, enter_svm_guest_mode is calling nested_prepare_vmcb_save and
> nested_prepare_vmcb_control. This results in is_guest_mode being false
> until the end of nested_prepare_vmcb_control.
>
> This is a problem because nested_prepare_vmcb_save can in turn cause
> changes to the intercepts and these have to be applied to the "host VMCB"
> (stored in svm->nested.hsave) and then merged with the VMCB12 intercepts
> into svm->vmcb.
>
> In particular, without this change we forget to set the CR0 read and CR0
> write intercepts when running a real mode L2 guest with NPT disabled.
> The guest is therefore able to see the CR0.PG bit that KVM sets to
> enable "paged real mode". This patch fixes the svm.flat mode_switch
> test case with npt=0. There are no other problematic calls in
> nested_prepare_vmcb_save.
It might be worth explicitly pointing out that get_host_vmcb() in
svm_clr_intercept() and svm_set_intercept() will grab the wrong VMCB.
> The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
> state only when guest state is complete", 2010-04-25). Unfortunately,
> it is not clear from the commit message what issue exactly led to the
> change back then. It was probably related to svm_set_cr0 however because
> the patch series cover letter[1] mentioned lazy FPU switching.
Aha! It was indeed related to svm_set_cr0(). Specifically, the next patch,
commit 66a562f7e257 ("KVM: SVM: Make lazy FPU switching work with nested svm"),
added is_nested() checks in update_cr0_intercept() to merge L1's intercepts with
L0's intercepts.
I dug through all other is_nested() usage, none of them were reachable via the
world switch logic.
> [1] https://lore.kernel.org/kvm/1266493115-28386-1-git-send-email-joerg.roedel@amd.com/
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Sean Christopherson <seanjc@google.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 92d3aaaac612..35891d9a1099 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -469,8 +469,8 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb12_gpa,
>
> svm->nested.vmcb12_gpa = vmcb12_gpa;
> load_nested_vmcb_control(svm, &vmcb12->control);
> - nested_prepare_vmcb_save(svm, vmcb12);
> nested_prepare_vmcb_control(svm);
> + nested_prepare_vmcb_save(svm, vmcb12);
>
> ret = nested_svm_load_cr3(&svm->vcpu, vmcb12->save.cr3,
> nested_npt_enabled(svm));
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true
2021-02-18 17:42 ` Sean Christopherson
@ 2021-02-18 18:00 ` Paolo Bonzini
2021-02-18 18:12 ` Sean Christopherson
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2021-02-18 18:00 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-kernel, kvm, jroedel, mlevitsk
On 18/02/21 18:42, Sean Christopherson wrote:
>> The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
>> state only when guest state is complete", 2010-04-25). Unfortunately,
>> it is not clear from the commit message what issue exactly led to the
>> change back then. It was probably related to svm_set_cr0 however because
>> the patch series cover letter[1] mentioned lazy FPU switching.
>
> Aha! It was indeed related to svm_set_cr0(). Specifically, the next patch,
> commit 66a562f7e257 ("KVM: SVM: Make lazy FPU switching work with nested svm"),
> added is_nested() checks in update_cr0_intercept() to merge L1's intercepts with
> L0's intercepts.
Yeah, the problem is I don't understand why 06fc7772690d fixed things in
11 year old KVM instead of breaking them, because effectively this patch
is reverting it.
I don't care _that_ much because so much has changed since then; the
world switch logic is abstracted better nowadays, and it is easier to
review the change. But it is weird, nevertheless.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true
2021-02-18 18:00 ` Paolo Bonzini
@ 2021-02-18 18:12 ` Sean Christopherson
2021-02-18 18:28 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2021-02-18 18:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, jroedel, mlevitsk
On Thu, Feb 18, 2021, Paolo Bonzini wrote:
> On 18/02/21 18:42, Sean Christopherson wrote:
> > > The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
> > > state only when guest state is complete", 2010-04-25). Unfortunately,
> > > it is not clear from the commit message what issue exactly led to the
> > > change back then. It was probably related to svm_set_cr0 however because
> > > the patch series cover letter[1] mentioned lazy FPU switching.
> >
> > Aha! It was indeed related to svm_set_cr0(). Specifically, the next patch,
> > commit 66a562f7e257 ("KVM: SVM: Make lazy FPU switching work with nested svm"),
> > added is_nested() checks in update_cr0_intercept() to merge L1's intercepts with
> > L0's intercepts.
>
> Yeah, the problem is I don't understand why 06fc7772690d fixed things in 11
> year old KVM instead of breaking them, because effectively this patch is
> reverting it.
11 year old KVM didn't grab a different VMCB when updating the intercepts, it
had already copied/merged L1's stuff to L0's VMCB, and then updated L0's VMCB
regardless of is_nested().
> I don't care _that_ much because so much has changed since then; the world
> switch logic is abstracted better nowadays, and it is easier to review the
> change. But it is weird, nevertheless.
>
> Paolo
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true
2021-02-18 18:12 ` Sean Christopherson
@ 2021-02-18 18:28 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-02-18 18:28 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-kernel, kvm, jroedel, mlevitsk
On 18/02/21 19:12, Sean Christopherson wrote:
>> Yeah, the problem is I don't understand why 06fc7772690d fixed things in 11
>> year old KVM instead of breaking them, because effectively this patch is
>> reverting it.
>
> 11 year old KVM didn't grab a different VMCB when updating the intercepts, it
> had already copied/merged L1's stuff to L0's VMCB, and then updated L0's VMCB
> regardless of is_nested().
Ok, so the bug was introduced when adding recalc_intercepts, which threw
away the intercept manipulations that svm_set_cr0 had done in the
meanwhile. That's commit 384c63684397 ("KVM: SVM: Add function to
recalculate intercept masks", 2011-01-12).
That piece of information makes me feel less uneasy about missing
something. recalc_intercepts has been there for a long time, but not as
long as 06fc7772690d.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true
2021-02-18 16:28 [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true Paolo Bonzini
2021-02-18 17:42 ` Sean Christopherson
@ 2021-02-22 15:25 ` Vitaly Kuznetsov
1 sibling, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2021-02-22 15:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: jroedel, seanjc, mlevitsk, linux-kernel, kvm
Paolo Bonzini <pbonzini@redhat.com> writes:
> Right now, enter_svm_guest_mode is calling nested_prepare_vmcb_save and
> nested_prepare_vmcb_control. This results in is_guest_mode being false
> until the end of nested_prepare_vmcb_control.
>
> This is a problem because nested_prepare_vmcb_save can in turn cause
> changes to the intercepts and these have to be applied to the "host VMCB"
> (stored in svm->nested.hsave) and then merged with the VMCB12 intercepts
> into svm->vmcb.
>
> In particular, without this change we forget to set the CR0 read and CR0
> write intercepts when running a real mode L2 guest with NPT disabled.
> The guest is therefore able to see the CR0.PG bit that KVM sets to
> enable "paged real mode". This patch fixes the svm.flat mode_switch
> test case with npt=0. There are no other problematic calls in
> nested_prepare_vmcb_save.
>
> The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
> state only when guest state is complete", 2010-04-25). Unfortunately,
> it is not clear from the commit message what issue exactly led to the
> change back then. It was probably related to svm_set_cr0 however because
> the patch series cover letter[1] mentioned lazy FPU switching.
>
> [1] https://lore.kernel.org/kvm/1266493115-28386-1-git-send-email-joerg.roedel@amd.com/
>
> Signed-off-by: Paolo Bonzini <pbonzini@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 92d3aaaac612..35891d9a1099 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -469,8 +469,8 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb12_gpa,
>
> svm->nested.vmcb12_gpa = vmcb12_gpa;
> load_nested_vmcb_control(svm, &vmcb12->control);
> - nested_prepare_vmcb_save(svm, vmcb12);
> nested_prepare_vmcb_control(svm);
> + nested_prepare_vmcb_save(svm, vmcb12);
>
> ret = nested_svm_load_cr3(&svm->vcpu, vmcb12->save.cr3,
> nested_npt_enabled(svm));
For the record,
this seems to fix the bug when Gen2 guests were not booting on Hyper-V
on KVM (SVM). They boot now, thanks!
Tested-by: Vitaly Kuznetsov <vkuznets@redhat.com>
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-22 15:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-18 16:28 [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true Paolo Bonzini
2021-02-18 17:42 ` Sean Christopherson
2021-02-18 18:00 ` Paolo Bonzini
2021-02-18 18:12 ` Sean Christopherson
2021-02-18 18:28 ` Paolo Bonzini
2021-02-22 15:25 ` Vitaly Kuznetsov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.