* [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete @ 2019-05-08 12:16 Paolo Bonzini 2019-05-08 14:20 ` Sean Christopherson 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2019-05-08 12:16 UTC (permalink / raw) To: linux-kernel, kvm; +Cc: Sean Christopherson, Peter Shier, Aaron Lewis From: Aaron Lewis <aaronlewis@google.com> nested_run_pending=1 implies we have successfully entered guest mode. Move setting from external state in vmx_set_nested_state() until after all other checks are complete. Based on a patch by Aaron Lewis. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/vmx/nested.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index cec77f30f61c..e58caff92694 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -5420,9 +5420,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) return 0; - vmx->nested.nested_run_pending = - !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); - if (nested_cpu_has_shadow_vmcs(vmcs12) && vmcs12->vmcs_link_pointer != -1ull) { struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); @@ -5446,9 +5443,14 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, return -EINVAL; vmx->nested.dirty_vmcs12 = true; + vmx->nested.nested_run_pending = + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); + ret = nested_vmx_enter_non_root_mode(vcpu, false); - if (ret) + if (ret) { + vmx->nested.nested_run_pending = 0; return -EINVAL; + } return 0; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete 2019-05-08 12:16 [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete Paolo Bonzini @ 2019-05-08 14:20 ` Sean Christopherson 2019-05-08 17:53 ` Aaron Lewis 0 siblings, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2019-05-08 14:20 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm, Peter Shier, Aaron Lewis On Wed, May 08, 2019 at 02:16:39PM +0200, Paolo Bonzini wrote: > From: Aaron Lewis <aaronlewis@google.com> If this is actually attributed to Aaron it needs his SOB. > nested_run_pending=1 implies we have successfully entered guest mode. > Move setting from external state in vmx_set_nested_state() until after > all other checks are complete. It'd be helpful to at least mention the flag is consumed by nested_vmx_enter_non_root_mode(). > Based on a patch by Aaron Lewis. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- For the code itself: Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > arch/x86/kvm/vmx/nested.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index cec77f30f61c..e58caff92694 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -5420,9 +5420,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) > return 0; > > - vmx->nested.nested_run_pending = > - !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > - > if (nested_cpu_has_shadow_vmcs(vmcs12) && > vmcs12->vmcs_link_pointer != -1ull) { > struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); > @@ -5446,9 +5443,14 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > return -EINVAL; > > vmx->nested.dirty_vmcs12 = true; > + vmx->nested.nested_run_pending = > + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > + > ret = nested_vmx_enter_non_root_mode(vcpu, false); > - if (ret) > + if (ret) { > + vmx->nested.nested_run_pending = 0; > return -EINVAL; > + } > > return 0; > } > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete 2019-05-08 14:20 ` Sean Christopherson @ 2019-05-08 17:53 ` Aaron Lewis 2019-05-08 18:13 ` Sean Christopherson 0 siblings, 1 reply; 6+ messages in thread From: Aaron Lewis @ 2019-05-08 17:53 UTC (permalink / raw) To: Sean Christopherson; +Cc: Paolo Bonzini, linux-kernel, kvm, Peter Shier From: Sean Christopherson <sean.j.christopherson@intel.com> Date: Wed, May 8, 2019 at 7:20 AM To: Paolo Bonzini Cc: <linux-kernel@vger.kernel.org>, <kvm@vger.kernel.org>, Peter Shier, Aaron Lewis > On Wed, May 08, 2019 at 02:16:39PM +0200, Paolo Bonzini wrote: > > From: Aaron Lewis <aaronlewis@google.com> > > If this is actually attributed to Aaron it needs his SOB. > > > nested_run_pending=1 implies we have successfully entered guest mode. > > Move setting from external state in vmx_set_nested_state() until after > > all other checks are complete. > > It'd be helpful to at least mention the flag is consumed by > nested_vmx_enter_non_root_mode(). > > > Based on a patch by Aaron Lewis. > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > For the code itself: > > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > arch/x86/kvm/vmx/nested.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > > index cec77f30f61c..e58caff92694 100644 > > --- a/arch/x86/kvm/vmx/nested.c > > +++ b/arch/x86/kvm/vmx/nested.c > > @@ -5420,9 +5420,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > > if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) > > return 0; > > > > - vmx->nested.nested_run_pending = > > - !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > > - > > if (nested_cpu_has_shadow_vmcs(vmcs12) && > > vmcs12->vmcs_link_pointer != -1ull) { > > struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); > > @@ -5446,9 +5443,14 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > > return -EINVAL; > > > > vmx->nested.dirty_vmcs12 = true; > > + vmx->nested.nested_run_pending = > > + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > > + > > ret = nested_vmx_enter_non_root_mode(vcpu, false); > > - if (ret) > > + if (ret) { > > + vmx->nested.nested_run_pending = 0; > > return -EINVAL; > > + } > > > > return 0; > > } > > -- > > 1.8.3.1 > > nested_run_pending is also checked in nested_vmx_check_vmentry_postreqs (https://elixir.bootlin.com/linux/v5.1/source/arch/x86/kvm/vmx/nested.c#L2709) so I think the setting needs to be moved to just prior to that call with Paolo's rollback along with another for if the prereqs and postreqs fail. I put a patch together below: ------------------------------------ nested_run_pending=1 implies we have successfully entered guest mode. Move setting from external state in vmx_set_nested_state() until after all other checks are complete. Signed-off-by: Aaron Lewis <aaronlewis@google.com> Reviewed-by: Peter Shier <pshier@google.com> --- arch/x86/kvm/vmx/nested.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 6401eb7ef19c..cf1f810223d2 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -5460,9 +5460,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) return 0; - vmx->nested.nested_run_pending = - !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); - if (nested_cpu_has_shadow_vmcs(vmcs12) && vmcs12->vmcs_link_pointer != -1ull) { struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); @@ -5480,14 +5477,21 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, return -EINVAL; } + vmx->nested.nested_run_pending = + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); + if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) || - nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) + nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) { + vmx->nested.nested_run_pending = 0; return -EINVAL; + } vmx->nested.dirty_vmcs12 = true; ret = nested_vmx_enter_non_root_mode(vcpu, false); - if (ret) + if (ret) { + vmx->nested.nested_run_pending = 0; return -EINVAL; + } return 0; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete 2019-05-08 17:53 ` Aaron Lewis @ 2019-05-08 18:13 ` Sean Christopherson 2019-05-08 21:13 ` Aaron Lewis 0 siblings, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2019-05-08 18:13 UTC (permalink / raw) To: Aaron Lewis; +Cc: Paolo Bonzini, linux-kernel, kvm, Peter Shier [-- Attachment #1: Type: text/plain, Size: 2696 bytes --] On Wed, May 08, 2019 at 10:53:12AM -0700, Aaron Lewis wrote: > nested_run_pending is also checked in > nested_vmx_check_vmentry_postreqs > (https://elixir.bootlin.com/linux/v5.1/source/arch/x86/kvm/vmx/nested.c#L2709) > so I think the setting needs to be moved to just prior to that call > with Paolo's rollback along with another for if the prereqs and > postreqs fail. I put a patch together below: Gah, I missed that usage (also, it's now nested_vmx_check_guest_state()). Side topic, I think the VM_ENTRY_LOAD_BNDCFGS check should be gated by nested_run_pending, a la the EFER check.' > ------------------------------------ > > nested_run_pending=1 implies we have successfully entered guest mode. > Move setting from external state in vmx_set_nested_state() until after > all other checks are complete. > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > Reviewed-by: Peter Shier <pshier@google.com> > --- > arch/x86/kvm/vmx/nested.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 6401eb7ef19c..cf1f810223d2 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -5460,9 +5460,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) > return 0; > > - vmx->nested.nested_run_pending = > - !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); Alternatively, it might be better to leave nested_run_pending where it is and instead add a label to handle clearing the flag on error. IIUC, the real issue is that nested_run_pending is left set after a failed vmx_set_nested_state(), not that its shouldn't be set in the shadow VMCS handling. Patch attached, though it's completely untested. The KVM selftests are broken for me right now, grrr. > - > if (nested_cpu_has_shadow_vmcs(vmcs12) && > vmcs12->vmcs_link_pointer != -1ull) { > struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); > @@ -5480,14 +5477,21 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > return -EINVAL; > } > > + vmx->nested.nested_run_pending = > + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > + > if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) || > - nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) > + nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) { > + vmx->nested.nested_run_pending = 0; > return -EINVAL; > + } > > vmx->nested.dirty_vmcs12 = true; > ret = nested_vmx_enter_non_root_mode(vcpu, false); > - if (ret) > + if (ret) { > + vmx->nested.nested_run_pending = 0; > return -EINVAL; > + } > > return 0; > } [-- Attachment #2: 0001-KVM-nVMX-Clear-nested_run_pending-if-setting-nested-.patch --] [-- Type: text/x-diff, Size: 1974 bytes --] From 279ce1be96d74aee41e93b597572e612a143cf3c Mon Sep 17 00:00:00 2001 From: Sean Christopherson <sean.j.christopherson@intel.com> Date: Wed, 8 May 2019 11:04:32 -0700 Subject: [PATCH] KVM: nVMX: Clear nested_run_pending if setting nested state fails VMX's nested_run_pending flag is subtly consumed when stuffing state to enter guest mode, i.e. needs to be set according before KVM knows if setting guest state is successful. If setting guest state fails, clear the flag as a nested run is obviously not pending. Reported-by: Aaron Lewis <aaronlewis@google.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/vmx/nested.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 04b40a98f60b..1a2a2f91b7e0 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -5428,29 +5428,33 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12)) - return -EINVAL; + goto error_guest_mode; if (copy_from_user(shadow_vmcs12, user_kvm_nested_state->data + VMCS12_SIZE, sizeof(*vmcs12))) - return -EFAULT; + goto error_guest_mode; if (shadow_vmcs12->hdr.revision_id != VMCS12_REVISION || !shadow_vmcs12->hdr.shadow_vmcs) - return -EINVAL; + goto error_guest_mode; } if (nested_vmx_check_controls(vcpu, vmcs12) || nested_vmx_check_host_state(vcpu, vmcs12) || nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual)) - return -EINVAL; + goto error_guest_mode; vmx->nested.dirty_vmcs12 = true; ret = nested_vmx_enter_non_root_mode(vcpu, false); if (ret) - return -EINVAL; + goto error_guest_mode; return 0; + +error_guest_mode: + vmx->nested.nested_run_pending = 0; + return -EINVAL; } void nested_vmx_vcpu_setup(void) -- 2.21.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete 2019-05-08 18:13 ` Sean Christopherson @ 2019-05-08 21:13 ` Aaron Lewis 2019-05-15 16:44 ` Aaron Lewis 0 siblings, 1 reply; 6+ messages in thread From: Aaron Lewis @ 2019-05-08 21:13 UTC (permalink / raw) To: Sean Christopherson; +Cc: Paolo Bonzini, linux-kernel, kvm, Peter Shier From: Sean Christopherson <sean.j.christopherson@intel.com> Date: Wed, May 8, 2019 at 11:13 AM To: Aaron Lewis Cc: Paolo Bonzini, <linux-kernel@vger.kernel.org>, <kvm@vger.kernel.org>, Peter Shier > On Wed, May 08, 2019 at 10:53:12AM -0700, Aaron Lewis wrote: > > nested_run_pending is also checked in > > nested_vmx_check_vmentry_postreqs > > (https://elixir.bootlin.com/linux/v5.1/source/arch/x86/kvm/vmx/nested.c#L2709) > > so I think the setting needs to be moved to just prior to that call > > with Paolo's rollback along with another for if the prereqs and > > postreqs fail. I put a patch together below: > > Gah, I missed that usage (also, it's now nested_vmx_check_guest_state()). > > Side topic, I think the VM_ENTRY_LOAD_BNDCFGS check should be gated by > nested_run_pending, a la the EFER check.' > > > ------------------------------------ > > > > nested_run_pending=1 implies we have successfully entered guest mode. > > Move setting from external state in vmx_set_nested_state() until after > > all other checks are complete. > > > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > > Reviewed-by: Peter Shier <pshier@google.com> > > --- > > arch/x86/kvm/vmx/nested.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > > index 6401eb7ef19c..cf1f810223d2 100644 > > --- a/arch/x86/kvm/vmx/nested.c > > +++ b/arch/x86/kvm/vmx/nested.c > > @@ -5460,9 +5460,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > > if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) > > return 0; > > > > - vmx->nested.nested_run_pending = > > - !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > > Alternatively, it might be better to leave nested_run_pending where it > is and instead add a label to handle clearing the flag on error. IIUC, > the real issue is that nested_run_pending is left set after a failed > vmx_set_nested_state(), not that its shouldn't be set in the shadow > VMCS handling. > > Patch attached, though it's completely untested. The KVM selftests are > broken for me right now, grrr. > > > - > > if (nested_cpu_has_shadow_vmcs(vmcs12) && > > vmcs12->vmcs_link_pointer != -1ull) { > > struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); > > @@ -5480,14 +5477,21 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > > return -EINVAL; > > } > > > > + vmx->nested.nested_run_pending = > > + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > > + > > if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) || > > - nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) > > + nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) { > > + vmx->nested.nested_run_pending = 0; > > return -EINVAL; > > + } > > > > vmx->nested.dirty_vmcs12 = true; > > ret = nested_vmx_enter_non_root_mode(vcpu, false); > > - if (ret) > > + if (ret) { > > + vmx->nested.nested_run_pending = 0; > > return -EINVAL; > > + } > > > > return 0; > > } Here is an update based on your patch. I ran these changes against the test vmx_set_nested_state_test, and it run successfully. That's correct that we are only concerned about restoring the state of nested_run_pending, so it's fine to set it where we do as long as we back the state change out before returning if we get an error. --------------------------------------------- nested_run_pending=1 implies we have successfully entered guest mode. Move setting from external state in vmx_set_nested_state() until after all other checks are complete. Signed-off-by: Aaron Lewis <aaronlewis@google.com> Tested-by: Aaron Lewis <aaronlewis@google.com> Reviewed-by: Peter Shier <pshier@google.com> --- arch/x86/kvm/vmx/nested.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 6401eb7ef19c..fe5814df5149 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -5468,28 +5468,36 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12)) - return -EINVAL; + goto error_guest_mode_einval; if (copy_from_user(shadow_vmcs12, user_kvm_nested_state->data + VMCS12_SIZE, - sizeof(*vmcs12))) - return -EFAULT; + sizeof(*vmcs12))) { + ret = -EFAULT; + goto error_guest_mode; + } if (shadow_vmcs12->hdr.revision_id != VMCS12_REVISION || !shadow_vmcs12->hdr.shadow_vmcs) - return -EINVAL; + goto error_guest_mode_einval; } if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) || nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) - return -EINVAL; + goto error_guest_mode_einval; vmx->nested.dirty_vmcs12 = true; ret = nested_vmx_enter_non_root_mode(vcpu, false); if (ret) - return -EINVAL; + goto error_guest_mode_einval; return 0; + +error_guest_mode_einval: + ret = -EINVAL; +error_guest_mode: + vmx->nested.nested_run_pending = 0; + return ret; } void nested_vmx_vcpu_setup(void) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete 2019-05-08 21:13 ` Aaron Lewis @ 2019-05-15 16:44 ` Aaron Lewis 0 siblings, 0 replies; 6+ messages in thread From: Aaron Lewis @ 2019-05-15 16:44 UTC (permalink / raw) To: Sean Christopherson; +Cc: Paolo Bonzini, linux-kernel, kvm, Peter Shier On Wed, May 8, 2019 at 2:13 PM Aaron Lewis <aaronlewis@google.com> wrote: > > From: Sean Christopherson <sean.j.christopherson@intel.com> > Date: Wed, May 8, 2019 at 11:13 AM > To: Aaron Lewis > Cc: Paolo Bonzini, <linux-kernel@vger.kernel.org>, > <kvm@vger.kernel.org>, Peter Shier > > > On Wed, May 08, 2019 at 10:53:12AM -0700, Aaron Lewis wrote: > > > nested_run_pending is also checked in > > > nested_vmx_check_vmentry_postreqs > > > (https://elixir.bootlin.com/linux/v5.1/source/arch/x86/kvm/vmx/nested.c#L2709) > > > so I think the setting needs to be moved to just prior to that call > > > with Paolo's rollback along with another for if the prereqs and > > > postreqs fail. I put a patch together below: > > > > Gah, I missed that usage (also, it's now nested_vmx_check_guest_state()). > > > > Side topic, I think the VM_ENTRY_LOAD_BNDCFGS check should be gated by > > nested_run_pending, a la the EFER check.' > > > > > ------------------------------------ > > > > > > nested_run_pending=1 implies we have successfully entered guest mode. > > > Move setting from external state in vmx_set_nested_state() until after > > > all other checks are complete. > > > > > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > > > Reviewed-by: Peter Shier <pshier@google.com> > > > --- > > > arch/x86/kvm/vmx/nested.c | 14 +++++++++----- > > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > > > index 6401eb7ef19c..cf1f810223d2 100644 > > > --- a/arch/x86/kvm/vmx/nested.c > > > +++ b/arch/x86/kvm/vmx/nested.c > > > @@ -5460,9 +5460,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > > > if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) > > > return 0; > > > > > > - vmx->nested.nested_run_pending = > > > - !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > > > > Alternatively, it might be better to leave nested_run_pending where it > > is and instead add a label to handle clearing the flag on error. IIUC, > > the real issue is that nested_run_pending is left set after a failed > > vmx_set_nested_state(), not that its shouldn't be set in the shadow > > VMCS handling. > > > > Patch attached, though it's completely untested. The KVM selftests are > > broken for me right now, grrr. > > > > > - > > > if (nested_cpu_has_shadow_vmcs(vmcs12) && > > > vmcs12->vmcs_link_pointer != -1ull) { > > > struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); > > > @@ -5480,14 +5477,21 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > > > return -EINVAL; > > > } > > > > > > + vmx->nested.nested_run_pending = > > > + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING); > > > + > > > if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) || > > > - nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) > > > + nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) { > > > + vmx->nested.nested_run_pending = 0; > > > return -EINVAL; > > > + } > > > > > > vmx->nested.dirty_vmcs12 = true; > > > ret = nested_vmx_enter_non_root_mode(vcpu, false); > > > - if (ret) > > > + if (ret) { > > > + vmx->nested.nested_run_pending = 0; > > > return -EINVAL; > > > + } > > > > > > return 0; > > > } > > Here is an update based on your patch. I ran these changes against > the test vmx_set_nested_state_test, and it run successfully. > > That's correct that we are only concerned about restoring the state of > nested_run_pending, so it's fine to set it where we do as long as we > back the state change out before returning if we get an error. > > --------------------------------------------- > > nested_run_pending=1 implies we have successfully entered guest mode. > Move setting from external state in vmx_set_nested_state() until after > all other checks are complete. > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > Tested-by: Aaron Lewis <aaronlewis@google.com> > Reviewed-by: Peter Shier <pshier@google.com> > --- > arch/x86/kvm/vmx/nested.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 6401eb7ef19c..fe5814df5149 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -5468,28 +5468,36 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu); > > if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12)) > - return -EINVAL; > + goto error_guest_mode_einval; > > if (copy_from_user(shadow_vmcs12, > user_kvm_nested_state->data + VMCS12_SIZE, > - sizeof(*vmcs12))) > - return -EFAULT; > + sizeof(*vmcs12))) { > + ret = -EFAULT; > + goto error_guest_mode; > + } > > if (shadow_vmcs12->hdr.revision_id != VMCS12_REVISION || > !shadow_vmcs12->hdr.shadow_vmcs) > - return -EINVAL; > + goto error_guest_mode_einval; > } > > if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) || > nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) > - return -EINVAL; > + goto error_guest_mode_einval; > > vmx->nested.dirty_vmcs12 = true; > ret = nested_vmx_enter_non_root_mode(vcpu, false); > if (ret) > - return -EINVAL; > + goto error_guest_mode_einval; > > return 0; > + > +error_guest_mode_einval: > + ret = -EINVAL; > +error_guest_mode: > + vmx->nested.nested_run_pending = 0; > + return ret; > } > > void nested_vmx_vcpu_setup(void) Sean, I updated this patch based on the one you sent out. Does it look good now? ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-15 16:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-08 12:16 [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete Paolo Bonzini 2019-05-08 14:20 ` Sean Christopherson 2019-05-08 17:53 ` Aaron Lewis 2019-05-08 18:13 ` Sean Christopherson 2019-05-08 21:13 ` Aaron Lewis 2019-05-15 16:44 ` Aaron Lewis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).