Aaron Lewis <aaronlewis@xxxxxxxxxx> writes: > On Wed, May 8, 2019 at 2:18 PM Aaron Lewis <aaronlewis@xxxxxxxxxx> wrote: >> >> From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> >> Date: Wed, May 8, 2019 at 12:55 PM >> To: Aaron Lewis >> Cc: Peter Shier, Paolo Bonzini, <rkrcmar@xxxxxxxxxx>, Jim Mattson, >> Marc Orr, <kvm@xxxxxxxxxxxxxxx> >> >> > Aaron Lewis <aaronlewis@xxxxxxxxxx> writes: >> > >> > > From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> >> > > Date: Fri, May 3, 2019 at 3:25 AM >> > > To: Aaron Lewis >> > > Cc: Peter Shier, <pbonzini@xxxxxxxxxx>, <rkrcmar@xxxxxxxxxx>, >> > > <jmattson@xxxxxxxxxx>, <marcorr@xxxxxxxxxx>, <kvm@xxxxxxxxxxxxxxx> >> > > >> > >> Aaron Lewis <aaronlewis@xxxxxxxxxx> writes: >> > >> >> > >> > Move call to nested_enable_evmcs until after free_nested() is complete. >> > >> > >> > >> > Signed-off-by: Aaron Lewis <aaronlewis@xxxxxxxxxx> >> > >> > Reviewed-by: Marc Orr <marcorr@xxxxxxxxxx> >> > >> > Reviewed-by: Peter Shier <pshier@xxxxxxxxxx> >> > >> > --- >> > >> > 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@xxxxxxxxxx> >> Reviewed-by: Marc Orr <marcorr@xxxxxxxxxx> >> Reviewed-by: Peter Shier <pshier@xxxxxxxxxx> >> --- >> 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
