KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state
PATCH 1/1 KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state
From: Jan Kiszka <[email protected]>
Likely a typo, but a fatal one as kvm_set_cr0 performs checks on the state transition that may prevent loading L1's cr0.
--- arch/x86/kvm/vmx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kvm/vmx.c +1 -1
@@ -7429,7 +7429,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,* fpu_active (which may have changed). * Note that vmx_set_cr0 refers to efer set above. */ -kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);/* * If we did fpu_activate()/fpu_deactivate() during L2's run, we need * to apply the same changes to L1's vmcs. We just set cr0 correctly,
Signature · 1 line
1.7.3.4
On Sat, Feb 23, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state":
arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7432
Quoted 2 lines
arch/x86/kvm/vmx.c +1
-1-kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);
I don't remember now why I did this (and I'm not looking at the code), but this you'll need to really test carefully, including shadow-on-shadow mode (ept=0 in L0), to verify you're not missing any important side-effect of kvm_set_cr0.
Also, if I remember correctly, during nVMX's review, Avi Kivity asked in several places that when I called vmx_set_cr0, I should instead call kvm_set_cr0(), because it does some extra stuff and does some extra checks. Hmm, see, see this:
http://markmail.org/message/hhidqyhbo2mrgxxc
where Avi asked for the reverse patch you're attempting now.
Signature · 4 lines
Nadav Har'El | Saturday, Feb 23 2013, 14 Adar 5773 [email protected] |----------------------------------------- Phone +972-523-790466, ICQ 13349191 |It is better to be thought a fool, then http://nadav.harel.org.il |to open your mouth and remove all doubt.
Quoted 9 lines · Nadav Har'El
On Sat, Feb 23, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Replace
kvm_set_cr0withvmx_set_cr0inload_vmcs12_host_state":arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7432
Quoted 2 lines
arch/x86/kvm/vmx.c +1
-1-kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);I don't remember now why I did this (and I'm not looking at the code), but this you'll need to really test carefully, including shadow-on-shadow mode (ept=0 in L0), to verify you're not missing any important side-effect of
kvm_set_cr0.
Quoted 7 lines
Also, if I remember correctly, during nVMX's review, Avi Kivity asked in several places that when I called
vmx_set_cr0, I should instead callkvm_set_cr0(), because it does some extra stuff and does some extra checks. Hmm, see, see this:
http://markmail.org/message/hhidqyhbo2mrgxxcwhere Avi asked for the reverse patch you're attempting now.
At least, kvm_set_cr0 can't be used as it assumes an otherwise consistent guest state and an explicitly initiated transition - which is naturally not the case while emulating a vmexit.
However, there are some side effects that need a closer look, likely also in other places where vmx_set_cr0 is used directly. E.g. load_pdptrs. The lack of calling them on host->guest switch may explain the PAE issues you once saw with your nEPT implementation.
OK, more work...
Jan
Quoted 22 lines · Jan Kiszka
Quoted 9 lines · Nadav Har'El
On Sat, Feb 23, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Replace
kvm_set_cr0withvmx_set_cr0inload_vmcs12_host_state":arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7432
Quoted 2 lines
arch/x86/kvm/vmx.c +1
-1-kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);I don't remember now why I did this (and I'm not looking at the code), but this you'll need to really test carefully, including shadow-on-shadow mode (ept=0 in L0), to verify you're not missing any important side-effect of
kvm_set_cr0.Quoted 7 lines
Also, if I remember correctly, during nVMX's review, Avi Kivity asked in several places that when I called
vmx_set_cr0, I should instead callkvm_set_cr0(), because it does some extra stuff and does some extra checks. Hmm, see, see this:
http://markmail.org/message/hhidqyhbo2mrgxxcwhere Avi asked for the reverse patch you're attempting now.
At least,
kvm_set_cr0can't be used as it assumes an otherwise consistent guest state and an explicitly initiated transition - which is naturally not the case while emulating a vmexit.
Quoted 6 lines
However, there are some side effects that need a closer look, likely also in other places where
vmx_set_cr0is used directly. E.g.load_pdptrs. The lack of calling them onhost->guestswitch may explain the PAE issues you once saw with your nEPT implementation.OK, more work...
Checked again, and the patch should be fine: load_pdptrs is executed by kvm_set_cr3 later in this functions (same for prepare_vmcs02, so no problem for PAE here), kvm_mmu_reset_context is even run explicitly and clearing the async-pf queue makes no sense when leaving the guest (the guest can't use it unless L1 is broken and passes this through). However, we should clear the queue when leaving the host - separate patch.
Jan
Quoted 28 lines · Jan Kiszka
Quoted 27 lines · Jan Kiszka
Quoted 9 lines · Nadav Har'El
On Sat, Feb 23, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Replace
kvm_set_cr0withvmx_set_cr0inload_vmcs12_host_state":arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7432
Quoted 2 lines
arch/x86/kvm/vmx.c +1
-1-kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);I don't remember now why I did this (and I'm not looking at the code), but this you'll need to really test carefully, including shadow-on-shadow mode (ept=0 in L0), to verify you're not missing any important side-effect of
kvm_set_cr0.Quoted 7 lines
Also, if I remember correctly, during nVMX's review, Avi Kivity asked in several places that when I called
vmx_set_cr0, I should instead callkvm_set_cr0(), because it does some extra stuff and does some extra checks. Hmm, see, see this:
http://markmail.org/message/hhidqyhbo2mrgxxcwhere Avi asked for the reverse patch you're attempting now.
At least,
kvm_set_cr0can't be used as it assumes an otherwise consistent guest state and an explicitly initiated transition - which is naturally not the case while emulating a vmexit.However, there are some side effects that need a closer look, likely also in other places where
vmx_set_cr0is used directly. E.g.load_pdptrs. The lack of calling them onhost->guestswitch may explain the PAE issues you once saw with your nEPT implementation.
Quoted 8 lines
Quoted 1 line
OK, more work...
Checked again, and the patch should be fine:
load_pdptrsis executed bykvm_set_cr3later in this functions (same forprepare_vmcs02, so no problem for PAE here),kvm_mmu_reset_contextis even run explicitly and clearing the async-pf queue makes no sense when leaving the guest (the guest can't use it unless L1 is broken and passes this through). However, we should clear the queue when leaving the host - separate patch.
Thought about the async-pf thing again: It rather looks to me like the host does not have to worry about it at all. It's L1's job to either disable async-pf before entering L2 or trap PFs and handle paravirtual reasons before they hit L2. I don't see that KVM already does this, but I don't think that matters at this point.
Jan
Quoted 16 lines · Jan Kiszka
Quoted 15 lines · Nadav Har'El
On Sat, Feb 23, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Replace
kvm_set_cr0withvmx_set_cr0inload_vmcs12_host_state":arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7432
Quoted 2 lines
arch/x86/kvm/vmx.c +1
-1-kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);I don't remember now why I did this (and I'm not looking at the code), but this you'll need to really test carefully, including shadow-on-shadow mode (ept=0 in L0), to verify you're not missing any important side-effect of
kvm_set_cr0.Also, if I remember correctly, during nVMX's review, Avi Kivity asked in several places that when I called
vmx_set_cr0, I should instead callkvm_set_cr0(), because it does some extra stuff and does some extra checks. Hmm, see, see this:
http://markmail.org/message/hhidqyhbo2mrgxxc
Quoted 5 lines
Quoted 1 line
where Avi asked for the reverse patch you're attempting now.
At least,
kvm_set_cr0can't be used as it assumes an otherwise consistent guest state and an explicitly initiated transition - which is naturally not the case while emulating a vmexit.
We have the same problem in KVM_SET_SREGS.
Quoted 17 lines · Avi Kivity
Quoted 16 lines · Jan Kiszka
Quoted 15 lines · Nadav Har'El
On Sat, Feb 23, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Replace
kvm_set_cr0withvmx_set_cr0inload_vmcs12_host_state":arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7432
Quoted 2 lines
arch/x86/kvm/vmx.c +1
-1-kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);I don't remember now why I did this (and I'm not looking at the code), but this you'll need to really test carefully, including shadow-on-shadow mode (ept=0 in L0), to verify you're not missing any important side-effect of
kvm_set_cr0.Also, if I remember correctly, during nVMX's review, Avi Kivity asked in several places that when I called
vmx_set_cr0, I should instead callkvm_set_cr0(), because it does some extra stuff and does some extra checks. Hmm, see, see this:
http://markmail.org/message/hhidqyhbo2mrgxxc
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
where Avi asked for the reverse patch you're attempting now.
At least,
kvm_set_cr0can't be used as it assumes an otherwise consistent guest state and an explicitly initiated transition - which is naturally not the case while emulating a vmexit.We have the same problem in
KVM_SET_SREGS.
I don't see the problem. kvm_arch_vcpu_ioctl_set_sregs open-codes the state update, not applying any transition checks.
Jan
Quoted 20 lines · Jan Kiszka
Quoted 19 lines · Avi Kivity
Quoted 18 lines · Jan Kiszka
Quoted 16 lines · Nadav Har'El
On Sat, Feb 23, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Replace
kvm_set_cr0withvmx_set_cr0inload_vmcs12_host_state":arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7432
Quoted 2 lines
arch/x86/kvm/vmx.c +1
-1-kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);I don't remember now why I did this (and I'm not looking at the code), but this you'll need to really test carefully, including shadow-on-shadow mode (ept=0 in L0), to verify you're not missing any important side-effect of
kvm_set_cr0.Also, if I remember correctly, during nVMX's review, Avi Kivity asked in several places that when I called
vmx_set_cr0, I should instead callkvm_set_cr0(), because it does some extra stuff and does some extra checks. Hmm, see, see this:
http://markmail.org/message/hhidqyhbo2mrgxxcwhere Avi asked for the reverse patch you're attempting now.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
At least,
kvm_set_cr0can't be used as it assumes an otherwise consistent guest state and an explicitly initiated transition - which is naturally not the case while emulating a vmexit.We have the same problem in
KVM_SET_SREGS.I don't see the problem.
kvm_arch_vcpu_ioctl_set_sregsopen-codes the state update, not applying any transition checks.
That's the problem. We have this open coding in three different places (KVM_SET_SREGS, nvmx, nsvm).
It's not as if vmx_set_cr0() is defined as "kvm_set_cr0() without the transition checks".
Quoted 27 lines · Avi Kivity
Quoted 26 lines · Jan Kiszka
Quoted 24 lines · Avi Kivity
Quoted 16 lines · Jan Kiszka
Quoted 15 lines · Nadav Har'El
On Sat, Feb 23, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Replace
kvm_set_cr0withvmx_set_cr0inload_vmcs12_host_state":arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7432
Quoted 2 lines
arch/x86/kvm/vmx.c +1
-1-kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);I don't remember now why I did this (and I'm not looking at the code), but this you'll need to really test carefully, including shadow-on-shadow mode (ept=0 in L0), to verify you're not missing any important side-effect of
kvm_set_cr0.Also, if I remember correctly, during nVMX's review, Avi Kivity asked in several places that when I called
vmx_set_cr0, I should instead callkvm_set_cr0(), because it does some extra stuff and does some extra checks. Hmm, see, see this:
http://markmail.org/message/hhidqyhbo2mrgxxcQuoted 5 lines
Quoted 1 line
where Avi asked for the reverse patch you're attempting now.
At least,
kvm_set_cr0can't be used as it assumes an otherwise consistent guest state and an explicitly initiated transition - which is naturally not the case while emulating a vmexit.We have the same problem in
KVM_SET_SREGS.
Quoted 8 lines
Quoted 2 lines
I don't see the problem.
kvm_arch_vcpu_ioctl_set_sregsopen-codes the state update, not applying any transition checks.That's the problem. We have this open coding in three different places (
KVM_SET_SREGS, nvmx, nsvm).It's not as if
vmx_set_cr0()is defined as "kvm_set_cr0()without the transition checks".
...and without mmu updates. The latter is done via or after the closing cr3 update. Interestingly, nsvm does not perform kvm_set_cr3 on vmexit when in npt mode. Seems things aren't that regular.
Jan
Quoted 8 lines · Jan Kiszka
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
We have the same problem in
KVM_SET_SREGS.I don't see the problem.
kvm_arch_vcpu_ioctl_set_sregsopen-codes the state update, not applying any transition checks.That's the problem. We have this open coding in three different places (
KVM_SET_SREGS, nvmx, nsvm).
Quoted 6 lines
Quoted 2 lines
It's not as if
vmx_set_cr0()is defined as "kvm_set_cr0()without the transition checks"....and without mmu updates. The latter is done via or after the closing cr3 update. Interestingly, nsvm does not perform
kvm_set_cr3on vmexit when in npt mode. Seems things aren't that regular.
We do want the mmu updates. Of course they can't be attached to kvm_set_cr0_without_the_checks() since there is cross-register dependencies.
Option 1 is kvm_set_multiple_cr(). This does the checks and updates, but only after all registers are updated.
Option 2 is kvm_begin_cr_transaction()/kvm_commit_cr_transaction(). Prettier and more flexible, but a more complicated to implement.
Quoted 16 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
We have the same problem in
KVM_SET_SREGS.I don't see the problem.
kvm_arch_vcpu_ioctl_set_sregsopen-codes the state update, not applying any transition checks.That's the problem. We have this open coding in three different places (
KVM_SET_SREGS, nvmx, nsvm).Quoted 6 lines
Quoted 2 lines
It's not as if
vmx_set_cr0()is defined as "kvm_set_cr0()without the transition checks"....and without mmu updates. The latter is done via or after the closing cr3 update. Interestingly, nsvm does not perform
kvm_set_cr3on vmexit when in npt mode. Seems things aren't that regular.
Quoted 9 lines
We do want the mmu updates. Of course they can't be attached to
kvm_set_cr0_without_the_checks()since there is cross-register dependencies.Option 1 is
kvm_set_multiple_cr(). This does the checks and updates, but only after all registers are updated.
Option 2 is kvm_begin_cr_transaction()/kvm_commit_cr_transaction(). Prettier and more flexible, but a more complicated to implement.
The only thing that these three use case truly have in common is the closing kvm_mmu_reset_context. Maybe nvmx and nsvm can share a bit more. But let's get nVMX right first, then think about sharing.
Jan
Quoted 27 lines · Jan Kiszka
Quoted 16 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
We have the same problem in
KVM_SET_SREGS.I don't see the problem.
kvm_arch_vcpu_ioctl_set_sregsopen-codes the state update, not applying any transition checks.That's the problem. We have this open coding in three different places (
KVM_SET_SREGS, nvmx, nsvm).Quoted 6 lines
Quoted 2 lines
It's not as if
vmx_set_cr0()is defined as "kvm_set_cr0()without the transition checks"....and without mmu updates. The latter is done via or after the closing cr3 update. Interestingly, nsvm does not perform
kvm_set_cr3on vmexit when in npt mode. Seems things aren't that regular.Quoted 9 lines
We do want the mmu updates. Of course they can't be attached to
kvm_set_cr0_without_the_checks()since there is cross-register dependencies.Option 1 is
kvm_set_multiple_cr(). This does the checks and updates, but only after all registers are updated.
Option 2 is kvm_begin_cr_transaction()/kvm_commit_cr_transaction(). Prettier and more flexible, but a more complicated to implement.
Quoted 3 lines
The only thing that these three use case truly have in common is the closing
kvm_mmu_reset_context. Maybe nvmx and nsvm can share a bit more. But let's get nVMX right first, then think about sharing.
They all need consistency checks, otherwise userspace or the guest and inject inconsistent values and perhaps exploit the host.
Quoted 28 lines · Avi Kivity
Quoted 27 lines · Jan Kiszka
Quoted 25 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
We have the same problem in
KVM_SET_SREGS.I don't see the problem.
kvm_arch_vcpu_ioctl_set_sregsopen-codes the state update, not applying any transition checks.That's the problem. We have this open coding in three different places (
KVM_SET_SREGS, nvmx, nsvm).Quoted 6 lines
Quoted 2 lines
It's not as if
vmx_set_cr0()is defined as "kvm_set_cr0()without the transition checks"....and without mmu updates. The latter is done via or after the closing cr3 update. Interestingly, nsvm does not perform
kvm_set_cr3on vmexit when in npt mode. Seems things aren't that regular.We do want the mmu updates. Of course they can't be attached to
kvm_set_cr0_without_the_checks()since there is cross-register dependencies.Option 1 is
kvm_set_multiple_cr(). This does the checks and updates, but only after all registers are updated.
Option 2 is kvm_begin_cr_transaction()/kvm_commit_cr_transaction(). Prettier and more flexible, but a more complicated to implement.
Quoted 6 lines
Quoted 3 lines
The only thing that these three use case truly have in common is the closing
kvm_mmu_reset_context. Maybe nvmx and nsvm can share a bit more. But let's get nVMX right first, then think about sharing.They all need consistency checks, otherwise userspace or the guest and inject inconsistent values and perhaps exploit the host.
To my understanding, the hardware does this for us: If we try to enter the guest (L1, L2) with invalid CRx bits set or cleared, we get an error, at least on Intel. But I bet AMD does so as well - and, if not, it would make this test specific again.
Jan
Quoted 8 lines · Jan Kiszka
Quoted 3 lines
They all need consistency checks, otherwise userspace or the guest and inject inconsistent values and perhaps exploit the host.
To my understanding, the hardware does this for us: If we try to enter the guest (L1, L2) with invalid CRx bits set or cleared, we get an error, at least on Intel. But I bet AMD does so as well - and, if not, it would make this test specific again.
The point is that kvm code may depend on this consistency, so we fail before that hardware has made the check.
Quoted 16 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
Likely a typo, but a fatal one as
kvm_set_cr0performs checks on the state transition that may prevent loading L1's cr0.Signed-off-by Jan Kiszka <[email protected]>--- arch/x86/kvm/vmx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/x86/kvm/vmx.c +0
-0
arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7435
Quoted 8 lines
arch/x86/kvm/vmx.c +1
-1@@ -7429,7 +7429,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,* fpu_active (which may have changed). * Note that vmx_set_cr0 refers to efer set above. */ -kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);/* * If we did fpu_activate()/fpu_deactivate() during L2's run, we need * to apply the same changes to L1's vmcs. We just set cr0 correctly,
This one still applies, is necessary for nested unrestricted guest mode, and I'm still convinced it's an appropriate way to fix the bug. How to proceed?
Jan
Quoted 21 lines · Jan Kiszka
Quoted 20 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
Likely a typo, but a fatal one as
kvm_set_cr0performs checks on the state transition that may prevent loading L1's cr0.Signed-off-by Jan Kiszka <[email protected]>--- arch/x86/kvm/vmx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/x86/kvm/vmx.c +1
-1@@ -7429,7 +7429,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,* fpu_active (which may have changed). * Note that vmx_set_cr0 refers to efer set above. */ -kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);
Quoted 9 lines
arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7435
Quoted 4 lines
arch/x86/kvm/vmx.c +0
-0/* * If we did fpu_activate()/fpu_deactivate() during L2's run, we need * to apply the same changes to L1's vmcs. We just set cr0 correctly, This one still applies, is necessary for nested unrestricted guest mode, and I'm still convinced it's an appropriate way to fix the bug. How to proceed?
What check that is done by kvm_set_cr0() fails?
--
Gleb.
Quoted 31 lines · Gleb Natapov
Quoted 21 lines · Jan Kiszka
Quoted 20 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
Likely a typo, but a fatal one as
kvm_set_cr0performs checks on the state transition that may prevent loading L1's cr0.Signed-off-by Jan Kiszka <[email protected]>--- arch/x86/kvm/vmx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/x86/kvm/vmx.c +1
-1@@ -7429,7 +7429,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,* fpu_active (which may have changed). * Note that vmx_set_cr0 refers to efer set above. */ -kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);Quoted 9 lines
arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7435
Quoted 4 lines
arch/x86/kvm/vmx.c +0
-0/* * If we did fpu_activate()/fpu_deactivate() during L2's run, we need * to apply the same changes to L1's vmcs. We just set cr0 correctly, This one still applies, is necessary for nested unrestricted guest mode, and I'm still convinced it's an appropriate way to fix the bug. How to proceed?
Quoted 1 line
What check that is done by
kvm_set_cr0()fails?
Would have to reproduce the bug to confirm, but from the top of my head and from looking at the code again:
if (!is_paging(vcpu) && (cr0 & X86_CR0_PG)) {
if ((vcpu->arch.efer & EFER_LME)) {
int cs_db, cs_l;
if (!is_pae(vcpu))
return 1;
kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
if (cs_l)
return 1;I think to remember this last check triggered. When we come from the guest with paging off, we may run through this check an incorrectly bail out here when the host state fulfills the conditions (PG, EFER_LME, and L bit set).
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Quoted 47 lines · Jan Kiszka
Quoted 31 lines · Gleb Natapov
Quoted 21 lines · Jan Kiszka
Quoted 20 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
Likely a typo, but a fatal one as
kvm_set_cr0performs checks on the state transition that may prevent loading L1's cr0.Signed-off-by Jan Kiszka <[email protected]>--- arch/x86/kvm/vmx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/x86/kvm/vmx.c +1
-1@@ -7429,7 +7429,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,* fpu_active (which may have changed). * Note that vmx_set_cr0 refers to efer set above. */ -kvm_set_cr0 (vcpu, vmcs12->host_cr0); +vmx_set_cr0 (vcpu, vmcs12->host_cr0);Quoted 9 lines
arch/x86/kvm/vmx.c static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, · line 7435
Quoted 4 lines
arch/x86/kvm/vmx.c +0
-0/* * If we did fpu_activate()/fpu_deactivate() during L2's run, we need * to apply the same changes to L1's vmcs. We just set cr0 correctly, This one still applies, is necessary for nested unrestricted guest mode, and I'm still convinced it's an appropriate way to fix the bug. How to proceed?
Quoted 1 line
What check that is done by
kvm_set_cr0()fails?Would have to reproduce the bug to confirm, but from the top of my head and from looking at the code again:
if (!is_paging(vcpu) && (cr0 & X86_CR0_PG)) { if ((vcpu->arch.efer & EFER_LME)) { int cs_db, cs_l; if (!is_pae(vcpu)) return 1; kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l); if (cs_l) return 1;
Quoted 4 lines
I think to remember this last check triggered. When we come from the guest with paging off, we may run through this check an incorrectly bail out here when the host state fulfills the conditions (PG,
EFER_LME, and L bit set).
Just retried, and actually the first check (!is_pae) fails right now (with nested unrestricted guest mode patched in). The second one stumbles if I set CR4 before CR1 in load_vmcs12_host_state.
So, however you put it, calling kvm_set_cr0 remains wrong.
Jan