* [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;
as well as URLs for NNTP newsgroup(s).