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:

> On Wed, May 8, 2019 at 2:18 PM Aaron Lewis <aaronlewis@google.com> wrote:
>>
>> 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)
>
> Hi Vitaly,
>
> Does this update look good or are any other changes needed?
>

Hi Aaron,

my apologies for not replying earlier. The changelog looks good to me
now, thanks!

-- 
Vitaly