* [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state @ 2019-05-02 18:31 Aaron Lewis 2019-05-03 10:25 ` Vitaly Kuznetsov 0 siblings, 1 reply; 7+ messages in thread From: Aaron Lewis @ 2019-05-02 18:31 UTC (permalink / raw) To: pbonzini, rkrcmar, jmattson, marcorr, vkuznets, kvm Cc: Aaron Lewis, Peter Shier 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) -- 2.21.0.593.g511ec345e18-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state 2019-05-02 18:31 [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state Aaron Lewis @ 2019-05-03 10:25 ` Vitaly Kuznetsov 2019-05-08 19:21 ` Aaron Lewis 0 siblings, 1 reply; 7+ messages in thread From: Vitaly Kuznetsov @ 2019-05-03 10:25 UTC (permalink / raw) To: Aaron Lewis; +Cc: Peter Shier, pbonzini, rkrcmar, jmattson, marcorr, kvm 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state 2019-05-03 10:25 ` Vitaly Kuznetsov @ 2019-05-08 19:21 ` Aaron Lewis 2019-05-08 19:54 ` Vitaly Kuznetsov 0 siblings, 1 reply; 7+ messages in thread From: Aaron Lewis @ 2019-05-08 19:21 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Peter Shier, Paolo Bonzini, rkrcmar, Jim Mattson, Marc Orr, kvm 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state 2019-05-08 19:21 ` Aaron Lewis @ 2019-05-08 19:54 ` Vitaly Kuznetsov 2019-05-08 21:18 ` Aaron Lewis 0 siblings, 1 reply; 7+ messages in thread From: Vitaly Kuznetsov @ 2019-05-08 19:54 UTC (permalink / raw) To: Aaron Lewis Cc: Peter Shier, Paolo Bonzini, rkrcmar, Jim Mattson, Marc Orr, kvm 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state 2019-05-08 19:54 ` Vitaly Kuznetsov @ 2019-05-08 21:18 ` Aaron Lewis 2019-05-15 16:42 ` Aaron Lewis 0 siblings, 1 reply; 7+ messages in thread From: Aaron Lewis @ 2019-05-08 21:18 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Peter Shier, Paolo Bonzini, rkrcmar, Jim Mattson, Marc Orr, kvm 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) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state 2019-05-08 21:18 ` Aaron Lewis @ 2019-05-15 16:42 ` Aaron Lewis 2019-05-15 18:48 ` Vitaly Kuznetsov 0 siblings, 1 reply; 7+ messages in thread From: Aaron Lewis @ 2019-05-15 16:42 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Peter Shier, Paolo Bonzini, rkrcmar, Jim Mattson, Marc Orr, kvm 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? Thanks, Aaron ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state 2019-05-15 16:42 ` Aaron Lewis @ 2019-05-15 18:48 ` Vitaly Kuznetsov 0 siblings, 0 replies; 7+ messages in thread From: Vitaly Kuznetsov @ 2019-05-15 18:48 UTC (permalink / raw) To: Aaron Lewis Cc: Peter Shier, Paolo Bonzini, rkrcmar, Jim Mattson, Marc Orr, kvm 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-15 18:48 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-05-02 18:31 [PATCH 2/3] KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state Aaron Lewis 2019-05-03 10:25 ` Vitaly Kuznetsov 2019-05-08 19:21 ` Aaron Lewis 2019-05-08 19:54 ` Vitaly Kuznetsov 2019-05-08 21:18 ` Aaron Lewis 2019-05-15 16:42 ` Aaron Lewis 2019-05-15 18:48 ` Vitaly Kuznetsov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox