* [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).