| Message ID | 20231025152406.1879274-10-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Make Hyper-V emulation optional | expand |
On Wed, 2023-10-25 at 17:24 +0200, Vitaly Kuznetsov wrote: > The "vmxon_pa == vmcs12_pa == -1ull" test happens to work by accident: as > Enlightened VMCS is always supported, set_default_vmx_state() adds > 'KVM_STATE_NESTED_EVMCS' to 'flags' and the following branch of > vmx_set_nested_state() is executed: > > if ((kvm_state->flags & KVM_STATE_NESTED_EVMCS) && > (!guest_can_use(vcpu, X86_FEATURE_VMX) || > !vmx->nested.enlightened_vmcs_enabled)) > return -EINVAL; > > as 'enlightened_vmcs_enabled' is false. In fact, "vmxon_pa == vmcs12_pa == > -1ull" is a valid state when not tainted by wrong flags so the test should > aim for this branch: > > if (kvm_state->hdr.vmx.vmxon_pa == INVALID_GPA) > return 0; > > Test all this properly: > - Without KVM_STATE_NESTED_EVMCS in the flags, the expected return value is > '0'. > - With KVM_STATE_NESTED_EVMCS flag (when supported) set, the expected > return value is '-EINVAL' prior to enabling eVMCS and '0' after. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > .../kvm/x86_64/vmx_set_nested_state_test.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c > index 41ea7028a1f8..67a62a5a8895 100644 > --- a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c > +++ b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c > @@ -125,21 +125,25 @@ void test_vmx_nested_state(struct kvm_vcpu *vcpu) > > /* > * Setting vmxon_pa == -1ull and vmcs_pa == -1ull exits early without > - * setting the nested state but flags other than eVMCS must be clear. > - * The eVMCS flag can be set if the enlightened VMCS capability has > - * been enabled. > + * setting the nested state. When the eVMCS flag is not set, the > + * expected return value is '0'. > */ > set_default_vmx_state(state, state_sz); > + state->flags = 0; > state->hdr.vmx.vmxon_pa = -1ull; > state->hdr.vmx.vmcs12_pa = -1ull; > - test_nested_state_expect_einval(vcpu, state); > + test_nested_state(vcpu, state); > > - state->flags &= KVM_STATE_NESTED_EVMCS; > + /* > + * When eVMCS is supported, the eVMCS flag can only be set if the > + * enlightened VMCS capability has been enabled. > + */ > if (have_evmcs) { > + state->flags = KVM_STATE_NESTED_EVMCS; > test_nested_state_expect_einval(vcpu, state); > vcpu_enable_evmcs(vcpu); > + test_nested_state(vcpu, state); > } > - test_nested_state(vcpu, state); > > /* It is invalid to have vmxon_pa == -1ull and SMM flags non-zero. */ > state->hdr.vmx.smm.flags = 1; Makes sense, but I am not 100% sure that I haven't missed something. Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c index 41ea7028a1f8..67a62a5a8895 100644 --- a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c +++ b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c @@ -125,21 +125,25 @@ void test_vmx_nested_state(struct kvm_vcpu *vcpu) /* * Setting vmxon_pa == -1ull and vmcs_pa == -1ull exits early without - * setting the nested state but flags other than eVMCS must be clear. - * The eVMCS flag can be set if the enlightened VMCS capability has - * been enabled. + * setting the nested state. When the eVMCS flag is not set, the + * expected return value is '0'. */ set_default_vmx_state(state, state_sz); + state->flags = 0; state->hdr.vmx.vmxon_pa = -1ull; state->hdr.vmx.vmcs12_pa = -1ull; - test_nested_state_expect_einval(vcpu, state); + test_nested_state(vcpu, state); - state->flags &= KVM_STATE_NESTED_EVMCS; + /* + * When eVMCS is supported, the eVMCS flag can only be set if the + * enlightened VMCS capability has been enabled. + */ if (have_evmcs) { + state->flags = KVM_STATE_NESTED_EVMCS; test_nested_state_expect_einval(vcpu, state); vcpu_enable_evmcs(vcpu); + test_nested_state(vcpu, state); } - test_nested_state(vcpu, state); /* It is invalid to have vmxon_pa == -1ull and SMM flags non-zero. */ state->hdr.vmx.smm.flags = 1;
The "vmxon_pa == vmcs12_pa == -1ull" test happens to work by accident: as Enlightened VMCS is always supported, set_default_vmx_state() adds 'KVM_STATE_NESTED_EVMCS' to 'flags' and the following branch of vmx_set_nested_state() is executed: if ((kvm_state->flags & KVM_STATE_NESTED_EVMCS) && (!guest_can_use(vcpu, X86_FEATURE_VMX) || !vmx->nested.enlightened_vmcs_enabled)) return -EINVAL; as 'enlightened_vmcs_enabled' is false. In fact, "vmxon_pa == vmcs12_pa == -1ull" is a valid state when not tainted by wrong flags so the test should aim for this branch: if (kvm_state->hdr.vmx.vmxon_pa == INVALID_GPA) return 0; Test all this properly: - Without KVM_STATE_NESTED_EVMCS in the flags, the expected return value is '0'. - With KVM_STATE_NESTED_EVMCS flag (when supported) set, the expected return value is '-EINVAL' prior to enabling eVMCS and '0' after. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- .../kvm/x86_64/vmx_set_nested_state_test.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)