Re: [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state

Aaron Lewis
2019-05-08T21:18:48Z

From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: Wed, May 8, 2019 at 12:55 PM
To: Aaron Lewis
Cc: Peter Shier, Paolo Bonzini, <rkrcmar@redhat.com>, Jim Mattson,
Marc Orr, <kvm@vger.kernel.org>

> Aaron Lewis <aaronlewis@google.com> writes:
>
> > From: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Date: Fri, May 3, 2019 at 3:25 AM
> > To: Aaron Lewis
> > Cc: Peter Shier, <pbonzini@redhat.com>, <rkrcmar@redhat.com>,
> > <jmattson@google.com>, <marcorr@google.com>, <kvm@vger.kernel.org>
> >
> >> Aaron Lewis <aaronlewis@google.com> writes:
> >>
> >> > Move call to nested_enable_evmcs until after free_nested() is complete.
> >> >
> >> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> >> > Reviewed-by: Marc Orr <marcorr@google.com>
> >> > Reviewed-by: Peter Shier <pshier@google.com>
> >> > ---
> >> > arch/x86/kvm/vmx/nested.c | 6 +++---
> >> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> >> > index 081dea6e211a..3b39c60951ac 100644
> >> > --- a/arch/x86/kvm/vmx/nested.c
> >> > +++ b/arch/x86/kvm/vmx/nested.c
> >> > @@ -5373,9 +5373,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> >> > if (kvm_state->format != 0)
> >> > return -EINVAL;
> >> >
> >> > - if (kvm_state->flags & KVM_STATE_NESTED_EVMCS)
> >> > - nested_enable_evmcs(vcpu, NULL);
> >> > -
> >> > if (!nested_vmx_allowed(vcpu))
> >> > return kvm_state->vmx.vmxon_pa == -1ull ? 0 : -EINVAL;
> >> >
> >> > @@ -5417,6 +5414,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> >> > if (kvm_state->vmx.vmxon_pa == -1ull)
> >> > return 0;
> >> >
> >> > + if (kvm_state->flags & KVM_STATE_NESTED_EVMCS)
> >> > + nested_enable_evmcs(vcpu, NULL);
> >> > +
> >> > vmx->nested.vmxon_ptr = kvm_state->vmx.vmxon_pa;
> >> > ret = enter_vmx_operation(vcpu);
> >> > if (ret)
> >>
> >> nested_enable_evmcs() doesn't do much, actually, in case it was
> >> previously enabled it doesn't do anything and in case it wasn't ordering
> >> with free_nested() (where you're aiming at nested_release_evmcs() I
> >> would guess) shouldn't matter. So could you please elaborate (better in
> >> the commit message) why do we need this re-ordered? My guess is that
> >> you'd like to perform checks for e.g. 'vmx.vmxon_pa == -1ull' before
> >> we actually start doing any changes but let's clarify that.
> >>
> >> Thanks!
> >>
> >> --
> >> Vitaly
> >
> > There are two reasons for doing this:
> > 1. We don't want to set new state if we are going to leave nesting and
> > exit the function (ie: vmx.vmxon_pa = -1), like you pointed out.
> > 2. To be more future proof, we don't want to set new state before
> > tearing down state. This could cause conflicts down the road.
> >
> > I can add this to the commit message if there are no objections to
> > these points.
>
> Sounds good to me, please do. Thanks!
>
> --
> Vitaly

Here is the updated patch:

Move call to nested_enable_evmcs until after free_nested() is
complete. There are two reasons for doing this:
1. We don't want to set new state if we are going to leave nesting and
exit the function (ie: vmx.vmxon_pa = -1).
2. To be more future proof, we don't want to set new state before
tearing down state. This could cause conflicts down the road.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
Reviewed-by: Marc Orr <marcorr@google.com>
Reviewed-by: Peter Shier <pshier@google.com>
---
arch/x86/kvm/vmx/nested.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index fe5814df5149..6ecc301df874 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5373,9 +5373,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
if (kvm_state->format != 0)
return -EINVAL;

- if (kvm_state->flags & KVM_STATE_NESTED_EVMCS)
- nested_enable_evmcs(vcpu, NULL);
-
if (!nested_vmx_allowed(vcpu))
return kvm_state->vmx.vmxon_pa == -1ull ? 0 : -EINVAL;

@@ -5417,6 +5414,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
if (kvm_state->vmx.vmxon_pa == -1ull)
return 0;

+ if (kvm_state->flags & KVM_STATE_NESTED_EVMCS)
+ nested_enable_evmcs(vcpu, NULL);
+
vmx->nested.vmxon_ptr = kvm_state->vmx.vmxon_pa;
ret = enter_vmx_operation(vcpu);
if (ret)