| Message ID | 20210908171731.18885-1-yu.c.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: fix comments of handle_vmon() | expand |
Yu Zhang <yu.c.zhang@linux.intel.com> writes: > "VMXON pointer" is saved in vmx->nested.vmxon_ptr since > commit 3573e22cfeca ("KVM: nVMX: additional checks on > vmxon region"). Also, handle_vmptrld() & handle_vmclear() > now have logic to check the VMCS pointer against the VMXON > pointer. > > So just remove the obsolete comments of handle_vmon(). > > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > --- > arch/x86/kvm/vmx/nested.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index bc6327950657..90f34f12f883 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4862,14 +4862,7 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu) > return -ENOMEM; > } > > -/* > - * Emulate the VMXON instruction. > - * Currently, we just remember that VMX is active, and do not save or even > - * inspect the argument to VMXON (the so-called "VMXON pointer") because we > - * do not currently need to store anything in that guest-allocated memory > - * region. Consequently, VMCLEAR and VMPTRLD also do not verify that the their > - * argument is different from the VMXON pointer (which the spec says they do). > - */ > +/* Emulate the VMXON instruction. */ > static int handle_vmon(struct kvm_vcpu *vcpu) > { > int ret; Indeed, Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> On a slightly related note: we don't seem to reset 'vmx->nested.vmxon_ptr' upon VMXOFF emulation; this is not a problem per se as we never access it when !vmx->nested.vmxon but I'd still suggest we do something like diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index bc6327950657..8beb41d02d21 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -290,6 +290,7 @@ static void free_nested(struct kvm_vcpu *vcpu) vmx->nested.vmxon = false; vmx->nested.smm.vmxon = false; + vmx->nested.vmxon_ptr = -1ull; free_vpid(vmx->nested.vpid02); vmx->nested.posted_intr_nv = -1; vmx->nested.current_vmptr = -1ull; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index d7c5257eb5c0..2214e6bd4713 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6884,6 +6884,7 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu) vcpu_setup_sgx_lepubkeyhash(vcpu); + vmx->nested.vmxon_ptr = -1ull; vmx->nested.posted_intr_nv = -1; vmx->nested.current_vmptr = -1ull; vmx->nested.hv_evmcs_vmptr = EVMPTR_INVALID; to avoid issues in the future.
On Wed, Sep 08, 2021 at 11:55:59AM +0200, Vitaly Kuznetsov wrote: > Indeed, > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > On a slightly related note: we don't seem to reset > 'vmx->nested.vmxon_ptr' upon VMXOFF emulation; this is not a problem per > se as we never access it when !vmx->nested.vmxon but I'd still suggest > we do something like > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index bc6327950657..8beb41d02d21 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -290,6 +290,7 @@ static void free_nested(struct kvm_vcpu *vcpu) > > vmx->nested.vmxon = false; > vmx->nested.smm.vmxon = false; > + vmx->nested.vmxon_ptr = -1ull; > free_vpid(vmx->nested.vpid02); > vmx->nested.posted_intr_nv = -1; > vmx->nested.current_vmptr = -1ull; > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index d7c5257eb5c0..2214e6bd4713 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6884,6 +6884,7 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu) > > vcpu_setup_sgx_lepubkeyhash(vcpu); > > + vmx->nested.vmxon_ptr = -1ull; > vmx->nested.posted_intr_nv = -1; > vmx->nested.current_vmptr = -1ull; > vmx->nested.hv_evmcs_vmptr = EVMPTR_INVALID; > > to avoid issues in the future. > Thanks, Vitaly. And above suggestion sounds reasonable to me. :) I can add this in V2 later, if no one objects. B.R. Yu
Yu Zhang <yu.c.zhang@linux.intel.com> writes: > On Wed, Sep 08, 2021 at 11:55:59AM +0200, Vitaly Kuznetsov wrote: >> Indeed, >> >> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> >> On a slightly related note: we don't seem to reset >> 'vmx->nested.vmxon_ptr' upon VMXOFF emulation; this is not a problem per >> se as we never access it when !vmx->nested.vmxon but I'd still suggest >> we do something like >> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >> index bc6327950657..8beb41d02d21 100644 >> --- a/arch/x86/kvm/vmx/nested.c >> +++ b/arch/x86/kvm/vmx/nested.c >> @@ -290,6 +290,7 @@ static void free_nested(struct kvm_vcpu *vcpu) >> >> vmx->nested.vmxon = false; >> vmx->nested.smm.vmxon = false; >> + vmx->nested.vmxon_ptr = -1ull; >> free_vpid(vmx->nested.vpid02); >> vmx->nested.posted_intr_nv = -1; >> vmx->nested.current_vmptr = -1ull; >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index d7c5257eb5c0..2214e6bd4713 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -6884,6 +6884,7 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu) >> >> vcpu_setup_sgx_lepubkeyhash(vcpu); >> >> + vmx->nested.vmxon_ptr = -1ull; >> vmx->nested.posted_intr_nv = -1; >> vmx->nested.current_vmptr = -1ull; >> vmx->nested.hv_evmcs_vmptr = EVMPTR_INVALID; >> >> to avoid issues in the future. >> > Thanks, Vitaly. And above suggestion sounds reasonable to me. :) > > I can add this in V2 later, if no one objects. > Sure, this is rather a separate patch. Thanks!
On Wed, Sep 08, 2021 at 03:42:13PM +0200, Vitaly Kuznetsov wrote: > > Sure, this is rather a separate patch. Thanks! Oh, right. Just sent out a seperate patch. :) @Paolo Any comment on this one? Thanks! B.R. Yu
On 08/09/21 19:17, Yu Zhang wrote: > "VMXON pointer" is saved in vmx->nested.vmxon_ptr since > commit 3573e22cfeca ("KVM: nVMX: additional checks on > vmxon region"). Also, handle_vmptrld() & handle_vmclear() > now have logic to check the VMCS pointer against the VMXON > pointer. > > So just remove the obsolete comments of handle_vmon(). > > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > --- > arch/x86/kvm/vmx/nested.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index bc6327950657..90f34f12f883 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4862,14 +4862,7 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu) > return -ENOMEM; > } > > -/* > - * Emulate the VMXON instruction. > - * Currently, we just remember that VMX is active, and do not save or even > - * inspect the argument to VMXON (the so-called "VMXON pointer") because we > - * do not currently need to store anything in that guest-allocated memory > - * region. Consequently, VMCLEAR and VMPTRLD also do not verify that the their > - * argument is different from the VMXON pointer (which the spec says they do). > - */ > +/* Emulate the VMXON instruction. */ > static int handle_vmon(struct kvm_vcpu *vcpu) > { > int ret; > Queued, thanks. Paolo
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index bc6327950657..90f34f12f883 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4862,14 +4862,7 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu) return -ENOMEM; } -/* - * Emulate the VMXON instruction. - * Currently, we just remember that VMX is active, and do not save or even - * inspect the argument to VMXON (the so-called "VMXON pointer") because we - * do not currently need to store anything in that guest-allocated memory - * region. Consequently, VMCLEAR and VMPTRLD also do not verify that the their - * argument is different from the VMXON pointer (which the spec says they do). - */ +/* Emulate the VMXON instruction. */ static int handle_vmon(struct kvm_vcpu *vcpu) { int ret;
"VMXON pointer" is saved in vmx->nested.vmxon_ptr since commit 3573e22cfeca ("KVM: nVMX: additional checks on vmxon region"). Also, handle_vmptrld() & handle_vmclear() now have logic to check the VMCS pointer against the VMXON pointer. So just remove the obsolete comments of handle_vmon(). Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> --- arch/x86/kvm/vmx/nested.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)