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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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