* [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state @ 2013-02-23 21:35 Jan Kiszka 2013-02-23 21:45 ` Nadav Har'El 2013-04-28 10:20 ` Jan Kiszka 0 siblings, 2 replies; 18+ messages in thread From: Jan Kiszka @ 2013-02-23 21:35 UTC (permalink / raw) To: Marcelo Tosatti, Gleb Natapov; +Cc: kvm, Nadav Har'El, Nakajima, Jun From: Jan Kiszka <jan.kiszka@siemens.com> Likely a typo, but a fatal one as kvm_set_cr0 performs checks on the state transition that may prevent loading L1's cr0. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- arch/x86/kvm/vmx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 26d47e9..94f3b66 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -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, -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-23 21:35 [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state Jan Kiszka @ 2013-02-23 21:45 ` Nadav Har'El 2013-02-23 21:57 ` Jan Kiszka 2013-04-28 10:20 ` Jan Kiszka 1 sibling, 1 reply; 18+ messages in thread From: Nadav Har'El @ 2013-02-23 21:45 UTC (permalink / raw) To: Jan Kiszka; +Cc: Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun, avi.kivity 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": > - 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. -- Nadav Har'El | Saturday, Feb 23 2013, 14 Adar 5773 nyh@math.technion.ac.il |----------------------------------------- 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-23 21:45 ` Nadav Har'El @ 2013-02-23 21:57 ` Jan Kiszka 2013-02-23 22:21 ` Jan Kiszka 2013-02-24 8:56 ` Avi Kivity 0 siblings, 2 replies; 18+ messages in thread From: Jan Kiszka @ 2013-02-23 21:57 UTC (permalink / raw) To: Nadav Har'El Cc: Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun, avi.kivity [-- Attachment #1: Type: text/plain, Size: 1373 bytes --] On 2013-02-23 22:45, Nadav Har'El wrote: > 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": >> - 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. 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-23 21:57 ` Jan Kiszka @ 2013-02-23 22:21 ` Jan Kiszka 2013-02-24 8:34 ` Jan Kiszka 2013-02-24 8:56 ` Avi Kivity 1 sibling, 1 reply; 18+ messages in thread From: Jan Kiszka @ 2013-02-23 22:21 UTC (permalink / raw) To: Nadav Har'El Cc: Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun, avi.kivity [-- Attachment #1: Type: text/plain, Size: 1881 bytes --] On 2013-02-23 22:57, Jan Kiszka wrote: > On 2013-02-23 22:45, Nadav Har'El wrote: >> 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": >>> - 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. > > 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... 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-23 22:21 ` Jan Kiszka @ 2013-02-24 8:34 ` Jan Kiszka 0 siblings, 0 replies; 18+ messages in thread From: Jan Kiszka @ 2013-02-24 8:34 UTC (permalink / raw) To: Nadav Har'El Cc: Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun, avi.kivity [-- Attachment #1: Type: text/plain, Size: 2294 bytes --] On 2013-02-23 23:21, Jan Kiszka wrote: > On 2013-02-23 22:57, Jan Kiszka wrote: >> On 2013-02-23 22:45, Nadav Har'El wrote: >>> 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": >>>> - 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. >> >> 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... > > 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. 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-23 21:57 ` Jan Kiszka 2013-02-23 22:21 ` Jan Kiszka @ 2013-02-24 8:56 ` Avi Kivity 2013-02-24 9:01 ` Jan Kiszka 1 sibling, 1 reply; 18+ messages in thread From: Avi Kivity @ 2013-02-24 8:56 UTC (permalink / raw) To: Jan Kiszka Cc: Nadav Har'El, Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun On Sat, Feb 23, 2013 at 11:57 PM, Jan Kiszka <jan.kiszka@web.de> wrote: > On 2013-02-23 22:45, Nadav Har'El wrote: >> 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": >>> - 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. > > 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. We have the same problem in KVM_SET_SREGS. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-24 8:56 ` Avi Kivity @ 2013-02-24 9:01 ` Jan Kiszka 2013-02-24 9:21 ` Avi Kivity 0 siblings, 1 reply; 18+ messages in thread From: Jan Kiszka @ 2013-02-24 9:01 UTC (permalink / raw) To: Avi Kivity Cc: Nadav Har'El, Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun [-- Attachment #1: Type: text/plain, Size: 1428 bytes --] On 2013-02-24 09:56, Avi Kivity wrote: > On Sat, Feb 23, 2013 at 11:57 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >> On 2013-02-23 22:45, Nadav Har'El wrote: >>> 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": >>>> - 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. >> >> 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. > > 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-24 9:01 ` Jan Kiszka @ 2013-02-24 9:21 ` Avi Kivity 2013-02-24 9:40 ` Jan Kiszka 0 siblings, 1 reply; 18+ messages in thread From: Avi Kivity @ 2013-02-24 9:21 UTC (permalink / raw) To: Jan Kiszka Cc: Nadav Har'El, Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun On Sun, Feb 24, 2013 at 11:01 AM, Jan Kiszka <jan.kiszka@web.de> wrote: > On 2013-02-24 09:56, Avi Kivity wrote: >> On Sat, Feb 23, 2013 at 11:57 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >>> On 2013-02-23 22:45, Nadav Har'El wrote: >>>> 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": >>>>> - 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. >>> >>> 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. >> >> 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. 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". ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-24 9:21 ` Avi Kivity @ 2013-02-24 9:40 ` Jan Kiszka 2013-02-24 10:11 ` Avi Kivity 0 siblings, 1 reply; 18+ messages in thread From: Jan Kiszka @ 2013-02-24 9:40 UTC (permalink / raw) To: Avi Kivity Cc: Nadav Har'El, Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun [-- Attachment #1: Type: text/plain, Size: 2012 bytes --] On 2013-02-24 10:21, Avi Kivity wrote: > On Sun, Feb 24, 2013 at 11:01 AM, Jan Kiszka <jan.kiszka@web.de> wrote: >> On 2013-02-24 09:56, Avi Kivity wrote: >>> On Sat, Feb 23, 2013 at 11:57 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >>>> On 2013-02-23 22:45, Nadav Har'El wrote: >>>>> 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": >>>>>> - 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. >>>> >>>> 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. >>> >>> 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. > > 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-24 9:40 ` Jan Kiszka @ 2013-02-24 10:11 ` Avi Kivity 2013-02-24 10:49 ` Jan Kiszka 0 siblings, 1 reply; 18+ messages in thread From: Avi Kivity @ 2013-02-24 10:11 UTC (permalink / raw) To: Jan Kiszka Cc: Nadav Har'El, Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun On Sun, Feb 24, 2013 at 11:40 AM, Jan Kiszka <jan.kiszka@web.de> wrote: >>>> 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. >> >> 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. 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-24 10:11 ` Avi Kivity @ 2013-02-24 10:49 ` Jan Kiszka 2013-02-24 18:56 ` Avi Kivity 0 siblings, 1 reply; 18+ messages in thread From: Jan Kiszka @ 2013-02-24 10:49 UTC (permalink / raw) To: Avi Kivity Cc: Nadav Har'El, Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun [-- Attachment #1: Type: text/plain, Size: 1369 bytes --] On 2013-02-24 11:11, Avi Kivity wrote: > On Sun, Feb 24, 2013 at 11:40 AM, Jan Kiszka <jan.kiszka@web.de> wrote: >>>>> 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. >>> >>> 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. > > 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-24 10:49 ` Jan Kiszka @ 2013-02-24 18:56 ` Avi Kivity 2013-02-24 19:15 ` Jan Kiszka 0 siblings, 1 reply; 18+ messages in thread From: Avi Kivity @ 2013-02-24 18:56 UTC (permalink / raw) To: Jan Kiszka Cc: Nadav Har'El, Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun On Sun, Feb 24, 2013 at 12:49 PM, Jan Kiszka <jan.kiszka@web.de> wrote: > On 2013-02-24 11:11, Avi Kivity wrote: >> On Sun, Feb 24, 2013 at 11:40 AM, Jan Kiszka <jan.kiszka@web.de> wrote: >>>>>> 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. >>>> >>>> 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. >> >> 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. They all need consistency checks, otherwise userspace or the guest and inject inconsistent values and perhaps exploit the host. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-24 18:56 ` Avi Kivity @ 2013-02-24 19:15 ` Jan Kiszka 2013-02-24 19:26 ` Avi Kivity 0 siblings, 1 reply; 18+ messages in thread From: Jan Kiszka @ 2013-02-24 19:15 UTC (permalink / raw) To: Avi Kivity Cc: Nadav Har'El, Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun [-- Attachment #1: Type: text/plain, Size: 1938 bytes --] On 2013-02-24 19:56, Avi Kivity wrote: > On Sun, Feb 24, 2013 at 12:49 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >> On 2013-02-24 11:11, Avi Kivity wrote: >>> On Sun, Feb 24, 2013 at 11:40 AM, Jan Kiszka <jan.kiszka@web.de> wrote: >>>>>>> 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. >>>>> >>>>> 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. >>> >>> 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. > > 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-24 19:15 ` Jan Kiszka @ 2013-02-24 19:26 ` Avi Kivity 0 siblings, 0 replies; 18+ messages in thread From: Avi Kivity @ 2013-02-24 19:26 UTC (permalink / raw) To: Jan Kiszka Cc: Nadav Har'El, Marcelo Tosatti, Gleb Natapov, kvm, Nakajima, Jun On Sun, Feb 24, 2013 at 9:15 PM, Jan Kiszka <jan.kiszka@web.de> wrote: >> >> 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-02-23 21:35 [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state Jan Kiszka 2013-02-23 21:45 ` Nadav Har'El @ 2013-04-28 10:20 ` Jan Kiszka 2013-04-30 11:46 ` Gleb Natapov 1 sibling, 1 reply; 18+ messages in thread From: Jan Kiszka @ 2013-04-28 10:20 UTC (permalink / raw) To: Marcelo Tosatti, Gleb Natapov; +Cc: kvm, Nadav Har'El, Nakajima, Jun [-- Attachment #1: Type: text/plain, Size: 1124 bytes --] On 2013-02-23 22:35, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Likely a typo, but a fatal one as kvm_set_cr0 performs checks on the > state transition that may prevent loading L1's cr0. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > arch/x86/kvm/vmx.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 26d47e9..94f3b66 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-04-28 10:20 ` Jan Kiszka @ 2013-04-30 11:46 ` Gleb Natapov 2013-04-30 12:42 ` Jan Kiszka 0 siblings, 1 reply; 18+ messages in thread From: Gleb Natapov @ 2013-04-30 11:46 UTC (permalink / raw) To: Jan Kiszka Cc: Marcelo Tosatti, kvm, Nadav Har'El, Nakajima, Jun, Avi Kivity On Sun, Apr 28, 2013 at 12:20:38PM +0200, Jan Kiszka wrote: > On 2013-02-23 22:35, Jan Kiszka wrote: > > From: Jan Kiszka <jan.kiszka@siemens.com> > > > > Likely a typo, but a fatal one as kvm_set_cr0 performs checks on the > > state transition that may prevent loading L1's cr0. > > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > --- > > arch/x86/kvm/vmx.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > index 26d47e9..94f3b66 100644 > > --- a/arch/x86/kvm/vmx.c > > +++ b/arch/x86/kvm/vmx.c > > @@ -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? > What check that is done by kvm_set_cr0() fails? -- Gleb. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-04-30 11:46 ` Gleb Natapov @ 2013-04-30 12:42 ` Jan Kiszka 2013-05-05 9:02 ` Jan Kiszka 0 siblings, 1 reply; 18+ messages in thread From: Jan Kiszka @ 2013-04-30 12:42 UTC (permalink / raw) To: Gleb Natapov Cc: Marcelo Tosatti, kvm, Nadav Har'El, Nakajima, Jun, Avi Kivity On 2013-04-30 13:46, Gleb Natapov wrote: > On Sun, Apr 28, 2013 at 12:20:38PM +0200, Jan Kiszka wrote: >> On 2013-02-23 22:35, Jan Kiszka wrote: >>> From: Jan Kiszka <jan.kiszka@siemens.com> >>> >>> Likely a typo, but a fatal one as kvm_set_cr0 performs checks on the >>> state transition that may prevent loading L1's cr0. >>> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>> --- >>> arch/x86/kvm/vmx.c | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>> index 26d47e9..94f3b66 100644 >>> --- a/arch/x86/kvm/vmx.c >>> +++ b/arch/x86/kvm/vmx.c >>> @@ -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? >> > 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 -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state 2013-04-30 12:42 ` Jan Kiszka @ 2013-05-05 9:02 ` Jan Kiszka 0 siblings, 0 replies; 18+ messages in thread From: Jan Kiszka @ 2013-05-05 9:02 UTC (permalink / raw) Cc: Gleb Natapov, Marcelo Tosatti, kvm, Nadav Har'El, Nakajima, Jun, Avi Kivity [-- Attachment #1: Type: text/plain, Size: 2282 bytes --] On 2013-04-30 14:42, Jan Kiszka wrote: > On 2013-04-30 13:46, Gleb Natapov wrote: >> On Sun, Apr 28, 2013 at 12:20:38PM +0200, Jan Kiszka wrote: >>> On 2013-02-23 22:35, Jan Kiszka wrote: >>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>> >>>> Likely a typo, but a fatal one as kvm_set_cr0 performs checks on the >>>> state transition that may prevent loading L1's cr0. >>>> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>> --- >>>> arch/x86/kvm/vmx.c | 2 +- >>>> 1 files changed, 1 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>>> index 26d47e9..94f3b66 100644 >>>> --- a/arch/x86/kvm/vmx.c >>>> +++ b/arch/x86/kvm/vmx.c >>>> @@ -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? >>> >> 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). 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2013-05-05 9:03 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-23 21:35 [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state Jan Kiszka 2013-02-23 21:45 ` Nadav Har'El 2013-02-23 21:57 ` Jan Kiszka 2013-02-23 22:21 ` Jan Kiszka 2013-02-24 8:34 ` Jan Kiszka 2013-02-24 8:56 ` Avi Kivity 2013-02-24 9:01 ` Jan Kiszka 2013-02-24 9:21 ` Avi Kivity 2013-02-24 9:40 ` Jan Kiszka 2013-02-24 10:11 ` Avi Kivity 2013-02-24 10:49 ` Jan Kiszka 2013-02-24 18:56 ` Avi Kivity 2013-02-24 19:15 ` Jan Kiszka 2013-02-24 19:26 ` Avi Kivity 2013-04-28 10:20 ` Jan Kiszka 2013-04-30 11:46 ` Gleb Natapov 2013-04-30 12:42 ` Jan Kiszka 2013-05-05 9:02 ` Jan Kiszka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox